Hello there,

I am an unsponsored reviewer, so this is an unofficial review, but I've done my best.  Any of the MUST/SHOULD guidelines which passed I haven't included here (for the sake of space) but I did test them.

>>> MUST: rpmlint must be run on the source rpm and all binary rpms the build produces. The output should be posted in the review.

Runs just fine:
$ rpmlint -v sugar-castle.spec
sugar-castle.spec: I: checking-url http://mirror.aarnet.edu.au/pub/sugarlabs/activities/4397/castle-23.xo (timeout 10 seconds)
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

>>> MUST: The License field in the package spec file must match the actual license. [3]
>>> MUST: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc.[4]

The spec file lists GPLv3+, but nowhere in the source is a license mentioned, nor is a license included with the package.  Perhaps upstream should be contacted for clarity?

>>> MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture.

Builds okay:
http://koji.fedoraproject.org/koji/taskinfo?taskID=4343387

>>> MUST: Permissions on files must be set properly. Executables should be set with executable permissions, for example.

All of the resource files (data/ and activity/) are marked executable unnecessarily.  It seems as though activity.svg must be executable, though, because '#!/usr/bin/python' is being put into it.

>>> MUST: Each package must consistently use macros.

%{buildroot} and $RPM_BUILD_ROOT are used interchangeably; just pick one of the two and use it consistently:
> %install
> rm -rf $RPM_BUILD_ROOT
> %{__python} ./setup.py install --prefix=%{buildroot}/%{_prefix}

Also, where you use 'sed -i -e '1i#!/usr/bin/python', maybe it would be good to use %{__python} instead of /usr/bin/python.

>>> SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it.

License information from upstream does not seem to be available (this is mentioned earlier).

>>> SHOULD: The reviewer should test that the package builds in mock.

Builds okay.
http://koji.fedoraproject.org/koji/taskinfo?taskID=4343387

>>> SHOULD: The package should compile and build into binary rpms on all supported architectures.

This is noarch so I assume that is not strictly necessary.

>>> SHOULD: The reviewer should test that the package functions as described. A package should not segfault instead of running, for example.

I attempted to call the executable with:

$ /usr/share/sugar/activities/Castle.activity/Castle.py

which may not be correct, so I apologize if that was the wrong way to call it.  I found that 'pygame' is an unlisted dependency.  Once that was installed, I seemed to run into a path issue:

----
Peter says: Can't find data/pointer.png
Traceback (most recent call last):
  File "/usr/share/sugar/activities/Castle.activity/Castle.py", line 219, in <module>
    game.run()
  File "/usr/share/sugar/activities/Castle.activity/Castle.py", line 145, in run
    g.init()
  File "/usr/share/sugar/activities/Castle.activity/g.py", line 78, in init
    pointer=utils.load_image('pointer.png',True)
  File "/usr/share/sugar/activities/Castle.activity/utils.py", line 56, in load_image
    print "Peter says: Can't find "+fname; exit()
  File "/usr/share/sugar/activities/Castle.activity/utils.py", line 10, in exit
    save()
  File "/usr/share/sugar/activities/Castle.activity/utils.py", line 20, in save
    f=open(fname, 'w')
IOError: [Errno 2] No such file or directory: 'data/castle.dat'
----

It is possible that my invocation of the program is incorrect, and when done properly some path-like variable is set correctly and this is not a problem.  

>>> SHOULD: If scriptlets are used, those scriptlets must be sane. This is vague, and left up to the reviewers judgement to determine sanity.

I think that maybe a patch file should be used instead of sed to get the '#!/usr/bin/python' in there.  If upstream changes how things work, the patch will then probably fail while the sed expressions would continue happily along when maybe they shouldn't.

>>> SHOULD: your package should contain man pages for binaries/scripts. If it doesn't, work with upstream to add them where they make sense.

I am not sure how applicable man pages are in this context, but none are provided.

Hopefully my review is helpful.  I apologize in advance for any errors I've made.

Castle.py contain following text:

"This program is free software: you can redistribute it and/or modify it under the terms of the GNU General Public License as published by the Free Software Foundation, either version 3 of the License, or (at your option) any later version."

There was an error for rpmlint while testing .rpm file.
"sugar-castle.noarch: E: script-without-shebang /usr/share/sugar/activities/Castle.activity/activity/activity.svg"

I have added the following entry and found no errors

sed -i -e '1i#!%{__python}' activity/activity.svg

They have mentioned the license in their web page as well. Please check the bellow link under "Source Code License"
http://activities.sugarlabs.org/en-US/sugar/addon/4397

> There was an error for rpmlint while testing .rpm file.
> "sugar-castle.noarch: E: script-without-shebang
> /usr/share/sugar/activities/Castle.activity/activity/activity.svg"
> 
> I have added the following entry and found no errors
> 
> sed -i -e '1i#!%{__python}' activity/activity.svg

A .svg is an image file, is it a script or an icon?

its an icon

(In reply to Danishka Navin from comment #5)
> its an icon

OK, so it's a false positive so can be ignored. rpmlint isn't perfect so there's cases where it reports an error and it's safe to ignore them

Peter,

shall I remove the following entry in the spec file?

sed -i -e '1i#!%{__python}' activity/activity.svg

yes

revert the spec file as per the comment #8

SPEC URL: http://snavin.fedorapeople.org/packages/sugar-castle/sugar-castle.spec

SRPM URL: http://snavin.fedorapeople.org/packages/sugar-castle/sugar-castle-23-2.fc18.src.rpm

reviewing above srpm

1) use BR:python2-devel

2) remove follwing line in spec
rm -rf  {buildroot}

3)Don't use any backslash between %{buildroot} and %{_prefix}, so your %install should look like
%{__python} ./setup.py install --prefix=%{buildroot}%{_prefix}

4) This package archive is really bad in file permissions. You can use following %prep which should address most of rpmlint issues
%setup -q -n Castle.activity
chmod -x *.py
chmod -x data/*.dat
chmod -x  activity/activity.svg
chmod +x setup.py Castle.py

sed -i "s|\r||g" Castle.py
sed -i "s|\r||g" activity/activity.svg

Changed the spec file as per the comment #10

SPEC URL: http://snavin.fedorapeople.org/packages/sugar-castle/sugar-castle.spec

SRPM URL: http://snavin.fedorapeople.org/packages/sugar-castle/sugar-castle-23-3.fc18.src.rpm

Looks good. Just remove the # line in %prep as its on no use in spec file.

APPROVED.

fixed the spec file.

SPEC URL: http://snavin.fedorapeople.org/packages/sugar-castle/sugar-castle.spec

SRPM URL: http://snavin.fedorapeople.org/packages/sugar-castle/sugar-castle-23-4.fc18.src.rpm

you miss to remove actually that line from above spec as well as spec inside srpm

Thanks Parag for catch :)

Here is the corrected files

SPEC URL: http://snavin.fedorapeople.org/packages/sugar-castle/sugar-castle.spec

SRPM URL: http://snavin.fedorapeople.org/packages/sugar-castle/sugar-castle-23-4.fc18.src.rpm

New Package SCM Request
=======================
Package Name: sugar-castle
Short Description: A game of discovery and strategy inspired by the Adventure games of the 70s
Owners: snavin
Branches: f17 f18 f19 f20
InitialCC:

New Package SCM Request
=======================
Package Name: sugar-castle
Short Description: A game of discovery and strategy inspired by the Adventure games of the 70s
Owners: snavin
Branches: f18 f19
InitialCC:

Git done (by process-git-requests).

sugar-castle-23-4.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/sugar-castle-23-4.fc19

sugar-castle-23-4.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/sugar-castle-23-4.fc18

sugar-castle-23-4.fc18 has been pushed to the Fedora 18 testing repository.

Can this be pushed to stable now and closed this review?

sugar-castle-23-4.fc19 has been pushed to the Fedora 19 stable repository.

sugar-castle-23-4.fc18 has been pushed to the Fedora 18 stable repository.