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
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.
ping?
pong. I was on vacation and didn't had time to work on it. I am back and going to finish it.
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.
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
Created attachment 330529 [details] License analysis
Created attachment 330530 [details] Licenses in need of audit
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?
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.
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
(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.
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!
(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.
Thank you for reviewing the license, spot.
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.
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
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
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,
That's an MIT variant, no problem.
(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.
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)
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!
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.
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!
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 ------------------------------------------------------------
Still there. Just busy. I will probably havesome time this week to do some pre-reviews.
ping again?
Still there and still busy.
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.
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!
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.
*** This bug has been marked as a duplicate of bug 507972 ***
Removing alias, searching for bullet should forward to the proper review ticket.