Bug 238270 - Review Request: widelands - realtime-strategy game
Review Request: widelands - realtime-strategy game
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Hans de Goede
Fedora Package Reviews List
:
: 201418 (view as bug list)
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-04-28 12:42 EDT by Karol Trzcionka
Modified: 2007-11-30 17:12 EST (History)
8 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-05-08 15:45:40 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
hdegoede: fedora‑review+
wtogami: fedora‑cvs+


Attachments (Terms of Use)
Diff between original spec file and what the changes suggested in comment #5 would implement (2.13 KB, patch)
2007-05-01 04:52 EDT, Nils Philippsen
no flags Details | Diff

  None (edit)
Description Karol Trzcionka 2007-04-28 12:42:38 EDT
Spec URL: http://karlik.nonlogic.org/widelands/widelands.spec
SRPM URL: http://karlik.nonlogic.org/widelands/widelands-0-0.1.build10.src.rpm
Description:
Widelands is an open source (GPLed), realtime-strategy game, using SDL and
other free libraries, which is still under development. Widelands is inspired
by Settlers II (Bluebyte) and is partly similar to it, so if you know it, you
perhaps will have a thought, what Widelands is all about.
Comment 1 Karol Trzcionka 2007-04-28 13:06:40 EDT
*** Bug 201418 has been marked as a duplicate of this bug. ***
Comment 2 Bernard Johnson 2007-04-29 16:35:24 EDT
I see several problems right away...

1. You might want to get some feedback on the version numbering.  Upstream is
pretty messed up.  You way of handling it seems reasonable, but I've never seen
it done that way.

2. During build: "WARNING: Could not find pngwrite. PNG compatification disabled."

3. Compile completely ignores CXXFLAGS, this is a blocker.

4. Does not update desktop database:
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#head-de6770dd9867fcd085a73a4700f6bcd0d10294ef

5. During build: "Checking for EF_newFrame() in C library efence... no"  Wants
to be linked to ElectricFence??

6. Several messages like this:
msgmerge: error while opening "widelands_en_EN.po" for reading: No such file or
directory
msgfmt: error while opening "widelands_en_EN.po" for reading: No such file or
directory

are these actual problems?

7. Is this important:
INFORMATION: Astyle produces malformed indentation (see for example
[https://sourceforge.net/tracker/index.php?func=detail&aid=1642489&group_id=2319&atid=102319])
and is disabled whilst waiting for repair. If you really want to use it, execute
"cd /usr/bin && ln -s astyle buggy-astyle" and try again.

8. Provides a lot of perl junk but doesn't require perl:
Provides: perl(Client) perl(ProtocolPacket) perl(ProtocolPacket_ChatMessage)
perl(ProtocolPacket_Connect) perl(ProtocolPacket_GetRoomInfo)
perl(ProtocolPacket_GetUserInfo) perl(ProtocolPacket_Hello)
perl(ProtocolPacket_Ping) perl(ProtocolPacket_UserEntered) perl(Server)

This seems to be because of /usr/share/widelands/game_server.  Possibly this
might be better split off.  But it certainly doesn't belong in the -data package.

9. Locale handling is incorrect:
http://fedoraproject.org/wiki/Packaging/Guidelines#head-8c605ebf8330f6d505f384e671986fa99a8f72ee

10. Desktop database is not updated:
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets?#head-de6770dd9867fcd085a73a4700f6bcd0d10294ef
Comment 3 Karol Trzcionka 2007-04-29 18:14:24 EDT
1. I thought a lot of time about it and when I was begining to write a spec, the
version was build10 or 10 (two ways :P ), but it is not a numerical version and
I think it will be easier to update when developers release normal versions
(like X.YZ), because I will not need epoch
2. It is not important, I asked developer about it and it is not require
3. I am working about it (setting flags is in python and I do not know how can I
fix it)
4. I forgot about it, added
5. I will check it.
6. I think it is not important (all works correctly I think)
7. I'll check it
8. I have added require perl in data subpackage
9. It might be a problem. The locales can't be in /usr/share/locale, because it
is not a one file and the names can conflict with other package and tehere is a
directory. I think in this situation using %find_lang does not make any sense.
10. Like 4
Comment 4 Karol Trzcionka 2007-04-30 13:35:27 EDT
Well...:
1. I have changed versions numerating for more clearly naming of packages. Now
it is 0-0.%{buildnum}.%{rel}.build%{buildnum}
3. I have prepared a patch and now it is compiling with %{optflags}
5. Efence is require only for one compiling-method and it is not require for
normal working
7. I think it might be ignored

New URLs:
http://karlik.nonlogic.org/widelands/widelands.spec
http://karlik.nonlogic.org/widelands/widelands-0-0.10.1.build10.src.rpm
Comment 5 Nils Philippsen 2007-05-01 04:49:23 EDT
I just noticed your widelands submission and since I've packaged an old version
of it privately, I have some comments about your spec file to add (I'll attach a
diff between your original and what my comments would implement for good measure):

1) Shorten the summary a bit, I suggest something like "Open source
realtime-strategy game"

2) Consider using just

Release:        0.build%{buildnum}.%{rel}%{?dist}

to avoid redundancy in the release tag and have the dist/version tag.

3) I'd add the version of the package for which my patches are built in the
filename, i.e. widelands-build10-workfix.patch and
widelands-build10-flagfix.patch. This way changes in the patches are more
obvious (because the version gets bumped then).

4) Add some blank lines between spec file sections and changelog entries, this
improves readability.

5) Capitalize the GenericName of the desktop file ("Realtime Strategy Game")

6) No need to use --delete-original when installing the spec file as the
buildroot will be emptied at the beginning of %install

I'll be on vacation from next week on, so unfortunately I can't volunteer for
the review now.
Comment 6 Nils Philippsen 2007-05-01 04:52:00 EDT
Created attachment 153852 [details]
Diff between original spec file and what the changes suggested in comment #5 would implement
Comment 7 Dominik 'Rathann' Mierzejewski 2007-05-01 04:54:05 EDT
(In reply to comment #5)
[...]
> 3) I'd add the version of the package for which my patches are built in the
> filename, i.e. widelands-build10-workfix.patch and
> widelands-build10-flagfix.patch. This way changes in the patches are more
> obvious (because the version gets bumped then).

It is a matter of preference, but I think using unversioned patch filenames is
better because their history can be preserved in CVS, which doesn't support file
renaming.
Comment 8 Nils Philippsen 2007-05-01 05:06:41 EDT
(In reply to comment #7)
> (In reply to comment #5)
> [...]
> It is a matter of preference, but I think using unversioned patch filenames is
> better because their history can be preserved in CVS, which doesn't support file
> renaming.

That's a point I've not thought of... Hard to say what's better with the current
SCM (CVS).

I think there are patches of at least two different natures: On the one side
some patches fix actual bugs in the upstream package that will hopefully get
fixed in the next upstream version, they are probably rather short-lived and the
history of the patch won't be that interesting (it's created and then dropped
later on). Then there are patches that change policy and have probably a longer
life than the other ones. Here preservation of history would be more worthwhile.

Eventually we'll hopefully move to a more modern SCM (which preserves renaming) ;-).
Comment 9 Michał Bentkowski 2007-05-01 06:37:39 EDT
(In reply to comment #5)
> 2) Consider using just
> 
> Release:        0.build%{buildnum}.%{rel}%{?dist}
> 
> to avoid redundancy in the release tag and have the dist/version tag.

It's written in Package Naming Guidelines that we have to use 
0.%{X}.%{alphatag} release tag for pre-releases and there's also an example: 
alsa-lib-0.9.2-0.1.beta1

So let's treat build10 as an %{alphatag} and it'll be indeed compatible with 
naming guidelines, won't it?
Comment 10 Mamoru TASAKA 2007-05-01 07:25:41 EDT
(In reply to comment #9)
> It's written in Package Naming Guidelines that we have to use 
> 0.%{X}.%{alphatag} release tag for pre-releases and there's also an example: 
> alsa-lib-0.9.2-0.1.beta1
> 
> So let's treat build10 as an %{alphatag} and it'll be indeed compatible with 
> naming guidelines, won't it?

This versioning is correct.
Comment 11 Nils Philippsen 2007-05-01 07:40:54 EDT
Agreed.
Comment 12 Karol Trzcionka 2007-05-01 11:03:37 EDT
I have returned to first method of versioning.
* Tue May 01 2007 Karol Trzcionka <karlikt at gmail.com> - 0-0.2.build10
- Return to first method of versioning
- Some changes in summary and GenericName
- Make spec-file more clear

New URLs:
http://karlik.nonlogic.org/widelands/widelands.spec
http://karlik.nonlogic.org/widelands/widelands-0-0.2.build10.src.rpm
Comment 13 Nils Philippsen 2007-05-02 00:24:19 EDT
One thing where I don't know if that's covered by the packaging guidelines:
There's a circular dependency between widelands and widelands-data. Given that
both come from the same source, I'd just include the data files with the main
package and make it easy for the dependency solvers.
Comment 14 Nigel Jones 2007-05-02 01:08:41 EDT
(In reply to comment #13)
> One thing where I don't know if that's covered by the packaging guidelines:
> There's a circular dependency between widelands and widelands-data. Given that
> both come from the same source, I'd just include the data files with the main
> package and make it easy for the dependency solvers.

The correct way, is drop the depends on widelands in -data as the -data package 
can sometimes be used standalone.
Comment 15 Nils Philippsen 2007-05-02 01:16:36 EDT
(In reply to comment #14)

> The correct way, is drop the depends on widelands in -data as the -data package 
> can sometimes be used standalone.

As rapidly as the project is changing, I'd say using that data files from
outside is not the road to sanity ;-). My opinion is still that the data
subpackage should go for now. If needed, it can always -- and easily -- be
resurrected.
Comment 16 Hans de Goede 2007-05-03 14:18:50 EDT
Karol,

I can do a full / official review on this if you want. I would appreciate it if
you would review a (small) package of mine in return:
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=237813

About the -data package, NP is correct, when its in the same tarbal as the
engine there is no use in having the data in a subpackage. If it was in a
seperate tarbal then that would need to be in its own srpm, and those should
circular depend on each other, so that if one gets removed the other also gets
removed.
Comment 17 Karol Trzcionka 2007-05-04 08:40:08 EDT
If it is needed, I can merge subpackage -data with "core" package.
I will do it in the next release with other "review" changes (if they are needed).
Comment 18 Hans de Goede 2007-05-05 04:38:03 EDT
MUST:
=====
* rpmlint output is:
W: widelands-data no-documentation
* Package and spec file named appropriately
* Packaged according to packaging guidelines
* License ok
* spec file is legible and in Am. English.
* Source matches upstream
* Compiles and builds on devel i386
* BR: ok
0 locales not handled properly
* No shared libraries, ldconfig not needed
* Not relocatable
* Package owns / or requires all dirs
* No duplicate files & Permissions ok
* %clean & macro usage OK
* Contains code and permissable content
* %doc does not affect runtime, and isn't large enough to warrent a sub package
* -devel package not needed
* .desktop file as needed

MUST FIX
========
* You install pics/wl-logo-64.png as /usr/share/pixmaps/widelands.png, however
  installing icons under /usr/share/pixmaps is obsolete, it should go under
  /usr/share/icons/hicolor/64x64/apps
* Add icon update cache code to %post and %postun, see scriptlets page on the
  wiki
* Remove update-desktop-database from %post(un) this is only needed when you
  install a new mimitype
* Remove "Version" and "TryExec" from the .desktop file. Version should be  
  set to the actual package version, not 1.0 since thats kinda hard todo for
  this package and since Version isn't actually used by anything just remove it.
  TryExec isn't needed here.
* Merge -data and main package into one, no need / use for a seperate package
  
Should Fix
==========
* You do %define buildnum 10 and then everywhere were you use it you write:
  build%{buildnum} why not just do: "%define build build10" and use %{build}
  where you now use build%{buildnum}?
* Why define rel, why not just directly enter it in the release field?
* I agree with your assesment made in comment #3 about the locale files being
  to generic named to go into the system dir, however they should still be
  marked %lang XX (just like config files should be marked %config)
* It doesn't seem to properly pickup the system language set with LANG,
  translations do work when you change the language from the menu
Comment 19 Karol Trzcionka 2007-05-06 11:55:43 EDT
Thanks for review.
(In reply to comment #18)

> MUST FIX
> ========
> * You install pics/wl-logo-64.png as /usr/share/pixmaps/widelands.png, however
>   installing icons under /usr/share/pixmaps is obsolete, it should go under
>   /usr/share/icons/hicolor/64x64/apps
Changed
> * Add icon update cache code to %post and %postun, see scriptlets page on the
>   wiki
Added
> * Remove update-desktop-database from %post(un) this is only needed when you
>   install a new mimitype
Removed
> * Remove "Version" and "TryExec" from the .desktop file. Version should be  
>   set to the actual package version, not 1.0 since thats kinda hard todo for
>   this package and since Version isn't actually used by anything just remove it.
>   TryExec isn't needed here.
Done.
> * Merge -data and main package into one, no need / use for a seperate package
Done.
> Should Fix
> ==========
> * You do %define buildnum 10 and then everywhere were you use it you write:
>   build%{buildnum} why not just do: "%define build build10" and use %{build}
>   where you now use build%{buildnum}?
%define build_id build10
> * Why define rel, why not just directly enter it in the release field?
fixed
> * I agree with your assesment made in comment #3 about the locale files being
>   to generic named to go into the system dir, however they should still be
>   marked %lang XX (just like config files should be marked %config)
It was a little problem, but now it is OK

New URLs:
http://karlik.nonlogic.org/widelands/widelands.spec
http://karlik.nonlogic.org/widelands/widelands-0-0.3.build10.src.rpm
Comment 20 Hans de Goede 2007-05-06 14:18:59 EDT
Looks good now, approved!
Comment 21 Karol Trzcionka 2007-05-06 14:28:59 EDT
New Package CVS Request
=======================
Package Name: widelands
Short Description: realtime-strategy game
Owners: karlikt@gmail.com
Branches: FC-6
InitialCC: 
Comment 22 Karol Trzcionka 2007-05-08 15:45:40 EDT
Built for fc-6 and fc-7. Closed.

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