[Bug 507030] New: Review Request: pymunk - Python wrapper for the chipmunk 2D physics engine

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

Summary: Review Request: pymunk - Python wrapper for the chipmunk 2D physics engine

https://bugzilla.redhat.com/show_bug.cgi?id=507030

Summary: Review Request: pymunk - Python wrapper for the
                    chipmunk 2D physics engine
           Product: Fedora
           Version: rawhide
          Platform: All
        OS/Version: Linux
            Status: NEW
          Severity: medium
          Priority: medium
         Component: Package Review
        AssignedTo: [e-mail]
        ReportedBy: [e-mail]
         QAContact: [e-mail]
                CC: [e-mail], [e-mail]
   Estimated Hours: 0.0
    Classification: Fedora

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

[Bug 507030] Review Request: pymunk - Python wrapper for the chipmunk 2D physics engine

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=507030

Jussi Lehtola  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
                 CC|                            |[e-mail]
         AssignedTo|[e-mail]    |[e-mail]
               Flag|                            |fedora-review?

--- Comment #1 from Jussi 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.

[Bug 507030] Review Request: pymunk - Python wrapper for the chipmunk 2D physics engine

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=507030

--- Comment #2 from Robert 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

[Bug 507030] Review Request: pymunk - Python wrapper for the chipmunk 2D physics engine

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=507030

--- Comment #3 from Jussi 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:...
> 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

[Bug 507030] Review Request: pymunk - Python wrapper for the chipmunk 2D physics engine

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=507030

--- Comment #4 from Jussi 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.]

[Bug 507030] Review Request: pymunk - Python wrapper for the chipmunk 2D physics engine

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=507030

--- Comment #5 from Robert 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

[Bug 507030] Review Request: pymunk - Python wrapper for the chipmunk 2D physics engine

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=507030

--- Comment #6 from Jussi 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.

[Bug 507030] Review Request: pymunk - Python wrapper for the chipmunk 2D physics engine

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=507030

--- Comment #7 from Jussi Lehtola   2009-07-05 06:40:48 EDT ---
ping?

[Bug 507030] Review Request: pymunk - Python wrapper for the chipmunk 2D physics engine

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=507030

--- Comment #8 from Robert 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 ;-)

[Bug 507030] Review Request: pymunk - Python wrapper for the chipmunk 2D physics engine

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=507030

--- Comment #9 from Jussi Lehtola   2009-07-22 08:39:05 EDT ---
ping

[Bug 507030] Review Request: pymunk - Python wrapper for the chipmunk 2D physics engine

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=507030

--- Comment #10 from Robert Spanton   2009-07-26 12:53:12 EDT ---
I hope to get time for this tomorrow.

[Bug 507030] Review Request: pymunk - Python wrapper for the chipmunk 2D physics engine

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=507030

--- Comment #11 from Jussi Lehtola   2009-08-16 05:12:40 EDT ---
ping

[Bug 507030] Review Request: pymunk - Python wrapper for the chipmunk 2D physics engine

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=507030

--- Comment #12 from Jussi Lehtola   2009-09-07 11:18:13 EDT ---
ping again?

[Bug 507030] Review Request: pymunk - Python wrapper for the chipmunk 2D physics engine

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=507030

--- Comment #13 from Robert Spanton   2009-11-04 22:45:20 EDT ---
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

[Bug 507030] Review Request: pymunk - Python wrapper for the chipmunk 2D physics engine

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=507030

--- Comment #14 from Jussi Lehtola   2009-11-05 09:38:29 EDT ---
- 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

[Bug 507030] Review Request: pymunk - Python wrapper for the chipmunk 2D physics engine

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=507030

--- Comment #15 from Robert Spanton   2009-11-05 11:31:35 EDT ---
> 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

[Bug 507030] Review Request: pymunk - Python wrapper for the chipmunk 2D physics engine

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=507030

--- Comment #16 from Jussi Lehtola   2009-11-05 12:22:09 EDT ---
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-%%7Bversion%7D.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.

[Bug 507030] Review Request: pymunk - Python wrapper for the chipmunk 2D physics engine

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=507030

--- Comment #17 from Robert Spanton   2009-11-05 12:41:19 EDT ---
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

[Bug 507030] Review Request: pymunk - Python wrapper for the chipmunk 2D physics engine

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=507030

Jussi Lehtola  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|fedora-review?              |fedora-review+

--- Comment #18 from Jussi Lehtola   2009-11-05 13:14:18 EDT ---
Well, I'll be. Mea culpa :)

APPROVED

[Bug 507030] Review Request: pymunk - Python wrapper for the chipmunk 2D physics engine

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=507030

Robert Spanton  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|                            |fedora-cvs?

--- Comment #19 from Robert Spanton   2009-11-05 13:21:19 EDT ---
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:

[Bug 507030] Review Request: pymunk - Python wrapper for the chipmunk 2D physics engine

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=507030

Kevin Fenzi  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|fedora-cvs?                 |fedora-cvs+

--- Comment #20 from Kevin Fenzi   2009-11-06 15:23:55 EDT ---
cvs done.

[Bug 507030] Review Request: pymunk - Python wrapper for the chipmunk 2D physics engine

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=507030

Jussi Lehtola  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |CLOSED
         Resolution|                            |CURRENTRELEASE

--- Comment #21 from Jussi Lehtola   2010-01-01 17:58:56 EDT ---
Seems like you forgot to mark this bug as solved by the first release of the
package.

Closing now.