Bug 251529 - Review Request: armacycles-ad - A lightcycle game in 3D
Review Request: armacycles-ad - A lightcycle game in 3D
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Hans de Goede
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-08-09 11:37 EDT by Jon Ciesla
Modified: 2008-01-18 12:45 EST (History)
3 users (show)

See Also:
Fixed In Version: 0.2.8.2.1-5.fc7
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-09-07 13:19:02 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
hdegoede: fedora‑review+
kevin: fedora‑cvs-


Attachments (Terms of Use)
Updated specfile (4.80 KB, text/plain)
2007-08-16 14:33 EDT, Hans de Goede
no flags Details

  None (edit)
Description Jon Ciesla 2007-08-09 11:37:23 EDT
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-1.final.1.fc7.src.rpm
Description: In this game you ride a lightcycle; that is a sort of motorbike that
cannot be stopped and leaves a wall behind it. The main goal of the game
is to make your opponents' lightcycles crash into a wall while avoiding
the same fate.
The focus of the game lies on the multiplayer mode, but it provides
challanging AI opponents for a quick training match.

This is Armagetron Advanced, rebranded as approved by upstream and spot.
Comment 1 Hans de Goede 2007-08-09 14:08:50 EDT
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.
Comment 2 Jon Ciesla 2007-08-09 14:13:32 EDT
Corrected both.  Oops! :)
Comment 3 Jon Ciesla 2007-08-09 15:31:18 EDT
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.
Comment 4 Hans de Goede 2007-08-10 17:49:28 EDT
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


 
Comment 5 Hans de Goede 2007-08-10 17:53:36 EDT
erm correction, make the hint about not using /usr/share games:
hint try using the --disable-games configure argument
Comment 6 Hans de Goede 2007-08-10 18:05:41 EDT
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).

Comment 7 Jon Ciesla 2007-08-14 14:21:41 EDT
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?
Comment 8 Jon Ciesla 2007-08-14 14:59:39 EDT
Ooh, made some progress, hang on. . .
Comment 9 Jon Ciesla 2007-08-16 12:42:47 EDT
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
Comment 10 Hans de Goede 2007-08-16 14:33:07 EDT
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!
Comment 11 Jon Ciesla 2007-08-31 22:11:26 EDT
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
Comment 12 Hans de Goede 2007-09-01 04:05:15 EDT
The latest version looks fine, approved!
Comment 13 Jon Ciesla 2007-09-04 10:27:34 EDT
Even with no initscripts for -dedicated, or should I add them later?  I suppose
it is usable without them.
Comment 14 Jon Ciesla 2007-09-04 10:35:20 EDT
New Package CVS Request
=======================
Package Name: armcycles-ad
Short Description: A lightcycle game in 3D
Owners: limb@jcomserv.net
Branches: FC-6 F-7
InitialCC: 
Cvsextras Commits:
Comment 15 Jon Ciesla 2007-09-04 10:36:01 EDT
Whoops, bad package name, corrected:

New Package CVS Request
=======================
Package Name: armacycles-ad
Short Description: A lightcycle game in 3D
Owners: limb@jcomserv.net
Branches: FC-6 F-7
InitialCC: 
Cvsextras Commits:
Comment 16 Hans de Goede 2007-09-04 13:30:34 EDT
(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.
Comment 17 Kevin Fenzi 2007-09-04 15:02:00 EDT
Please use your FAS name for Owner fields for any further requests. 

cvs done. 
Comment 18 Fedora Update System 2007-09-07 13:19:00 EDT
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.
Comment 19 Jon Ciesla 2008-01-18 07:46:00 EST
Package Change Request
======================
Package Name: armacycles-ad
Updated Fedora CC: z-man@users.sourceforge.net
Comment 20 Kevin Fenzi 2008-01-18 12:45:49 EST
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. 

Note You need to log in before you can comment on or make changes to this bug.