Bug 476404

Summary: Review Request: bullet - 3D Collision Detection and Rigid Body Dynamics Library
Product: [Fedora] Fedora Reporter: Bruno Mahe <bruno>
Component: Package ReviewAssignee: Mamoru TASAKA <mtasaka>
Status: CLOSED DUPLICATE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: fedora-package-review, itamar, konrad, mtasaka, notting, tcallawa, tim
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2009-04-23 05:43:12 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Bug Depends On:    
Bug Blocks: 201449    
Attachments:
Description Flags
License analysis
none
Licenses in need of audit
none
License in need of audit - no2 none

Description Bruno Mahe 2008-12-14 03:23:01 EST
Hi,

This is my first package and I am looking for a sponsor.


Spec URL: http://www.gnoll.org/download/bullet/bullet.spec
SRPM URL: http://www.gnoll.org/download/bullet/bullet-2.73-1.fc10.src.rpm
Description: Bullet 3D Game Multiphysics Library provides state of the art collision detection, soft body and rigid body dynamics.

Thanks,
Bruno
Comment 1 Conrad Meyer 2008-12-16 04:48:52 EST
Some thoughts:
- Summary would be nice if it could be a bit shorter (I'm not sure if there's policy about this, but it makes it nicer on e.g. PackageKit).
- Group: I would say Development/Libraries (I think System Environment/Libraries is reserved for pretty core parts of Fedora)
- License: Zlib is acceptable in Fedora; I think we like it lowercase, though (http://fedoraproject.org/wiki/Licensing).
- %description: policy is to keep it below 80 characters wide, IIRC.
- In general I think it improves legibility to align the sections with spaces but it's certainly not a requirement.

- If it builds on koji (could you add a link to that build in a comment, please?) then you probably have the BuildRequires. 'make' is unnecessary as it is part of the default build root.
- Same notes as above about Group for -devel subpackage.
- Generally for Summary/%description for -devel subpackages we put something like "Development files for %{name}" and "Development headers and libraries for %{name}."

- %prep: move cmake to %build
- %build: As we talked about on IRC, use %cmake.
- %install: NEED 'rm -rf $RPM_BUILD_ROOT' as the first thing.
- I believe you need ldconfig in %post as well as %postun. rpmlint should warn about not having one of those.

- Same doc files between main package and -devel subpackage is redundant.
- /usr/lib/*.so should be in -devel, not main package.
- Replace literal "/usr/lib" with "%{_libdir}".
- /usr/include/ with %{_includedir}
- %changelog needs a version per entry.

ex:
%changelog
* Mon Dec 15 2008 Bruno Mahe <bruno[AT]gnoll.org> - 2.73-2
- Some changes.

* Sat Dec 13 2008 Bruno Mahe <bruno[AT]gnoll.org> - 2.73-1
- Initial build.
Comment 2 Mamoru TASAKA 2009-01-02 04:35:48 EST
ping?
Comment 3 Bruno Mahe 2009-01-11 20:21:15 EST
pong.

I was on vacation and didn't had time to work on it.
I am back and going to finish it.
Comment 4 Bruno Mahe 2009-01-18 04:47:41 EST
I discovered a bug in the makefiles for 64bit architectures. I will send a patch to upstream and add a patch to the rpm in the mean time.


It is not yet ready for a second review.
Comment 5 Bruno Mahe 2009-01-19 05:28:15 EST
About the issue from my previous message, I have opened a ticket in bullet's ticket tracking system. See http://code.google.com/p/bullet/issues/detail?id=174
I included the patch in the srpm and put a link to the ticket in the spec file as recommended by the packaging guidelines.


I also applied most of/all the suggestion made.

I can successfully build packages locally and through koji (see http://koji.fedoraproject.org/koji/taskinfo?taskID=1065627 for instance)

See http://www.gnoll.org/download/bullet-2.73-2.fc10.src.rpm and 
http://www.gnoll.org/download/bullet.spec

Could someone review it ?
This is my first package and I am looking for a sponsor.

Thanks,
Bruno
Comment 6 Mamoru TASAKA 2009-01-31 13:33:22 EST
Created attachment 330529 [details]
License analysis
Comment 7 Mamoru TASAKA 2009-01-31 13:34:29 EST
Created attachment 330530 [details]
Licenses in need of audit
Comment 8 Mamoru TASAKA 2009-01-31 13:43:49 EST
Scratch build on dist-f11:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1094409

CCing to spot.

To spot:
* Would you audit the licenses in the attached file in my
  comment 7?
  For this package, GPL compatibility check is also needed.
* I found in Fedora Licensing wiki
  http://fedoraproject.org/wiki/Licensing
  that CPL is incompatible with GPLv2. Would you examine
  if CPL is incompatible also with GPLv3?

  If they are incompatible, this package already contains legal
  problem, because for example
--------------------------------------------------------------------
libLinearMath.so.2.73: contains:

src/BulletMultiThreaded/SpuNarrowPhaseCollisionTask/SpuGatheringCollisionTask.cpp .. includes
Extras/software_cache/cache/include/spe_cache.h
=> CPL

Demos/BspDemo/BspLoader.cpp
=> GPLv2+
---------------------------------------------------------------------

To Bruno:
* Would you check if or how files in the attachment in my
  comment 6 which are marked as under "NVIDIA 1/2" are used in
  this software?
Comment 9 Tom "spot" Callaway 2009-01-31 14:23:11 EST
NVIDIA 1 and NVIDIA 2 are non-free. I'm asking RH Legal about the SCEA license.

CPL is incompatible with GPLv3 because it contains a choice of law clause, which is expressly prohibited in section 7. 

It looks like this will need some upstream relicensing work in order for this to be okay for Fedora, even if SCEA comes back okay, due to the CPL issues.
Comment 10 Bruno Mahe 2009-01-31 14:49:24 EST
Demos are not included in the rpm. Does that still count ?
We could also not include libBulletMultiThreaded which include some CPL code.

Generally speaking, is it ok to not include non-free parts in the resulting rpm so we end up with only free parts ?

To Mamoru:
* I will look into that
Comment 11 Mamoru TASAKA 2009-01-31 14:54:02 EST
(In reply to comment #8)
>   If they are incompatible, this package already contains legal
>   problem, because for example
> --------------------------------------------------------------------
> libLinearMath.so.2.73: contains:
> 
> src/BulletMultiThreaded/SpuNarrowPhaseCollisionTask/SpuGatheringCollisionTask.cpp
> .. includes
> Extras/software_cache/cache/include/spe_cache.h
> => CPL
> 
> Demos/BspDemo/BspLoader.cpp
> => GPLv2+
> ---------------------------------------------------------------------

Ah, sorry, please ignore this part. libLinearMath.so.2.73 does not
use BspLoader.cpp. I will recheck what files are used in
the libraries in this software.
Comment 12 Mamoru TASAKA 2009-01-31 15:08:35 EST
Okay, after recheck actually codes under GPLv2+ are *NOT USED*
(only used to create some demo binaries, which are not included
 in binary rpms).

BulletDino.c is not used, either


So if
- non-free codes are not used
- SCEA 1.0 is free
this package should be okay for Fedora. Again sorry!
Comment 13 Tom "spot" Callaway 2009-02-02 09:52:02 EST
(In reply to comment #10)
> Demos are not included in the rpm. Does that still count ?

They are however in the SRPM, and the case of the NVIDIA bits, it is not clear that we have permission to redistribute them. You'll need to make a modified tarball.

The SCEA license is Free but GPL incompatible. Use "SCEA" for the License tag.

I'm going to lift FE-Legal, assuming that Mamoru will ensure the tarball is clean before approving it.
Comment 14 Mamoru TASAKA 2009-02-02 09:58:36 EST
Thank you for reviewing the license, spot.
Comment 15 Mamoru TASAKA 2009-02-02 11:00:53 EST
Assigning to myself. I tried to remove all files under NVIDIA
license and Demos/ directory and this still builds. So license
issue is now gone.

I will try a full review later.
Comment 16 Mamoru TASAKA 2009-02-02 11:58:11 EST
Bruno, would you tell me if files under Extras/ are really
needed?

I tried to remove all files under Gemos/ Extras/ Glut/ and
it still builds with
%cmake . -DBUILD_DEMOS=OFF -DBUILD_EXTRAS=OFF
Comment 17 Mamoru TASAKA 2009-02-13 10:49:19 EST
ping?
Comment 18 Bruno Mahe 2009-02-15 18:15:56 EST
The Extras directory is not really needed. It just provides some extras libraries. They are not required.
I already have successfully compiled and linked some demos apps against the bullet libraries from the built rpm.

I removed the Demos/, Extras/ and Glut/ directories from the source archive.
But then it's not a pristine source anymore. Is it ok ?

I added -DBUILD_DEMOS=OFF -DBUILD_EXTRAS=OFF to the %cmake command.
I can successfully build packages locally and through koji (see
http://koji.fedoraproject.org/koji/taskinfo?taskID=1128897 for instance)

See http://www.gnoll.org/download/bullet-2.73-3.fc10.src.rpm and 
http://www.gnoll.org/download/bullet.spec

Could someone review it ?
This is my first package and I am looking for a sponsor.

Thanks,
Bruno
Comment 19 Mamoru TASAKA 2009-02-16 12:46:07 EST
Created attachment 332070 [details]
License in need of audit - no2

To spot:

This time would you review the license of the file attached?
It seems that I overlooked this file in the previous check,
Comment 20 Tom "spot" Callaway 2009-02-16 12:50:59 EST
That's an MIT variant, no problem.
Comment 21 Mamoru TASAKA 2009-02-16 13:28:48 EST
(In reply to comment #20)
> That's an MIT variant, no problem.

Thank you. Now the license issue on this package is all cleared.
Full review will follow later.
Comment 22 Mamoru TASAKA 2009-02-17 11:37:33 EST
For 2.73-3:

* License
  - Now the license tag for this package should be
    "zlib and MIT and BSD".
    * All codes under Demos/ Extras/ Glut/ are now removed.
    * Also all files under mk/ are not used.

* Source0
  - By the way the tarball included in your srpm differs from
    what I could download from the URL written in the spec file.
    If you created the tarball by yourself, please write some
    comments in the spec file how you created the tarball.
    https://fedoraproject.org/wiki/Packaging/SourceURL#When_Upstream_uses_Prohibited_Code

* Redundant BuildRequires
  - "BuildRequires: gcc-c++" is redundant on Fedora. ref:
    https://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2

* Compiler flags
  - On Fedora the default optimation level is -O2 (you can check
    this by $ rpm --eval %optflags). However currently this software
    uses -O3.
------------------------------------------------------------------------
    96  [  0%] [  2%] [  2%] [  3%] Building CXX object src/LinearMath/CMakeFiles/LinearMath.dir/btAlignedAllocator.o
    97  Building CXX object src/LinearMath/CMakeFiles/LinearMath.dir/btQuickprof.o
    98  cd /builddir/build/BUILD/bullet-2.73/src/LinearMath && /usr/bin/c++    -DLinearMath_EXPORTS -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic -fasynchronous-unwind-tables  -O3 -DNDEBUG -fPIC -I/usr/local/include -I/builddir/build/BUILD/bullet-2.73/src -I/builddir/build/BUILD/bullet-2.73/src/LinearMath/}   -o CMakeFiles/LinearMath.dir/btAlignedAllocator.o -c /builddir/build/BUILD/bullet-2.73/src/LinearMath/btAlignedAllocator.cpp
------------------------------------------------------------------------
    If you don't have some reason you prefer to use -O3 optimization
    level, please remove this.

* ldconfig symlinks
------------------------------------------------------------------------
]# ls -al /usr/lib/libBulletCollision*
lrwxrwxrwx 1 root root     26 2009-02-18 00:56 /usr/lib/libBulletCollision.so -> libBulletCollision.so.2.73
-rwxr-xr-x 1 root root 706252 2009-02-17 00:34 /usr/lib/libBulletCollision.so.2.73
------------------------------------------------------------------------
  - Usually in this case the (soft) symlink named "libBulletCollision.so.2"
    which points to libBulletCollision.so.2.73 should also be provided
    and the symlink (libBulletCollision.so.2) should be included in
    bullet package
    (see libjpeg and libjpeg-devel rpms for example)

* Directory ownership issue
  - For example:
------------------------------------------------------------------------
$ LANG=C rpm -qf /usr/include/LinearMath/btList.h 
bullet-devel-2.73-3.fc11.i386
$ LANG=C rpm -qf /usr/include/LinearMath/
file /usr/include/LinearMath is not owned by any package
------------------------------------------------------------------------
    Here -devel subpackage installs btList.h under %_includedir/Linearmath,
    however the directory %_includedir/Linearmath itself is not owned
    by any packages, which must be owned by bullet-devel.
    ref:
    https://fedoraproject.org/wiki/Packaging/Guidelines#File_and_Directory_Ownership
    https://fedoraproject.org/wiki/Packaging/UnownedDirectories

* Duplicate documents
  - There is no need to include the same document files into both
    main package and -devel subpackage.

* Misc rpmlint issue
------------------------------------------------------------------------
bullet.src:77: W: macro-in-%changelog post
bullet.src:78: W: macro-in-%changelog description
bullet.src:79: W: macro-in-%changelog summary
bullet.src:80: W: macro-in-%changelog group
bullet.src:82: W: macro-in-%changelog description
bullet.i386: W: file-not-utf8 /usr/share/doc/bullet-2.73/ChangeLog
bullet-debuginfo.i386: W: spurious-executable-perm /usr/src/debug/bullet-2.73/src/LinearMath/btPoolAllocator.h
bullet-debuginfo.i386: E: wrong-script-end-of-line-encoding /usr/src/debug/bullet-2.73/src/LinearMath/btPoolAllocator.h
bullet-debuginfo.i386: W: spurious-executable-perm /usr/src/debug/bullet-2.73/src/BulletDynamics/ConstraintSolver/btSliderConstraint.cpp
bullet-debuginfo.i386: E: wrong-script-end-of-line-encoding /usr/src/debug/bullet-2.73/src/BulletDynamics/ConstraintSolver/btSliderConstraint.cpp
bullet-debuginfo.i386: W: spurious-executable-perm /usr/src/debug/bullet-2.73/src/BulletDynamics/ConstraintSolver/btSliderConstraint.h
bullet-debuginfo.i386: E: wrong-script-end-of-line-encoding /usr/src/debug/bullet-2.73/src/BulletDynamics/ConstraintSolver/btSliderConstraint.h
bullet-devel.i386: W: file-not-utf8 /usr/share/doc/bullet-devel-2.73/ChangeLog
-----------------------------------------------------------------------
  - In %changelog, please use %% instead of single % to prevent
    macros from being expanded.
  - Please change the encoding of ChangeLog file to UTF-8 (use iconv)
  - For spurious-executable-perm and wrong-script-end-of-line-encoding
    rpmlint for debuginfo rpm, this usually means that the source code
    file has executable permission. Change the permission
    of those files to 0644 (at %prep)
Comment 23 Bruno Mahe 2009-02-20 06:04:31 EST
Thank you very much for this thorough review.

I believe I have fixed most of the issues you pointed out.
But I don't understand one of them.

From your previous message :
"* ldconfig symlinks
------------------------------------------------------------------------
]# ls -al /usr/lib/libBulletCollision*
lrwxrwxrwx 1 root root     26 2009-02-18 00:56 /usr/lib/libBulletCollision.so
-> libBulletCollision.so.2.73
-rwxr-xr-x 1 root root 706252 2009-02-17 00:34
/usr/lib/libBulletCollision.so.2.73
------------------------------------------------------------------------
  - Usually in this case the (soft) symlink named "libBulletCollision.so.2"
    which points to libBulletCollision.so.2.73 should also be provided
    and the symlink (libBulletCollision.so.2) should be included in
    bullet package
    (see libjpeg and libjpeg-devel rpms for example)"

CMake doesn't generate any libBullet*.so.2.
Here is the content of my buildroot if I don't clean it up :
lrwxrwxrwx 1 makerpm makerpm     26 2009-02-20 01:56 libBulletCollision.so -> libBulletCollision.so.2.73
-rwxr-xr-x 1 makerpm makerpm 684996 2009-02-20 01:56 libBulletCollision.so.2.73
lrwxrwxrwx 1 makerpm makerpm     25 2009-02-20 01:56 libBulletDynamics.so -> libBulletDynamics.so.2.73
-rwxr-xr-x 1 makerpm makerpm 235368 2009-02-20 01:56 libBulletDynamics.so.2.73
lrwxrwxrwx 1 makerpm makerpm     25 2009-02-20 01:56 libBulletSoftBody.so -> libBulletSoftBody.so.2.73
-rwxr-xr-x 1 makerpm makerpm 225420 2009-02-20 01:56 libBulletSoftBody.so.2.73
lrwxrwxrwx 1 makerpm makerpm     21 2009-02-20 01:56 libLinearMath.so -> libLinearMath.so.2.73
-rwxr-xr-x 1 makerpm makerpm  41196 2009-02-20 01:56 libLinearMath.so.2.73

And my spec file I include %{_libdir}/*.so.* files for the bullet package and %{_libdir}/*.so for bullet-devel.
libjpeg spec file is doing similar things.

Does that mean I should dig around CMake and make sure files like libBullet*.so.2 are also generated ?



I updated the spec file and the SRPM.
Spec file : http://www.gnoll.org/download/bullet.spec
SRPM : http://www.gnoll.org/download/bullet-2.73-4.fc10.src.rpm


Thanks again!
Comment 24 Mamoru TASAKA 2009-02-20 13:49:54 EST
For 2.73-4:

* Description
------------------------------------------------------------
Free for commercial use, including Playstation 3, open source under the
ZLib License.
------------------------------------------------------------
  - I think this part can be deleted (actually all packages
    in Fedora must generally be free, and the correct license
    information can be received from License tag)

* Directory ownership issue
  - Still some directories are not correctly owned
    (%{_includedir}/BulletCollision/, %{_includedir}/BulletDynamics/)

  ! Note
    Writing %files list verbosely like this way is usually
    error-prone. Instead I recommend to use the format like:
------------------------------------------------------------
%files
%defattr(-,root,root,-)
%doc ... ....
%{_includedir}/*.h
%{_includedir}/BulletCollision/
%{_includedir}/LinearMath/
......
------------------------------------------------------------
    Here the %files entry
------------------------------------------------------------
%files
foo/
------------------------------------------------------------
    (where foo/ is a directory) contains the directory foo/ itself
    and all files/directories/etc under foo/, so writing %files
    list in this way is much safer. ref:
    https://fedoraproject.org/wiki/Packaging/UnownedDirectories#Wildcarding_Files_inside_a_Created_Directory

* Missing ldconfig symlinks
(In reply to comment #23)
> Does that mean I should dig around CMake and make sure files like
> libBullet*.so.2 are also generated ?

  - The following is sufficient.
------------------------------------------------------------
make install DESTDIR=$RPM_BUILD_ROOT

pushd $RPM_BUILD_ROOT%{_libdir}
for f in lib*.so.*.*
do
	 ln -sf $f ${f%\.*}
done
popd
-------------------------------------------------------------

* Misc rpmlint issue
-------------------------------------------------------------
bullet.i586: W: file-not-utf8 /usr/share/doc/bullet-2.73/ChangeLog
bullet.src: W: strange-permission generate-tarball.sh 0775
-------------------------------------------------------------
  - Please change the encoding of "ChangeLog" file to UTF-8.
  - All files in srpm must have 0644 permission.
Comment 25 Bruno Mahe 2009-02-23 04:53:29 EST
I have fixed most of the issues.

About the directory ownership issue note in comment #24, I have been quite verbose to avoid unnecessary and unrelated directory inclusion.
Otherwise I would end up with empty directories named "CMakeFiles" or "ibmsdk".
Or should I explicitly delete them ?


Here is the updated SPEC file: http://www.gnoll.org/download/bullet.spec
Here is the updated SRPM : http://www.gnoll.org/download/bullet-2.73-5.fc10.src.rpm


Thank you very much for your patience!
Comment 26 Mamoru TASAKA 2009-02-23 11:39:46 EST
Okay, now as this package itself is good:

-------------------------------------------------------------
NOTE: Before being sponsored:

This package will be accepted with another few work. 
But before I accept this package, someone (I am a candidate) 
must sponsor you.

Once you are sponsored, you have the right to review other 
submitters' review requests and approve the packages formally. 
For this reason, the person who want to be sponsored (like you) 
are required to "show that you have an understanding 
of the process and of the packaging guidelines" as is described
on :
http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

Usually there are two ways to show this.
A. submit other review requests with enough quality.
B. Do a "pre-review" of other person's review request
   (at the time you are not sponsored, you cannot do
   a formal review)

When you have submitted a new review request or have pre-reviewed other 
person's review request, please write the bug number on this bug report 
so that I can check your comments or review request.

Fedora package collection review requests which are waiting for someone to
review can be checked on my wiki page:
http://fedoraproject.org/wiki/User:Mtasaka#B._Review_request_tickets
(Check "No one is reviewing")

Review guidelines are described mainly on:
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
------------------------------------------------------------
Comment 27 Mamoru TASAKA 2009-03-01 11:00:22 EST
ping?
Comment 28 Bruno Mahe 2009-03-04 20:02:52 EST
Still there. Just busy.
I will probably havesome time this week to do some pre-reviews.
Comment 29 Mamoru TASAKA 2009-03-26 10:23:54 EDT
ping again?
Comment 30 Bruno Mahe 2009-04-01 14:31:06 EDT
Still there and still busy.
Comment 31 Mamoru TASAKA 2009-04-12 12:50:53 EDT
Would you have some time to work on review request now?

Usually review request is closed when no response is received
within one month so that someone else can take over the package.
Comment 32 Bruno Mahe 2009-04-23 04:57:48 EDT
I don't have enough time right now.
So I give up and someone else can take over the package.

Thanks a lot for your reviews and patience!
Comment 33 Mamoru TASAKA 2009-04-23 05:42:44 EDT
No worry.
Now I once close this bug. If you can have some time for review
request again, please feel free to open a new review request and
mark this bug as a duplicate of the new one. Thank you anyway.
Comment 34 Jason Tibbitts 2009-06-24 19:27:01 EDT

*** This bug has been marked as a duplicate of bug 507972 ***
Comment 35 Tim Niemueller 2012-09-28 07:30:53 EDT
Removing alias, searching for bullet should forward to the proper review ticket.