Bug 191005

Summary: Review Request: glob2 - Realtime Strategy game
Product: [Fedora] Fedora Reporter: Nikolai <brkamikaze>
Component: Package ReviewAssignee: Hans de Goede <hdegoede>
Status: CLOSED DUPLICATE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: hdegoede, wart
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2007-01-27 08:07:02 EST Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Bug Depends On:    
Bug Blocks: 201449    

Description Nikolai 2006-05-07 22:06:06 EDT
Spec URL: http://www.sendspace.com/file/i4udak
SRPM URL: http://www.sendspace.com/file/yd9bk8
Description: Globulation2 is a realtime strategy game designed with a new approach on micromanagement.
Comment 1 Brian Pepple 2006-05-07 22:20:02 EDT
Couple of quick items: 

1. In the files section, you need to use macros instead of hard-coding the path.
http://fedoraproject.org/wiki/Packaging/Guidelines#head-255d52ff18f82fa184a32946b82ed81e4fd8885a
2. The desktop file is not handled correctly.
3. The documentation (AUTHORS file) is not handled correctly.
4. Instead of listing every file, you should use a less verbose method (ie.
%{_datadir}/pixmaps/*.png).
5. A lot of your Requires aren't necessary, since the BuildRequires sonames will
cause these to be pulled in automatically.

Most of the solutions to these issues can be found on the wiki.
http://fedoraproject.org/wiki/Packaging/Guidelines
Comment 2 Nikolai 2006-05-08 11:13:34 EDT
(In reply to comment #1) 
> 2. The desktop file is not handled correctly. 
The desktop file is supplyed by the application, and the documentation only 
explains how to add a custom made one. By the documentation, specifically the 
line "Many applications will come with their own .desktop file, but if not, 
just make your own, and include it", the handling seems correct. 
> 5. A lot of your Requires aren't necessary, since the BuildRequires sonames 
will cause these to be pulled in automatically. 
The rpmbuild complains if my Requires line is empty (everything needed to run 
is also needed to compile). I will leave SDL only, to stop it from 
complaining. 
 
I also removed the desktop-file-utils dependency since I don't use the command 
"desktop-file-install" on the spec. 
Comment 3 Brian Pepple 2006-05-08 11:46:57 EDT
(In reply to comment #2)
> (In reply to comment #1) 
> > 2. The desktop file is not handled correctly. 
> The desktop file is supplyed by the application, and the documentation only 
> explains how to add a custom made one. By the documentation, specifically the 
> line "Many applications will come with their own .desktop file, but if not, 
> just make your own, and include it", the handling seems correct.

As is, this does not meet that FE standards.  Please look at the wiki some more,
and if that is not enough, also look at packages in CVS to help you find the
solution to this. 

http://cvs.fedora.redhat.com/viewcvs/devel/?root=extras


Comment 4 Nikolai 2006-05-08 12:35:34 EDT
I based myself on the yumex spec file to try a fix the desktop file problem.

Spec: http://www.sendspace.com/file/d6snuu

I'm having problems to upload the SRPM, I will post it here later if the spec
file is ok.
Comment 5 Nikolai 2006-05-08 13:15:15 EDT
I managed to upload the new SRPM: http://www.sendspace.com/file/u6urw9
Comment 6 Brian Pepple 2006-05-08 14:57:47 EDT
Couple of items:

1. Package fails to build in Mock.  Your missing a BuildRequirement.  For
information on Mock refer to: http://fedoraproject.org/wiki/Extras/MockTricks
2. Not a blocker, but in the files section, use %{_datadir}/%{name}/, instead of
individually listing the children directories.
3. The docs can be listed on one line with a space between each document.  If
this doesn't make sense, look at this for an example of what I'm talking about:
http://cvs.fedora.redhat.com/viewcvs/devel/gnomebaker/gnomebaker.spec?root=extras&view=markup
4. Instead of one long line in your desktop-file-install call how about some
line breaks to make it easier to read.  Refer to the above spec, if you've got
questions.
Comment 7 Nikolai 2006-05-08 17:24:03 EDT
I can't test the package in Mock (prep stage takes more time than I have
available today), but I believe I have fixed the depedency problem, or at least
most of it.

Spec: http://www.sendspace.com/file/f5w635
SRPM: http://www.sendspace.com/file/8eyxmn
Comment 8 Brian Pepple 2006-05-08 17:47:59 EDT
Your still missing a BuildRequirement.  Also, when you change the spec file,
please update the Release number, and add a ChangeLog w/ the changes made.
Comment 9 Michael Schwendt 2006-05-25 08:04:49 EDT
> The rpmbuild complains if my Requires line is empty (everything
> needed to run is also needed to compile). I will leave SDL only,
> to stop it from complaining. 

This doesn't sound right. Just delete the empty "Requires" line.
And SDL most certainly is in the automatic soname dependencies.
Query your binary package: rpm -qp --requires filename.i386.rpm
Comment 10 John Mahowald 2006-05-27 01:04:20 EDT
Your files are no longer available, please get some real hosting.

I'm sure someone on the mailing list could find some space for you to put them.
Comment 11 Hans de Goede 2006-06-08 07:47:40 EDT
Nikolai,

Are you still interested in this? ifso please ask for some better ftp / http
hosting on the Fedora Extras mailinglist, I'm sure someone will offer you some
space if you ask. Then upload your latest work to this new space and post the
URL's here.

Shouldn't you respond within one week from now, I'll presume you have
lost interest into getting this package into FE and close this PR.
Comment 12 Hans de Goede 2006-06-17 08:50:48 EDT
Nikolai,

I think this game would make a nice addition to FE, so I'm give you one last
chance to respond. If you don't I'll have to close this as wontfix.
Comment 13 Nikolai 2006-06-21 05:10:11 EDT
I'm sorry for the big delay, I had some financial problems and had to stay
without internet for some time. I will return to work on this package this week.
Comment 14 Nikolai 2006-06-27 21:15:30 EDT
I compiled the game without trouble on a fresh minimal install of Fedora; but I
added "automake14" to the BuildRequires line to satisfy a configure test. I
will upload the SRPM tomorrow.
Comment 15 Nikolai 2006-06-28 17:09:37 EDT
I got a hosting on the http://nikolai.thecodergeek.com/ page. I just made a page
with a small review and links to the SPEC and SRPM files. The files are already
uploaded, including a binary package.
Comment 16 Hans de Goede 2006-07-04 04:41:20 EDT
Good to hear you've got some decent hosting now and to see you active in
bugzilla again. I'll do a full review of glob2 now. But please understand that
having just one good package isn't enough to get you sponsored. In order to get
you sponsored you must show a thorough understanding of the packaging
guidelines. There are 2 ways to show this understanding:

1)
Do some reviews of other peoples packages, you can use the review below as a
template. Please put a comment above the review that it is not an official
review as you're not yet a Fedora Extras contributer. For the review guidelines
see: http://fedoraproject.org/wiki/Packaging/ReviewGuidelines

2)
Have a couple (I usually require 3) packages submitted which have been reviewed
and are approvable (They cannot be actually approved untill you're sponsored).

Your SRPM btw seems to be corrupt, luckily I managed to extract the patch you
use from it, so I can still do a review.

MUST:
=====
O rpmlint output is:
E: glob2 description-line-too-long Globulation 2 is a realtime strategy game
with a different approach on micromanagement.
E: glob2 description-line-too-long Globulation 2 is a realtime strategy game
with a different approach on micromanagement.
W: glob2 no-version-in-last-changelog
W: glob2-debuginfo no-version-in-last-changelog
E: glob2-debuginfo script-without-shellbang
/usr/src/debug/glob2-0.8.19/src/MultiplayersOfferScreen.cpp
E: glob2-debuginfo script-without-shellbang
/usr/src/debug/glob2-0.8.19/src/MultiplayersOfferScreen.h
E: glob2-debuginfo script-without-shellbang
/usr/src/debug/glob2-0.8.19/src/MultiplayersCrossConnectable.cpp
E: glob2-debuginfo script-without-shellbang
/usr/src/debug/glob2-0.8.19/src/MultiplayersChooseMapScreen.h
E: glob2-debuginfo script-without-shellbang
/usr/src/debug/glob2-0.8.19/src/MainMenuScreen.cpp
E: glob2-debuginfo script-without-shellbang
/usr/src/debug/glob2-0.8.19/src/MultiplayersCrossConnectable.h
E: glob2-debuginfo script-without-shellbang
/usr/src/debug/glob2-0.8.19/src/MultiplayersChooseMapScreen.cpp
E: glob2-debuginfo script-without-shellbang
/usr/src/debug/glob2-0.8.19/src/MainMenuScreen.h
These must all be fixed.
* Package and spec file named appropriately
* Packaged according to packaging guidelines
* License (GPL) ok, license file included
* spec file is legible and in Am. English.
O Could not verify that Sources matches upstream because of corrupt SRPM
* Compiles and builds on FC5-i386
* BR: ok (see below)
* No locales
* No shared libraries
* Not relocatable
* Package does not owns / or requires all dirs (see Must fix below)
* No duplicate files & Permissions ok
* %clean & macro usage OK
* Contains code only
* %doc does not affect runtime, and isn't large enough to warrent a sub package
* no -devel package needed, no libs / .la files.
* .desktop file as required and properly installed


MUST fix:
=========
* All rpmlint messages. Hint, to fix these:
  E: glob2-debuginfo script-without-shellbang /usr/src/debug/glob2-0.8.19/src/xx
  do "chmod -x src/*" in %prep
* Completly drop the bogus (already commented) Requires: SDL
* Drop unnescesarry automake14 BR, you do not call autoXXX from the specs so
  this isn't needed. Also using old versions of libs/tools in new packages is
  highly discouraged. I now you explicitly added this for mock, mock used to 
  have automake installed by default and you probably needed to add automake14
  because the package breaks with automake15. With neither installed it should
  build fine though as you don't call autoXXX from the spec file.
* Split the BuildRequires line over multiple lines (you can repeat the 
  BuildRequires: on a next line and add more BR's there).
* Remove INSTALL from %doc, since the users use an rpm to have no need for 
  install instructions.
* unowned dir %{_datadir}/%{name} you can fix this by replacing all the
  %{_datadir}/%{name}/xxxx entries under %files with just one entry containging:
  %{_datadir}/%{name}
* Under %files I see %{_datadir}/pixmaps/glob2-icon-xxx.png, that is not 
  according to the freedesktop.org icon standard, it should go under:
  %{_datadir}/icons/hicolor/32x32/apps
  Where 32x32 is the size of the icon, please do ls /usr/share/icons/hicolor/
  to see the available valid sizes, if the icon doesn't match any pick the 
  closest. Also under these dirs it should be named just glob2.png not
  glob2-icon-size.png. The fact that its an icon and its size our made
  clear by the dir its under.
* Once the icons are in the proper case you must add %post(un) script to update 
  the icon-cache see:
http://fedoraproject.org/wiki/ScriptletSnippets#head-fc74f078205565f961f6d836b77c3428619c689d
* Don't forget to fix the .desktop file to contain just:
  Icon=glob2.png instead of Icon=glob2-icon-48x48.png

Should fix:
===========
*Redundant BR: SDL_ttf-devel already requires SDL-devel, please remove SDL-devel
 from the BR-s
Comment 17 Nikolai 2006-07-05 06:46:11 EDT
Most things fixed. The host is already updated and I believe this time the file
isn't corrupted.
Comment 18 Hans de Goede 2006-07-05 13:47:54 EDT
The new SRPM indeeds is not corrupted like the old one. This has allowed me to
verify that the source matches upstream, good.

Not so good is this:
mode of `src/CVS' changed to 0644 (rw-r--r--)
chmod: `src/CVS': Permission denied
mode of `src/MainMenuScreen.cpp' changed to 0644 (rw-r--r--)
mode of `src/MainMenuScreen.h' changed to 0644 (rw-r--r--)
mode of `src/MultiplayersChooseMapScreen.cpp' changed to 0644 (rw-r--r--)
mode of `src/MultiplayersChooseMapScreen.h' changed to 0644 (rw-r--r--)
mode of `src/MultiplayersCrossConnectable.cpp' changed to 0644 (rw-r--r--)
mode of `src/MultiplayersCrossConnectable.h' changed to 0644 (rw-r--r--)
mode of `src/MultiplayersOfferScreen.cpp' changed to 0644 (rw-r--r--)
mode of `src/MultiplayersOfferScreen.h' changed to 0644 (rw-r--r--)
error: Bad exit status from /var/tmp/rpm-tmp.93697 (%prep)

After which the build exits I guess you're not seeing this because you build as
root? (bad idea). Anywas change the chmod from:
chmod -Rc -x src/*
to:
chmod -x src/*.cpp src/*.h
Because chmod -x on dirs is a bad idea.

Other items which still need work:
*some of the BR lines are still longer then 80 chars.
*you can drop the requires zlib-devel as that is already required by 
 libpng-devel I missed that the last time because the one long long line 
 approach was kinda impossible to read
*same goes for libX11-devel because its required by libXext-devel
* you still have no version in your changelog, instead of this:
 "* Thu Jul 04 2006 Nikolai <brkamikaze at gmail.com>"
 you dhould write:
 "* Thu Jul 04 2006 Nikolai <brkamikaze at gmail.com> 0.8.19-3"
* You should install all size icons into their appropriate places the whole idea
  behind the sized dirs is that instead of using sometimes ugly software scaling
  there are different handmade sizes of icons available for cases like menu,
  desktop icon and (large/medium/small) panel icon.
* please put empty lines between your different changelog entries, so that the
  changleog looks like this:
%changelog
* Thu Jul 04 2006 Nikolai <brkamikaze at gmail.com>
- Fixed almost all rpmlint errors; can't figure out how to get rid of the 'no-v
- Fixed the desktop file and the icons to use more standard standards :-)

* Tue Jun 27 2006 Nikolai <brkamikaze at gmail.com>
- Added the automake14 to BuildRequires to help 'mocking' the package.

* Sun May 7 2006 Nikolai <brkamikaze at gmail.com>
- First created.

Comment 19 Nikolai 2006-07-06 15:05:42 EDT
I will do some chmods on the build system because my build fails when not built
as root even before that chmod line. For the sake of completeness, I will put
the md5sums on the download pages too. When I finish building/uploading the new
RPM, I will answer here.

About your requirements for sponsorship, I'd prefer to do one package at a time.
When I get this one working, I'll move to another one :)
Comment 20 Hans de Goede 2006-07-06 15:33:24 EDT
(In reply to comment #19)
> About your requirements for sponsorship, I'd prefer to do one package at a time.
> When I get this one working, I'll move to another one :)

Fine by me, I agree this is better then juggling 3 reviews at the same time. You
won't be able to import glob2 though untill you've done one or more other packages.

While we're talking process I'll be on holiday for 5 days starting next monday.
Comment 21 Nikolai 2006-07-06 16:49:36 EDT
Files uploaded. I also made a new index page to support other projects.
Comment 22 Hans de Goede 2006-07-07 16:17:36 EDT
Okay,

This package now is approvable. But as discussed I cannot approve it untill I
sponsor you, we have some automated scripts which check things like approved
packages actually getting imported into CVS and the likes, and until sponsored
you cannot approve. I'm looking forward to your next package

Some things to make this one perfect:
-remove -Rc param from chmod, there is nothing to _R_ecurse and begin verbose
 isn't nescesarry either as rpm already show the command with the full
 (wildcard expanded) filelist.
-remove this no longer valid comment:
"# Get the used icon on the right dir with the right name and get rid of the
unused icons"
-please put empty lines between your different changelog entries, so that the
 changlog looks like this:

%changelog
* Thu Jul 04 2006 Nikolai <brkamikaze at gmail.com>
- Fixed almost all rpmlint errors; can't figure out how to get rid of the 'no-v
- Fixed the desktop file and the icons to use more standard standards :-)

* Tue Jun 27 2006 Nikolai <brkamikaze at gmail.com>
- Added the automake14 to BuildRequires to help 'mocking' the package.

* Sun May 7 2006 Nikolai <brkamikaze at gmail.com>
- First created.



Comment 23 Hans de Goede 2006-07-07 16:19:47 EDT
p.s.

1) I'm going on a short vacation from monday 10 juli till friday 14 juli, so if
I'm quiet thats why.

2) If you open a new review request please put me in the CC.
Comment 24 Nikolai 2006-07-08 07:14:11 EDT
I'm building with those changes and I will upload the package soon. Have a nice
vacation :)
Comment 25 Brian Pepple 2006-07-31 11:12:51 EDT
Switching back to FE-NEW, so that when Nikolai uploads his new package it can be
reviewed.
Comment 26 Hans de Goede 2006-07-31 13:58:52 EDT
(In reply to comment #25)
> Switching back to FE-NEW, so that when Nikolai uploads his new package it can be
> reviewed.

?? This has been reviewed and is basicly approved, the only reason I didn't move
it to FE-ACCEPT is because I'm not ready to sponsor Nikolai.

Nikolai speaking of which you were going to submit a few other packages for
review to show your packaging skills, I'm still waiting for those.

Comment 27 Brian Pepple 2006-07-31 14:08:16 EDT
(In reply to comment #26)
> (In reply to comment #25)
> > Switching back to FE-NEW, so that when Nikolai uploads his new package it can be
> > reviewed.
> 
> ?? This has been reviewed and is basicly approved, the only reason I didn't move
> it to FE-ACCEPT is because I'm not ready to sponsor Nikolai.
> 

Then maybe you should assign the bug to yourself if you are reviewing it.
Comment 28 Hans de Goede 2006-07-31 15:06:36 EDT
Seems I forgot that yes, thats still no reason to reset it to FE-NEW though,
which you could have known if you would have read the comments.
Comment 29 Nikolai 2006-07-31 17:31:41 EDT
Sorry for the delay, I've been busy for a while. By the end of this week I
should have uploaded the game.

I have one question tough: The game has optional support for a library that
isn't on extras or core (libggz) for online gaming, but it still doesn't support
online gaming. Should I include the library now, or should I include it only
when the game supports online gaming?
Comment 30 Hans de Goede 2006-08-01 01:37:46 EDT
(In reply to comment #29)
> Sorry for the delay, I've been busy for a while. By the end of this week I
> should have uploaded the game.
> 
Good.

> I have one question tough: The game has optional support for a library that
> isn't on extras or core (libggz) for online gaming, but it still doesn't support
> online gaming. Should I include the library now, or should I include it only
> when the game supports online gaming?

Thats up to you, you cannot however just include the library, the library should
be packaged and reviewed seperatly once that has been done you can add the
-devel subpackage of the library to the BuildRequires of glob2.
Comment 31 Nikolai 2006-08-04 15:28:04 EDT
I should upload the package today and open the bug report. If there is no time,
it will be online by sunday.
Comment 32 Hans de Goede 2006-08-04 18:07:11 EDT
Okay, I'll be on a short vacation though, so don't expect any reply from me
until wednesday.
Comment 33 Nikolai 2006-08-04 19:24:19 EDT
Ok. The package is uploaded and I'll file the bug report.
Comment 34 Nikolai 2006-08-05 07:21:50 EDT
The bug is filed as bug #201418 
(https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=201418).
Comment 35 Hans de Goede 2006-09-28 10:40:47 EDT
ping?
Comment 36 RafaƂ Psota 2007-01-27 08:07:02 EST

*** This bug has been marked as a duplicate of 225010 ***
Comment 37 Hans de Goede 2007-02-16 09:43:58 EST
Nikolai, as seen in comment 36 someone else who is already sponsored has also 
submitted glob2 for review. Probably because he thought this review is dead. 
Thats my fault as I've been swamped with other stuff lately and thus have 
neglected this review and sponsering you. Are you still interested in becoming 
an FE contributer? I assume you are, if you are please confirm then I shall 
review your other submission and sponsor you ASAP so that you can import your 
other submissions.

I've also added you to the cc-list of the other glob2 review, I think it would 
be a good idea for you to co-maintain glob2 with the other submitter. Maybe you 
can add a comment to the new glob2 review how you think about this?