Bug 474149 - Review Request: chipmunk - A rigid body physics library
Summary: Review Request: chipmunk - A rigid body physics library
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
Target Milestone: ---
Assignee: Conrad Meyer
QA Contact: Fedora Extras Quality Assurance
Depends On:
TreeView+ depends on / blocked
Reported: 2008-12-02 14:48 UTC by Gwyn Ciesla
Modified: 2009-01-15 02:58 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Last Closed: 2009-01-12 18:57:48 UTC
Type: ---
cse.cem+redhatbugz: fedora-review+
kevin: fedora-cvs+

Attachments (Terms of Use)
Review. (9.17 KB, text/plain)
2009-01-01 23:45 UTC, Conrad Meyer
no flags Details

Description Gwyn Ciesla 2008-12-02 14:48:24 UTC
    *  Designed for 2D video games.
    * Circle, convex polygon, and line segment collision primitives.
    * Multiple collision primitives can be attached to the same rigid body.
    * Fast collision detection by using a spatial hash for the broad phase.
    * Extremely fast impulse solving by utilizing Erin Catto’s contact persistence algorithm.
    * Support for collision callbacks based on object types.
    * Impulses applied to contact points can be retrieved after the impulse solver has finished.
    * Several kinds of joints available.
    * C99 implementation, no external dependencies.
    * Ruby extension available.
    * Simple, read the documentation.
    * Unrestrictive MIT license.

SRPM URL: http://zanoni.jcomserv.net/fedora/chipmunk/chipmunk-4.1.0-1.fc9.src.rpm
SPEC URL: http://zanoni.jcomserv.net/fedora/chipmunk/chipmunk.spec

Comment 1 Conrad Meyer 2009-01-01 23:45:31 UTC
Created attachment 328048 [details]

Attached is my initial review. Please fix the problems indicated by rpmlint; also, I have a question. Otherwise looks good.

Comment 2 Gwyn Ciesla 2009-01-02 13:38:43 UTC
The comment about *.so applies to *.so.*.

I'm actually not sure how to correct the debuginfo issue, which I was hoping to find someone to assist with.  I see strip mentioned a few places in the source tree, but my cmake-fu is insufficient to finding the most clueful way to remove it.

What's the question?

Comment 3 Conrad Meyer 2009-01-02 13:47:04 UTC
(In reply to comment #2)
> The comment about *.so applies to *.so.*.

(This was my question.)

> I'm actually not sure how to correct the debuginfo issue, which I was hoping to
> find someone to assist with.  I see strip mentioned a few places in the source
> tree, but my cmake-fu is insufficient to finding the most clueful way to remove
> it.

Probably easiest just to remove strip wherever it's mentioned (my cmake-fu is no better than yours).

Comment 4 Gwyn Ciesla 2009-01-07 16:04:22 UTC
SRPM URL: http://zanoni.jcomserv.net/fedora/chipmunk/chipmunk-4.1.0-2.fc10.src.rpm
SPEC URL: http://zanoni.jcomserv.net/fedora/chipmunk/chipmunk.spec

Fixed it, but now it complains that the ruby lib is unstripped.  I don't see anything in the ruby packaging guidelines that would help.

Comment 5 Conrad Meyer 2009-01-07 20:09:49 UTC
Some more comments:
- Should this be System Environment/Libraries (and not Development/Libraries)?
- Line 46 of the spec has a tab instead of spaces.
- Line 61 should be %{cmake} . (sets everything automatically).
- Line 72, the mode should be 755 not 644. Rpm only strips 755 files when looking for debuginfo, this is why you are getting the warnings about unstripped binaries.
- Line 88, you should use %{_libdir} instead of /usr/lib
- Line 93, this should be "%{_includedir}/chipmunk" not "%{_includedir}/chipmunk/*.h" (the latter leaves an unowned directory).
- Line 87, should be "...*.so.*" (as opposed to "...*.so*"). This is because un-suffixed .so files belong in the -devel package.
- Add %{_libdir}/*.so to %files devel.

Comment 6 Conrad Meyer 2009-01-07 20:10:53 UTC
Oh, and line 72, you might want to replace ".so.4" with ".so.*" so that your spec doesn't need to be updated when the SONAME gets bumped upstream.

Comment 8 Conrad Meyer 2009-01-07 20:55:45 UTC
Looks good to me. APPROVED.

Comment 9 Conrad Meyer 2009-01-07 21:00:54 UTC
Um, oops, this doesn't build in mock yet:
447 Installed (but unpackaged) file(s) found:
448    /usr/lib/libchipmunk.a
449    /usr/lib/libchipmunk.so
450    /usr/lib/libchipmunk.so.4

It seems that your package is somehow installing libraries to /usr/lib instead of %{_libdir}; please fix this before importing it to CVS. Thanks.

Comment 10 Gwyn Ciesla 2009-01-07 21:02:51 UTC
Excellent, thanks!

New Package CVS Request
Package Name: chipmunk
Short Description: A rigid body physics library
Owners: limb
Branches: F-10

Comment 11 Gwyn Ciesla 2009-01-07 21:10:50 UTC
Will do, good catch.

Comment 12 Gwyn Ciesla 2009-01-07 21:30:16 UTC
Weird, my mock build works.  Macros seem ok, I'll see what happened after import.

Comment 13 Conrad Meyer 2009-01-07 23:35:27 UTC
(In reply to comment #12)
> Weird, my mock build works.  Macros seem ok, I'll see what happened after
> import.

I'm on x86_64. %{_libdir} != /usr/lib here, which is why rpmbuild (on x86_64) catches it.

Comment 15 Conrad Meyer 2009-01-08 17:32:51 UTC
(In reply to comment #14)
> %if "%{?_lib}" == "lib64" 
>   %{cmake} -DLIB_SUFFIX=64
> %else
>   %{cmake}
> %endif 

%{cmake} already expands to something like this:
  CFLAGS="${CFLAGS:--O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic}" ; export CFLAGS ; 
  CXXFLAGS="${CXXFLAGS:--O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic}" ; export CXXFLAGS ; 
  FFLAGS="${FFLAGS:--O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic}" ; export FFLAGS ; 
  /usr/bin/cmake \
        -DCMAKE_INSTALL_LIBDIR:PATH=/usr/lib64 \
        -DINCLUDE_INSTALL_DIR:PATH=/usr/include \
        -DLIB_INSTALL_DIR:PATH=/usr/lib64 \
        -DSHARE_INSTALL_PREFIX:PATH=/usr/share \
%if "lib64" == "lib64" 
        -DLIB_SUFFIX=64 \

So I think what is passed to cmake is not the problem. As a "hack" solution you could move stuff from /usr/lib to %{_libdir} in %install, or else fix the CMakeLists.txt files.

But this is already approved and you will have trouble building in koji until this is fixed anyways, so no need to check back in with me unless you want help :).

Comment 16 Conrad Meyer 2009-01-08 17:33:44 UTC
Or rather, it expands to that on lib64 arches. But you get the idea.

Comment 17 Kevin Fenzi 2009-01-09 05:54:23 UTC
cvs done.

Comment 18 Gwyn Ciesla 2009-01-12 18:57:48 UTC
I got it to build, but it's the ugly mv in %install method.  Bleah.

Imported and build in rawhide.

Comment 19 Fedora Update System 2009-01-12 19:05:14 UTC
chipmunk-4.1.0-4.fc10 has been submitted as an update for Fedora 10.

Comment 20 Conrad Meyer 2009-01-12 19:48:28 UTC
Any chance you want to work with the maintainer of xmoto to get xmoto 0.5 to use the system chipmunk?

Comment 21 Conrad Meyer 2009-01-12 19:52:29 UTC
Just kidding, it appears you are the xmoto maintainer. Good job :D.

Comment 22 Gwyn Ciesla 2009-01-12 19:57:36 UTC
Had a reply half typed to #20 when I saw #21. . .:)

I've made a few attempts, but it looks like a lot of paths might reply on the presence of the bundled version.  I'm in conversation with upstream about it.  They have no problem using the system version, it's not not in any distros.  I let them know Fedora has it now, so we'll see what develops.  If I get it working before then, I will of course patch.

Comment 23 Conrad Meyer 2009-01-12 20:10:40 UTC
I think there is a packaging guideline that is something along the lines of: MUST not use private copy of system libraries. So please do work this out with upstream, or failing that hack up a patch.

Comment 24 Gwyn Ciesla 2009-01-12 20:18:45 UTC
I think it's actually a SHOULD, but I feel pretty strongly about it and have done quite a bit with several of my PHP packages to that end.  The sooner the better.

Comment 25 Fedora Update System 2009-01-15 02:58:47 UTC
chipmunk-4.1.0-4.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

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