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

[bugzilla at redhat.com]

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 #35 from David A. Wheeler <dwheeler at dwheeler.com> 2010-07-16 17:49:31 EDT ---
Okay, I've started reviewing.  This new package builds on my system, and the
only rpmlint messages were the ones noted above.

Comment 19 noted a number of "to-do" items, so I'm going to first go through
all of the ones that had problems before:

* "I don't understand why this package installs "/usr/bin/ptests.byte"."

That's now gone, good.

* "Excluding the ".byte" files is fine.  But that means that this package won't
work on some architectures, so you need some ExcludeArch statements."

It now has an ExcludeArch statement, and I notice that you did a Koji test
(that should tell you of problems like this).

* "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."

 I'm sorry, the comment changed, but new comment somehow got messed
 up and I *still* don't understand it.  It now says:
# The configure file uses graph.cmo in compiling a test program to
# check for ocamlgraph, but Fedora doesn't include this file in itshas changed
the
# extension in later distributions to graph.cma .

  But "rpmls ocaml-ocamlgraph" shows:
-rw-r--r--  /usr/lib/ocaml/ocamlgraph/graph.cma
-rw-r--r--  /usr/lib/ocaml/ocamlgraph/graph.cmi
-rw-r--r--  /usr/lib/ocaml/ocamlgraph/graph.cmo

  The Fedora package *does* include the ".cmo" *and ".cma" file.
  I still 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-"."

  Fixed, thanks!!

* "Patch2 (ptests) has *no* explanation, and that is NOT okay."

  All patches now have an explanation, good!

* "Nit: "plug in's" is still not a word."

  Using "plug ins" is not my preference, but I understand that you're
  trying to keep the spellchecker happy, so fine.

* "No %check section.  It's not required..."

  Still no %check section, but since it's not required... it's not required
:-).

* "I'll check the %files list separately."

  Which I still need to do.

* "You really need to do a Koji build (see comment #18)...."

  Which you did, see comment 34. Thanks.

*  "MUST: rpmlint must be run on every package. The output should be posted
in the review.[1]"

  OK.

* MUST: The package must be licensed with a Fedora approved license and
meet the Licensing Guidelines .

OK, see comment 32.  I don't know why legal hasn't responded yet,
they really should.  But they approved the QPL, and the only
changes cannot affect its Freeness (see comment 32).
I'd like to hear anyone else's comments about this.

* MUST: The License field in the package spec file must match the actual
license. [3]

OK.

* 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]

OK.  I checked the binary package with rpmls, and they're there.

* MUST: The package MUST successfully compile and build into binary rpms on
at least one primary architecture. [7]

OK.  I just did it, "works for me".

* 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]

OK.  Added ExcludeArch, tested with Koji.

* 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.

OK.  Worked with Koji.

* MUST: Packages must NOT bundle copies of system libraries.[11]

OK.  As noted in earlier comments, this has a forked version of cil;
I don't see any way around this :-(, and you have
a reasonable justification in the spec comments.

* MUST: A Fedora package must not list a file more than once in the spec
file's %files listings. [14]

OK.  I didn't find any dups this time.

* 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]

OK.  We now have some files marked as documentation.

* MUST: Header files must be in a -devel package. [19]

OK.

* 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]

OK, we now have a .desktop file.

This is looking really good.  I want to test it out, and do a clean
read-through.  But other than my complaint about a comment (which I *still*
don't understand) it's looking great so far.

-- 
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.