Bug 473330 - Review Request: gmp-ecm - Elliptic Curve Method for Integer Factorization
Summary: Review Request: gmp-ecm - Elliptic Curve Method for Integer Factorization
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
Target Milestone: ---
Assignee: manuel wolfshant
QA Contact: Fedora Extras Quality Assurance
Depends On:
TreeView+ depends on / blocked
Reported: 2008-11-27 19:44 UTC by Conrad Meyer
Modified: 2008-12-05 17:44 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Last Closed: 2008-12-05 17:44:32 UTC
Type: ---
manuel.wolfshant: fedora-review+
kevin: fedora-cvs+

Attachments (Terms of Use)

Description Conrad Meyer 2008-11-27 19:44:44 UTC
Spec URL: http://konradm.fedorapeople.org/fedora/SPECS/ecm.spec
SRPM URL: http://konradm.fedorapeople.org/fedora/SRPMS/ecm-6.2.1-1.fc9.src.rpm
Programs and libraries employing elliptic curve method for factoring
integers (with GMP for arbitrary precision integers).

Comment 1 manuel wolfshant 2008-11-28 20:41:48 UTC
 I've just taken a quick glance at a mock build and there are several problems which must be fixed and also some improvements which I recommend
MUSTFIX: according to the build log (and to your %files section), you are not compiling the shared libs, but you do compile static libs:
   checking whether to build shared libraries... no
   checking whether to build static libraries... yes
   %files devel

 You should do the exact opposite ( http://fedoraproject.org/wiki/Packaging/Guidelines#Exclusion_of_Static_Libraries ), unless you have a very clear motif (in which case a note should be added to the spec). Plus, static libs do not go into -devel.rpm but into -static.rpm. If you need both static and shared, then package both.

MUSTFIX ecm-devel should require the base package

RECOMMENDED  please try to preserve the timestamps of the included files. probably
   make install DESTDIR=$RPM_BUILD_ROOT INSTALL="install -p" 
should do

  I have also noticed that configure complains about a missing xsltproc. Is it intentional or a missing BR?

Comment 2 manuel wolfshant 2008-11-28 20:48:40 UTC
You should also use a different name for the package, see https://fedoraproject.org/wiki/PackageMaintainers/Packaging_Tricks#Use_of_common_namespace for details. Taking into consideration upstream's website and the documents bundled in the source tarball (starting with the very first line of README), I think that gmp-ecm is a better suited name.

Comment 3 Conrad Meyer 2008-11-28 21:03:22 UTC
Yes, I thought of gmp-ecm as a name but the tarball goes by ecm. I'm ok with either. I'll fix the problems you mention when I get home tonight. Thanks for spotting the problems!

Comment 4 Conrad Meyer 2008-11-28 21:11:17 UTC
Also Debian's version of this package is also named gmp-ecm. So I am happy to change the name to that when I get home.

Comment 5 manuel wolfshant 2008-11-28 21:16:58 UTC
The name of the tarball is not that relevant as the name of the project is.

Comment 7 manuel wolfshant 2008-11-29 14:01:18 UTC
Looks much better now. However, I suggest to add AUTHORS, ChangeLog, NEWS and TODO to the %doc line of the main package and README.lib in -devel

rpmlint of gmp-ecm gives:
  gmp-ecm.x86_64: E: library-without-ldconfig-postin /usr/lib64/libecm.so.0.0.0
  gmp-ecm.x86_64: E: library-without-ldconfig-postun /usr/lib64/libecm.so.0.0.0
  1 packages and 0 specfiles checked; 2 errors, 0 warnings.
which probably needs fixing, too

Comment 8 manuel wolfshant 2008-11-29 14:03:18 UTC
And last but not least, I strongly suggest to install the binary and the manpage under the name gmp-ecm. See comment #2 why. (Yes, I know that Debian uses ecm, but we are not Debian).

Comment 10 manuel wolfshant 2008-11-30 16:23:28 UTC
Package Review

 - = N/A
 x = Check
 ! = Problem
 ? = Not evaluated

 [x] Package is named according to the Package Naming Guidelines.
 [x] Spec file name must match the base package %{name}, in the format %{name}.spec.
 [x] Package meets the Packaging Guidelines.
 [x] Package successfully compiles and builds into binary rpms on at least one supported architecture.
     Tested on: devel/x86_64
 [x] Rpmlint output:
source RPM: empty
binary RPMs:
  rpmlint of gmp-ecm:
  gmp-ecm.x86_64: W: file-not-utf8 /usr/share/doc/gmp-ecm-6.2.1/AUTHORS
--> please use the same iconv/touch -r trick as on README
  gmp-ecm.x86_64: W: incoherent-version-in-changelog 6.2.1-3 ['6.2.1-2.fc11', '6.2.1-2']
--> you forgot to increase the release tag
  rpmlint of gmp-ecm-devel: empty
  rpmlint of gmp-ecm-static:
  gmp-ecm-static.x86_64: W: no-documentation
--> ignorable
 [x] Package is not relocatable.
 [x] Buildroot is correct (%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n))
 [x] Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines.
 [x] License field in the package spec file matches the actual license.
     License type: LGPLv2+ and GPLv2+
 [x] 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] Spec file is legible and written in American English.
 [x] Sources used to build the package match the upstream source, as provided in the spec URL.
     SHA1SUM of package: bb08c4f1b412110ef64572c387baa5bc45ae8a60 ecm-6.2.1.tar.gz
 [x] Package is not known to require ExcludeArch
 [x] All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines.
 [-] The spec file handles locales properly.
 [x] ldconfig called in %post and %postun if required.
 [x] Package must own all directories that it creates.
 [x] Package requires other packages for directories it uses.
 [x] Package does not contain duplicates in %files.
 [x] Permissions on files are set properly.
 [x] Package has a %clean section, which contains rm -rf $RPM_BUILD_ROOT.
 [x] Package consistently uses macros.
 [x] Package contains code, or permissable content.
 [-] Large documentation files are in a -doc subpackage, if required.
 [x] Package uses nothing in %doc for runtime.
 [x] Header files in -devel subpackage, if present.
 [x] Static libraries in -static subpackage, if present.
 [-] Package requires pkgconfig, if .pc files are present.
 [x] Development .so files in -devel subpackage, if present.
 [!] Fully versioned dependency in subpackages, if present.
 [x] Package does not contain any libtool archives (.la).
 [-] Package contains a properly installed %{name}.desktop file if it is a GUI application.
 [x] Package does not own files or directories owned by other packages.
 [x] Final provides and requires are sane.

 [x] Latest version is packaged.
 [x] Package does not include license text files separate from upstream.
 [-] Description and summary sections in the package spec file contains translations for supported Non-English
 languages, if available.
 [x] Reviewer should test that the package builds in mock.
     Tested on: devel/x86_64
 [?] Package should compile and build into binary rpms on all supported architectures.
     Tested on:
 [x] Package functions as described.
 [x] Scriptlets must be sane, if used.
 [-] The placement of pkgconfig(.pc) files is correct.
 [-] File based requires are sane.
 [x] %check is present and the test passes.

=== Issues ===
1. Please use iconv to convert the AUTHORS file
2. You forgot to increase the release tag (you only modified the changelog). And btw, you did not upload the new src.rpm, I used only the new spec and rebuilt the src.rpm locally.
3. Are you sure that the -static package needs the -devel? I would test, but I am not sure that I know how to do it

Comment 11 Conrad Meyer 2008-11-30 18:11:31 UTC
(In reply to comment #10)
> 3. Are you sure that the -static package needs the -devel? I would test, but I
> am not sure that I know how to do it

Yes. Think about it. Just because you're statically linking against a library doesn't mean you don't need the header files when compiling your code.

New URLs:

(Actually, I did upload the new SRPM last time, it was just to the old URL, because I forgot to bump the Release. I have an automated script to upload specs/srpms/rpms to fedorapeople.)

Comment 12 manuel wolfshant 2008-12-04 09:32:51 UTC
no issues left, APPROVED

Comment 13 Conrad Meyer 2008-12-04 11:00:50 UTC
Thank you very much for the review.

New Package CVS Request
Package Name: gmp-ecm
Short Description: Elliptic Curve Method for Integer Factorization
Owners: konradm
Branches: F-10 F-9

Comment 14 Kevin Fenzi 2008-12-05 05:25:26 UTC
cvs done.

Comment 15 Conrad Meyer 2008-12-05 17:44:32 UTC
Built in rawhide -- http://koji.fedoraproject.org/koji/taskinfo?taskID=981766 . Closing.

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