Bug 225809

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

[ "$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.

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

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.

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.

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

On BZ #248363 a review for a separate mpfr package was started.

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

Package Change Request
======================
Package Name: gmp: 
Updated Fedora Owners: varekova

- change %{dist} to %{?dist} 
- consider "make check" after "make all"

ping?

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?

I don't believe this should have been closed.  Nobody has approved it yet, and I
believe that the current package has outstanding issues.

You are right, it should be open. I will go through it now. Thanks.

Could anybody grab this review and write the list of "should be fixed" things here?

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

Split out the static libs from *-devel into a *-static package or (if appropriate)
abandon shipping them.

Thanks for both comments the problems are fixed in gmp-4.2.2-5.fc9 - (the
license tag is LGPLv3+).

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.

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.

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)

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.

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.

tibbs: ping?

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.

Thanks - fixed in gmp-4.3.1-6.fc13

(Same as comment #12)

Why has this bug been closed? As far as I know, the review process is stalled and not complete.

Definitely should not be closed.  I will do my best to have another look at this soon.

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.

The closing was prematured - so sorry for that. And thanks for the review. I will look for the scriplets and fix the problem.