Bug 507030 - Review Request: pymunk - Python wrapper for the chipmunk 2D physics engine
Review Request: pymunk - Python wrapper for the chipmunk 2D physics engine
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Susi Lehtola
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-06-19 17:56 EDT by Rob Spanton
Modified: 2010-01-01 17:58 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-01-01 17:58:56 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
susi.lehtola: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Rob Spanton 2009-06-19 17:56:57 EDT
Spec URL: http://users.ecs.soton.ac.uk/rds/rpm/pymunk/pymunk.spec
SRPM URL: http://users.ecs.soton.ac.uk/rds/rpm/pymunk/pymunk-0.8.2-1.fc11.src.rpm
Description: I just packaged pymunk.  I think it complies with all the regs.

Pymunk is a Python wrapper for the wrapper for the chipmunk 2D physics
engine.

A review would be marvellous :-D
Comment 1 Susi Lehtola 2009-06-21 07:11:46 EDT
- Your patch "0001-Use-the-chipmunk-shared-library-from-the-chipmunk-pa.patch" is incorrectly named. The name should be pymunk-(something).patch, e.g. pymunk-sharedlib.patch.

- You need to change the patch from
 libfn = "/usr/lib/libchipmunk.so"
to e.g.
 lib=`ls %{_libdir}/libchipmunk.so.*`
 sed -i "s|/usr/lib/libchipmunk.so|$lib|g" pymunk/libload.py
in the setup phase, since
 a) the directory is wrong on multiarch systems
 b) the unversioned .so file symlink is provided by chipmunk-devel.

- You can drop BR: python-setuptools if you're not targetting EPEL.

- I'd remove completely the chipmunk_src directory in %setup:
 rm -rf chipmunk_src/

- Add README.txt, LICENSE.txt, PKG-INFO and THANKS.txt to %doc.

- Change
 %{python_sitelib}/*
to
 %{python_sitelib}/%{name}/
 %{python_sitelib}/%{name}-*.egg-info/

- Drop CFLAGS="$RPM_OPT_FLAGS" from %build, as this is a noarch package.

- Fix the newlines in %setup.
Comment 2 Rob Spanton 2009-06-21 11:18:00 EDT
Cheers Jussi,

I've done the changes you mention.  There are a few things I'd like to query:

 - I can't find anything about patch naming in the packaging guidelines.

 - I think I do need python-setuptools as a BuildRequires, as setup.py requires
   it and it's not listed in the exception list:
     https://fedoraproject.org/wiki/Packaging:Guidelines#Exceptions_2
   Building without python-setuptools results in fail:
     http://koji.fedoraproject.org/koji/getfile?taskID=1427723&name=build.log

 - Why did you require me to make the change to %files, as the RPM ends up owning exactly the same files and directories?

Updated spec: http://users.ecs.soton.ac.uk/rds/rpm/pymunk/pymunk.spec
New SRPM: http://users.ecs.soton.ac.uk/rds/rpm/pymunk/pymunk-0.8.2-2.fc11.src.rpm

Thanks,

Rob
Comment 3 Susi Lehtola 2009-06-21 12:13:54 EDT
(In reply to comment #2)
> Cheers Jussi,
> 
> I've done the changes you mention.  There are a few things I'd like to query:
> 
>  - I can't find anything about patch naming in the packaging guidelines.

This is an unwritten rule, which is really necessary if you have the sources of N packages in the SOURCES directory. If you don't have a reasonable naming system, there's no way to see what sources correspond to which package.

This system of naming is implicit in
https://fedoraproject.org/wiki/How_to_create_an_RPM_package#.25prep_section:_.25patch_commands

>  - I think I do need python-setuptools as a BuildRequires, as setup.py requires
>    it and it's not listed in the exception list:
>      https://fedoraproject.org/wiki/Packaging:Guidelines#Exceptions_2
>    Building without python-setuptools results in fail:
>      http://koji.fedoraproject.org/koji/getfile?taskID=1427723&name=build.log

Oh okay, no problem then.

>  - Why did you require me to make the change to %files, as the RPM ends up
> owning exactly the same files and directories?

For a couple of reasons: clearness and picking up errors. For example if the egg-info is not produced in the build you won't pick it up if you're using wildcards. By specifying the two directories you will know if the build is incomplete.

> 
> Updated spec: http://users.ecs.soton.ac.uk/rds/rpm/pymunk/pymunk.spec
> New SRPM:
> http://users.ecs.soton.ac.uk/rds/rpm/pymunk/pymunk-0.8.2-2.fc11.src.rpm
> 
> Thanks,
> 
> Rob
Comment 4 Susi Lehtola 2009-06-21 12:16:35 EDT
The %doc files 
%doc LICENSE.txt PKG-INFO THANKS.txt README.txt

should go into the main package, since they're small in size and essential for the package. [At least the license is.]
Comment 5 Rob Spanton 2009-06-21 12:46:25 EDT
Cool.  Thanks for clarifying those points for me :-)

I've moved those text files into the main package.

Spec: http://users.ecs.soton.ac.uk/rds/rpm/pymunk/pymunk.spec
SRPM: http://users.ecs.soton.ac.uk/rds/rpm/pymunk/pymunk-0.8.2-3.fc11.src.rpm

Thanks,

Rob
Comment 6 Susi Lehtola 2009-06-21 13:10:42 EDT
- You have to BuildRequires: chipmunk to get the filename of the library:

++ ls '/usr/lib64/libchipmunk.so.*'
++ head -n 1
ls: cannot access /usr/lib64/libchipmunk.so.*: No such file or directory

(Don't BR: chipmunk-devel, since you only want the versioned so file. Make a comment about this.)

- Remove the .so and .dll file from pymunk.egg-info/SOURCES.txt with the sharedlib patch and remove the files in %setup. Then you can drop
 # We use chipmunk's shared library, so get rid of the inbuilt version 
 rm -f $RPM_BUILD_ROOT/%{python_sitelib}/pymunk/{chipmunk.dll,libchipmunk.so}
from %install altogether, since nothing is installed.

- If you want, you can use "python" instead of "%{__python}". (%{__python} evaluates to /usr/bin/python)

- I'm not quite sure if there is a need to have a separate -doc package. After all:
 $ du -hc *
 56K	pymunk-0.8.2-3.fc11.noarch.rpm
 72K	pymunk-doc-0.8.2-3.fc11.noarch.rpm
 128K	total
IMHO 72k is not worthwhile to separate from the main package.

- Examples contain pymunx which is under GPL+ license, this needs to be reflected in the license tag of the package this file is in. If you want to go with a separate -doc package, its license should be "MIT and GPL+", with the main package having MIT license. Otherwise this must be the license of the main package.


Once you have fixed these I will do the full review.
Comment 7 Susi Lehtola 2009-07-05 06:40:48 EDT
ping?
Comment 8 Rob Spanton 2009-07-05 10:05:27 EDT
> ping?

Yep, I'm still on this.  No time slices have been allocated recently, but they will soon ;-)
Comment 9 Susi Lehtola 2009-07-22 08:39:05 EDT
ping
Comment 10 Rob Spanton 2009-07-26 12:53:12 EDT
I hope to get time for this tomorrow.
Comment 11 Susi Lehtola 2009-08-16 05:12:40 EDT
ping
Comment 12 Susi Lehtola 2009-09-07 11:18:13 EDT
ping again?
Comment 13 Rob Spanton 2009-11-04 22:45:20 EST
Hi Jussi,

Sorry about the long delay :-/

I've done the things you said, and I've also bumped up to pymunk 0.8.4

https://fedoraproject.org/wiki/Packaging:Python recommends using %{__python} in the sitelib statement, and I think I'd rather stick with the tried and tested method.

Spec: http://users.ecs.soton.ac.uk/rds/rpm/pymunk/pymunk.spec
SRPM: http://users.ecs.soton.ac.uk/rds/rpm/pymunk/pymunk-0.8.4-1.fc11.src.rpm

Cheers,

Rob
Comment 14 Susi Lehtola 2009-11-05 09:38:29 EST
- Fix the tabbing of the version, release, summary and license fields of the spec file, now it looks terrible.

- Please conserve time stamps during sed - instead of
 sed -i -e 's/\r//g' docs/api/*.{html,txt,css,js} examples/*.py *.txt PKG-INFO
run
 for file in docs/api/*.{html,txt,css,js} examples/*.py *.txt PKG-INFO; do
  sed -e  's/\r//g' docs/api/*.{html,txt,css,js} $file > $file.new &&
  touch -r $file $file.new &&
  mv $file.new $file
 done


**

rpmlint output is clean.


MUST: The package does not yet exist in Fedora. The Review Request is not a duplicate. OK

MUST: The spec file for the package is legible and macros are used consistently. NEEDSWORK
- See top of comment.

MUST: The package must be named according to the Package Naming Guidelines. OK
MUST: The spec file name must match the base package %{name}. OK
MUST: The package must be licensed with a Fedora approved license and meet the  Licensing Guidelines. OK
MUST: The License field in the package spec file must match the actual license. OK
MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. OK
MUST: The package MUST successfully compile and build into binary rpms. OK
MUST: The spec file MUST handle locales properly. N/A

MUST: Optflags are used and time stamps preserved. NEEDSWORK
- See above.

MUST: Packages containing shared library files must call ldconfig. N/A
MUST: A package must own all directories that it creates or require the package that owns the directory. OK
MUST: Files only listed once in %files listings. OK
MUST: Debuginfo package is complete. N/A
MUST: Permissions on files must be set properly. OK
MUST: Clean section exists. OK
MUST: Large documentation files must go in a -doc subpackage. N/A
MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. OK
MUST: Header files must be in a -devel package. N/A
MUST: Static libraries must be in a -static package. N/A
MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. N/A
MUST: If a package contains library files with a suffix then library files ending in .so must go in a -devel package. N/A
MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency. N/A
MUST: Packages does not contain any .la libtool archives. N/A
MUST: Desktop files are installed properly. N/A
MUST: No file conflicts with other packages and no general names. OK
MUST: Buildroot cleaned before install. OK
SHOULD: %{?dist} tag is used in release. OK
SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. OK
SHOULD: The package builds in mock. OK
Comment 15 Rob Spanton 2009-11-05 11:31:35 EST
> Fix the tabbing of the version, release, summary and license fields of the
spec file, now it looks terrible.

Not sure what you mean here.  I can't see anything wrong with these fields -- they're all indented using tabs.  rpmlint checks for this sort of thing too, and doesn't output any warnings or errors about it (deliberately injecting some spaces dos).

I've done the timestamp thing.  

New SRPM: http://users.ecs.soton.ac.uk/rds/rpm/pymunk/pymunk-0.8.4-2.fc11.src.rpm
Spec: http://users.ecs.soton.ac.uk/rds/rpm/pymunk/pymunk.spec

Cheers,

Rob
Comment 16 Susi Lehtola 2009-11-05 12:22:09 EST
For me in chromium 

Name:		pymunk
Version:	0.8.4
Release:	2%{?dist}
Summary:	Python wrapper for the chipmunk 2D physics engine
Group:		Development/Languages
# pymunx is under GPL+
License:	MIT and GPL+
URL:		http://code.google.com/p/pymunk/
Source0:	http://pymunk.googlecode.com/files/pymunk-%{version}.zip
# Use the shared library provided by the chipmunk package
Patch0:		pymunk-sharedlib.patch

so there's clearly something wrong. Have you changed your editor's tab width setting? Please set the width to 8 characters, or convert the tabs into spaces so that the spec file looks nice by default.
Comment 17 Rob Spanton 2009-11-05 12:41:19 EST
I think the problem is in your chromium.  Here's a set of screenshots in a variety of text editors and browsers on my system:
http://users.ecs.soton.ac.uk/rds/rpm/pymunk/whitespace/

Note that *all* of them are aligned correctly.

Also, you can look at the contents of the file in a hex-editor if you like to see that the tab characters are there.

Rob
Comment 18 Susi Lehtola 2009-11-05 13:14:18 EST
Well, I'll be. Mea culpa :)

APPROVED
Comment 19 Rob Spanton 2009-11-05 13:21:19 EST
New Package CVS Request
=======================
Package Name: pymunk
Short Description: Python wrapper for the chipmunk 2D physics engine
Owners: rspanton
Branches: F-11 F-12
InitialCC:
Comment 20 Kevin Fenzi 2009-11-06 15:23:55 EST
cvs done.
Comment 21 Susi Lehtola 2010-01-01 17:58:56 EST
Seems like you forgot to mark this bug as solved by the first release of the package.

Closing now.

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