https://bugzilla.redhat.com/show_bug.cgi?id=843678 Ryan Curtin <ryan@xxxxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |ryan@xxxxxxxxxxxx --- Comment #1 from Ryan Curtin <ryan@xxxxxxxxxxxx> --- 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. -- You are receiving this mail because: You are on the CC list for the bug. _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review