Bug 441415 - Review Request: wastesedge - Official game package for Adonthell
Review Request: wastesedge - Official game package for Adonthell
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: 441411
Blocks:
  Show dependency treegraph
 
Reported: 2008-04-07 18:01 EDT by Mathieu Bridon
Modified: 2008-07-09 09:36 EDT (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-07-09 09:36:13 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 18:01:21 EDT
Spec URL: http://dl.free.fr/jLC82iDWw/wastesedge.spec
SRPM URL: 
(this is a french host server, click on "Télécharger ce fichier" to download the files)
Description:
As a loyal servant of the elven Lady Silverhair, you arrive at the remote
trading post of Waste's Edge, where she is engaged in negotiations with the
dwarvish merchant Bjarn Fingolson. But not all is well at Waste's Edge, and
soon you are confronted with circumstances that are about to destroy your
mistress' high reputation. And you are the only one to avert this ...

Notes:
1. This is my first package submission (along with the one in bug 441411).

2. This package depends on adonthell, which I submitted in bug 441411.
Comment 1 Mathieu Bridon 2008-04-07 18:24:44 EDT
Sorry, I forgot the URL for the SRPM. Here it is:
http://dl.free.fr/oTkAOMwRF/wastesedge-0.3.4-0.5.fc8.src.rpm
Comment 2 Mathieu Bridon 2008-04-24 17:47:32 EDT
I've been told in bug 441411 that the web host I'm using is making things
difficult for people to dwonload the files. Here they are, more easily accessible.

SPEC: http://fedora.c-bien.fr/wastesedge.spec
SRPM: http://fedora.c-bien.fr/wastesedge-0.3.4-0.5.fc8.src.rpm
Comment 3 Hans de Goede 2008-05-03 04:42:18 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.

I'll look into reviewing this once adonthell is in an approvable state.
Comment 4 Mathieu Bridon 2008-05-31 20:01:16 EDT
I reworked it a little, trying to take into accounts remarks that were made to
me in adonthell review.

SPEC: ftp://91.121.156.173/pub/wastesedge.spec
SRPM: ftp://91.121.156.173/pub/wastesedge-0.3.4-0.5.fc9.src.rpm
Comment 5 Mathieu Bridon 2008-05-31 21:03:18 EDT
My bad, I uploaded the old SRPM. Here is the good one:
SRPM: ftp://91.121.156.173/pub/wastesedge-0.3.4-0.6.fc9.src.rpm

Spec file was right though, but it's always better to have the two of them in
the same comment:
SPEC: ftp://91.121.156.173/pub/wastesedge.spec
Comment 6 Hans de Goede 2008-06-01 04:47:10 EDT
Full review done, results:

Must Fix
--------

* The GPL version to use does not get specified anywhere, so the correct License
  tag is GPL+ not GPLv2+

* Drop the obsolete Encoding and Version entries from the .desktop file



Should Fix
----------

* Drop the #%files line (why is that there anyways?)

* Add INSTALL="install -p" as argument to make install to preserve the 
  timestamps of the files getting installed

* Install icons in freedesktop.org icon standard location (/usr/share/pixmaps
  is deprecated) :
  mv wastesedge_16x16.xpm /usr/share/icons/hicolor/16x16/apps/wastesedge.xpm
  mv wastesedge_32x32.xpm /usr/share/icons/hicolor/32x32/apps/wastesedge.xpm
  Add a "Requires: hicolor-icon-theme" for icon dirs ownership
  Add ican-cache update scriptlets from:
   http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#GTK.2B_icon_cache
  Change Icon line in .desktop to just "Icon=wastesedge"


One all that is done its time for the next step as described in my initial comment:

"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."

So I would like you to either create one more package, or do 2 good reviews of
other peoples submissions.


Comment 7 Mathieu Bridon 2008-06-01 09:37:58 EDT
About the icons location, I looked for it so I could have discuss this upstream,
and I found this in the Freedektop Icon Theme Spec
(http://standards.freedesktop.org/icon-theme-spec/latest/ar01s03.html):
> Icons and themes are looked for in a set of directories. By default, apps
> should look in $HOME/.icons (for backwards compatibility), in
> $XDG_DATA_DIRS/icons and in /usr/share/pixmaps (in that order).

Are you sure it has been deprecated ? Do you have another link explaining it so
I can sell this point upstream for future release ?

Another question, you said:
>  mv wastesedge_16x16.xpm /usr/share/icons/hicolor/16x16/apps/wastesedge.xpm
>  mv wastesedge_32x32.xpm /usr/share/icons/hicolor/32x32/apps/wastesedge.xpm

So I assume I should do that in a %post section, is that the preferred method
over simply patching the Makefile so that it installs in the right place ?

I corrected the "install -p" point, but you didn't make this remark in my other
submission. Is there a reason why I should do this in this package and not in
the other ? Or should I correct it in adonthell too ?
Comment 8 Hans de Goede 2008-06-01 15:17:53 EDT
(In reply to comment #7)
> About the icons location, I looked for it so I could have discuss this upstream,
> and I found this in the Freedektop Icon Theme Spec
> (http://standards.freedesktop.org/icon-theme-spec/latest/ar01s03.html):
> > Icons and themes are looked for in a set of directories. By default, apps
> > should look in $HOME/.icons (for backwards compatibility), in
> > $XDG_DATA_DIRS/icons and in /usr/share/pixmaps (in that order).
> 
> Are you sure it has been deprecated ? Do you have another link explaining it so
> I can sell this point upstream for future release ?
> 

Well I don't have a document saying so, but /usr/share/pixmaps is the old way of
installing icons. It does not allow for theming, nor does it allow installing
multiple sizes of an icon in such a way that a using application (like the
gnome/kde/xfce applications menu) can automatically use the correct size
depending upon the size it needs.

With the /usr/share/icons way, if you have a large panel and place a launcher
there you will automatically get the more detailed 32x32 icon, with the
/usr/share/pixmaps way you are stuck to whatever size you've hardcoded in the
.desktop file (as the size now is encoded into the name you've specified in the
.desktop file).

But as said this is a should, so if you feel this is not worth the trouble you
do not have todo this.

> Another question, you said:
> >  mv wastesedge_16x16.xpm /usr/share/icons/hicolor/16x16/apps/wastesedge.xpm
> >  mv wastesedge_32x32.xpm /usr/share/icons/hicolor/32x32/apps/wastesedge.xpm
> 
> So I assume I should do that in a %post section, is that the preferred method
> over simply patching the Makefile so that it installs in the right place ?
> 

Indeed it is not necessary to patch the makefiles, but using things like mv in
%post is a really bad idea, just move the files to the new location in
$RPM_BUILD_ROOT in %install (after calling "make install")

> I corrected the "install -p" point, but you didn't make this remark in my other
> submission. Is there a reason why I should do this in this package and not in
> the other ? Or should I correct it in adonthell too ?

You should correct it in adonthell too, I realised this myself too when making
the remark here, but with adonthell its less important as AFAIK no datafiles are
installed there, only compiled files which will have the time of build as
timestamp anyways, so preserving there timestamp is not really important, but
for autoxxx generated makefiles you should really _always_ specify
INSTALL="install -p"
Comment 9 Mathieu Bridon 2008-06-02 16:12:42 EDT
Corrected the 5 points. Here are the new files.

SRPM: ftp://91.121.156.173/pub/wastesedge-0.3.4-0.7.fc9.src.rpm
SPEC: ftp://91.121.156.173/pub/wastesedge.spec
Comment 10 Mathieu Bridon 2008-06-03 06:26:10 EDT
Just had a revelation: I forgot the icon-cache update scriptlet :S

I'm terribly sorry about that. I'll update this tonight (in France) as I can't
rebuild it from work.
Comment 11 Hans de Goede 2008-06-03 07:20:27 EDT
Ok, I'll wait for the next version then.
Comment 12 Mathieu Bridon 2008-06-03 14:58:23 EDT
Sorry for forgetting something that was written black on white here -_-

SPEC: ftp://ks359320.kimsufi.com/pub/wastesedge.spec
SRPM: ftp://ks359320.kimsufi.com/pub/wastesedge-0.3.4-0.8.fc9.src.rpm
Comment 13 Hans de Goede 2008-06-13 05:24:49 EDT
Sorry for the long delay, I git hit by a wall / mountain of work, but I've
managed to get over the top now :)

Anyways this package is approved!

Given the good work you've done here I'm ready to sponsor you now. Head over the
the fedora account system, and apply for cvs extra membership there. Once you've
done that add a comment here where you tell me your fas username, then I can
sponsor you.
Comment 14 Mathieu Bridon 2008-06-15 08:08:24 EDT
Hi,

Thanks a lot for the review.

I just applied for cvsextras in FAS, my user name is bochecha.
Comment 15 Hans de Goede 2008-06-16 02:14:02 EDT
Ok, I've sponsored you, now you can do yhe cvs requests.
Comment 16 Mathieu Bridon 2008-06-16 18:33:47 EDT
New Package CVS Request
=======================
Package Name: wastesedge
Short Description: Official game package for Adonthell
Owners: bochecha
Branches: F-8 F-9
InitialCC:
Cvsextras Commits: yes
Comment 17 Kevin Fenzi 2008-06-17 13:01:20 EDT
cvs done.

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