Bug 398791 - (BlockOutII) Review Request: BlockOutII - A free adaptation of the original BlockOut DOS game
Review Request: BlockOutII - A free adaptation of the original BlockOut DOS game
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Christopher Stone
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-11-25 17:24 EST by Hans de Goede
Modified: 2007-12-01 17:43 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-12-01 04:53:54 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
chris.stone: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Hans de Goede 2007-11-25 17:24:48 EST
Spec URL: http://people.atrpms.net/~hdegoede/BlockOutII.spec
SRPM URL: http://people.atrpms.net/~hdegoede/BlockOutII-2.3-1.fc9.src.rpm
Description:
BlockOut II is a free adaptation of the original BlockOut
DOS game edited by California Dreams in 1989. BlockOut II
has the same features than the original game with few graphic
improvements. The score calculation is also nearly similar to
the original game. BlockOut II has been designed by an addicted
player for addicted players. BlockOut II is an open source
project available for both Windows and Linux.

Blockout is a registered trademark of Kadon Enterprises, Inc.,
used by permission for the BlockOut II application by Jean-Luc
Pons.
Comment 1 Christopher Stone 2007-11-25 17:58:31 EST
Does not build. :(
http://koji.fedoraproject.org/koji/taskinfo?taskID=257881
Comment 2 Christopher Stone 2007-11-25 18:41:44 EST
Hans, another quick comment with respect to your regeneration comments,

* cvs -z3 -d:pserver:anonymous@blockout.cvs.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??
Comment 3 Hans de Goede 2007-11-26 05:38:20 EST
(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@blockout.cvs.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
Comment 4 Christopher Stone 2007-11-26 12:31:57 EST
(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.

Comment 5 Christopher Stone 2007-11-28 20:32:27 EST
oops I completely spaced on this, starting review now...
Comment 6 Christopher Stone 2007-11-28 21:03:52 EST
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.
Comment 7 Hans de Goede 2007-11-29 10:58:54 EST
(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

Comment 8 Christopher Stone 2007-11-30 12:33:18 EST
(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.
Comment 9 Christopher Stone 2007-11-30 12:51:38 EST
==== 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 ****
Comment 10 Hans de Goede 2007-11-30 13:38:05 EST
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
Comment 11 Kevin Fenzi 2007-11-30 14:26:24 EST
cvs done.
Comment 12 Hans de Goede 2007-11-30 15:24:07 EST
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).
Comment 13 Hans de Goede 2007-12-01 04:53:54 EST
Imported and build, closing.

Note the imported version contains a fix for the 1024x768 problem.
Comment 14 Christopher Stone 2007-12-01 10:41:31 EST
One last comment, should this game use the opengl wrapper script?
Comment 15 Hans de Goede 2007-12-01 17:43:49 EST
(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.

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