Bug 251529

Ugh, you are using the "Tron" trademark in the Summary, please don't, thats
considered user visible. Also since this is going in I think it would be wise to
not use the word Armagetron on the Games SIG page to refer to this review either.

Corrected both.  Oops! :)

Spec URL: http://zanoni.jcomserv.net/fedora/armacycles-ad/armacycles-ad.spec
SRPM URL:
http://zanoni.jcomserv.net/fedora/armacycles-ad/armacycles-ad-0.2.8.2.1-2.final.1.fc7.src.rpm

New build with .desktop and icon, as well as fixed summary.

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


 

erm correction, make the hint about not using /usr/share games:
hint try using the --disable-games configure argument

More stuff:

* files / dirs owned by both the main and dedicated packages:
  /etc/armagetronad
  /usr/share/games/armagetronad/language
  /usr/share/games/armagetronad/resource
  /usr/share/games/armagetronad/scripts
  Please put these in a -common packae and make both packages require the -common 
  package

* dirs which are bogus and must not be shipped:
  /usr/etc
  /usr/share/games/armagetronad/desktop

* the -dedicated package should have a /etc/rc.d/init.d script to stop / start
  it as a server, and run as its own user. See batch/rcd_startstop for 
  inspiration for writing a service start / stop script. Or alternatively you
  could completely forgo the -dedicated sub package, I doubt anyone will use it,
  then you also only have to call configure and make once, can forget about the 
  common package, all in all then this would become a pretty normal package.

  (you could then also start over with /etc/rpmdevtools/spectemplate-minimal as
   basis to make a much better spec).

Started working in issues in #4.
Spec URL: http://zanoni.jcomserv.net/fedora/armacycles-ad/armacycles-ad.spec

No SRPM, because it won't build.  It seems to make a difference which directory
I called the configure script from.  Sorry, I'm an autoconf n00b.  And
--disable-games seems not to work.  Might I need to patch the Makefile?

Ooh, made some progress, hang on. . .

Ok, I've addressed most of #4-#5, and part of #6.  Not sure how to rid my self
of /usr/etc.

Also, for the language, resource and script bits that are duplicated, should I
just put them in /usr/share/armacycles-ad-common/language||resource||scripts and
put symlinks in the main and -dedicated packages?

Still need to work on the init script.

Spec URL: http://zanoni.jcomserv.net/fedora/armacycles-ad/armacycles-ad.spec
SRPM URL:
http://zanoni.jcomserv.net/fedora/armacycles-ad/armacycles-ad-0.2.8.2.1-3.final.1.fc7.src.rpm

Created attachment 161672 [details]
Updated specfile

(In reply to comment #9)
> Ok, I've addressed most of #4-#5, and part of #6.  Not sure how to rid my
self
> of /usr/etc.
> 

Attached is a specfile which fixes /usr/etc and a couple of other things. I
hope you don't mind me doing it this way, thats easier then typing a long list
of Must Fix items.

Notice that rpmlint still complains about some none executable scripts /
scripts without shebang, these need to be fixed too, I wonder if we need to
ship these scripts at all, they seam maintainer oriented and are probably not
needed to play the game.

> Also, for the language, resource and script bits that are duplicated, should
I
> just put them in /usr/share/armacycles-ad-common/language||resource||scripts
and
> put symlinks in the main and -dedicated packages?
> 

Ah, I thought the 2 packages used the same dir under /usr/share, as it turns
out they both have there own dir, its ok to leave things as as.

Last, please remove Version=1.0 from the .desktop, .desktop files should not
contain a Version= field. An also please remove "Applcation;" from the
Categories field in the .desktop file, thats wrong too.


p.s.

Great job in getting the trademark issue cleared, it will be good to have this
in Fedora!

Thanks, I agree completely. :)

Addressed the above, rpmlint is now clean.  Sorry for the delay, life has been
complicated lately.

Spec URL: http://zanoni.jcomserv.net/fedora/armacycles-ad/armacycles-ad.spec
SRPM URL:
http://zanoni.jcomserv.net/fedora/armacycles-ad/armacycles-ad-0.2.8.2.1-5.fc7.src.rpm

The latest version looks fine, approved!

Even with no initscripts for -dedicated, or should I add them later?  I suppose
it is usable without them.

New Package CVS Request
=======================
Package Name: armcycles-ad
Short Description: A lightcycle game in 3D
Owners: limb
Branches: FC-6 F-7
InitialCC: 
Cvsextras Commits:

Whoops, bad package name, corrected:

New Package CVS Request
=======================
Package Name: armacycles-ad
Short Description: A lightcycle game in 3D
Owners: limb
Branches: FC-6 F-7
InitialCC: 
Cvsextras Commits:

(In reply to comment #13)
> Even with no initscripts for -dedicated, or should I add them later?  I suppose
> it is usable without them.

Yes, I was thinking that we actually already have quite a few game servers
without initscripts, and that making fixing that a must is a bit harsh. Still
something worth doing though, but you might want to wait till the final form of
initscripts for F-8 is clear.

Please use your FAS name for Owner fields for any further requests. 

cvs done. 

armacycles-ad-0.2.8.2.1-5.fc7 has been pushed to the Fedora 7 stable repository.  If problems still persist, please make note of it in this bug report.

Package Change Request
======================
Package Name: armacycles-ad
Updated Fedora CC: z-man.net

pkgdb doesn't allow us to add arbitrary email addresses to initialCC. 
The person would need to sign up for a fedora account and then we can add them.. 
I don't see them in the account system off hand, if they are in there, list
their fedora account name.