Bug 486758

Note: There are two issues known to me:

1.) Version number is made up
2.) Licensing is not clear enough

Don't let that discourage you from reviewing, both were communicated upstream and I'm currently waiting for a reply.

Just a brief look:
1. Don't forget to fix the version number in your changelog section. Currently it doesn't match with the version of the package itself.

2. I don't know why, but it doesn't run on my system (with an Intel graphic card!) and crashes with a segmentation fault :(

(In reply to comment #2)
> Just a brief look:
> 1. Don't forget to fix the version number in your changelog section. Currently
> it doesn't match with the version of the package itself.

Good catch, will do.

> 2. I don't know why, but it doesn't run on my system (with an Intel graphic
> card!) and crashes with a segmentation fault :(

This is probably not an error of this packages, since it's basically just a game data. What release of Fedora and blender package did you use? Are you able to launch/use blender?

I'm wondering if the upstream binary version of the game engine runs for you? Would you mind trying to download the binary distribution from http://www.yofrankie.org/download/ [1] and trying it?

http://download.blender.org/apricot/yofrankie_bge.zip

Yes, I think that's an issue with intel graphic card and its drivers :(
I use Fedora 10 and its latest blender package. Yes, I can run blender itself.
I'll check the main binary soon. :)
Anyway, please do something about the Licensing issue for now.

(In reply to comment #4)
> Yes, I think that's an issue with intel graphic card and its drivers :(

Yup, they're not in the healthiest state now. I can't try that myself, since F10 intel drivers don't work for me at all (cause lockups), so I use large part of X11 from F9.

> Anyway, please do something about the Licensing issue for now.

I don't think there's not much I can do about it now. I did not get a reply from the recipient of the original mail, so I'll resend it to a couple more addresses and wait a couple of days. I'll proceed replacing the logos just in case I won't get any reply even then.

Good news. Got a reply from upstream, answering both my concerns.
They'll do a new release once the levels from the competition are done.
Hopefully, they'll include the license file there. I'm including the DVD license text for now, which sufficiently clarifies the situation with logo as well.

New package (just added the license, lot of comments to clarify and fixed version in accordance to what I was told by upstream):

SPEC: http://v3.sk/~lkundrak/SPECS/yofrankie-bge.spec
SRPM: http://v3.sk/~lkundrak/SRPMS/yofrankie-bge-1.4-0.1.20081221svn.src.rpm

Uploading not complete yet. Slow link, rsyncin' will finish in an hour or two; Sorry.

yofrankie-bge.noarch: W: incoherent-version-in-changelog 1.4-0.2.20081221svn 1.4-0.1.20081221svn
The last entry in %changelog contains a version identifier that is not
coherent with the epoch:version-release tuple of the package.

2 packages and 0 specfiles checked; 0 errors, 1 warnings.

I've messed it up again. I'll fix on next package spin.

I don't have permission to download the srpm file.

(In reply to comment #9)
> I don't have permission to download the srpm file.

Sorry, I turned off the read bits to prevent downloading until it is complete, and forgot to turned them off. Should be ok now.

It is crashing for me after going "Start game" in menu.
Original binary distribution is working fine though.

(In reply to comment #11)
> It is crashing for me after going "Start game" in menu.
> Original binary distribution is working fine though.  

That's most likely a blender bug. Please install blender-debuginfo and file a bug against blender with a traceback.

Bug #489018 was filed against blender. Seems crash is happening only with compressed blender files. If run it directly with svn sources, it works fine.

Two notes about the package:
- If logos are not distributed under CC-BY, should they have proper license in License tag?
- In desktop-file-install call there is no need of --vendor=''

(In reply to comment #13)
> Bug #489018 was filed against blender. Seems crash is happening only with
> compressed blender files. If run it directly with svn sources, it works fine.

Hah, that explains my confusion when trying to reproduce that. Thanks!

> Two notes about the package:
> - If logos are not distributed under CC-BY, should they have proper license in
> License tag?

I do not think so -- firefox doesn't have it either. If you disagree, we should probably block FE_LEGAL and have someone more familiar with it look  at this.

> - In desktop-file-install call there is no need of --vendor=''

Older versions of desktop-file-install require it, while it's harmess for newer.

One more thing:
- Version in changelog is incoherent with version of package (that is the only warning of rpmlint).

(In reply to comment #14)
> > Two notes about the package:
> > - If logos are not distributed under CC-BY, should they have proper license in
> > License tag?
> 
> I do not think so -- firefox doesn't have it either. If you disagree, we should
> probably block FE_LEGAL and have someone more familiar with it look  at this.

I think, "Freely redistributable without restriction" should be added for logos.

Anyway, blocking FE_LEGAL: could someone take a look, what right license tag to use here?

SPEC: http://v3.sk/~lkundrak/SPECS/yofrankie-bge.spec
SRPM: http://v3.sk/~lkundrak/SRPMS/yofrankie-bge-1.4-0.3.20081221svn.src.rpm

Investigation of crash issue [1] lead me to do the following change:

Do not compress PNG images into JPEG:
* It causes artifacts to appear (in main menu)
* Replacing the references via python bindings produces file that crashes BGE
* Upstream doesn't compress the images on DVD

[1] https://projects.blender.org/tracker/index.php?func=detail&aid=18377&group_id=151&atid=445

NOTE: In case you have any of the previous versions of the package, you don't have to re-download the huge SRPM. You can just refresh the SPEC file, and download this additional source:
http://v3.sk/~lkundrak/yofrankie-bge-1.3-imgcompr.patch

Adding "Freely redistributable without restriction" along with the explanatory comments is correct. Lifting FE-Legal.

(In reply to comment #17)
> Adding "Freely redistributable without restriction" along with the explanatory
> comments is correct. Lifting FE-Legal.  

I'll include that in next package spin, which will be after someone does a full review.

I went forward and did the review. Here it is with few comments:

+ rpmlint output clean
2 packages and 0 specfiles checked; 0 errors, 0 warnings.

+ The package named according to the Package Naming Guidelines.
+ The spec file name matches the base package %{name}, in the format
  %{name}.spec.
+ The package meets the Packaging Guidelines.
+ The package licensed with a Fedora approved license and meets the
  Licensing Guidelines.
+ The License field in the package spec file matches the actual license.

That is providing, that it'll be changed to "CC-BY and Freely redistributable
without restriction".

+ File, containing the text of the licenses for the package is included in
  %doc.
+ The spec file is written in American English.
+ The spec file for the package is legible.
+ The sources used to build the package matches the upstream source.

Source command to export SVN is actually wrong. It should have either "-r 9"
or "@9" but not combination of those.

+ The package successfully compiles and builds into binary rpms on at least
  one primary architecture.
+ All build dependencies are listed in BuildRequires.
+ No need to deal with locales.
+ The package does not designed to be relocatable.
+ A package owns all directories that it creates.
+ A package does not list a file more than once in %files listings.
+ Permissions on files are set properly.
+ Package has a %clean section, which contains rm -rf $RPM_BUILD_ROOT.
+ Package consistently uses macros.
+ Does not contain large documentation files.
+ Includes only doc files in %doc.
+ Includes %{name}.desktop file. Properly installed with desktop-file-install.
+ Package does not own files or directories already owned by other packages.
+ At the beginning of %install, package runs rm -rf $RPM_BUILD_ROOT.
+ All filenames in rpm packages are valid UTF-8.
+ Package builds in mock.
+ Package functions as described.

Unfortunately, upstream's make system is made in such way so it doesn't
support paralleling - it compresses one file per time. Perhaps, this
should be addressed upstream.


This package is APPROVED.

(In reply to comment #19)
> I went forward and did the review. Here it is with few comments:
[...]
> + The License field in the package spec file matches the actual license.
> 
> That is providing, that it'll be changed to "CC-BY and Freely redistributable
> without restriction".

Will do, upon import.

> Source command to export SVN is actually wrong. It should have either "-r 9"
> or "@9" but not combination of those.

Right. It should have been -r9 ..@9 (meaning revision 9 of what was at this patch in revision 9).

> Unfortunately, upstream's make system is made in such way so it doesn't
> support paralleling - it compresses one file per time. Perhaps, this
> should be addressed upstream.

This would not be hard to fix, and I may eventually do that in future (either in make, or just modifying the python script to run number to threads equal to CPU cores). What would be tricky is compression of the png images into jpeg, because we only keep those, which are smaller than originals, therefore we decide whether a target is needed only after we've already remade it.

> This package is APPROVED.  

Thanks a lot for review!

New Package CVS Request
=======================
Package Name: yofrankie-bge
Short Description: 3D Game with characters from Big Buck Bunny movie
Owners: lkundrak
Branches: F-10 EL-5

cvs done.

Imported & built & consumed.