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=587315 Jussi Lehtola <jussi.lehtola@xxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Flag| |needinfo? --- Comment #5 from Jussi Lehtola <jussi.lehtola@xxxxxx> 2010-05-06 01:24:30 EDT --- - Summary seems to be misspelled: "corewar" vs. "Core War" in %description. - Add a comment about the patch in the spec file. E.g. # Patch to disable stripping of binary in spec file Patch0: pmars-0.9.2-nostrip.patch - Please also get rid of the @ in front of the CC commands in the Makefile, so that the used compiler commands are actually shown. - I suggest making a backup of the patched files, i.e. changing to %patch0 -p1 -b .nostrip - You are using %{buildroot}, so use %{optflags} instead of $RPM_OPT_FLAGS. - The %build line can be written more shortly as make -C src CFLAGS="%{optflags} -DEXT94 -DXWINGRAPHX -DPERMUTATE" Note that here I have dropped the -O flag, which will conflict with the Fedora flags. Also, if you move the CFLAGS definition here, it will override any CFLAGS definition in the makefile. - Please preserve time stamps in %install. Also, you can be more terse: %install rm -rf %{buildroot} install -D -p -m 755 src/pmars %{buildroot}%{_bindir}/pmars install -D -p -m 644 doc/pmars.6 %{buildroot}%{_mandir}/man6/pmars.6 Removing files from the build directory in %install is bad behavior, since then one cannot use a short circuit build. Switching to a temporary directory # Make temporary doc dir rm -rf doc_install cp -a doc doc_install rm doc_install/doc/pmars.6 and using that one in %files will do the trick %doc doc_install/doc/ - Please be more verbose in %files. I abhor statements like %{_mandir}/man6/* since one doesn't really know what gets pulled in. Especially here, when there is only one file. So just %{_mandir}/man6/pmars.6.* will do fine (the file will be compressed by rpm, but the compression format might change in the future). After you have addressed these issues I will perform the full review. -- 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