Bug 608206

Some suggests:
1.License:        (GPLv2 or GPLv3) and GPLv2+ and LGPLv2+
-> GPLv3
Because license field only applies to binary rpm
2.
It'll be better to delete .a files instead of shipping it in -static subpackage. 

From packaging guideline:
Static libraries should only be included in exceptional circumstances

3.

You can add a make test to %check

rpmlint output:
zn_poly.src: W: spelling-error %description -l en_US zn -> Zn, z, n
zn_poly.src: W: spelling-error %description -l en_US nZ -> NZ, Zn, n
zn_poly.src:48: W: configure-without-libdir-spec
zn_poly.x86_64: W: spelling-error %description -l en_US zn -> Zn, z, n
zn_poly.x86_64: W: spelling-error %description -l en_US nZ -> NZ, Zn, n
zn_poly-debuginfo.x86_64: W: spelling-error Summary(en_US) zn -> Zn, z, n
zn_poly-debuginfo.x86_64: W: spelling-error %description -l en_US zn -> Zn, z, n
zn_poly-devel.x86_64: W: spelling-error Summary(en_US) zn -> Zn, z, n
zn_poly-devel.x86_64: W: spelling-error %description -l en_US zn -> Zn, z, n
zn_poly-devel.x86_64: W: spelling-error %description -l en_US nZ -> NZ, Zn, n
zn_poly-devel.x86_64: E: no-ldconfig-symlink /usr/lib64/libzn_poly.so
zn_poly-devel.x86_64: W: no-documentation
zn_poly-static.x86_64: W: spelling-error Summary(en_US) zn -> Zn, z, n
zn_poly-static.x86_64: W: spelling-error %description -l en_US zn -> Zn, z, n
zn_poly-static.x86_64: W: spelling-error %description -l en_US nZ -> NZ, Zn, n
zn_poly-static.x86_64: W: no-documentation
5 packages and 0 specfiles checked; 1 errors, 15 warnings.

- You can fix the configure-without-libdir warning by replacing
 ./configure --cflags="%{optflags} -fPIC" --prefix=%{_prefix} \
    --gmp-prefix=%{_prefix} \
    --ntl-prefix=%{_prefix} \
    --flint-prefix=%{_prefix}
with
 python makemakefile.py \
    --cflags="%{optflags} -fPIC" --prefix=%{_prefix} \
    --gmp-prefix=%{_prefix} --ntl-prefix=%{_prefix} \
    --flint-prefix=%{_prefix} > makefile


- Fix the no-ldconfig-symlink either by patching makemakefile.py so that libzn_poly.so becomes a symlink, or by replacing the installed duplicate with a symlink at the end of %install.


MUST: The package does not yet exist in Fedora. The Review Request is not a duplicate. OK
MUST: The spec file for the package is legible and macros are used consistently. OK
MUST: The package must be named according to the Package Naming Guidelines. OK
MUST: The spec file name must match the base package %{name}. OK
MUST: The package must be licensed with a Fedora approved license and meet the  Licensing Guidelines. OK

MUST: The License field in the package spec file must match the actual license. OK
- Specifying the multiple license scenario is OK, although you could shorten it to "GPLv2 or GPLv3", since that's what comes out at the end.

MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. OK
0eeaae2524addf558de94bfbc914d22e  zn_poly-0.9.tar.gz
0eeaae2524addf558de94bfbc914d22e  ../SOURCES/zn_poly-0.9.tar.gz

MUST: The package MUST successfully compile and build into binary rpms. OK
MUST: The spec file MUST handle locales properly. N/A

MUST: Optflags are used and time stamps preserved. NEEDSWORK
- Preserve time stamps in %install by adding -p (or -a) to the cp argument.

MUST: Packages containing shared library files must call ldconfig. OK

MUST: A package must own all directories that it creates or require the package that owns the directory. OK
- I recommend using a trailing / for directories in %files to make it clearer, i.e.
%{_includedir}/zn_poly/

MUST: Files only listed once in %files listings. OK
MUST: Debuginfo package is complete. OK
MUST: Permissions on files must be set properly. OK
MUST: Clean section exists. OK
MUST: Large documentation files must go in a -doc subpackage. N/A

MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. NEEDSWORK
- Don't include README, it only contains instructions for compilation, which aren't relevant to the binary rpm.
- Maybe include doc/REFERENCES ?

MUST: Header files must be in a -devel package. OK

MUST: Static libraries must be in a -static package. OK
- There's nothing wrong with shipping static libraries (in -static), but normally it isn't done in Fedora.
- If you want, you can just not ship the static library by either not installing it in %install, or by deleting it at the end of %install.

MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. N/A
MUST: If a package contains library files with a suffix then library files ending in .so must go in a -devel package. OK
MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency. OK
MUST: Packages does not contain any .la libtool archives. N/A
MUST: Desktop files are installed properly. N/A
MUST: No file conflicts with other packages and no general names. OK
MUST: Buildroot cleaned before install. OK
SHOULD: %{?dist} tag is used in release. OK
SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. OK
SHOULD: The package builds in mock. OK

Jussi, thanks for the review.

(In reply to comment #1)
> Some suggests:
> 1.License:        (GPLv2 or GPLv3) and GPLv2+ and LGPLv2+
> -> GPLv3
> Because license field only applies to binary rpm

It's in summary GPLv3, yes, but I refer to:
https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Multiple_Licensing_Scenarios

If there are some files under a different license, it must stated in the spec, and refering to the COPYING file is one possibiliy.
e.g. wide_arith.h is licensed under LGPLv2+, so this needs to be reflected somewhere.

> 2.
> It'll be better to delete .a files instead of shipping it in -static
> subpackage. 
> 
> From packaging guideline:
> Static libraries should only be included in exceptional circumstances

Yes, I usually delete them, but because this is a dependency of flint, which shipps static libs, I choosed to ship them here too. (Furthermore, I don't know yet, if sagemath needs static libs, if it doesn't maybe we can delete them later on.)

> 3.
> 
> You can add a make test to %check    

I think, I already did... :)

###########################################################################

(In reply to comment #2)
> rpmlint output:
[snip]
> 
> - You can fix the configure-without-libdir warning by replacing
>  ./configure --cflags="%{optflags} -fPIC" --prefix=%{_prefix} \
>     --gmp-prefix=%{_prefix} \
>     --ntl-prefix=%{_prefix} \
>     --flint-prefix=%{_prefix}
> with
>  python makemakefile.py \
>     --cflags="%{optflags} -fPIC" --prefix=%{_prefix} \
>     --gmp-prefix=%{_prefix} --ntl-prefix=%{_prefix} \
>     --flint-prefix=%{_prefix} > makefile

Done.

> - Fix the no-ldconfig-symlink either by patching makemakefile.py so that
> libzn_poly.so becomes a symlink, or by replacing the installed duplicate with a
> symlink at the end of %install.

Done in %install.

[snip]
> MUST: Optflags are used and time stamps preserved. NEEDSWORK
> - Preserve time stamps in %install by adding -p (or -a) to the cp argument.

Done.

> MUST: A package must own all directories that it creates or require the package
> that owns the directory. OK
> - I recommend using a trailing / for directories in %files to make it clearer,
> i.e.
> %{_includedir}/zn_poly/

Changed.

> MUST: All relevant items are included in %doc. Items in %doc do not affect
> runtime of application. NEEDSWORK
> - Don't include README, it only contains instructions for compilation, which
> aren't relevant to the binary rpm.
> - Maybe include doc/REFERENCES ?

ok, done.

> MUST: Static libraries must be in a -static package. OK
> - There's nothing wrong with shipping static libraries (in -static), but
> normally it isn't done in Fedora.
> - If you want, you can just not ship the static library by either not
> installing it in %install, or by deleting it at the end of %install.

flint has static libs, so I have to ship them too, or the libs from flint would be nonsense. (Maybe if sage doesn't require the static libs, we'll delete it.)

###########################################################################

Spec URL: http://tomspur.fedorapeople.org/review/zn_poly.spec
SRPM URL: http://tomspur.fedorapeople.org/review/zn_poly-0.9-2.fc13.src.rpm

flint wants to have the headers in zn_poly/src

Spec URL: http://tomspur.fedorapeople.org/review/zn_poly.spec
SRPM URL: http://tomspur.fedorapeople.org/review/zn_poly-0.9-3.fc13.src.rpm

(In reply to comment #3)
> Jussi, thanks for the review.
> (In reply to comment #1)
> > Some suggests:
> > 1.License:        (GPLv2 or GPLv3) and GPLv2+ and LGPLv2+
> > -> GPLv3
> > Because license field only applies to binary rpm
> It's in summary GPLv3, yes, but I refer to:
> 

See http://lists.fedoraproject.org/pipermail/packaging/2010-May/007075.html for more explanation. I think the mainpackage is a pure GPLv3 license, the license for -devel may be a bit complicated.

I'm not sure I agree with
 cp -pv include/*.h %{buildroot}%{_includedir}/zn_poly/src/

If flint assumes a nonstandard location for the headers, then it should be fixed.

(In reply to comment #1)
> Some suggests:
> 1.License:        (GPLv2 or GPLv3) and GPLv2+ and LGPLv2+
> -> GPLv3
> Because license field only applies to binary rpm

Note that this is not true, since "(GPLv2 or GPLv3) and GPLv2+ and LGPLv2+" resolves to "GPLv2 or GPLv3".

(In reply to comment #6)
> I'm not sure I agree with
>  cp -pv include/*.h %{buildroot}%{_includedir}/zn_poly/src/
> 
> If flint assumes a nonstandard location for the headers, then it should be
> fixed.    

Ok, reverted in:

Spec URL: http://tomspur.fedorapeople.org/review/zn_poly.spec
SRPM URL: http://tomspur.fedorapeople.org/review/zn_poly-0.9-4.fc13.src.rpm

Will do the flint fix with the update in bug #608199.

Very well,

APPROVED


PS. The soname = %{version} looks a bit funny. Maybe the library still is under heavy development; the soname changing with updates might cause troubles with other packages later on.

Thanks again.

New Package CVS Request
=======================
Package Name: zn_poly
Short Description: C library for polynomial arithmetic
Owners: tomspur konradm
Branches: F-12 F-13
InitialCC:

CVS done (by process-cvs-requests.py).

zn_poly-0.9-4.fc13 has been submitted as an update for Fedora 13.
http://admin.fedoraproject.org/updates/zn_poly-0.9-4.fc13

zn_poly-0.9-4.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/zn_poly-0.9-4.fc12

zn_poly-0.9-4.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.

zn_poly-0.9-4.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.