[Bug 592579] Review Request: Frama-c - Framework for source code analysis of C software

Tue May 25 17:54:27 UTC 2010

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 at dwheeler.com> 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.