Bug 732200

The packages will be also available at https://sourceforge.net/projects/berusky2

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.

Don't use that: --vendor fedora -- See http://fedoraproject.org/wiki/Packaging:Guidelines#desktop-file-install_usage

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?

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.

It was x86_64 I tried.

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

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

Created attachment 520103 [details]
Updated spec file

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.

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

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

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

Created attachment 520142 [details]
Updates spec for data package

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

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.

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

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.

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

+: 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 ***

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

Git done (by process-git-requests).

Great, Thanks! Can you please create the berusky2-data package too?

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.

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

Git done (by process-git-requests).

Uploaded to Git and submitted as updates for F15/16. Thanks all, any feedback/bug reports/patches are welcome.