Bug 719757

Summary: Review Request: apron - Numerical abstract domain library
Product: [Fedora] Fedora Reporter: Jerry James <loganjerry>
Component: Package ReviewAssignee: Brendan Jones <brendan.jones.it>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: brendan.jones.it, notting, package-review, volker27
Target Milestone: ---Flags: brendan.jones.it: fedora-review+
gwync: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: apron-0.9.10-4.fc16 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2011-11-19 06:06:53 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Jerry James 2011-07-07 21:30:34 UTC
Spec URL: http://jjames.fedorapeople.org/apron/apron.spec
SRPM URL: http://jjames.fedorapeople.org/apron/apron-0.9.10-1.fc15.src.rpm
Description: The APRON library is dedicated to the static analysis of the numerical variables of a program by Abstract Interpretation.  The aim of such an analysis is to infer invariants about these variables, like 1<=x+y<=z, which holds during any execution of the program.

The APRON library is intended to be a common interface to various underlying libraries/abstract domains and to provide additional services that can be implemented independently from the underlying library/abstract domain.

This package is needed to enable extra functionality in the why package.

Comment 1 Volker Fröhlich 2011-08-23 19:28:38 UTC
Use the name macro.

[makerpm@lenovo apron-0.9.10-1.fc15.src]$ rpmlint ../apron-0.9.10-1.fc15.src.rpm ../../RPMS/x86_64/apron-*
apron.src: W: spelling-error %description -l en_US invariants -> invariant, in variants, in-variants
apron.src:158: E: hardcoded-library-path in %{_prefix}/lib/*
apron.src:159: E: hardcoded-library-path in %{_prefix}/lib
apron.x86_64: W: spelling-error %description -l en_US invariants -> invariant, in variants, in-variants
apron-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/apron-0.9.10/newpolka/mf_qsort.c

Please ask the author to update the license file. Correct the address at your ease.

The mf_qsort.c is originally from MySQl or something. It is licensed as GPLv2+.

Don't ship static libraries, until you have a good reason: http://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries

Comment 2 Jerry James 2011-08-24 23:10:00 UTC
Thanks for the notes.  I have made the following changes:
- increased the usage of %{name}
- corrected the License field
- build the C and C++ interfaces even on platforms without ocaml support
- move the debug libraries to separate -debug packages

New URLs:
Spec URL: http://jjames.fedorapeople.org/apron/apron.spec
SRPM URL: http://jjames.fedorapeople.org/apron/apron-0.9.10-2.fc15.src.rpm

Comment 3 Jerry James 2011-08-24 23:13:16 UTC
Oh, I forgot to add that the incorrect-fsf-address warning is due to mf_qsort.c!  Every other file in the APRON distribution has the correct address.  The APRON developers don't want to change the address in mf_qsort.c, because that is the way it appears in the MySQL code base.  I checked today, and the incorrect address still appears in MySQL 5.5.15.  So if somebody can get the MySQL developers to fix the FSF's address, then APRON will get the correct address, too.

Also, the static libraries are part of the Ocaml interface.  See https://fedoraproject.org/wiki/Packaging:OCaml#-devel_subpackage.

Comment 4 Brendan Jones 2011-11-04 08:07:13 UTC
Hi Jerry,

looking pretty good. Just a few questions below:

Required
========
+ - OK
- - N/A
X - attention
? - comment please

[+] named according to the Package Naming Guidelines 
[+] The spec file name must match the base package %{name}, in the format
%{name}.spec 
[+] Meet the Packaging Guidelines
unless building for F12 and below  or EPEL   
[?] Be licensed with a Fedora approved license and meet the Licensing
Guidelines 
*** Just need to clarify the multiple licenses in the SPEC file. May also need to reflect in the relevant %file section (ie 
http://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Multiple_Licensing_Scenarios

[+] The License field in the package spec file must match the actual license 
[+] License file must be included in %doc
[+] The spec file must be written in American English
[+] The spec file for the package MUST be legible
[+] The sources used to build the package must match the upstream source
*** b108de2f4a8c4ecac1ff76a6d282946fd3bf1466a126cf5344723955f305ec8e OK
[+] Successfully compile and build into binary rpms on at least one primary
architecture
[-] Proper use of ExcludeArch 
[+] All build dependencies must be listed in BuildRequires
[+] The spec file MUST handle locales properly
[+] Shared library files (not just symlinks) in any of the dynamic linker's
default paths, must call ldconfig in %post and %postun
[+] Packages must NOT bundle copies of system libraries
[-] If the package is designed to be relocatable, the packager must state this
fact in the request for review, along with the rationalization for relocation
of that specific package
[+] A package must own all directories that it creates
directories under this
[+] A Fedora package must not list a file more than once in the spec file's
%files listings
[+] Permissions on files must be set properly.
[+] Each package must consistently use macros
[+] The package must contain code, or permissable content
[-] Large documentation files must go in a -doc subpackage
[+] If a package includes something as %doc, it must not affect the runtime of
the application
[+] Header files must be in a -devel package
[-] Static libraries must be in a -static package
[?] library files that end in .so (without suffix) must go in a -devel package
*** You have separated the *_debug.so files out into a separate package?
[+] devel packages must require the base package using a fully versioned
dependency
[-] Packages must NOT contain any .la libtool archives
[-] GUI apps must include a %{name}.desktop file, properly installed with
desktop-file-install in the %install section 
[+] Packages must not own files or directories already owned by other packages
[+] All filenames in rpm packages must be valid UTF-8

[?] Packaged according to Fedora OCAML packaging guidelines
**** Do you need an explicit Requires: apron-devel in package ocaml-apron-devel? 
Similiarly -debug packages.
see: http://fedoraproject.org/wiki/Packaging:OCaml#-devel_subpackage


Should Items
============
[+] the packager SHOULD query upstream for any missing license text files to
include it
[-] Non-English language support for description and summary sections in the
package spec if available

[+] The reviewer should test that the package builds in mock
 rpmlint /var/lib/mock/fedora-16-x86_64/result/*apron*.rpm
apron.src: W: spelling-error %description -l en_US invariants -> invariant, in variants, in-variants
apron.x86_64: W: spelling-error %description -l en_US invariants -> invariant, in variants, in-variants
apron-debug.x86_64: W: no-documentation
apron-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/apron-0.9.10/newpolka/mf_qsort.c
ocaml-apron-debug.x86_64: W: no-documentation
8 packages and 0 specfiles checked; 1 errors, 4 warnings.

[+] The package should compile and build into binary rpms on all supported
architectures
[-] The reviewer should test that the package functions as described
[+] If scriptlets are used, those scriptlets must be sane
[?] Usually, subpackages other than devel should require the base package using
a fully versioned dependency
Do you need an explicit Requires: apron-debug in package ocaml-apron-debug?
[-] The placement of pkgconfig(.pc) should usually be placed in a -devel pkg
[-] 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
[-] Should contain man pages for binaries/scripts

Comment 5 Jerry James 2011-11-04 21:21:55 UTC
(In reply to comment #4)
> [?] Be licensed with a Fedora approved license and meet the Licensing
> Guidelines 
> *** Just need to clarify the multiple licenses in the SPEC file. May also need
> to reflect in the relevant %file section (ie 
> http://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Multiple_Licensing_Scenarios

I added a comment to explain the license situation.  Unfortunately, there is no way to address the problem via %files, as the differently licensed file gets compiled into the library along with everything else.

> [?] library files that end in .so (without suffix) must go in a -devel package
> *** You have separated the *_debug.so files out into a separate package?

On second thought, we don't really want these in Fedora.  I have removed the debug versions of the libraries altogether.

> [?] Packaged according to Fedora OCAML packaging guidelines
> **** Do you need an explicit Requires: apron-devel in package
> ocaml-apron-devel? 
> Similiarly -debug packages.
> see: http://fedoraproject.org/wiki/Packaging:OCaml#-devel_subpackage

Ah, right.  I was thinking apron-devel wasn't needed because it just contains C header files, but it also contains the *.so files, doesn't it?  Okay, added.

> [?] Usually, subpackages other than devel should require the base package using
> a fully versioned dependency
> Do you need an explicit Requires: apron-debug in package ocaml-apron-debug?

I removed them, so the point is moot. :-)  New versions:

Spec URL: http://jjames.fedorapeople.org/apron/apron.spec
SRPM URL: http://jjames.fedorapeople.org/apron/apron-0.9.10-3.fc15.src.rpm

Comment 6 Brendan Jones 2011-11-04 22:24:08 UTC
I noticed in the README file in the root directory of the source it is mentioned that those files requiring the PPL library are also GPL rather than LGPL.

Apart from that I'm happy to approve this review.

Comment 7 Jerry James 2011-11-04 22:37:02 UTC
Argh, you're right.  How about if I change the comment on the License tag to read as follows:

# The entire package is LGPLv2+ except newpolka/mf_qsort.c and ppl/*, all of
# which are GPLv2+.

Thanks for the review.  Are you okay with me just making that change before I import into git, or do you want to see another spec file & rpm before approving this package?

Comment 8 Brendan Jones 2011-11-04 22:40:59 UTC
That's fine Jerry. This package is APPROVED.

Comment 9 Jerry James 2011-11-04 22:46:38 UTC
Thanks again, Brendan.

New Package SCM Request
=======================
Package Name: apron
Short Description: Numerical abstract domain library
Owners: jjames
Branches: f16
InitialCC:

Comment 10 Jason Tibbitts 2011-11-04 23:05:54 UTC
Just wanted to be sure that everyone's aware that the license tag reflects what's in the binary package.  So if you had a bunch of LGPLv2+ code and a single GPLv2+ file was compiled together with it to make a binary, the resulting binary is simply GPLv2+ and if that's all you had in the resulting package, the license tag would simply show GPLv2+.

Or, to put it another way, it looks like "newpolka/mf_qsort.c and ppl/*" refer to files in the source package, not the resulting binary package, and thus aren't terribly relevant to what goes in the License tag.

Comment 11 Jerry James 2011-11-05 21:01:33 UTC
In this case, the newpolka/mf_qsort.c file winds up in %{_libdir}/libpolkaMPQ.so.* and %{_libdir}/libpolkaRll.so.*, and the ppl/* files wind up in %{_libdir}/libap_ppl.so.*, so those binaries are GPLv2+, and all the rest of the libraries are LGPLv2+.  I'll add that information to the spec file comment.

Comment 12 Gwyn Ciesla 2011-11-06 20:37:02 UTC
Git done (by process-git-requests).

Comment 13 Fedora Update System 2011-11-07 16:52:38 UTC
apron-0.9.10-3.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/apron-0.9.10-3.fc16

Comment 14 Fedora Update System 2011-11-10 17:45:48 UTC
apron-0.9.10-4.fc16 has been pushed to the Fedora 16 testing repository.

Comment 15 Fedora Update System 2011-11-19 06:06:53 UTC
apron-0.9.10-4.fc16 has been pushed to the Fedora 16 stable repository.