Bug 473330
Summary: | Review Request: gmp-ecm - Elliptic Curve Method for Integer Factorization | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Conrad Meyer <cse.cem+redhatbugz> |
Component: | Package Review | Assignee: | manuel wolfshant <manuel.wolfshant> |
Status: | CLOSED RAWHIDE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | fedora-package-review, notting |
Target Milestone: | --- | Flags: | manuel.wolfshant:
fedora-review+
kevin: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2008-12-05 17:44:32 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: |
Description
Conrad Meyer
2008-11-27 19:44:44 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 %{_libdir}/lib%{name}.a 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? 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. 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! 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. The name of the tarball is not that relevant as the name of the project is. 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 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 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). 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 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 (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: 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.) no issues left, APPROVED 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: cvs done. Built in rawhide -- http://koji.fedoraproject.org/koji/taskinfo?taskID=981766 . Closing. |