Bug 817981 - Review Request: ratpoints - Find rational points on hyperelliptic curves
Review Request: ratpoints - Find rational points on hyperelliptic curves
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Jerry James
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-01 20:26 EDT by Paulo Andrade
Modified: 2012-05-19 15:37 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-05-19 15:37:43 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
loganjerry: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Paulo Andrade 2012-05-01 20:26:54 EDT
Spec URL: http://kenobi.mandriva.com/~pcpa/ratpoints.spec
SRPM URL: http://kenobi.mandriva.com/~pcpa/ratpoints-2.1.3-1.fc16.src.rpm
Description: Ratpoints is a program that uses an optimized quadratic sieve
algorithm in order to find rational points on hyperelliptic curves.
Comment 1 Jerry James 2012-05-04 11:10:16 EDT
I'll take this review.  Here are a some initial comments in advance of a full review.

You don't need "BuildRequires: gzip".  See https://fedoraproject.org/wiki/Packaging:Guidelines#Exceptions_2.  Also, is help2man actually used in the build?  It doesn't seem to be, in which case you can also drop "BuildRequires: help2man".

Use %global instead of %define: see https://fedoraproject.org/wiki/Packaging:Guidelines#.25global_preferred_over_.25define.

Unless you plan to build this package for EPEL 5, you can remove the defattr() statements from the %files sections.  Those statements became optional with the release of rpm 4.4.

Looking at the source code shows that USE_SSE really means that SSE2 instructions can be emitted into the binary.  This is fine for x86_64, but is not fine for %{ix86}.  Some CPUs in that class do not support SSE2.  You have 2 options for that platform: (1) don't build with SSE2 support at all, or (2) build 2 shared libraries, one without SSE2 support in %{_libdir}, and one with SSE2 support in %{_libdir}/sse2.  On CPUs with SSE2 support, ld.so will pick up the library in %{_libdir}/sse2 at runtime.  You can see examples of doing this in the atlas, gmp, gmp-ecm, and m4ri packages, among others.
Comment 2 Paulo Andrade 2012-05-04 21:10:24 EDT
  I had updated the spec to handle sse in %{ix86} like this, in %build:

%ifarch %{ix86}
mkdir sse2
mv -f libratpoints.so.%{major} sse2
rm -f libratpoints.so
make clean
make %{?_smp_mflags} CCFLAGS="%{optflags}"
%endif

in %install

%ifarch %{ix86}
mv sse2 $RPM_BUILD_ROOT%{_libdir}
%endif

and in %files

%ifarch %{ix86}
%{_libdir}/sse2/libratpoints.so.%{major}
%endif

and also added to %changelog
- Build with and without sse2 support on ix86.


but, then, after ensuring everything was correct in a koji --scratch build,
I did a last review, to find this in ratpoints-2.1.3/rp-private.h:

/* Check if SSE instructions can be used.
   We assume that one SSE word of 128 bit is two long's,
   so check that a long is 64 bit = 8 byte.
*/
#ifndef __SSE2__
#undef USE_SSE
#endif
#if __WORDSIZE != 64
#undef USE_SSE
#endif


I failed :-) So I changed to only enable sse2 on x86_64

Updated spec and srpm at:

Spec URL: http://kenobi.mandriva.com/~pcpa/ratpoints.spec
SRPM URL: http://kenobi.mandriva.com/~pcpa/ratpoints-2.1.3-2.fc16.src.rpm
Comment 3 Jerry James 2012-05-07 13:25:34 EDT
Just a few more issues, I think.  The build system is adding -fomit-frame-pointer to the compiler flags, which should not be done: https://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags.

Also, the shared library has a number of undefined non-weak symbols which would be satisfied by linking with libgmp, as well as two symbols (ceil and floor) that would be satisfied by linking with libm.  To see this, install ratpoints, then run "rpmlint ratpoints".  In addition, the shared library is not linked with RPM_LD_FLAGS, which enables partial relro.

To fix all of the above, add this to %prep and drop CCFLAGS="..." from the make invocation in %build:

sed -e "s/-Wall -O2 -fomit-frame-pointer/%{optflags} %{use_sse}/" \
    -e "s/-shared/& $RPM_LD_FLAGS -lgmp -lm/" \
    -i Makefile

Also, gpl-2.0.txt should be in %doc: https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text.

Finally, is there any reason that multi-lib would be helpful?  That's when, on x86_64 for example, you can install both i686 and x86_64 versions of the library.  (You only get the x86_64 version of the binary, though.)  If that would be useful, then you should put the binary and the library into separate packages.  This can be done either way: the main package contains the library and a subpackage (say, -tools) contains the binary, or the main package contains the binary and a subpackage (say, -libs) contains the library.  In either case, make sure the binary subpackage requires the library subpackage.  Catering to multi-lib is not required.  I'm just bringing it up in case you consider it a good thing.
Comment 4 Paulo Andrade 2012-05-07 23:39:25 EDT
Many thanks for the review.
I added verbatim your suggestions to correct the issues you pointed out
in the Makefile. Also added gpl-2.0.txt to the main package.

I agree about the point of multilib, besides I myself do not use it and
always purge ix86 rpms :-)

I think I would go with the -tools suggestion. But I am used to do it
in Mandriva by creating a library package, in this case, it would be:

ratpoints
libratpoints0
libratpoints-devel

but in Mandriva I had packaged all in one, because there is only one
header, one small library and one small binary...

I think better to keep things simple for now :-) As a lot of other
things would break if pretending to install sagemath i686 and x86_64
in the same system.


rpmlint output:

$ rpmlint SRPMS/ratpoints-2.1.3-3.fc16.src.rpm RPMS/x86_64/ratpoints-*
ratpoints.src: W: spelling-error Summary(en_US) hyperelliptic -> hyper elliptic, hyper-elliptic, hypercritical
ratpoints.src: W: spelling-error %description -l en_US hyperelliptic -> hyper elliptic, hyper-elliptic, hypercritical
ratpoints.x86_64: W: spelling-error Summary(en_US) hyperelliptic -> hyper elliptic, hyper-elliptic, hypercritical
ratpoints.x86_64: W: spelling-error %description -l en_US hyperelliptic -> hyper elliptic, hyper-elliptic, hypercritical
ratpoints-devel.x86_64: W: no-documentation
4 packages and 0 specfiles checked; 0 errors, 5 warnings.

and rpmlint on installed package:

$ rpmlint ratpoints ratpoints-devel
ratpoints.x86_64: W: spelling-error Summary(en_US) hyperelliptic -> hyper elliptic, hyper-elliptic, hypercritical
ratpoints.x86_64: W: spelling-error %description -l en_US hyperelliptic -> hyper elliptic, hyper-elliptic, hypercritical
ratpoints.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libratpoints.so.0 linux-vdso.so.1
ratpoints-devel.x86_64: W: no-documentation
2 packages and 0 specfiles checked; 0 errors, 4 warnings.

also tested a koji --scratch build and it finishes without errors.


Updated spec and srpm at:

Spec URL: http://kenobi.mandriva.com/~pcpa/ratpoints.spec
SRPM URL: http://kenobi.mandriva.com/~pcpa/ratpoints-2.1.3-3.fc16.src.rpm
Comment 5 Jerry James 2012-05-08 16:38:24 EDT
All MUST items are satisfied, so this package is APPROVED.  If you decide to address the 2 SHOULD items below, just do so before you import the package into git.

Package Review
==============

Key:
- = N/A
x = Pass
! = Fail
? = Not evaluated



==== C/C++ ====
[x]: MUST Header files in -devel subpackage, if present.
[x]: MUST ldconfig called in %post and %postun if required.
[x]: MUST Package does not contain any libtool archives (.la)
[x]: MUST Package does not contain kernel modules.
[x]: MUST Package contains no static executables.
[x]: MUST Rpath absent or only used for internal libs.
[x]: MUST Package is not relocatable.
[x]: MUST Development (unversioned) .so files in -devel subpackage, if
     present.


==== Generic ====
[x]: MUST Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: MUST Package successfully compiles and builds into binary rpms on at
     least one supported primary architecture.
[x]: MUST %build honors applicable compiler flags or justifies otherwise.
[x]: MUST All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[x]: MUST Buildroot is not present
     Note: Unless packager wants to package for EPEL5 this is fine
[x]: MUST Package contains no bundled libraries.
[x]: MUST Changelog in prescribed format.
[x]: MUST Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: Clean would be needed if support for EPEL is required
[x]: MUST Sources contain only permissible code or content.
[x]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: Note: defattr macros not found. They would be needed for EPEL5
[x]: MUST Macros in Summary, %description expandable at SRPM build time.
[x]: MUST Package requires other packages for directories it uses.
[x]: MUST Package uses nothing in %doc for runtime.
[x]: MUST Package is not known to require ExcludeArch.
[x]: MUST Permissions on files are set properly.
[x]: MUST Package does not contain duplicates in %files.
[x]: MUST Fully versioned dependency in subpackages, if present.
[x]: MUST Spec file lacks Packager, Vendor, PreReq tags.
[x]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf would be needed if support for EPEL5 is required
[x]: MUST Large documentation files are in a -doc subpackage, if required.
[x]: MUST If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %doc.
[x]: MUST License field in the package spec file matches the actual license.
[x]: MUST License file installed when any subpackage combination is installed.
[x]: MUST Package consistently uses macros (instead of hard-coded directory
     names).
[x]: MUST Package is named according to the Package Naming Guidelines.
[x]: MUST Package does not generate any conflict.
[x]: MUST Package obeys FHS, except libexecdir and /usr/target.
[x]: MUST Package must own all directories that it creates.
[x]: MUST Package does not own files or directories owned by other packages.
[x]: MUST Package installs properly.
[x]: MUST Requires correct, justified where necessary.
[!]: MUST Rpmlint output is silent.

rpmlint ratpoints-debuginfo-2.1.3-3.fc18.i686.rpm

1 packages and 0 specfiles checked; 0 errors, 0 warnings.


rpmlint ratpoints-2.1.3-3.fc18.src.rpm

ratpoints.src: W: spelling-error Summary(en_US) hyperelliptic -> hyper elliptic, hyper-elliptic, hypercritical
ratpoints.src: W: spelling-error %description -l en_US hyperelliptic -> hyper elliptic, hyper-elliptic, hypercritical
1 packages and 0 specfiles checked; 0 errors, 2 warnings.


rpmlint ratpoints-2.1.3-3.fc18.i686.rpm

ratpoints.i686: W: spelling-error Summary(en_US) hyperelliptic -> hyper elliptic, hyper-elliptic, hypercritical
ratpoints.i686: W: spelling-error %description -l en_US hyperelliptic -> hyper elliptic, hyper-elliptic, hypercritical
1 packages and 0 specfiles checked; 0 errors, 2 warnings.


rpmlint ratpoints-devel-2.1.3-3.fc18.i686.rpm

ratpoints-devel.i686: W: no-documentation
1 packages and 0 specfiles checked; 0 errors, 1 warnings.


[x]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
/home/jamesjer/817981/ratpoints-2.1.3.tar.gz :
  MD5SUM this package     : 597fee3856ef2f80fffc0a440e926208
  MD5SUM upstream package : 597fee3856ef2f80fffc0a440e926208

[x]: MUST Spec file is legible and written in American English.
[x]: MUST Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: MUST Package contains a SysV-style init script if in need of one.
[x]: MUST File names are valid UTF-8.
[x]: MUST Useful -debuginfo package or justification otherwise.
[x]: SHOULD Reviewer should test that the package builds in mock.
[-]: SHOULD If the source package does not include license text(s) as a
     separate file from upstream, the packager SHOULD query upstream to
     include it.
[x]: SHOULD Dist tag is present.
[x]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin,
     /usr/sbin.
[x]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[x]: SHOULD Package functions as described.
[x]: SHOULD Latest version is packaged.
[x]: SHOULD Package does not include license text files separate from
     upstream.
[x]: SHOULD Patches link to upstream bugs/comments/lists or are otherwise
     justified.
[x]: SHOULD Scriptlets must be sane, if used.
[!]: SHOULD SourceX / PatchY prefixed with %{name}.
     Note: Source0: http://www.mathe2.uni-
     bayreuth.de/stoll/programs/ratpoints-2.1.3.tar.gz
     (ratpoints-2.1.3.tar.gz) Source1: ratpoints.1 (ratpoints.1) Patch0:
     ratpoints-2.1.3-shared.patch (ratpoints-2.1.3-shared.patch)
[x]: SHOULD SourceX is a working URL.
[x]: SHOULD Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: SHOULD Package should compile and build into binary rpms on all supported
     architectures.
[x]: SHOULD %check is present and all tests pass.
[!]: SHOULD Packages should try to preserve timestamps of original installed
     files.
[x]: SHOULD Spec use %global instead of %define.

Issues:
[!]: MUST Rpmlint output is silent.

rpmlint ratpoints-debuginfo-2.1.3-3.fc18.i686.rpm

1 packages and 0 specfiles checked; 0 errors, 0 warnings.


rpmlint ratpoints-2.1.3-3.fc18.src.rpm

ratpoints.src: W: spelling-error Summary(en_US) hyperelliptic -> hyper elliptic, hyper-elliptic, hypercritical
ratpoints.src: W: spelling-error %description -l en_US hyperelliptic -> hyper elliptic, hyper-elliptic, hypercritical
1 packages and 0 specfiles checked; 0 errors, 2 warnings.


rpmlint ratpoints-2.1.3-3.fc18.i686.rpm

ratpoints.i686: W: spelling-error Summary(en_US) hyperelliptic -> hyper elliptic, hyper-elliptic, hypercritical
ratpoints.i686: W: spelling-error %description -l en_US hyperelliptic -> hyper elliptic, hyper-elliptic, hypercritical
1 packages and 0 specfiles checked; 0 errors, 2 warnings.


rpmlint ratpoints-devel-2.1.3-3.fc18.i686.rpm

ratpoints-devel.i686: W: no-documentation
1 packages and 0 specfiles checked; 0 errors, 1 warnings.


See: http://fedoraproject.org/wiki/Packaging/Guidelines#rpmlint

None of the above is a problem, in my opinion.  Just ignore it.

[!]: SHOULD SourceX / PatchY prefixed with %{name}.
     Note: Source0: http://www.mathe2.uni-
     bayreuth.de/stoll/programs/ratpoints-2.1.3.tar.gz
     (ratpoints-2.1.3.tar.gz) Source1: ratpoints.1 (ratpoints.1) Patch0:
     ratpoints-2.1.3-shared.patch (ratpoints-2.1.3-shared.patch)

This is a style issue, so I don't care a whole lot.  If you'd like to prevent
this warning in the future, replace the string "ratpoints" with "%{name}" in
all Source and Patch entries.  You might consider making Source0 look like
this, in fact:

Source0:        http://www.mathe2.uni-bayreuth.de/stoll/programs/%{name}-%{version}.tar.gz

That way, you don't have to change it on every new release.

[!]: SHOULD Packages should try to preserve timestamps of original installed
     files.

Add "-p" to the "install -D -m644 ..." invocation to preserve the timestamp on
the man page.  Again, not a big deal.

Generated by fedora-review 0.1.3
External plugins:
Comment 6 Paulo Andrade 2012-05-08 20:55:22 EDT
  Changed spec to use %{name} and %{version}.

  Moved gpl-2.0.txt file to the -devel package to silence rpmlint.

  Changed hyperelliptic to hyper-elliptic to silence rpmlint.

  Added -p to install of manual page.


  Now I only see rpmlint output of the installed package:

ratpoints.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libratpoints.so.0 linux-vdso.so.1
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

Updated spec and srpm at:

Spec URL: http://kenobi.mandriva.com/~pcpa/ratpoints.spec
SRPM URL: http://kenobi.mandriva.com/~pcpa/ratpoints-2.1.3-4.fc16.src.rpm
Comment 7 Thomas Spura 2012-05-09 07:26:26 EDT
(In reply to comment #6)
>   Moved gpl-2.0.txt file to the -devel package to silence rpmlint.

The license MUST be in the main package, as any package needs to have a license shipped, when it's available. The devel package depends on the main package, so that has one too...
Comment 8 Thomas Spura 2012-05-09 07:27:26 EDT
(In reply to comment #7)
> (In reply to comment #6)
> >   Moved gpl-2.0.txt file to the -devel package to silence rpmlint.
> 
> The license MUST be in the main package, as any package needs to have a license
> shipped, when it's available. The devel package depends on the main package, so
> that has one too...

"Must" in this case, when the main package Requires another subpackage, that contains the license (e.g. a "-common" subpackage), it's fine too.
Comment 9 Ralf Corsepius 2012-05-09 09:05:20 EDT
The "make install" line of the *.spec contains 2 "install"s:
make install LIBDIR=%{_libdir} install DESTDIR=$RPM_BUILD_ROOT

This usually triggers installation twice (I haven't tried building this package).
Comment 10 Jerry James 2012-05-09 10:02:42 EDT
(In reply to comment #6)
>   Moved gpl-2.0.txt file to the -devel package to silence rpmlint.

Thomas is absolutely right here.  Put that back in the main package and ignore rpmlint's complaints.

>   Changed hyperelliptic to hyper-elliptic to silence rpmlint.

If upstream uses the spelling "hyperelliptic", then you should, too.  Ignore rpmlint in this case; it's dictionary does not contain advanced mathematical terms.

With respect to comment 9, good catch, Ralf!  But it doesn't seem to have any bad side effects.  Here's the relevant section from the mock build log:

+ make install LIBDIR=/usr/lib install DESTDIR=/builddir/build/BUILDROOT/ratpoints-2.1.3-3.fc18.i386
install -D -m755 ratpoints /builddir/build/BUILDROOT/ratpoints-2.1.3-3.fc18.i386/usr/bin/ratpoints
install -D -m644 ratpoints.h /builddir/build/BUILDROOT/ratpoints-2.1.3-3.fc18.i386/usr/include/ratpoints.h
install -d /builddir/build/BUILDROOT/ratpoints-2.1.3-3.fc18.i386/usr/lib
install -m755 libratpoints.so.0 /builddir/build/BUILDROOT/ratpoints-2.1.3-3.fc18.i386/usr/lib
ln -s libratpoints.so.0  /builddir/build/BUILDROOT/ratpoints-2.1.3-3.fc18.i386/usr/lib/libratpoints.so
make: Nothing to be done for `install'.
Comment 11 Paulo Andrade 2012-05-10 19:50:19 EDT
  I reverted the technically incorrect changes to silence rpmlint,
and also removed the double install target in the make command.

Now it again shows the warnings about misspelling and no documentation
in the -devel package.

Updated spec and srpm at:

Spec URL: http://fedorapeople.org/~pcpa/ratpoints.spec
SRPM URL: http://fedorapeople.org/~pcpa/ratpoints-2.1.3-5.fc16.src.rpm

Now using fedorapeople.org :-)
Comment 12 Paulo Andrade 2012-05-18 19:31:57 EDT
New Package SCM Request
=======================
Package Name: ratpoints
Short Description: Find rational points on hyperelliptic curves
Owners: pcpa
Branches: 
InitialCC:
Comment 13 Jason Tibbitts 2012-05-19 14:31:59 EDT
Git done (by process-git-requests).
Comment 14 Paulo Andrade 2012-05-19 15:37:43 EDT
Package was successfully built for rawhide.
Since it is a dependency for packages targeted to fedora 18
everything is good :-)

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