Bug 235588 - Review Request: escape - an extensible puzzle game
Review Request: escape - an extensible puzzle game
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jon Ciesla
Fedora Package Reviews List
:
Depends On:
Blocks: 235589
  Show dependency treegraph
 
Reported: 2007-04-07 12:35 EDT by Adam Goode
Modified: 2007-11-30 17:12 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-04-22 17:34:11 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
limburgher: fedora‑review+
jwboyer: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Adam Goode 2007-04-07 12:35:33 EDT
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 18:57:30 EDT
This is mutually dependent on escape-data, also pending review.
Comment 2 Jon Ciesla 2007-04-12 13:27:31 EDT
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 06:06:26 EDT
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 Jon Ciesla 2007-04-13 07:23:47 EDT
(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 19:23:00 EDT
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 00:51:26 EDT
Backup location if spicenitz.org is down:

http://www.andrew.cmu.edu/user/agoode/fedora/
Comment 7 Jon Ciesla 2007-04-16 11:42:13 EDT
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 12:05:33 EDT
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 Jon Ciesla 2007-04-16 12:09:39 EDT
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 12:55:27 EDT
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-16 22:40:09 EDT
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 Jon Ciesla 2007-04-17 07:41:52 EDT
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 14:05:47 EDT
Jon, you should set the fedora review flag to ? when doing a review.
Comment 14 Jon Ciesla 2007-04-17 14:18:57 EDT
Whoops.  Thanks. :)
Comment 15 Adam Goode 2007-04-17 19:08:17 EDT
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 Jon Ciesla 2007-04-18 08:56:08 EDT
(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-18 21:35:21 EDT
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 Jon Ciesla 2007-04-19 07:58:33 EDT
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-19 20:56:52 EDT
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.