Bug 245431 - Review Request: libtommath - portable number theoretic multiple-precision integer library
Review Request: libtommath - portable number theoretic multiple-precision in...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 245432
  Show dependency treegraph
 
Reported: 2007-06-23 01:32 EDT by Jeremy Hinegardner
Modified: 2013-06-05 10:03 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-07-10 00:37:58 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
tibbs: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Jeremy Hinegardner 2007-06-23 01:32:25 EDT
Spec URL: http://jeremy.hinegardner.org/fedora/libtommath.spec
SRPM URL: http://jeremy.hinegardner.org/fedora/libtommath-0.41-1.fc7.src.rpm
Description: LibTomMath is a free open source portable number theoretic
multiple-precision integer library written entirely in C. (phew!) The
library is designed to provide a simple to work with API that provides
fairly efficient routines that build out of the box without
configuration.
Comment 1 Jason Tibbitts 2007-06-23 13:16:07 EDT
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.
Comment 2 Jeremy Hinegardner 2007-06-23 23:31:44 EDT
(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


Comment 3 Ralf Corsepius 2007-06-26 01:58:37 EDT
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).
Comment 4 Jeremy Hinegardner 2007-06-26 02:38:25 EDT
(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.

Comment 5 Jeremy Hinegardner 2007-06-29 03:48:33 EDT
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
Comment 6 Jason Tibbitts 2007-06-29 15:53:01 EDT
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
Comment 7 Jeremy Hinegardner 2007-06-29 23:08:55 EDT
(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!
Comment 8 Jeremy Hinegardner 2007-06-29 23:13:15 EDT
New Package CVS Request
=======================
Package Name: libtommath
Short Description:  portable number theoretic multiple-precision integer library
Owners: jeremy@hinegardner.org
Branches: F-7
InitialCC: 
Comment 9 Kevin Fenzi 2007-07-02 14:47:25 EDT
cvs done.
Comment 10 Jason Tibbitts 2007-07-09 21:07:51 EDT
This has already been built; please don't forget to close this ticket.
Comment 11 Simone Caronni 2013-06-05 09:48:10 EDT
Package Change Request
======================
Package Name: libtommath
New Branches: el6
Owners: slaanesh
Comment 12 Jon Ciesla 2013-06-05 10:03:53 EDT
Git done (by process-git-requests).

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