Bug 225809 - Merge Review: gmp
Merge Review: gmp
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Package Reviews List
: Reopened
Depends On: 211762 218041 223692
Blocks: 225778 248363
  Show dependency treegraph
 
Reported: 2007-01-31 13:50 EST by Nobody's working on this, feel free to take it
Modified: 2010-01-11 02:59 EST (History)
8 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-01-06 21:17:52 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tibbs: fedora‑review+
wtogami: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 13:50:09 EST
Fedora Merge Review: gmp

http://cvs.fedora.redhat.com/viewcvs/devel/gmp/
Initial Owner: twoerner@redhat.com
Comment 1 Karsten Hopp 2007-02-23 10:10:18 EST
most common review issues fixed in gmp-4.1.4-12

> - remove trailing dot from summary
> - fix buildroot
> - fix post/postun/... requirements
> - use make install DESTDIR=...
> - replace tabs with spaces
> - convert changelog to utf-8
Comment 2 manuel wolfshant 2007-02-24 05:50:42 EST
[ "$RPM_BUILD_ROOT" != "/" ] in %install and %clean should be removed

According to upstream (http://www.swox.com/gmp/), the most current version is
4.2.1, while the packaged version is 4.1.4. At least a note explaining the
reason for not upgrading should be included in the spec.
Comment 3 Laurent Rineau 2007-03-05 10:04:04 EST
(In reply to comment #2)
> According to upstream (http://www.swox.com/gmp/), the most current version 
is
> 4.2.1, while the packaged version is 4.1.4. At least a note explaining the
> reason for not upgrading should be included in the spec.

That is bug #211762. gmp is a build-dependency of gcc. That is why the upgrade 
is not that safe.

Another bug that concerns this review is bug #218041: the gmp-4.1.x package 
ships a static version of libmpfr. Static libraries should be removed from 
Fedora, according to guidelines.

A bug about unsafe use of something in scriptlets, with a patch from Ville 
Skyttä: bug #223692.

Comment 4 Jochen Schmitt 2007-05-23 16:34:07 EDT
As soon as I can see, we need to create a seperate mpfr package.

Such a package doesn't exist in Fedora nowaday, becouse it was a integrated part
of the gmp package.
Comment 5 Jochen Schmitt 2007-05-23 17:00:54 EDT
I have create a mpfr-package, which you may find at:

http://www.herr-schmitt.de/pub/mpfr/mpfr.spec
http://www.herr-schmitt.de/pub/mpfr/mpft-2.2.1-1.src.rpm

It will be nice, if you can take the maintainership for this package, becouse
then you can best organiz the spit of gmp into the two seperate packages gmp and
mpfr.
Comment 6 Jochen Schmitt 2007-06-20 14:52:51 EDT
An Additional Note:

In the gcc/doc/install.texi of gcc-4.2.0 I have found a parapraph which says,
that the mpfr library bundled with gmp is buggy and some FORTRAN issue are
unfixable, if you use this buggy version
Comment 7 Jochen Schmitt 2007-07-16 12:07:56 EDT
On BZ #248363 a review for a separate mpfr package was started.
Comment 8 Jochen Schmitt 2007-07-22 15:39:04 EDT
I have create a suggested package for the most recent gmp without the mpfr library.

I have uploaded it to

http://www.herr-schmitt.de/pub/gmp/gmp-4.2.1-1.src.rpm
Comment 9 Ivana Varekova 2007-07-26 10:20:44 EDT
Package Change Request
======================
Package Name: gmp: 
Updated Fedora Owners: varekova@redhat.com
Comment 10 Harald Hoyer 2007-09-19 09:50:23 EDT
- change %{dist} to %{?dist} 
- consider "make check" after "make all"
Comment 11 Harald Hoyer 2007-09-19 09:52:34 EDT
ping?
Comment 12 Laurent Rineau 2007-09-20 07:38:07 EDT
Hi,

Why is his bug closed?
Has GMP been reviewed (then, where is the review?)?
Is there another GMP review somewhere? Then, this bug should have been closed 
as duplicate. What is the new bug number anyway?
Comment 13 Jason Tibbitts 2008-02-06 14:50:33 EST
I don't believe this should have been closed.  Nobody has approved it yet, and I
believe that the current package has outstanding issues.
Comment 14 Ivana Varekova 2008-02-07 04:17:14 EST
You are right, it should be open. I will go through it now. Thanks.
Comment 15 Ivana Varekova 2008-02-07 07:18:53 EST
Could anybody grab this review and write the list of "should be fixed" things here?
Comment 16 Laurent Rineau 2008-02-07 09:42:41 EST
rpmlint says:
gmp.i386: W: invalid-license LGPL
gmp.i386: W: 
unused-direct-shlib-dependency /usr/lib/libgmpxx.so.4.0.2 /lib/libm.so.6

IMO the License tag should be "LGPL2+".
Comment 17 Ralf Corsepius 2008-02-07 10:08:57 EST
Split out the static libs from *-devel into a *-static package or (if appropriate)
abandon shipping them.
Comment 18 Ivana Varekova 2008-02-08 06:09:20 EST
Thanks for both comments the problems are fixed in gmp-4.2.2-5.fc9 - (the
license tag is LGPLv3+).
Comment 19 Robert Scheck 2008-12-20 09:58:59 EST
For gmp-4.2.4-3, rpmlint doesn't seem to be silent (or is my rpmlint just
broken?):

gmp-devel.i386: E: tag-not-utf8 %changelog
gmp-static.i386: E: tag-not-utf8 %changelog

Ivana, can you explain why a separate %configure is defined, rather using
the default configure from redhat-rpm-config? So if there still is a valid 
reason for this own define, please add an explaining comment to the spec
file, otherwise use %configure (the config.* is no longer copied since F-10
or so, if that was the reason for the own %configure).

Please use "if [ $1 = 0 ]; then" in %preun, currently you're comparing something quoted.

Please only use "rm -rf $RPM_BUILD_ROOT" in %clean rather the current.
Comment 20 Robert Scheck 2008-12-20 10:00:06 EST
Ah...

+ ./configure --build=i686-redhat-linux-gnu --host=i686-redhat-linux-gnu --target=i386-redhat-linux-gnu --program-prefix= --prefix=/usr --exec-prefix=/usr --bindir=/usr/bin --sbindir=/usr/sbin --sysconfdir=/etc --datadir=/usr/share --includedir=/usr/include --libdir=/usr/lib --libexecdir=/usr/libexec --localstatedir=/var --sharedstatedir=/usr/com --mandir=/usr/share/man --infodir=/usr/share/info --enable-mpbsd --enable-cxx
configure: error: --target is not appropriate for GMP
Use --build=CPU-VENDOR-OS if you need to specify your CPU and/or system
explicitly.  Use --host if cross-compiling (see "Installing GMP" in the
manual for more on this).

...looks like a broken configure? Ivana, can you a bit investigate, whether there is really no fix for this and/or explain the %define in the spec file
how it should be? Thanks.
Comment 21 Ivana Varekova 2008-12-23 04:46:42 EST
Thanks. 
* my rpmlint was silent - so I don't fix the first point,
* %preun script changed 
* %clean part changed
* configure definition removed (can't find any reason to have it there)
(gmp-4.2.4-4.fc11)
Comment 22 Jason Tibbitts 2009-01-14 23:03:21 EST
I took a look at this.  I had some weird problem getting it to build in mock, but it builds fine in koji so that must be on my end.

The only rpmlint complaint is:
  gmp.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libgmpxx.so.4.0.4 
   /lib64/libm.so.6
which really isn't that big of a deal.  I tried the usual libtool tweak to pass -Wl,--as-needed but it didn't work, and I don't think it's worth trying to find a fix that works.

I'll run through a full review tomorrow.
Comment 23 Michael Schwendt 2009-01-15 07:57:25 EST
The reasons is a [probably years old] configure check, which explicitly adds -lm to the linker options, GMP_CHECK_LIBM_FOR_BUILD in configure.in. Since autoreconf is run anyway, this check could be removed. Alternatively, and more "dirty", it's possible to drop -lm from config.status and rerun configure afterwards.
Comment 24 Susi Lehtola 2009-05-21 06:45:41 EDT
tibbs: ping?
Comment 25 Jason Tibbitts 2009-05-27 18:16:10 EDT
I note that 4.3.1 has recently been released, but that doesn't really have any bearing on this review.

There is no reason for:
  Requires(post): /sbin/install-info
  Requires(preun): /sbin/install-info
because you're using the "-p" forms of the scriptlets:
  %post -p /sbin/ldconfig
  %postun -p /sbin/ldconfig

The license of the demos is GPL, not LGPL, but those aren't packaged so the license tag is correct.

The README file is duplicated between the three packages; it should not be.

Normally I would complain about such a generic name as /usr/include/mp.h, but this package has been around approximately forever.

So I see only two minor issues.  If you like, I can just go ahead and fix them up and call this done.


* source files match upstream.  sha256sum:
   5420b0e558a69a53b36f2b2c70a69f547e075d98366a585fc80cbbcce1efe368  
   gmp-4.2.4.tar.bz2
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* summary is OK.
* description is OK.
* dist tag is present.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
* license text included in package.
* BuildRequires are proper.
* compiler flags are appropriate.
* %clean is present.
* package builds in mock (rawhide, x86_64).
* package installs properly.
* debuginfo package looks complete.
* rpmlint has acceptable complaints.
* final provides and requires are sane:
  gmp-4.2.4-6.fc11.x86_64.rpm
   libgmp.so.3()(64bit)
   libgmpxx.so.4()(64bit)
   libmp.so.3()(64bit)
   gmp = 4.2.4-6.fc11
   gmp(x86-64) = 4.2.4-6.fc11
  =
   /sbin/ldconfig
   libgcc_s.so.1()(64bit)
   libgcc_s.so.1(GCC_3.0)(64bit)
   libgmp.so.3()(64bit)
   libgmpxx.so.4()(64bit)
   libmp.so.3()(64bit)
   libstdc++.so.6()(64bit)
   libstdc++.so.6(CXXABI_1.3)(64bit)
   libstdc++.so.6(GLIBCXX_3.4)(64bit)
   libstdc++.so.6(GLIBCXX_3.4.11)(64bit)

  gmp-devel-4.2.4-6.fc11.x86_64.rpm
   gmp-devel = 4.2.4-6.fc11
   gmp-devel(x86-64) = 4.2.4-6.fc11
  =
   /bin/sh
   /sbin/install-info
   gmp = 4.2.4-6.fc11
   libgmp.so.3()(64bit)
   libgmpxx.so.4()(64bit)
   libmp.so.3()(64bit)

  gmp-static-4.2.4-6.fc11.x86_64.rpm
   gmp-static = 4.2.4-6.fc11
   gmp-static(x86-64) = 4.2.4-6.fc11
  =
   gmp-devel = 4.2.4-6.fc11

* %check is present and all tests pass:
   All 9 tests passed
   All 10 tests passed
   All 56 tests passed
   All 11 tests passed
   All 26 tests passed
   All 7 tests passed
   All 3 tests passed
   All 14 tests passed
   All 3 tests passed

* shared libraries are installed:
  ldconfig is called properly.
  unversioned .so links are in the -devel package.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
X README file is duplicated in %files.
* file permissions are appropriate.
* scriptlets are OK (ldconfig, info installation)
* code, not content.
* headers are in the -devel package.
* no pkgconfig files.
* static libraries are in a separate -static package.
* no libtool .la files.
Comment 26 Ivana Varekova 2009-11-27 07:47:19 EST
Thanks - fixed in gmp-4.3.1-6.fc13
Comment 27 Laurent Rineau 2010-01-05 04:48:20 EST
(Same as comment #12)

Why has this bug been closed? As far as I know, the review process is stalled and not complete.
Comment 28 Jason Tibbitts 2010-01-06 15:48:33 EST
Definitely should not be closed.  I will do my best to have another look at this soon.
Comment 29 Jason Tibbitts 2010-01-06 21:17:52 EST
I checked current CVS.  Things look good; the only thing I can see currently is that I don't really understand the need for the conditional in the devel scriptlets:

if [ -f %{_infodir}/gmp.info.gz ]; then
    /sbin/install-info %{_infodir}/gmp.info.gz %{_infodir}/dir || :
fi

I guess the idea is to protect against an --excludedocs install, but that's not necessary as the recommended scriptlet (just the second line) handles that case just fine. In any case, it's only four pointless lines and this has gone on long enough, so I guess we're done.
Comment 30 Ivana Varekova 2010-01-11 02:59:56 EST
The closing was prematured - so sorry for that. And thanks for the review. I will look for the scriplets and fix the problem.

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