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=769958 --- Comment #1 from Jerry James <loganjerry@xxxxxxxxx> 2011-12-22 18:39:17 EST --- Created attachment 549255 --> https://bugzilla.redhat.com/attachment.cgi?id=549255 Patch to fix printf on 64-bit systems John, as discussed in email, I'll sponsor you. I have a few minor preliminary comments, and one showstopper to bring up before doing a full review. First the showstopper: the license. The text you quote in eqp-09e-license.patch does NOT say that the authors are abandoning copyright, which is what a public domain declaration amounts to. It merely says they want to make eqp available to everyone, which they have done by putting it on their web site. For Fedora's purposes, we need to know that they are granting the rights to redistribute, and to modify. That can probably be read into the "with no restrictions" clause, but I'm not comfortable making that judgment myself. You need to either: (1) get the authors to clarify the rights they are granting, preferably with a standard license; or (2) write to fedora-legal-list@xxxxxxxxxx and see if this statement counts as "Freely redistributable without restriction"; see https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#.22Distributable.22. I strongly encourage you to try (1) before (2). Now for some minor stuff. The first line of %install and the entire %clean script are not needed in Fedora; rpm automatically removes the build root in both cases. Likewise, the %defattr in %files is now automatic in Fedora. They are still needed for RHEL, though, so if you intend to share the spec file between the two distributions, then just leave it as is. I'll note that I haven't seen the [ "$RPM_BUILD_ROOT" != "/" ] part for years; wow, that takes me back to the time when RPM_BUILD_ROOT *was* "/" and I trashed my system while trying to build an rpm. I can laugh now, but it wasn't so funny then.... Anyway, rpm hasn't allowed RPM_BUILD_ROOT to be "/" for years, so that part is superfluous, even on RHEL. The script examples/go pulls in /bin/csh as a dependency. This is a no-no for documentation; see https://fedoraproject.org/wiki/Packaging:Guidelines#Documentation. You either need to remove the executable bits from that script, drop the script from %doc altogether, or create a separate subpackage for the examples, with that script in %{_bindir} (suitably renamed, of course). The patch to use long int on 64 bit systems is being applied whether the system is 64 bit or not. On 32 bit systems, this leads to warnings like these: unify.c: In function 'print_unify_mem': unify.c:142:236: warning: format '%ld' expects argument of type 'long int', but argument 3 has type 'unsigned int' [-Wformat] See the attached patch for an alternate approach that should work on both 32 and 64 bit systems. Also, each patch needs a comment about its upstream status; see https://fedoraproject.org/wiki/Packaging:Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment. This is totally up to you, but you could eliminate eqp-09e-makefile.patch by invoking make like this: make CFLAGS="$RPM_OPT_FLAGS -DTP_RUSAGE" eqp and then installing like this: install -m 755 eqp%{version} ${RPM_BUILD_ROOT}%{_bindir}/eqp That would be one less patch to put an upstream status comment on. -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the bug. _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review