Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=592579 --- Comment #19 from David A. Wheeler <dwheeler@xxxxxxxxxxxx> 2010-05-25 13:54:23 EDT --- Thanks, I'm now looking over the updated 1.4-2 version. Much progress, but a few more things to do. The rpmbuild now works on 32-bit x86, and "rpmlint" is now much happier compared to 2 versions ago. I did: rpmlint frama-c.spec ../RPMS/i586/frama-c-*-2.* ../SRPMS/frama-c-1.4-2.fc11.src.rpm The only rpmlint reports involve the license, which you're already addressing with fedora legal: frama-c.i586: W: invalid-license QPL with modifications frama-c-devel.i586: W: invalid-license QPL with modifications frama-c.src: W: invalid-license QPL with modifications 3 packages and 1 specfiles checked; 0 errors, 3 warnings. I don't understand why this package installs "/usr/bin/ptests.byte". I don't think that is a user-level program; I believe that is just a test program that should be *used* during a "make check" as part of the build. Thus, I think you should NOT install that program as part of this package. Based on comment #3: * All rpmlint errors are gone, except for the licensing issues being addressed. There's no longer a "chcon" command (inc. the unneeded comment) * The "-jessie" option will require a change to "why" after frama-c enters the repository, so that's addressed. * I see your name ("Mark Rader") in the ChangeLog at the end, good! Usually there's a blank line between major entries (E.g., between leading "*"), you probably should add that. Regarding comment #6 and comment #12: * Excluding the ".byte" files is fine. But that means that this package won't work on some architectures, so you need some ExcludeArch statements. Regarding comment #13: * Revision number bumped, good! Thanks. * I *still* don't understand this comment: "The configure file uses graph.cmo in compiling a test program to check for ocamlgraph, but Fedora doesn't include this file in its distributions." But Fedora *does* include the ocaml-ocamlgraph package, and that package specifically installs "/usr/lib/ocaml/ocamlgraph/graph.cmo". So I really don't understand what this spec is trying to say. (I realize that you inherited this package from others, so this is probably inherited as well, but it sure isn't clear as it is.) * I really think you should prefix the filename of Patch1 with "frama-c-". Otherwise, if someone builds many packages inside a shared rpmbuild/SOURCES directory, there's a (tiny) risk that this will overlap with something else. The risk is tiny, but better to avoid the question. * Patch2 (ptests) has *no* explanation, and that is NOT okay. Need to briefly explain what it does, and either (1) when it was sent upstream, or (2) why it wasn't sent upstream. I believe that is a MUST. * Nit: "plug in's" is still not a word. An "'s" is either a possessive or an appreviation of "is", and this is neither (it's just a plural). I'd suggest "plug-ins" instead. If the spellchecker whines, too bad. * No %check section. It's not required, but it'd be nice if it worked. In the long term, a %check section - even one with just one trivial test - will catch problems like silently failed builds. * I'll check the %files list separately. You really need to do a Koji build (see comment #18). I can't do that this at this moment from where I am (due to SSL munging where I am), but I did "mock --rebuild" on 32-bit, and found a problem that Koji would have found. I think you're missing a BuildRequires. When I use mock --rebuild, I get: ocamlc.opt -o lib/gui/Scope.cmo -w Ael -warn-error A -annot -g -I src/misc -I src/ai -I src/memory_state -I src/toplevel -I src/slicing_types -I src/pdg_types -I src/kernel -I src/logic -I src/lib -I src/project -I src/buckx -I src/gui -I external -I cil/src -I cil/src/ext -I cil/src/frontc -I cil/src/logic -I cil/ocamlutil -I lib/plugins -I lib -I +ocamlgraph ........ /usr/bin/ld: cannot find -lgnomecanvas-2 collect2: ld returned 1 exit status File "_none_", line 1, characters 0-1: Error: Error while building custom runtime system In short, the build is complaining that there is no library gnomecanvas-2. A quick "rpm -qf /usr/lib/libgnomecanvas-2.so" suggests to me that you need to add this to the spec file: BuildRequires: libgnomecanvas-devel Once I added that "BuildRequires: libgnomecanvas-devel", the mock build worked for me. You should still do a Koji build, since that will detect problems on many architectures. (Be sure to add ExcludeArch's first; we already know that this package will NOT work on some other architectures because it doesn't generate proper .byte files.) As noted in "How to create an RPM package", I've found it to be very helpful to "yum install auto-buildrequires" and then run "auto-br-rpmbuild -ba SPECFILE". Unfortunately, it didn't seem to be enlightening in this case. Per comment #16, here are the previously-unresolved guidelines: * MUST: rpmlint must be run on every package. The output should be posted in the review.[1] IN PROGRESS. rpmlint output shown earlier, and issues being worked on. * MUST: The package must meet the Packaging Guidelines . OK generally, but see other comments (including this one). * MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines . IN PROGRESS. I suspect the license is fine, but it *must* be specifically approved. * MUST: The License field in the package spec file must match the actual license. [3] FAIL. The "License" says "GPL+". I found "GPLv2+", but no "GPL+". I think you should remove the "GPL+", or find out why it's there. * MUST: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc.[4] FAIL. Under %files, need to add entries for the license files that are there. E.G.: %doc licenses/LGPLv2.1 %doc licenses/LGPLv3 %doc licenses/Q_MODIFIED_LICENSE %doc cil/LICENSE * MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture. [7] IN PROGRESS. Need to add at least one BuildRequires. You should test with Koji. * MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in bugzilla, describing the reason that the package does not compile/build/work on that architecture. The bug number MUST be placed in a comment, next to the corresponding ExcludeArch line. [8] FAIL. Need to add ExcludeArch info. * MUST: All build dependencies must be listed in BuildRequires, except for any that are listed in the exceptions section of the Packaging Guidelines ; inclusion of those as BuildRequires is optional. Apply common sense. FAIL. See below about Koji, BuildRequires. * MUST: Packages must NOT bundle copies of system libraries.[11] ISSUE. This has its own version of cil, yet there is an ocaml-cil. Need to at least document this in the spec file. Since upstream has forked their own version of cil, I don't see any way around this :-(, but you need to include a reasonable justification in the spec comments. * MUST: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory. [13] OK. * MUST: A Fedora package must not list a file more than once in the spec file's %files listings. [14] FAIL. These files are in the main package *AND* in the -devel package: /usr/share/man/man1/frama-c.1.gz /usr/share/man/man1/frama-c-gui.1.gz You can find duplicates by doing: cd ~/rpmbuild/RPMS/ARCH # Substitute "ARCH" for your architecture rpm -qlp frama-c-1.4-2.fc11.*.rpm | sort > ,1 rpm -qlp frama-c-devel-1.4-2.fc11.*.rpm | sort > ,2 comm -12 ,1 ,2 * MUST: Permissions on files must be set properly. Executables should be set with executable permissions, for example. Every %files section must include a %defattr(...) line. [15] OK. * MUST: If a package includes something as %doc, it must not affect the runtime of the application. To summarize: If it is in %doc, the program must run properly if it is not present. [18] NA/FAIL. Nothing is marked as %doc, but I think that's a mistake. * MUST: Header files must be in a -devel package. [19] ISSUE. This is *primarily* an application-level program, but it provides OCaml headers for plug-ins, yes? Shouldn't those be separate? * MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package. [19] NA. No ".so.SUFFIX" files. * MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built.[20] OK. There aren't any. * MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. If you feel that your packaged GUI application does not need a .desktop file, you must put a comment in the spec file with your explanation. [22] FAIL. You need a desktop file - that way, users can access the GUI from their graphical menu. These are easy to create. For a trivial example see: gwhy.desktop from "why". Once you create a nice .desktop file, you need to submit it to upstream so that they'll include it in the future. The .desktop spec is now widely used (GNOME and KDE use them in particular), and they make GUI users happy :-). * MUST: Packages must not own files or directories already owned by other packages. The rule of thumb here is that the first package to be installed should own the files or directories that other packages may rely upon. This means, for example, that no package in Fedora should ever share ownership with any of the files or directories owned by the filesystem or man package. If you feel that you have a good reason to own a file or directory that another package owns, then please present that at package review time. [23] OK * MUST: All filenames in rpm packages must be valid UTF-8. [24] OK. I tested with this, and there were no errors: rpmls ../RPMS/i586/frama-c-*-2* | iconv -f utf-8 -t utf-8 As promised, here are the SHOULDs: # SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [25] NA. There are separate license files. # SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available. [26] Nice-to-have, but not required. Unless you can easily translate the text to another language, carry on. # SHOULD: The reviewer should test that the package builds in mock. [27] OK. I did that, as described earlier, and found a missing BuildRequire. # SHOULD: The package should compile and build into binary rpms on all supported architectures. [28] Won't happen. As it is, it'll only work on systems where the OCaml compiler generates machine code. Maybe in the future? # SHOULD: The reviewer should test that the package functions as described. A package should not segfault instead of running, for example. OK. I tried out the GUI, and it came up. # SHOULD: If scriptlets are used, those scriptlets must be sane. This is vague, and left up to the reviewers judgement to determine sanity. [29] OK. A very short (2-line) scriptlet is included; looks sane to me. # SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency. [21] NA # SHOULD: The placement of pkgconfig(.pc) files depends on their usecase, and this is usually for development purposes, so should be placed in a -devel pkg. A reasonable exception is that the main pkg itself is a devel tool not installed in a user runtime, e.g. gcc or gdb. [30] NA. No .pc files. # SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself. [31] NA. # SHOULD: your package should contain man pages for binaries/scripts. If it doesn't, work with upstream to add them where they make sense.[32] OK. The man pages are included in *both* the main and -devel package, and they should only be in the *main* package. -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the bug. _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review