Bug 732200 - Review Request: Berusky2 - 3D sequel of Berusky
Summary: Review Request: Berusky2 - 3D sequel of Berusky
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Richard Shaw
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-08-20 15:50 UTC by Martin Stransky
Modified: 2011-09-08 07:06 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-09-08 07:06:28 UTC
hobbes1069: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)
Updated spec file (48 bytes, text/plain)
2011-08-26 14:58 UTC, Richard Shaw
no flags Details
Updates spec for data package (53 bytes, text/plain)
2011-08-26 21:09 UTC, Richard Shaw
no flags Details

Description Martin Stransky 2011-08-20 15:50:17 UTC
Spec URL: http://anakreon.cz/download/berusky2.spec
SRPM URL: http://anakreon.cz/download/berusky2-0.3-1.fc14.src.rpm
Description: 

Berusky 2 is a game that challenges your visual/spatial thinking
and ability to find a way to resolve a logic task. Using five bugs,
you'll go through an adventure full of various puzzles spread across
nine episodes. Individual episodes differ in appearance and difficulty,
which increases throughout the game.

berusky2 package contains game binary and configuration.

Spec URL: http://anakreon.cz/download/berusky2-data.spec
SRPM URL: http://anakreon.cz/download/berusky2-data-0.4-1.fc14.src.rpm
Description: berusky2-data package contains game data.

Comment 1 Martin Stransky 2011-08-20 15:52:20 UTC
The packages will be also available at https://sourceforge.net/projects/berusky2

Comment 2 Volker Fröhlich 2011-08-21 00:40:05 UTC
Some quick comments:

The package doesn't build for me in either mock or with rpmbuild.

"This package contains a binary for the game." -- I think you can remove that from the description!

Don't mix %{buildroot} and $RPM_BUILD_ROOT.

Use -p when installing your sources.

Remove " -n %{name}-%{version}". That is exactly the format it expects.

You can remove the defattr.

Don't put the summary in the first line of the spec file.

You haven't defined the game_name macro.

Rpmlint on the SRPM: berusky2.src: W: strange-permission berusky3d.ini 0600L

[makerpm@fedora15 berusky2-0.3-1.fc14.src]$ desktop-file-validate berusky2.desktop 
berusky2.desktop: warning: key "Encoding" in group "Desktop Entry" is deprecated
berusky2.desktop: error: (will be fatal in the future): value "berusky2.png" for key "Icon" in group "Desktop Entry" is an icon name with an extension, but there should be no extension as described in the Icon Theme Specification if the value is not an absolute path

Ship NEWS, README, AUTHORS and COPYING as doc.

Comment 3 Volker Fröhlich 2011-08-21 00:42:12 UTC
Don't use that: --vendor fedora -- See http://fedoraproject.org/wiki/Packaging:Guidelines#desktop-file-install_usage

Comment 4 Martin Stransky 2011-08-22 20:17:05 UTC
Thanks for the comments, Will try to address them soon.

(In reply to comment #2)
> The package doesn't build for me in either mock or with rpmbuild.

I forget to add ExclusiveArch: %{ix86}, x86_64 is not supported for now. Do you build it on x86_64? If not, can you post the build error please?

Comment 5 Martin Stransky 2011-08-22 20:35:39 UTC
An updated spec/source is available at:

Spec URL: http://anakreon.cz/download/berusky2.spec
SRPM URL: http://anakreon.cz/download/berusky2-0.3-3.fc14.src.rpm

> Use -p when installing your sources.

What do you mean with it?

> Ship NEWS, README, AUTHORS and COPYING as doc.

The should be already shipped as doc.

Comment 6 Volker Fröhlich 2011-08-22 20:42:58 UTC
It was x86_64 I tried.

I mean, that you should preserve timestamps, when you run cp or install.

Comment 7 Richard Shaw 2011-08-26 14:56:11 UTC
Ok, here's my first pass:

1. You don't need BuildRoot:, %clean, or the rm -rf %{buildroot} in %install if you're only planning on building for F14+

You also don't need the two "Requires:" lines since rpmbuild is pretty good at figuring out dependencies.

What Volker was meaning by "-p" was the preserve timestamp option on "cp" and "install". 

2. I could not get the package to build because it was failing to find some headers. I had to add the following:

# Fix header references
sed -i "s/menu.h/Menu.h/g" src/kofola/Menu.cpp
sed -i "s/menu.h/Menu.h/g" src/kofola/game_main.cpp
sed -i "s/menu.h/Menu2.h/g" src/kofola/Menu2.cpp

I'm not sure how you got it to build without this. Perhaps you're not using gcc 4.6 and in previous versions this was only a warning?

3. There's an easier trick to getting the documentation installed to the right directory. (Thanks to Hans for teaching me this one!) We don't need INSTALL since we're providing a package.

Move the documentation back to the build directory in a temporary directory. Then reference that directory in the %doc macro, e.g.:

# Move documentation so it can get installed to the right place.
mkdir _tmpdoc
mv %{buildroot}%{_usr}/doc/%{name}/* _tmpdoc/
rm -f _tmpdoc/INSTALL

Then in your %doc:

%doc _tmpdoc/*

4. This package has a manual requires for berusky2-data. From your sourceforge link it appears that the data source is separate for a good reason? 

I noticed that it has a version of 0.4 while this has a version of 0.3. Are they intended to revise separately?

If so we should probably cover the review request for the data package here as well.


I would usually leave the updates to be made by the requester as it's a good way to learn but there are so many changes I'll attach my updated spec file for you to review.

Richard

Comment 8 Richard Shaw 2011-08-26 14:58:42 UTC
Created attachment 520103 [details]
Updated spec file

Comment 9 Martin Stransky 2011-08-26 19:51:00 UTC
Thanks for the review and updated spec file!

> 2. I could not get the package to build because it was failing to find some
> headers. I had to add the following:
> 
> # Fix header references
> sed -i "s/menu.h/Menu.h/g" src/kofola/Menu.cpp
> sed -i "s/menu.h/Menu.h/g" src/kofola/game_main.cpp
> sed -i "s/menu.h/Menu2.h/g" src/kofola/Menu2.cpp
> 
> I'm not sure how you got it to build without this. Perhaps you're not using gcc
> 4.6 and in previous versions this was only a warning?

You're right, I'm using gcc-4.5 on F14.

> 4. This package has a manual requires for berusky2-data. From your sourceforge
> link it appears that the data source is separate for a good reason? 

Yes, the data tar ball is 300MB big and stable. It's the original game data so there's no need to change it.

> I noticed that it has a version of 0.4 while this has a version of 0.3. Are
> they intended to revise separately?

Yes, they're independent. The game binary version is going to upgrade rapidly :)

> If so we should probably cover the review request for the data package here as
> well.

Yes, I submitted both to one bug (this).

> I would usually leave the updates to be made by the requester as it's a good
> way to learn but there are so many changes I'll attach my updated spec file for
> you to review.

Thanks a lot, looks okay to me.

Comment 10 Richard Shaw 2011-08-26 20:22:12 UTC
Sorry, I completely missed the second set of spec and SRPM links. Downloading now.

Are you the package owner at sourceforge? If so, we could probably go ahead and roll in the header fixs instead of using sed hacks.

Richard

Comment 11 Martin Stransky 2011-08-26 20:36:49 UTC
(In reply to comment #10)
> Are you the package owner at sourceforge? If so, we could probably go ahead and
> roll in the header fixs instead of using sed hacks.

Yes, I'm the package owner at sourceforge, but I prefer to leave this change to next version. I have some other fixes in queue so I'd like to release them together.

Comment 12 Richard Shaw 2011-08-26 21:08:19 UTC
Ok, preliminary review for berusky2-data:

1. Typical stuff:
BuildRoot, %clean, "rm -rf %{buildroot}" in %install and defattrib's can be
removed.

2. Changed macro "define" to "global" as it's preferred[1].

3. I fixed the Source: line to use the official sourceforge URL[1] but left the
URL: for you to update to whatever is most appropriate.

4. In this case since there's nothing to build, %build can be removed. rpmlint
will complain but we can ignore it.

5. I cleaned up the "mv" operation to a bit to improve readability.

6. If a package owns a directory and all items in it then you can just
reference the directory (without leading "/" I believe) and it will grab
everything. That simplifies your:
%dir %{_datadir}/%{game_name}
%{_datadir}/%{game_name}/*

to:
%{_datadir}/%{game_name}

I'll upload a new sepc but you should update the changelog and bump the
revision as I did not.

Richard

[1]
http://fedoraproject.org/wiki/Packaging:Guidelines#.25global_preferred_over_.25define
[2] http://fedoraproject.org/wiki/Packaging:SourceURL#Sourceforge.net

Comment 13 Richard Shaw 2011-08-26 21:09:14 UTC
Created attachment 520142 [details]
Updates spec for data package

Comment 14 Richard Shaw 2011-08-26 21:15:08 UTC
I forgot to mention:

7. Since the data package is totally separate, we probably need some kind of license file put in the archive. You could probably just use the same COPYING file that's in base package.

8. Do we need the "Save/profiles" directories? It's the only thing left in the source archive that's not installed.

If so, are they designed to be written to? 

Richard

Comment 15 Martin Stransky 2011-08-29 08:12:15 UTC
Great, thanks for the fixes!

(In reply to comment #14)
> 7. Since the data package is totally separate, we probably need some kind of
> license file put in the archive. You could probably just use the same COPYING
> file that's in base package.

I see...I'll put it there.

> 8. Do we need the "Save/profiles" directories? It's the only thing left in the
> source archive that's not installed.

The Save/profiles are created run time by game in ~, we don't need to install them.

Comment 16 Richard Shaw 2011-08-29 13:06:01 UTC
I was able to install and play the game, however, I was able to crash it almost immediately. I can't remember exactly what I did but I didn't put any characters into the Name portion of the screen and tried to bypass the screen.

Also, some future work for you :) Some of the English translations were incomplete or could be improved. I've never worked with language files but if you would like some help with that (outside of this review of course) let me know.

When you post new specs and SRPMS (don't forgot to update the changelog!) I'll start the full guideline review.

Richard

Comment 17 Martin Stransky 2011-08-30 08:52:05 UTC
Great, Thanks! It would be nice if you can provide bactrace of the crashes, but let's do the package review first and the fine tune it. I'm going to create some debug how-to page too.

The updated files (tar, specs, rpm) are at:

https://sourceforge.net/projects/berusky2/files/0.4/

Thanks again.

Comment 18 Richard Shaw 2011-08-30 14:34:16 UTC
Ok, just a couple of things and I think it will be ready:

The source for the game package needs to be updated to:

Source:         http://downloads.sourceforge.net/%{name}/%{name}-%{version}.tar.gz

In the berusky2.desktop file, the .png extension should be removed from the Icon field. Basically the rule is if you only specify the name it should be without the extension, if it's the full path to the icon name, then it should be included.

I'll do the full review sometime today so don't worry about building a new release until I'm done in case I find something.

Richard

Comment 19 Richard Shaw 2011-09-02 16:30:02 UTC
+: OK
-: must be fixed
=: should be fixed (at your discretion)
?: Question or clairification needed
N: not applicable

MUST:
[+] rpmlint output: shown in comment: No show stoppers. Changelog should probably contain something since you're the upstream maintainer now.
[+] follows package naming guidelines
[+] spec file base name matches package name
[+] package meets the packaging guidelines
[+] package uses a Fedora approved license: GPLv2+
[+] license field matches the actual license.
[+] license file is included in %doc: COPYING
[+] spec file is in American English
[+] spec file is legible
[+] sources match upstream: md5sum matches (f60e75a49ae167945a2c21027fac08fb)
[+] package builds on at least one primary arch: Tested F15 i686
[+] appropriate use of ExcludeArch
[+] all build requirements in BuildRequires
[N] spec file handles locales properly
[N] ldconfig in %post and %postun
[+] no bundled copies of system libraries
[N] no relocatable packages
[+] package owns all directories that it creates
[+] no files listed twice in %files
[+] proper permissions on files
[+] consistent use of macros
[+] code or permissible content
[N] large documentation in -doc
[+] no runtime dependencies in %doc
[N] header files in -devel
[N] static libraries in -static
[N] .so in -devel
[N] -devel requires main package
[+] package contains no libtool archives
[+] package contains a desktop file, uses desktop-file-install/validate
[+] package does not own files/dirs owned by other packages
[+] all filenames in UTF-8

SHOULD:
[+] query upstream for license text
[N] description and summary contains available translations
[+] package builds in mock
[+] package builds on all supported arches
[+] package functions as described: Managed to crash it, but will submit a bug report once it's made it into the available components.
[+] sane scriptlets
[+] subpackages require the main package
[N] placement of pkgconfig files
[N] file dependencies versus package dependencies
[N] package contains man pages for binaries/scripts

I'm not going to worry about dong a "full review" on the data package because most of it would be N/A. 

I'm downloading the data archive now to verify the md5sum but since you're the upstream maintainer I trust you.

*** APPROVED ***

Comment 20 Martin Stransky 2011-09-02 18:57:17 UTC
Great, Thanks!

New Package SCM Request
=======================
Package Name: berusky2
Short Description: 3D logic game
Owners: stransky
Branches: f15 f16 el6

New Package SCM Request
=======================
Package Name: berusky2-data
Short Description: 3D logic game
Owners: stransky
Branches: f15 f16 el6

Comment 21 Gwyn Ciesla 2011-09-02 21:12:43 UTC
Git done (by process-git-requests).

Comment 22 Martin Stransky 2011-09-02 21:19:28 UTC
Great, Thanks! Can you please create the berusky2-data package too?

Comment 23 Richard Shaw 2011-09-02 21:29:16 UTC
Hmm.. I think the problem is that the SCM request is processed in a largely automated way. You'll probably need to set the CVS flag again and just put the data package request in the comment.

Comment 24 Martin Stransky 2011-09-02 21:56:36 UTC
I see...let's try it again :)

New Package SCM Request
=======================
Package Name: berusky2-data
Short Description: 3D logic game
Owners: stransky
Branches: f15 f16 el6

Comment 25 Gwyn Ciesla 2011-09-03 16:54:37 UTC
Git done (by process-git-requests).

Comment 26 Martin Stransky 2011-09-08 07:06:28 UTC
Uploaded to Git and submitted as updates for F15/16. Thanks all, any feedback/bug reports/patches are welcome.


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