Bug 441411 - Review Request: adonthell - A 2D graphical RPG game
Review Request: adonthell - A 2D graphical RPG game
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Hans de Goede
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 441415
  Show dependency treegraph
 
Reported: 2008-04-07 17:55 EDT by Mathieu Bridon
Modified: 2008-07-09 09:35 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-07-09 09:35:51 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)

  None (edit)
Description Mathieu Bridon 2008-04-07 17:55:29 EDT
Spec URL: http://dl.free.fr/iWoKmtpoj/adonthell.spec
SRPM URL: http://dl.free.fr/o4pMN1LvT/adonthell-0.3.4-0.6.fc8.src.rpm
(this is a french host server, click on "Télécharger ce fichier" to download the files)
Description:
A 2D, graphical, single player role playing game inspired by good old
console RPGs from the SNES like Secret of Mana or Chrono Trigger.

This package contains the Adonthell engine. You will also need a game
package to play Adonthell. For this release, the official package is
Waste's Edge.

Notes:
1. This is my first package submission :)

2. rpmlint runs fine, except for the adonthell-doc package:
$ rpmlint RPMS/i386/adonthell-doc-0.3.4-0.6.fc8.i386.rpm 
adonthell-doc.i386: E: zero-length /usr/share/doc/adonthell-doc-0.3.4/html/classmapsquare__area__coll__graph.map
(same thing for a few other files)

I am not very familiar with doxygen, so I don't know if those files can be safely removed or not...

3. Something is bugging me: the adonthell-debuginfo package does not depend on the adonthell package. Isn't that a problem? Did I forget something?

4. This is only a game engine, I'll submit a second package for Waste's Edge, which is a game using the Adonthell engine (I'll add a link to the other submission in the comments, might be better if the two were reviewed simultaneously).
Comment 1 Mathieu Bridon 2008-04-07 18:03:46 EDT
This package is needed for the one I submitted in bug 441415 (as I said in the
submission)
Comment 2 Mamoru TASAKA 2008-04-16 15:23:23 EDT
Rebuild failed on dist-f9 (I just tried to rebuild this package and
did not do anything else!)

http://koji.fedoraproject.org/koji/taskinfo?taskID=569167
Comment 3 Mathieu Bridon 2008-04-16 15:32:12 EDT
In fact I did not try to build it on dist-f9 as it wasn't released yet and I
don't have an available machine for tests (not even virtualization).

I'll try to build it in Fedora 9 as soon as I can.

By the way, I thought review for inclusion in F8 and F9 were 2 different
processes, that's why I submitted it after having built it in only F8. Sorry
about that.
Comment 4 Mamoru TASAKA 2008-04-17 02:59:33 EDT
(In reply to comment #3)
> In fact I did not try to build it on dist-f9 as it wasn't released yet and I
> don't have an available machine for tests (not even virtualization).

Rebuild fails also on dist-f8-updates-candidate:
http://koji.fedoraproject.org/koji/taskinfo?taskID=570219
Comment 5 Mathieu Bridon 2008-04-17 16:24:01 EDT
The path for python is hardcoded in the configure script (with /lib, not /lib64)
so it doesn't build on 64 bits system.

This should be fixed with this new spec file:
http://dl.free.fr/jFioGRj4k/adonthell.spec

And btw, this might make it build on F9 as well, but I still can't test it
before next week.
Comment 6 Mamoru TASAKA 2008-04-17 23:01:12 EDT
Please also provide srpm.
And would you place your spec/srpm on the URL from which we
can directly download your files by "wget -N", for example?
Comment 7 Mathieu Bridon 2008-04-21 17:29:14 EDT
I just checked, and the URL I provided for spec/SRPM are the one from which you
can directly download the files. However, when you open it for the first time in
a session, the web host will intercept your request and redirect you to the
download page you saw. Try to open the same URL again, and you will access the
files... Dumb host -_-

I'm really sorry about that, but I do not know about any other free web host. Do
you know one?

Anyway, here is the SRPM:
http://dl.free.fr/fjr1hvFRD/adonthell-0.3.4-0.7.fc8.src.rpm
(still the same host, until I can find a better one... :S)
Comment 8 Mathieu Bridon 2008-04-24 17:46:01 EDT
I finally found someone to host the files (thanks go to TeeWee). Here they are,
more easily accessible :)

SPEC: http://fedora.c-bien.fr/adonthell.spec
SRPM: http://fedora.c-bien.fr/adonthell-0.3.4-0.7.fc8.src.rpm
Comment 9 Mamoru TASAKA 2008-04-25 14:06:32 EDT
Some comments:
* As far as I checked the tarball, the license tag should be "GPLv2+".
* The explicit Requires:
-----------------------------------------------------------------
Requires:       SDL
Requires:       python
Requires:       zlib
Requires:       freetype
Requires:       SDL_mixer
------------------------------------------------------------------
   should all be removed. These library related Reqires are automatically
   checked and added to binary rpms by rpmbuild itself.
* Fedora specific compilation flags (you can check this by
 $ rpm --eval %optflags) are not correctly honored:
   the section "Compiler flags" of
   http://fedoraproject.org/wiki/Packaging/Guidelines
* Please check directory ownership issue
------------------------------------------------------------------
[root@localhost ~]# LANG=C rpm -qf /usr/share/adonthell/modules/adonthell.pyc 
adonthell-0.3.4-0.7.fc9.i386
[root@localhost ~]# LANG=C rpm -qf /usr/share/adonthell
file /usr/share/adonthell is not owned by any package
[root@localhost ~]# LANG=C rpm -qf /usr/share/adonthell/modules/
file /usr/share/adonthell/modules is not owned by any package
------------------------------------------------------------------
  Note:
  When you write
------------------------------------------------------------------
%files
%{_datadir}/%{name}/
------------------------------------------------------------------
  This contains the directory %_datadir/%name itself and
  all files/directories/etc under the directory, while
------------------------------------------------------------------
%files
%dir %{_datadir}/%{name}
------------------------------------------------------------------
  contains the directory %{_datadir}/%name only.
Comment 10 Hans de Goede 2008-05-03 04:42:09 EDT
Hi Mathieu,

Short intro: I'm a long time Fedora contributer and a sponsor. I haven't been
sponsoring anyone in a while as I've been rather busy with other stuff. But
getting new people into Fedora is important so I've decided to make some time
for sponsering again. Most of the things I do within Fedora are gaming related
so I'm very happy to see adonthell getting packaged.

As such I would like to sponsor you. The first step here would be to finish up
the packaging of adonthell and wastesedge so that they are in a state where they
can be approved. The next step then is for you to either package one or two more
packages, for example from these lists:
http://fedoraproject.org/wiki/SIGs/Games/WishList
http://fedoraproject.org/wiki/PackageMaintainers/WishList

Or you can do some reviews of other people packages, when you do this please add
a note that it is not an official review as you aren't a contributer yet.

The purpose here is for you to show a good understanding of the packaging
guidelines. Once you've done a couple of good reviews and / or submitted one or
two more good packages, then you can apply for cvsextras membership in the
account system and I'll sponsor you.

As for the review of this package, Mamoru has done a fine job so far, please fix
all the items he has mentioned in his last comment and then I'll do a full
review pointing out any last issues.
Comment 11 Mamoru TASAKA 2008-05-18 13:12:37 EDT
ping?
Comment 12 Mathieu Bridon 2008-05-18 14:23:06 EDT
Sorry for spending so much time without any answer, I've been battling since
then with the compilation flags.

I think I finally found something out (they were hardcoded in configure.in), but
I now have a problem with the configure not finding python.

Python is in fact in /usr/lib64/python2.5/ on my system (default python install
with yum) and the configure script is looking fot it in
/usr/%{_lib}/python$PYLIBVER, that is /usr/lib64/python (as the env var is not set).

This causes rpmbuild to fail, however, I didn't have this problem on Fedora 9
preview :S

All the problems you noted should be corrected, but this one is really giving me
troubles... I'll post the new spec and srpm when I could fix this.

Sorry for the delay and thanks for being so patient :)
Comment 13 Mathieu Bridon 2008-05-31 15:37:33 EDT
It's been a long time, but here it is.

I worked some time with upstreal dev and we managed to fix some issues in
adonthell, as well as some issues in my own spec. The result is the 0.3.5
release (which also includes lots of bugfixes for non Fedora-related issues).

I think it should now cover what you remarked above.

Here is the spec file: ftp://91.121.156.173/pub/adonthell.spec
Here is the SRPM: ftp://91.121.156.173/pub/adonthell-0.3.5-0.1.fc9.src.rpm

PS: I now have my own host, so you will not have to click through the ads to
find the files ;)
Comment 14 Hans de Goede 2008-05-31 16:16:14 EDT
Looks good, I've done a full review, here are the results:

Must Fix
--------
* Currently you do:
  "sed -i 's|^CFLAGS|^#CFLAGS|g' configure.in"
  Make that:
  "sed -i 's|^CFLAGS|^#CFLAGS|g' configure"

  By modifying configure.in instead of configure, you are causing all
  the autoxxx files to be regenerated and configure being run twice
  once you've changed the sed, you can also drop the BuildRequires libtool. as
  that then won't be needed anymore either.

* Currently in the description you say:
  "inspired by good old console RPGs from the SNES like Secret of Mana or
   Chrono Trigger."

  This contains several references to protected Trademarks, please don't do that
  instaed just write (for example):
  "inspired by good old console RPGs from the 16 bit console gaming era"


Should Fix:
-----------

* The following BuildRequires are redundant and thus should be removed:
  zlib-devel, SDL-devel


So all in all the package is almost good to go, what remains is really easy to fix.

So the next step is to update your wastesedge candidate package to 0.3.5 (I
assume that together with adonthell 0.3.5 there also has been a 0.3.5 wastesedge
release) and then I'll review that next. Also it would help if that could happen
somewhat faster then the times between steps taken in this review.

Comment 15 Mathieu Bridon 2008-05-31 17:41:25 EDT
"By modifying configure.in instead of configure, you are causing all
  the autoxxx files to be regenerated and configure being run twice"
Wow! It's been bugging me a lot and I couldn't understand why it was running twice.

Anyway, fixed the 3 issues.
SPEC: ftp://91.121.156.173/pub/adonthell.spec
SRPM: ftp://91.121.156.173/pub/adonthell-0.3.5-0.2.fc9.src.rpm

About wastesedge, it was not yet released in 0.3.5 but it doesn't matter as it
remains fully compatible (only minor version changing). Translations are being
worked on for Waste's Edge, and upstream wants to wait for those before
releasing it.

I am currently applying all the comments you made here to wastesedge.rpm, so you
might want to wait a little before reviewing it so I can upload new versions of
SPEC and SRPM ;)
Comment 16 Hans de Goede 2008-05-31 17:49:32 EDT
(In reply to comment #15)
> "By modifying configure.in instead of configure, you are causing all
>   the autoxxx files to be regenerated and configure being run twice"
> Wow! It's been bugging me a lot and I couldn't understand why it was running
twice.
> 
> Anyway, fixed the 3 issues.
> SPEC: ftp://91.121.156.173/pub/adonthell.spec
> SRPM: ftp://91.121.156.173/pub/adonthell-0.3.5-0.2.fc9.src.rpm
> 

Looks fine now!

> About wastesedge, it was not yet released in 0.3.5 but it doesn't matter as it
> remains fully compatible (only minor version changing). Translations are being
> worked on for Waste's Edge, and upstream wants to wait for those before
> releasing it.
> 

Ok.

> I am currently applying all the comments you made here to wastesedge.rpm, so you
> might want to wait a little before reviewing it so I can upload new versions of
> SPEC and SRPM ;)

Ok, let me know when you've got a version of wastesedge ready for review.
Comment 17 Mathieu Bridon 2008-06-03 15:01:21 EDT
I added the -p option to the install call, as indicated in the wastesedge
submission.

SPEC: ftp://ks359320.kimsufi.com/pub/adonthell.spec
SRPM: ftp://ks359320.kimsufi.com/pub/adonthell-0.3.5-0.3.fc9.src.rpm
Comment 18 Hans de Goede 2008-06-16 17:29:09 EDT
Sponsored, approved!
Comment 19 Mathieu Bridon 2008-06-16 18:07:22 EDT
New Package CVS Request
=======================
Package Name: adonthell
Short Description: A 2D graphical RPG game
Owners: bochecha
Branches: F-8 F-9
InitialCC: 
Cvsextras Commits: yes
Comment 20 Kevin Fenzi 2008-06-17 12:59:35 EDT
cvs done.

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