- 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.

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

(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

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.]

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

- 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.

ping?

> ping?

Yep, I'm still on this.  No time slices have been allocated recently, but they will soon ;-)

ping

I hope to get time for this tomorrow.

ping

ping again?

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

- 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

> 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

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.

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

Well, I'll be. Mea culpa :)

APPROVED

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:

cvs done.

Seems like you forgot to mark this bug as solved by the first release of the package.

Closing now.