Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 235588 - Review Request: escape - an extensible puzzle game
Summary: Review Request: escape - an extensible puzzle game
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Gwyn Ciesla
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: 235589
TreeView+ depends on / blocked
 
Reported: 2007-04-07 16:35 UTC by Adam Goode
Modified: 2007-11-30 22:12 UTC (History)
1 user (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-04-22 21:34:11 UTC
gwync: fedora-review+
jwboyer: fedora-cvs+


Attachments (Terms of Use)

Description Adam Goode 2007-04-07 16:35:33 UTC
Spec URL: http://www.spicenitz.org/fedora/escape.spec
SRPM URL: http://www.spicenitz.org/fedora/escape-200704070-1.fc7.src.rpm
Description: 
Escape is a tile-based puzzle game in the style of "Adventures of
Lolo" or "Chip's Challenge." Unlike either of those games, Escape
doesn't rely at all on reflexes--it's all about your brain.

Although Escape comes with hundreds of levels, the game places an
emphasis on the composition of new puzzles. Thus Escape has a
built-in level editor and facilities for automatically sharing
puzzles with other players.

Comment 1 Adam Goode 2007-04-07 22:57:30 UTC
This is mutually dependent on escape-data, also pending review.

Comment 2 Gwyn Ciesla 2007-04-12 17:27:31 UTC
Wiki is down, but this is an initial list of issues:
escape owns all of %{bindir}.  Should only own what it puts there.
Remove X-Fedora category
Icon-cache is not updated on install and remove, see the wiki when it's back up.
Source0 should include the full download URL.

Builds in mock, though.  I'll peek at -data.

Comment 3 Andrea Musuruane 2007-04-13 10:06:26 UTC
You should also add hicolor-icon-theme to Requires.
This is an empty package owning the /usr/share/icons/hicolor dir hierarchy,
sortoff like the filesystem package but then for icons. Note that all packages
which have an icon should Require this to ensure proper icon dir ownership.

Version is wrong. Please read:
http://fedoraproject.org/wiki/Packaging/NamingGuidelines?action=show#head-cfd71146dbb6f00cec9fe3623ea619f843394837

BTW, why do you use a snapshot version (CVS checkout) instead of a stable
release? There may be good reasons, but I'd like to know :)


Comment 4 Gwyn Ciesla 2007-04-13 11:23:47 UTC
(In reply to comment #3)

> BTW, why do you use a snapshot version (CVS checkout) instead of a stable
> release? There may be good reasons, but I'd like to know :)
> 

+1.  Would probably also make the URL issue go away more easily.

Comment 5 Adam Goode 2007-04-13 23:23:00 UTC
New release:

http://www.spicenitz.org/fedora/escape-200704130-1.fc7.src.rpm
http://www.spicenitz.org/fedora/escape.spec

 - Merge -data package into this
 - Install icon into correct place and update the icon cache
 - Use upstream tarball, created by upstream at my request
 - Remove X-Fedora category


Comment 6 Adam Goode 2007-04-14 04:51:26 UTC
Backup location if spicenitz.org is down:

http://www.andrew.cmu.edu/user/agoode/fedora/

Comment 7 Gwyn Ciesla 2007-04-16 15:42:13 UTC
Ok, let's see.

rpmlint -i clean, except for debuginfo, which complains that several source
files are marked executable.  Might want to submit to upstream, but not a
showstopper IMHO:
W: escape-debuginfo spurious-executable-perm /usr/src/debug/escape-src/dirt.h
W: escape-debuginfo spurious-executable-perm /usr/src/debug/escape-src/sound.h
E: escape-debuginfo script-without-shebang /usr/src/debug/escape-src/dirt.cpp
E: escape-debuginfo script-without-shebang /usr/src/debug/escape-src/sound.cpp
W: escape-debuginfo spurious-executable-perm /usr/src/debug/escape-src/progress.h
W: escape-debuginfo spurious-executable-perm /usr/src/debug/escape-src/sound_enum.h
E: escape-debuginfo script-without-shebang /usr/src/debug/escape-src/progress.cpp

Good package/spec names.
Meets packaging guidelines.
License OK.
Spec is legible American English.

[limb@fawkes fedora]$ md5sum escape-src-200704130.tar.bz2 
07263976a54607792dbbe4bc442f0735  escape-src-200704130.tar.bz2
[limb@fawkes fedora]$ md5sum /usr/src/redhat/SOURCES/escape-src-200704130.tar.bz2 
1764ef8aad634e6f71c5533fc7eafe21 
/usr/src/redhat/SOURCES/escape-src-200704130.tar.bz2

ISSUE: MD5 mismatch.  Put the tarball from upstream straight in the SRPM, no
modification.

Comment 8 Andrea Musuruane 2007-04-16 16:05:33 UTC
If I may add, even if I'm not the reviewer, the version is wrong. 

Using the release or snapshot date as the version is going to cause you trouble
when version x.y will be released. E.g. 200704130 > 1.0. 

If a stable release is not yet planned upstream, and therefore it is not
possibile to identify this as a prerelease of a planned release, I would suggest
to use 0.0 as version and 0.release.200704130pre as the release.

For a complete overview, please read from the following link onwards.

http://fedoraproject.org/wiki/Packaging/NamingGuidelines?action=show&redirect=PackageNamingGuidelines#head-18aa467fc6925455e44be682fd336667a17e8933


Comment 9 Gwyn Ciesla 2007-04-16 16:09:39 UTC
Good point.  What are upstream's plans?  If they never plan on a sane versioning
system 2007x would work, but even so I agree with Andrea.

Comment 10 Adam Goode 2007-04-16 16:55:27 UTC
I'll have to look into the MD5 mismatch thing later tonight. Something weird has
happened.

As for the version, upstream has no plans to move from date-based version
number. So for now the version number is correct. If upstream changes its
scheme, that's what epoch is for.

Comment 11 Adam Goode 2007-04-17 02:40:09 UTC
http://www.spicenitz.org/fedora/escape-200704130-2.fc7.src.rpm
http://www.spicenitz.org/fedora/escape.spec

 - Fix permissions in debuginfo package
 - Generate SRPM with source matching upstream MD5


Comment 12 Gwyn Ciesla 2007-04-17 11:41:52 UTC
MD5 matches.

Still some -debug perm issues.

W: escape-debuginfo spurious-executable-perm /usr/src/debug/escape-src/dirt.h
W: escape-debuginfo spurious-executable-perm /usr/src/debug/escape-src/sound.h
W: escape-debuginfo spurious-executable-perm /usr/src/debug/escape-src/progress.h
W: escape-debuginfo spurious-executable-perm /usr/src/debug/escape-src/sound_enum.h


Comment 13 Hans de Goede 2007-04-17 18:05:47 UTC
Jon, you should set the fedora review flag to ? when doing a review.


Comment 14 Gwyn Ciesla 2007-04-17 18:18:57 UTC
Whoops.  Thanks. :)

Comment 15 Adam Goode 2007-04-17 23:08:17 UTC
New version:

http://www.spicenitz.org/fedora/escape.spec
http://www.spicenitz.org/fedora/escape-200704130-3.fc7.src.rpm

Changes:

 - REALLY fix spurious-executable-perm


Comment 16 Gwyn Ciesla 2007-04-18 12:56:08 UTC
(In reply to comment #15)
>
>  - REALLY fix spurious-executable-perm
> 
Confirmed. :)

Builds on FC6 1386.
BRs are good.
No shared libs.
Not relocatable.
ISSUE: Ownership is OK, except for the .desktop line.  It should own it's own
desktop file, not all of them (*.desktop).

Comment 17 Adam Goode 2007-04-19 01:35:21 UTC
What's wrong with having something like this?

%files
%defattr(-,root,root,-)
%doc COPYING design.txt escape.txt README
%{_bindir}/*
%{_datadir}/%{name}
%{_datadir}/icons/hicolor/32x32/apps/*.png
%{_datadir}/applications/*.desktop


The * doesn't mean I own everything, just the files that I installed to
$RPM_BUILD_ROOT. This can be verified using rpm -ql escape.

Comment 18 Gwyn Ciesla 2007-04-19 11:58:33 UTC
Oh, so it is.  I didn't know that.  Nevermind. :)
No duplicate files.
Permissions are ok.
%clean present and correct.
Macros OK.
Code, not content.
No huge docs.
No runtime doc issues.
No shared or static libs, or headers.
No pkgconfig files.
No devel package.
No .la.
.desktop file handled properly.
No cross-ownership.
%install starts right.
Good filenames.

All MUSTS met.

Builds in mock of FC-6 i386.
Runs as advertised.
APPROVED.

Comment 19 Adam Goode 2007-04-20 00:56:52 UTC
New Package CVS Request
=======================
Package Name: escape
Short Description: an extensible puzzle game
Owners: adam@spicenitz.org
Branches: FC-5 FC-6
InitialCC: 


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