Bug 843678

Summary: Review Request: sugar-castle - A game of discovery and strategy inspired by the Adventure games of the 70s
Product: [Fedora] Fedora Reporter: Danishka Navin <danishka>
Component: Package ReviewAssignee: Parag AN(पराग) <panemade>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: unspecified    
Version: rawhideCC: callkalpa, notting, panemade, pbrobinson, ryan
Target Milestone: ---Flags: panemade: fedora-review+
gwync: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: sugar-castle-23-4.fc18 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2013-08-26 22:24:36 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Danishka Navin 2012-07-27 03:11:27 UTC
Spec URL: http://snavin.fedorapeople.org/packages/sugar-castle/sugar-castle.spec

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

Description: 

A game of discovery and strategy inspired by the Adventure games of the 70s.

http://activities.sugarlabs.org/en-US/sugar/addon/4397

Fedora Account System Username:  snavin

Comment 1 Ryan Curtin 2012-07-31 03:54:02 UTC
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.

Comment 2 Danishka Navin 2013-06-06 04:51:09 UTC
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

Comment 3 Danishka Navin 2013-06-06 04:57:51 UTC
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

Comment 4 Peter Robinson 2013-06-06 05:34:51 UTC
> 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?

Comment 5 Danishka Navin 2013-06-06 05:48:31 UTC
its an icon

Comment 6 Peter Robinson 2013-06-06 08:57:05 UTC
(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

Comment 7 Danishka Navin 2013-06-06 09:00:20 UTC
Peter,

shall I remove the following entry in the spec file?

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

Comment 8 Peter Robinson 2013-06-06 09:13:46 UTC
yes

Comment 10 Parag AN(पराग) 2013-08-06 08:09:48 UTC
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

Comment 12 Parag AN(पराग) 2013-08-06 09:01:37 UTC
Looks good. Just remove the # line in %prep as its on no use in spec file.

APPROVED.

Comment 14 Parag AN(पराग) 2013-08-06 09:18:09 UTC
you miss to remove actually that line from above spec as well as spec inside srpm

Comment 15 Danishka Navin 2013-08-06 09:25:14 UTC
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

Comment 16 Danishka Navin 2013-08-06 09:26:17 UTC
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:

Comment 17 Danishka Navin 2013-08-06 10:17:26 UTC
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:

Comment 18 Gwyn Ciesla 2013-08-06 13:08:12 UTC
Git done (by process-git-requests).

Comment 19 Fedora Update System 2013-08-07 01:43:01 UTC
sugar-castle-23-4.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/sugar-castle-23-4.fc19

Comment 20 Fedora Update System 2013-08-07 01:43:14 UTC
sugar-castle-23-4.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/sugar-castle-23-4.fc18

Comment 21 Fedora Update System 2013-08-07 22:57:06 UTC
sugar-castle-23-4.fc18 has been pushed to the Fedora 18 testing repository.

Comment 22 Parag AN(पराग) 2013-08-26 09:14:35 UTC
Can this be pushed to stable now and closed this review?

Comment 23 Fedora Update System 2013-08-26 22:24:36 UTC
sugar-castle-23-4.fc19 has been pushed to the Fedora 19 stable repository.

Comment 24 Fedora Update System 2013-08-26 22:25:32 UTC
sugar-castle-23-4.fc18 has been pushed to the Fedora 18 stable repository.