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.
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.
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
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.
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
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:
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
(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...
(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.
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).
(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'.
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 :-)
New Package SCM Request ======================= Package Name: ratpoints Short Description: Find rational points on hyperelliptic curves Owners: pcpa Branches: InitialCC:
Git done (by process-git-requests).
Package was successfully built for rawhide. Since it is a dependency for packages targeted to fedora 18 everything is good :-)