Bug 1172719 - Review Request: isl - Integer point manipulation library
Summary: Review Request: isl - Integer point manipulation library
Keywords:
Status: CLOSED UPSTREAM
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Michael Schwendt
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2014-12-10 15:39 UTC by David Howells
Modified: 2015-02-11 20:21 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-02-11 20:21:24 UTC
Type: ---
Embargoed:
bugs.michael: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description David Howells 2014-12-10 15:39:29 UTC
Spec URL: http://people.redhat.com/dhowells/isl.spec
SRPM URL: http://people.redhat.com/dhowells/isl-0.14-1.fc20.src.rpm
Description:
isl is a library for manipulating sets and relations of integer points
bounded by linear constraints.  Supported operations on sets include
intersection, union, set difference, emptiness check, convex hull,
(integer) affine hull, integer projection, computing the lexicographic
minimum using parametric integer programming, coalescing and parametric
vertex enumeration.  It also includes an ILP solver based on generalized
basis reduction, transitive closures on maps (which may encode infinite
graphs), dependence analysis and bounds on piecewise step-polynomials.

Fedora Account System Username: dhowells

Comment 1 David Howells 2014-12-10 15:44:34 UTC
The isl package is used by gcc and cross-gcc.  Currently it's built statically into these (resulting it being built >20 times).  This extracts it into its own package.

There are the following rpmlint warnings, the first two repeated in the isl-devel package.

isl.x86_64: W: spelling-error %description -l en_US affine -> caffeine, fine
isl.x86_64: W: spelling-error %description -l en_US piecewise -> piece wise, piece-wise, piecework

These are spelt correctly, just not in the word list used by rpmlint.

isl.x86_64: W: shared-lib-calls-exit /usr/lib64/libisl.so.13.1.0 exit.5

Not a lot I can do about this other than asking the isl package authors to make it return instead.

isl-devel.x86_64: W: only-non-binary-in-usr-lib

It's objecting to the libisl.so symlink for some reason.

Comment 2 Michael Schwendt 2014-12-27 11:04:55 UTC
> expanded our pkgconfig_libdir... /usr/lib64/pkgconfig

That's what the build.log says, but you move it to %{_datadir}/pkgconfig/isl.pc in %install. Why? Its contents are _not_ arch-independent.

Further, isl.pc relinks with -lgmp, which is not necessary when linking shared. 

And isl.pc doesn't specify a pkgconfig dependency on gmp/gmp-devel, because that one does not include any pkgconfig file.


>   CC       isl_gmp.lo
>   CC       isl_ast_int.lo

Build output is non-verbose, which makes it almost useless. One cannot see the linker and/or compiler options ( https://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags )

Try building with V=1 passed to "make", or configure with --disable-silent-rules. That may be enough. See --help.


> -rwxr-xr-x  /usr/lib64/libisl.la

These libtool archives are covered here:
https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries


> $ rpm -qpR isl-devel-0.14-1.fc21.x86_64.rpm |grep isl
> isl = 0.14-1.fc21
> libisl.so.13()(64bit)

https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package


> Requires: gmp

https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires


> Group: System Environment/Base

Group tag for C/C++ library -devel packages has been "Development/Libraries" for many years.

The Group tag for your base package is missing. It would be "System Environment/Libraries", but notice:

https://fedoraproject.org/wiki/Packaging:Guidelines#Group_tag


> %setup -q
> %configure

%configure is to be executed in %build (not just for --short-circuit builds).


> %install
> make install ...

Use %makeinstall:

https://fedoraproject.org/wiki/Packaging:Guidelines#Why_the_.25makeinstall_macro_should_not_be_used


> %defattr(-,root,root,-)

%defattr is not needed anymore for a long time:
https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions

Unless you want to reuse exactly the same spec for EL5.

Comment 3 David Howells 2015-01-05 11:43:48 UTC
(In reply to Michael Schwendt from comment #2)
> Further, isl.pc relinks with -lgmp, which is not necessary when linking
> shared.

True, but it also shouldn't hurt.

> And isl.pc doesn't specify a pkgconfig dependency on gmp/gmp-devel, because
> that one does not include any pkgconfig file.

I'm not sure what I can do about that without modifying the gmp package.

> > -rwxr-xr-x  /usr/lib64/libisl.la
> 
> These libtool archives are covered here:
> https://fedoraproject.org/wiki/Packaging:
> Guidelines#Packaging_Static_Libraries

That shouldn't be there - I have a line that supposedly removes that in the spec file and I don't see it when I build the RPMs.

> > %install
> > make install ...
> 
> Use %makeinstall:
> 
> https://fedoraproject.org/wiki/Packaging:Guidelines#Why_the_.
> 25makeinstall_macro_should_not_be_used

Why?  Did you mean %make_install instead?  Note that it doesn't seem like this should be necessary.

Anyway, revised:

Spec URL: http://people.redhat.com/dhowells/isl.spec
SRPM URL: http://people.redhat.com/dhowells/isl-0.14-2.fc20.src.rpm

Comment 4 Michael Schwendt 2015-01-05 13:09:09 UTC
[relinking with -lgmp]

> True, but it also shouldn't hurt.

It can lead to extra rebuilds, e.g. you would need to rebuild anything that links with -lisl, if only GMP bumped its SONAME. Being aware of that can be helpful. Often the pkgconfig files are tailored for static linking.



> I'm not sure what I can do about that without modifying the gmp package.

The added "Requires: gmp-devel" is almost the right fix. You really want "Requires: gpm-devel%{?_isa}" to get the correct build-arch.


> I have a line that supposedly removes that in the spec file and
> I don't see it when I build the RPMs.

The added lines are the right fix. Note that "rm -f" would be better, because unlike "rm" it would not make the build fail, if the file to be removed does not exist. => You want to get rid of the file anyway, so if it doesn't exist that's fine.


> Did you mean %make_install instead?

Yes. The macro saves some typing, but it's not a MUST.


You've not fixed/removed the Group tag.

Comment 5 David Howells 2015-01-05 14:40:40 UTC
(In reply to Michael Schwendt from comment #4)
> [relinking with -lgmp]
> 
> > True, but it also shouldn't hurt.
> 
> It can lead to extra rebuilds, e.g. you would need to rebuild anything that
> links with -lisl, if only GMP bumped its SONAME. Being aware of that can be
> helpful. Often the pkgconfig files are tailored for static linking.

Some of the isl macros are just wrappers around the gmp function calls in the header - in that case, shouldn't -lgmp also be passed?

Comment 6 Michael Schwendt 2015-01-05 16:21:14 UTC
Ah, I missed that deprecated/int.h and val_gmp.h even include gmp.h, so then it's okay to -lgmp.

Comment 7 David Howells 2015-01-05 16:33:43 UTC
In that case:

Spec URL: http://people.redhat.com/dhowells/isl.spec
SRPM URL: http://people.redhat.com/dhowells/isl-0.14-3.fc20.src.rpm

Comment 8 Michael Schwendt 2015-01-06 12:50:54 UTC
* There's a test-suite included, which could be run in %check. It is successful here:

--- isl.spec.orig	2015-01-05 17:31:50.000000000 +0100
+++ isl.spec	2015-01-06 13:42:13.608267534 +0100
@@ -62,6 +62,9 @@
 mkdir -p %{buildroot}/%{gdbprettydir}
 mv %{buildroot}/%{_libdir}/*-gdb.py* %{buildroot}/%{gdbprettydir}
 
+%check
+make check
+
 %post -p /sbin/ldconfig
 %postun -p /sbin/ldconfig
 


* "fedora-review -b 1172719" doesn't find any issue either. The GPLv2+ license it recognizes is only in libtool's ltmain.sh.


* Modern packaging guidelines prefer %global over %define:
https://fedoraproject.org/wiki/Packaging:Guidelines#.25global_preferred_over_.25define


* APPROVED

Comment 9 David Howells 2015-01-06 13:42:11 UTC
Thanks.  I'll look at making the check and define changes.

Comment 10 Gwyn Ciesla 2015-01-06 14:12:26 UTC
No SCM request found.

Comment 11 David Howells 2015-01-06 14:34:37 UTC
New Package SCM Request
=======================
Package Name: isl
Short Description: Integer point manipulation library
Upstream URL: http://isl.gforge.inria.fr/
Owners: dhowells jakub
Branches: f20 f21 f22
InitialCC: dhorak

Comment 12 Gwyn Ciesla 2015-01-06 16:08:38 UTC
Git done (by process-git-requests).

Comment 13 Pierre-YvesChibon 2015-01-06 17:18:26 UTC
@Jon,

I think you have had a warning about the user `dhorak` when processing this request, haven't you?
The user `dhorak` in fact does not exist in FAS, so if there was no warning then there is a bug in the script.

Also, the warning about groups being invalid users should be gone now that Kevin merged the patch from Thomas yesterday (in case you have not seen it).

Comment 14 David Howells 2015-01-06 17:28:47 UTC
Yeah, "dhorak" should've been "sharkcz".


Note You need to log in before you can comment on or make changes to this bug.