Bug 398791 (BlockOutII)

Does not build. :(
http://koji.fedoraproject.org/koji/taskinfo?taskID=257881

Hans, another quick comment with respect to your regeneration comments,

* cvs -z3 -d:pserver:anonymous.sourceforge.net:/cvsroot/blockout co
-P blockout

This line should probably include date or a tag to ensure that the same sources
are checked out at any point in the future.

* mv blockout %{name}-%{version}

Please remove the %{name}-%{version} from this line

* <convert BlockOut/sounds/music.mp3 to ogg>

How do I perform this step??

(In reply to comment #1)
> Does not build. :(
> http://koji.fedoraproject.org/koji/taskinfo?taskID=257881

Hmm, oops I've patched it to use the system version of libpng instead of the
included copy, but I forgot to add a BuildRequires for libpng-devel

(In reply to comment #2)
> Hans, another quick comment with respect to your regeneration comments,
> 
> * cvs -z3 -d:pserver:anonymous.sourceforge.net:/cvsroot/blockout co
> -P blockout
> 
> This line should probably include date or a tag to ensure that the same sources
> are checked out at any point in the future.
> 

Fixed

> * mv blockout %{name}-%{version}
> 
> Please remove the %{name}-%{version} from this line
> 

? You mean explicitly type the name and version instead of using macros? That
will most likely get stale when the version changes.

> * <convert BlockOut/sounds/music.mp3 to ogg>
> 
> How do I perform this step??

I use mp32ogg (simple perl script, all deps are in fedora except for mpg321),
but I'm leaving this open to the reader as there is no supported way todo this
under Fedora.

Here is a new fixed version:
Spec URL: http://people.atrpms.net/~hdegoede/BlockOutII.spec
SRPM URL: http://people.atrpms.net/~hdegoede/BlockOutII-2.3-2.fc9.src.rpm

(In reply to comment #3)
> (In reply to comment #2)
> > * mv blockout %{name}-%{version}
> > 
> > Please remove the %{name}-%{version} from this line
> > 
> 
> ? You mean explicitly type the name and version instead of using macros? That
> will most likely get stale when the version changes.

Yes, there is no point at all in keeping the %{name}-%{version} in the comments
field, it does not get filled in by rpmbuild, and the only purpose of the
comments is to allow people to easily cut and paste from the comments to the
shell.  By using %{name}-%{version} you make it difficult to do this.

# mv blockout %{name}-%{version}
# cd BlockOutII-2.3

Please change the mv line to match the cd line, that is, replace the
%{name}-%{version} with BlockOutII-2.3

I don't expect you to keep bumping the versions in the comments, but someone
cutting and pasting will have an easier time of it.  Thanks.


> 
> > * <convert BlockOut/sounds/music.mp3 to ogg>
> > 
> > How do I perform this step??
> 
> I use mp32ogg (simple perl script, all deps are in fedora except for mpg321),
> but I'm leaving this open to the reader as there is no supported way todo this
> under Fedora.

Okay, I would suggest to go ahead and add a reference to mpg321 in the comments
for now, and try to get upstream to make the conversion there.

Thanks for fixing the build error, I'll try and complete the review sometime today.

oops I completely spaced on this, starting review now...

ugh, I'm getting an rpmlint error about the desktop file not having an Encoding
entry, this should be easy to fix, but thats not the main problem.

First I tried in full screen mode adjusting the resolution in the game, and
1024x768 does not seem to work properly for me, but also when you quit the game,
it does not restore your desktop to the proper resolution.

It was a total pain trying to get my desktop back to its proper resolution, and
I also noticed that half of my windows were vertically maximized.

If there is any way you can take a look at this, that would be great.  This
seems similar to the gcompis bug I ran into.

(In reply to comment #6)
> ugh, I'm getting an rpmlint error about the desktop file not having an Encoding
> entry, this should be easy to fix, but thats not the main problem.
> 

Then you're using an old Fedora version todo this on explicitly stating an
encoding is deprecated now.

> First I tried in full screen mode adjusting the resolution in the game, and
> 1024x768 does not seem to work properly for me, but also when you quit the game,
> it does not restore your desktop to the proper resolution.
> 

Good catch, fixed:
Spec URL: http://people.atrpms.net/~hdegoede/BlockOutII.spec
SRPM URL: http://people.atrpms.net/~hdegoede/BlockOutII-2.3-3.fc9.src.rpm

(In reply to comment #7)
> Then you're using an old Fedora version todo this on explicitly stating an
> encoding is deprecated now.

Do you have a reference to where this is documented?  desktop-file-validate
needs to be fixed on F-7 if this is the case.  Please file a bug if you have the
documentation.

==== REVIEW CHECKLIST ====
- rpmlint output
BlockOutII.x86_64: E: invalid-desktopfile
/usr/share/applications/fedora-BlockOutII.desktop

see comment #8

- package named according to package naming guidelines
- spec filename matches %{name}
- package meets packaging guidelines
- package licensed with fedora approved license
- license matches actual license
- license file in %doc
- written in American english
- spec file legible
- sources match upstream:
$ diff -ur BlockOutII-2.3/ ../SOURCES/BlockOutII-2.3
Only in BlockOutII-2.3/BlockOut/sounds: music.mp3
Only in ../SOURCES/BlockOutII-2.3/BlockOut/sounds: music.ogg
- successfully compiles and builds on F-7 x86_64
- All dependencies listed in BR
- no locales
- no shared libraries
- package is not relocatable
- package owns all directories it creates
- all other directories brought in from requires
- no duplicates in %files
- contains proper %clean
- macro usage is consistent
- contains code
- no large documentation
- no header files
- no static libraries
- no pkgconfig files
- package contains proper desktop file
see comment #7
- package does not own files or directories owned by other packages
- buildroot removed at beginning of %install
- all filenames valid UTF-8

==== SHOULD FIX ====
- investigate why 1024x768 does not display properly in full screen (are you
able to reproduce)
- file bug mention in comment #7

**** APPROVED ****

Thanks for the review! As for the Encoding keyword being deprecated see:
http://standards.freedesktop.org/desktop-entry-spec/desktop-entry-spec-latest.html#deprecated-items


New Package CVS Request
=======================
Package Name:      BlockOutII
Short Description: A free adaptation of the original BlockOut DOS game
Owners:            jwrdegoede
Branches:          F-7 F-8 devel
InitialCC:         <empty>
Cvsextras Commits: Yes

cvs done.

I've managed to reproduce the 1024x768 problem, seems to be ati and / or 64 bit
specific as I couldn't reproduce it on my work system. Its fixed now (I believe
this really is a bug in the new xrandr supporting ati driver, but its easy to
work around).

Imported and build, closing.

Note the imported version contains a fix for the 1024x768 problem.

One last comment, should this game use the opengl wrapper script?

(In reply to comment #14)
> One last comment, should this game use the opengl wrapper script?

Good point a fixed version is on its way to rawhide.