Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report. Summary: Review Request: armacycles-ad - A lightcycle game in 3D https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=251529 j.w.r.degoede@xxxxxx changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED AssignedTo|nobody@xxxxxxxxxxxxxxxxx |j.w.r.degoede@xxxxxx Flag| |fedora-review? ------- Additional Comments From j.w.r.degoede@xxxxxx 2007-08-10 17:49 EST ------- I tried to do a review on this, I really did, but ... UGH, specfile makes eyes hurt. Can you please clean it up considerably and then resubmit? Here are some things to fix: * Don't write: " # inform automake of the rpm build directory DESTDIR=$RPM_BUILD_ROOT export DESTDIR pushd bindist-dedicated make install popd pushd bindist make install popd " instead write (as all other specs do) : " pushd bindist-dedicated make install DESTDIR=$RPM_BUILD_ROOT popd pushd bindist make install DESTDIR=$RPM_BUILD_ROOT popd " * There is no need to activate the armacycles default sdl audiodriver hack, our SDL already defaults to alsa. So no need to pass CXXFLAGS="-DDEFAULT_SDL_AUDIODRIVER=alsa" * This is a gnu autoconf configure script, so call it using %configure instead of passing all the dir options and CXXFLAGS yourself * please check all the non dir configure options if they are really necessary, for example atleast --disable-restoreold is useless as you don't also pass --enable-multiver (and you don't want to do that either). * even more bogus are the configure options (esp the combination): "--enable-useradd --disable-useradd" ?? * also please do not make (configure) lines wider then 80 chars, please put a \ at the end and continue on the next line * this also is bogus: --enable-automakedefaults --localstatedir=/var Quoting from ./configure --help: --enable-automakedefaults enforce the default installation directories as set by automake. localstatedir=prefix/var violates the FHS, so this is off by default. * looking even more at the configure flags, I notice that they both have: --disable-sysinstall Yet also both specify: --enable-etc --enable-initscript --enable-useradd Which (according to ./configure --help) have no influence when --disable-sysinstall is passed * long story short, it would seem that this is all thats needed for configure: %configure --disable-sysinstall --disable-uninstall --disable-glout resp: %configure --disable-sysinstall --disable-uninstall * these 2 shell variables are not used, please remove them: CLIENTPATH=%{_tmppath}/%{name}-%{version}-root/share/games/armagetronad/ SERVERPATH=%{_tmppath}/%{name}-%{version}-root/share/games/armagetronad-dedicat * Please use macros where ever possible for example do not write: mkdir $RPM_BUILD_ROOT/etc but write: mkdir -p $RPM_BUILD_ROOT/%{_sysconfdir} * please put the description and tags of subpackages at the top of the spec directly after the main package, not between the %files * please use %defattr instead of %attr for each file * please do not use /usr/share/games/armagetronad but /usr/share/armagetronad (hint try using the --enable-games configure argument) * I think you will want to buildrequire libxml2-devel not libxml2 * why patch progtitle into configure, but not progname? why still have progtitle bits in the .spec if its patched in why not just write: export progtitle="Armacycles Advanced" export progname=armacyclesad before the 2 calls to %configure, then the patching is no longer needed -- Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the bug, or are watching someone who is. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review