Bug 245431

The web server serves that spec file as a application/octet-stream instead of
text/plain so you can't just click and view it in a browser.

The URL should be the URL of the upstream web pages describing the project
(probably http://www.libtom.org/?page=features&newsitems=5&whatfile=ltm), not a
pointer to the tarball.  Instead, Source0: should be the complete URL of the
tarball.

This fails to build for me:

RPM build errors:
    File not found by glob:
/var/tmp/libtommath-0.41-1.fc8-root-mockbuild/usr/lib64/*.so.*
    File not found by glob:
/var/tmp/libtommath-0.41-1.fc8-root-mockbuild/usr/lib64/*.so

It looks like the makefile puts the libraries in /usr/lib, not /usr/lib64; it
will need to be fixed.  Unfortunately it just sets LIBPATH, but you could patch
the makefile to wrap that in an ifndef and then pass it on the make install line.

(In reply to comment #1)
> The web server serves that spec file as a application/octet-stream instead of
> text/plain so you can't just click and view it in a browser.

Fixed.

> The URL should be the URL of the upstream web pages describing the project
> (probably http://www.libtom.org/?page=features&newsitems=5&whatfile=ltm), not a
> pointer to the tarball.  Instead, Source0: should be the complete URL of the
> tarball.

Fixed.  Not sure why I mixed that up this time
 
> This fails to build for me:
> 
> RPM build errors:
>     File not found by glob:
> /var/tmp/libtommath-0.41-1.fc8-root-mockbuild/usr/lib64/*.so.*
>     File not found by glob:
> /var/tmp/libtommath-0.41-1.fc8-root-mockbuild/usr/lib64/*.so
> 
> It looks like the makefile puts the libraries in /usr/lib, not /usr/lib64; it
> will need to be fixed.  Unfortunately it just sets LIBPATH, but you could patch
> the makefile to wrap that in an ifndef and then pass it on the make install line.

I created a patch for makefile.shared to allow LIBPATH to be set via the environment and it being set 
via the %{_libdir} from the spec.  I do not have a x86_84 machine, so I can't test that this works 
correctly at this time.

New Spec URL: http://jeremy.hinegardner.org/fedora/libtommath/libtommath.spec
New SPRM URL: http://jeremy.hinegardner.org/fedora/libtommath/libtommath-0.41-2.fc7.src.rpm

Some remarks.

MUSTFIX:

-  Package doesn't acknowledge RPM_OPT_FLAGS.
makefile.shared overrides CFLAGS causing this package to be compiled with
non-standard CFLAGS:
E.g.:
... 
libtool --mode=compile --tag=CC gcc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2
-fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i386
-mtune=generic -fasynchronous-unwind-tables -I./ -Wall -W -Wshadow
-Wsign-compare -O3 -funroll-loops -fomit-frame-pointer   -c -o bn_mp_exch.o
bn_mp_exch.c
...

The -W flags are harmless, the usefulness of -funroll-loops is arguable, 
but -O3 and -fomit-frame-pointer should not be used in rpms.

I would suggest you to remove all -f* flags from makefile.shared or to change
your spec file in such a way these -f*-flags are not being used.

CONSIDER:
- The package's name doesn't match the tarball name. 
Please read: http://fedoraproject.org/wiki/Packaging/NamingGuidelines

- The devel package installs its headers to /usr/include.
A better design would be it to install them to a "per package header
subdirectory" (e.g. /usr/include/tommath or similar).

(In reply to comment #3)
> Some remarks.
> 
> MUSTFIX:
> 
> -  Package doesn't acknowledge RPM_OPT_FLAGS.
> makefile.shared overrides CFLAGS causing this package to be compiled with
> non-standard CFLAGS:
> E.g.:
> ... 
> libtool --mode=compile --tag=CC gcc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2
> -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i386
> -mtune=generic -fasynchronous-unwind-tables -I./ -Wall -W -Wshadow
> -Wsign-compare -O3 -funroll-loops -fomit-frame-pointer   -c -o bn_mp_exch.o
> bn_mp_exch.c
> ...
> 
> The -W flags are harmless, the usefulness of -funroll-loops is arguable, 
> but -O3 and -fomit-frame-pointer should not be used in rpms.
> 
> I would suggest you to remove all -f* flags from makefile.shared or to change
> your spec file in such a way these -f*-flags are not being used.

I'll look into different ways to fix this.

> CONSIDER:
> - The package's name doesn't match the tarball name. 
> Please read: http://fedoraproject.org/wiki/Packaging/NamingGuidelines

Yes, the tarball is ltm, but the project name is libtommath, and the tarball
creates a libtommath directory.  The Guidelines say use the tarball name or the
project name.  Additionnally Gentoo, Debian, PLD distros along with OpenBSD and
FreeBSD all package libtommath as 'libtommath'.  There are other libtom*
libraries, libtomcrypt among them which I also have up for review and will use
the feedback here to help with that package.

> - The devel package installs its headers to /usr/include.
> A better design would be it to install them to a "per package header
> subdirectory" (e.g. /usr/include/tommath or similar).

I'll look into fixing this and seeing how that affects packages that depend on
libtommath too.

I'll have a new spec and srpm up in a couple of days.

New Spec and SRPM available that address:
- making sure that RPM_OPT_FLAGS is honored
- putting the header files in their own package header subdirectory 

SRPM: http://jeremy.hinegardner.org/fedora/libtommath/libtommath-0.41-4.fc7.src.rpm
SPEC: http://jeremy.hinegardner.org/fedora/libtommath/libtommath.spec

Sorry for all the nitpicking; here's a full review:

You usually shouldn't include the package name in the summary.

It's a bit odd to see the buildrequires split like that, but it's not a real issue.

Generally we like to see any available test suites run run if at all possible. 
The "test" and "mtest" makefile targets exist but I have no idea if they're
useful at all for testing.

I don't see that any of these are blockers.

APPROVED

Review:
* source files match upstream:
   8e397fc42a12f520ea50c29e477a8768a6f974af2470636f6f807141c13a240c  
   ltm-0.41.tar.bz2
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
? summary starts with the package name.
* description is OK.
* dist tag is present.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
* license text (grant to public domain) included in package.
* latest version is being packaged.
* BuildRequires are proper.
* compiler flags are appropriate.
* %clean is present.
* package builds in mock (development, x86_64).
* package installs properly
* debuginfo package looks complete.
* rpmlint is silent.
* final provides and requires are sane:
  libtommath-0.41-4.fc8.x86_64.rpm
   libtommath.so.0()(64bit)
   libtommath = 0.41-4.fc8
  =
   /sbin/ldconfig
   libtommath.so.0()(64bit)

  libtommath-devel-0.41-4.fc8.x86_64.rpm
   libtommath-devel = 0.41-4.fc8
  =
   libtommath = 0.41-4.fc8
   libtommath.so.0()(64bit)
* %check is not present.  There are some test-related things in the tarball (the 
   "test" and "mtest" makefile targets) but I have no idea how to run them.
* shared libraries present; ldconfig called as necessary and unversioned .so 
   files are in the -devel subpackage.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* scriptlets are OK (ldconfig)
* code, not content.
* documentation is all in the -devel subpackage.
* %docs are not necessary for the proper functioning of the package.
* headers are in the -devel subpackage.
* no pkgconfig files (unfortunately; it would make the move of the include files 
   easier)
* no static libraries.
* no libtool .la files.

APPROVED

(In reply to comment #6)
> Sorry for all the nitpicking; here's a full review:
> 
> You usually shouldn't include the package name in the summary.

Fixed.

> It's a bit odd to see the buildrequires split like that, but it's not a real
issue.

And here I thought I was being smart because all the tex stuff is only
there to build the developer documentation.


> Generally we like to see any available test suites run run if at all possible. 
> The "test" and "mtest" makefile targets exist but I have no idea if they're
> useful at all for testing.

I took a look, the test and mtest makefile targets  both require the 
statically compiled libtommath.a.  'test' creates a program that is run 
with user interaction and 'mtest' creates a program that runs
infinitely.  I'm betting neither of those would be useful or
appreciated by koji :-).

> I don't see that any of these are blockers.
> 
> APPROVED

Thanks!

New Package CVS Request
=======================
Package Name: libtommath
Short Description:  portable number theoretic multiple-precision integer library
Owners: jeremy
Branches: F-7
InitialCC: 

cvs done.

This has already been built; please don't forget to close this ticket.

Package Change Request
======================
Package Name: libtommath
New Branches: el6
Owners: slaanesh

Git done (by process-git-requests).