Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
Summary: Review Request: ecm - Elliptic Curve Method for Integer Factorization
https://bugzilla.redhat.com/show_bug.cgi?id=473330
Summary: Review Request: ecm - Elliptic Curve Method for Integer Factorization Product: Fedora Version: rawhide Platform: All OS/Version: Linux Status: NEW Severity: medium Priority: medium Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: konrad@tylerc.org QAContact: extras-qa@fedoraproject.org CC: notting@redhat.com, fedora-package-review@redhat.com Estimated Hours: 0.0 Classification: Fedora
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 Description: Programs and libraries employing elliptic curve method for factoring integers (with GMP for arbitrary precision integers).
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=473330
--- Comment #1 from manuel wolfshant wolfy@nobugconsulting.ro 2008-11-28 15:41:48 EDT --- 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 %{_libdir}/lib%{name}.a
You should do the exact opposite ( http://fedoraproject.org/wiki/Packaging/Guidelines#Exclusion_of_Static_Libra... ), 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?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=473330
--- Comment #2 from manuel wolfshant wolfy@nobugconsulting.ro 2008-11-28 15:48:40 EDT --- You should also use a different name for the package, see https://fedoraproject.org/wiki/PackageMaintainers/Packaging_Tricks#Use_of_co... 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=473330
--- Comment #3 from Conrad Meyer konrad@tylerc.org 2008-11-28 16:03:22 EDT --- 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!
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=473330
--- Comment #4 from Conrad Meyer konrad@tylerc.org 2008-11-28 16:11:17 EDT --- 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=473330
--- Comment #5 from manuel wolfshant wolfy@nobugconsulting.ro 2008-11-28 16:16:58 EDT --- The name of the tarball is not that relevant as the name of the project is.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=473330
Conrad Meyer konrad@tylerc.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Summary|Review Request: ecm - |Review Request: gmp-ecm - |Elliptic Curve Method for |Elliptic Curve Method for |Integer Factorization |Integer Factorization
--- Comment #6 from Conrad Meyer konrad@tylerc.org 2008-11-28 19:22:26 EDT --- New spec, srpm: http://konradm.fedorapeople.org/fedora/SPECS/gmp-ecm.spec http://konradm.fedorapeople.org/fedora/SRPMS/gmp-ecm-6.2.1-2.fc9.src.rpm
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=473330
manuel wolfshant wolfy@nobugconsulting.ro changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED AssignedTo|nobody@fedoraproject.org |wolfy@nobugconsulting.ro Flag| |fedora-review?
--- Comment #7 from manuel wolfshant wolfy@nobugconsulting.ro 2008-11-29 09:01:18 EDT --- 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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=473330
--- Comment #8 from manuel wolfshant wolfy@nobugconsulting.ro 2008-11-29 09:03:18 EDT --- 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).
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=473330
--- Comment #9 from Conrad Meyer konrad@tylerc.org 2008-11-29 16:54:03 EDT --- New spec, srpm: http://konradm.fedorapeople.org/fedora/SPECS/gmp-ecm.spec http://konradm.fedorapeople.org/fedora/SRPMS/gmp-ecm-6.2.1-3.fc9.src.rpm
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=473330
--- Comment #10 from manuel wolfshant wolfy@nobugconsulting.ro 2008-11-30 11:23:28 EDT --- Package Review ==============
Key: - = N/A x = Check ! = Problem ? = Not evaluated
=== REQUIRED ITEMS === [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.
=== SUGGESTED ITEMS === [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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=473330
--- Comment #11 from Conrad Meyer konrad@tylerc.org 2008-11-30 13:11:31 EDT --- (In reply to comment #10)
- 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: http://konradm.fedorapeople.org/fedora/SPECS/gmp-ecm.spec http://konradm.fedorapeople.org/fedora/SRPMS/gmp-ecm-6.2.1-4.fc9.src.rpm
(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.)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=473330
manuel wolfshant wolfy@nobugconsulting.ro changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-review? |fedora-review+
--- Comment #12 from manuel wolfshant wolfy@nobugconsulting.ro 2008-12-04 04:32:51 EDT --- no issues left, APPROVED
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=473330
Conrad Meyer konrad@tylerc.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag| |fedora-cvs?
--- Comment #13 from Conrad Meyer konrad@tylerc.org 2008-12-04 06:00:50 EDT --- 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 InitialCC:
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=473330
Kevin Fenzi kevin@tummy.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-cvs? |fedora-cvs+
--- Comment #14 from Kevin Fenzi kevin@tummy.com 2008-12-05 00:25:26 EDT --- cvs done.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=473330
Conrad Meyer konrad@tylerc.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Resolution| |RAWHIDE
--- Comment #15 from Conrad Meyer konrad@tylerc.org 2008-12-05 12:44:32 EDT --- Built in rawhide -- http://koji.fedoraproject.org/koji/taskinfo?taskID=981766 . Closing.
package-review@lists.fedoraproject.org
-
Red Hat Bugzilla