[Bug 251529] Review Request: armacycles-ad - A lightcycle game in 3D

[bugzilla at redhat.com]

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 at hhs.nl changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
         AssignedTo|nobody at fedoraproject.org    |j.w.r.degoede at hhs.nl
               Flag|                            |fedora-review?




------- Additional Comments From j.w.r.degoede at hhs.nl  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.