Bug 817193

Summary: Review Request: libccd - Library for collision detection between convex shapes
Product: [Fedora] Fedora Reporter: Rich Mattes <richmattes>
Component: Package ReviewAssignee: Ankur Sinha (FranciscoD) <sanjay.ankur>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: lemenkov, martin.gieseking, notting, package-review, rc040203, sanjay.ankur
Target Milestone: ---Flags: sanjay.ankur: fedora‑review+
limburgher: fedora‑cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2012-12-20 10:01:10 EST 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: 825409    

Description Rich Mattes 2012-04-27 21:13:19 EDT
Spec URL: http://rmattes.fedorapeople.org/RPMS/libccd/libccd.spec
SRPM URL: http://rmattes.fedorapeople.org/RPMS/libccd/libccd-1.2-1.fc16.src.rpm
Description: libccd implements variation on Gilbert-Johnson-Keerthi (GJK) algorithm + Expand Polytope Algorithm (EPA). It also implements Minkowski Portal
Refinement (MPR, a.k.a. XenoCollide) algorithm as published in Game Programming Gems 7.

rpmlint:
$ rpmlint libccd.spec ../RPMS/x86_64/libccd-*
libccd.x86_64: W: shared-lib-calls-exit /usr/lib64/libccd.so.1.2 exit@GLIBC_2.2.5
libccd-static.x86_64: W: no-documentation
4 packages and 1 specfiles checked; 0 errors, 2 warnings.

I'm looking into the exit() warning, but I don't know if it's a showstopper.
Comment 1 Peter Lemenkov 2012-04-28 05:09:34 EDT
(In reply to comment #0)

> rpmlint:
> $ rpmlint libccd.spec ../RPMS/x86_64/libccd-*
> libccd.x86_64: W: shared-lib-calls-exit /usr/lib64/libccd.so.1.2
> exit@GLIBC_2.2.5
> libccd-static.x86_64: W: no-documentation
> 4 packages and 1 specfiles checked; 0 errors, 2 warnings.
> 
> I'm looking into the exit() warning, but I don't know if it's a showstopper.

From my pov - this is a defect in the library's architecture but not a review blocker.

For those who are not aware of this issue - it is considered as a good practice to return descriptive error values from the library in case of errors and not to die immediately (this should be up to the underlying application(s) how to behave in this sorrow situation).
Comment 2 Rich Mattes 2012-04-30 23:13:15 EDT
Updated package:

Spec URL: http://rmattes.fedorapeople.org/RPMS/libccd/libccd.spec
SRPM URL: http://rmattes.fedorapeople.org/RPMS/libccd/libccd-1.2-2.fc16.src.rpm

rpmlint:
$ rpmlint libccd.spec ../RPMS/x86_64/libccd-*
libccd.x86_64: W: shared-lib-calls-exit /usr/lib64/libccd.so.1.2 exit@GLIBC_2.2.5
libccd-static.x86_64: W: no-documentation
4 packages and 1 specfiles checked; 0 errors, 2 warnings.

I contacted the upstream author with the patch I created and he was kind enough to pull it into git for the next release.  I also dug into the exit call, it's in a function that wraps realloc(). If the realloc call fails then the function will call exit.  I asked the upstream author if it would be better to just return a null pointer and let the application check for the realloc failure on 2012-04-28, but haven't yet received a response.
Comment 3 Ralf Corsepius 2012-05-01 00:11:07 EDT
The exit doesn't bother me at all.

However, the static lib package does. Feel strongly encouraged not to build and ship the static library package.

Also, this package seems to contain a testsuite. Please consider activating it.
Comment 4 Rich Mattes 2012-05-23 22:13:18 EDT
Updated package:

Spec URL: http://rmattes.fedorapeople.org/RPMS/libccd/libccd.spec
SRPM URL: http://rmattes.fedorapeople.org/RPMS/libccd/libccd-1.2-3.fc17.src.rpm

rpmlint:

$ rpmlint ../SRPMS/libccd-1.2-3.fc17.src.rpm ../RPMS/x86_64/libccd*
libccd.x86_64: W: shared-lib-calls-exit /usr/lib64/libccd.so.1.2 exit@GLIBC_2.2.5
libccd.x86_64: W: shared-lib-calls-exit /usr/lib64/libccd.so.1.2 exit@GLIBC_2.2.5
7 packages and 0 specfiles checked; 0 errors, 2 warnings.


I've removed the static subpackage from the spec, as it wasn't really necessary and nothing that I know of will depend on it.

I've also been working with the upstream developer.  The next release of libccd will have the exit() called removed, and instead pass an error code to the caller (changes are already in git upstream).
Comment 5 Rich Mattes 2012-05-25 18:33:40 EDT
Update to version 1.3:

Spec URL: http://rmattes.fedorapeople.org/RPMS/libccd/libccd.spec
SRPM URL: http://rmattes.fedorapeople.org/RPMS/libccd/libccd-1.3-1.fc17.src.rpm

$ rpmlint libccd.spec ../RPMS/x86_64/libccd-*
3 packages and 1 specfiles checked; 0 errors, 0 warnings.

The exit() call has been removed, and the buildsystem is now setting a SONAME.
Comment 6 Ralf Corsepius 2012-05-25 23:56:29 EDT
- Package does not build in mock:
...
cc -g -Wall -pedantic --std=gnu99 -I./ -I../ -Icu/ -o test main.c common.o support.o vec3.o polytope.o boxbox.o spheresphere.o cylcyl.o boxcyl.o mpr_boxbox.o mpr_cylcyl.o mpr_boxcyl.o  -L./ -Lcu/ -l
/usr/bin/ld: cannot find -lccd
collect2: error: ld returned 1 exit status
make: *** [test] Error 1
error: Bad exit status from /var/tmp/rpm-tmp.jqQs8y (%check)

- Package does not honor RPM_OPT_FLAGS:
...
Building C object CMakeFiles/ccd_static.dir/src/ccd.c.o
/usr/lib64/ccache/gcc   -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4  -m64 -mtune=generic  -O2 -g
-I/builddir/build/BUILD/libccd-1.3/src    -o CMakeFiles/ccd_static.dir/src/ccd.c.o   -c /builddir/build/BUILD/libccd-1.3/src/ccd.c

Here, it accepts $RPM_OPT_FLAGS, but later overrides add "-O2 -g". 
I.e. it overrides some settings from $RPM_OPT_FLAGS.

Later on during the built, the testsuite doesn't honor RPM_OPT_LFLAGS at all:
...
make[1]: Entering directory `/builddir/build/BUILD/libccd-1.3/src/testsuites/cu'
cc -g -Wall -pedantic -DCU_ENABLE_TIMER -c -o cu.o cu.c
ar cr libcu.a cu.o
ranlib libcu.a
make[1]: Leaving directory `/builddir/build/BUILD/libccd-1.3/src/testsuites/cu'
cc -g -Wall -pedantic --std=gnu99 -I./ -I../ -Icu/ -c -o common.o common.c
Comment 7 Rich Mattes 2012-05-26 14:29:18 EDT
Thanks a lot for the input Ralf.  I was able to solve all of the issues:

- The extra cflags were coming from CMake, since I set the build type to RelWithDebInfo.  The default CMAKE_C_FLAGS_RELWITHDEBINFO are "-O2 -g" and they get appended to the optflags.  I set the build type to None so that CMake won't insert any extra flags during the build process.

- The testsuites were built with a separate makefile which wasn't really friendly to environment variables.  I decided to integrate the tests into the CMake-based build using CTest.  Now all of the test programs honor the optflags, and they find libccd during linking.  I will contribute my ctest patch upstream and see if he is interested.

Spec URL: http://rmattes.fedorapeople.org/RPMS/libccd/libccd.spec
SRPM URL: http://rmattes.fedorapeople.org/RPMS/libccd/libccd-1.3-2.fc17.src.rpm

$ rpmlint libccd.spec ../RPMS/x86_64/libccd-*
3 packages and 1 specfiles checked; 0 errors, 0 warnings.
Comment 8 Martin Gieseking 2012-09-14 04:41:45 EDT
Rich, are you still interested in this package? Your SRPM is unavailable at the moment.
Comment 9 Rich Mattes 2012-09-16 23:20:08 EDT
Sorry about that, I accidentally deleted them while i was cleaning out some packages that had already finished review.  I've re-uploaded the package and spec, you can find them here:

Spec URL: http://rmattes.fedorapeople.org/RPMS/libccd/libccd.spec
SRPM URL: http://rmattes.fedorapeople.org/RPMS/libccd/libccd-1.3-3.fc17.src.rpm

$ rpmlint libccd.spec ../RPMS/x86_64/libccd-*
3 packages and 1 specfiles checked; 0 errors, 0 warnings.


I am still interested in maintaining this package.
Comment 10 Ankur Sinha (FranciscoD) 2012-09-21 00:18:03 EDT
Hi folks,

I'm interested in helping out with the Fedora Robotics SIG. If you folks don't mind, I could review this package for Rich? 

If no one takes the review by Monday, 24th September, I'll take over the review :)

Thanks,
Warm regards,
Ankur
Comment 11 Ankur Sinha (FranciscoD) 2012-09-25 08:58:01 EDT
Hi folks, 

I'll review this one. 

Thanks,
Warm regards,
Ankur
Comment 12 Ankur Sinha (FranciscoD) 2012-09-25 09:19:00 EDT
Hi,

Review:

[+] OK
[-] NA
[?] Issue

[+] Package meets naming and packaging guidelines
[+] Spec file matches base package name.
[+] Spec has consistant macro usage.
[+] Meets Packaging Guidelines.
[+] License
[+] License field in spec matches
[ankur@ankur libccd-1.3]$ !find
find . -name "*" -exec licensecheck '{}' \; | sed '/UNKNOWN/ d'
./src/testsuites/cu/cu.c: LGPL (v3 or later)
./src/testsuites/cu/cu.h: LGPL (v3 or later)
./src/testsuites/cu/check-regressions: LGPL (v3 or later)
./src/testsuites/cu/cu.c: LGPL (v3 or later)
./src/testsuites/cu/cu.h: LGPL (v3 or later)
./BSD-LICENSE: BSD (3 clause)
./ccd.pc.in: *No copyright* GENERATED FILE

As you see, some of the tests are under the LGPL license. The License should be
LGPLv3 and BSD?

[+] License file included in package
[+] Spec in American English
[+] Spec is legible.
[+] Sources match upstream md5sum:
[ankur@ankur SPECS]$ review-md5check.sh libccd.spec
Getting http://libccd.danfis.cz/files/libccd-1.3.tar.gz to
/tmp/review/libccd-1.3.tar.gz
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  127k    0  127k    0     0  16072      0 --:--:--  0:00:08 --:--:-- 69374
2c4fcb78174ebf9441a1706961a669cd  /tmp/review/libccd-1.3.tar.gz
2c4fcb78174ebf9441a1706961a669cd
/home/ankur/rpmbuild/SOURCES/libccd-1.3.tar.gz
removed `/tmp/review/libccd-1.3.tar.gz'
removed directory: `/tmp/review'
[ankur@ankur SPECS]$

[+] BuildRequires correct
[+] Spec handles locales/find_lang
[+] Package is code or permissible content.
[+] Packages %doc files don't affect runtime.

[+] Headers/static libs in -devel subpackage.
[+] Spec has needed ldconfig in post and postun
[+] .pc files in -devel subpackage/requires pkgconfig
[+] .so files in -devel subpackage.
[+] -devel package Requires: %{name} = %{version}-%{release}


[+] Package compiles and builds on at least one arch.
[+] Package has no duplicate files in %files.
[+] Package doesn't own any directories other packages own.
[+] Package owns all the directories it creates.
[+] No rpmlint output.
[ankur@ankur SRPMS]$ rpmlint ./libccd-1.3-3.fc17.src.rpm ../SPECS/libccd.spec /var/lib/mock/fedora-rawhide-x86_64/result/*.rpm
5 packages and 1 specfiles checked; 0 errors, 0 warnings.
[
[+] final provides and requires are sane:
== libccd-1.3-3.fc19.src.rpm ==
Provides:

Requires:
cmake
python
valgrind

== libccd-1.3-3.fc19.x86_64.rpm ==
Provides:
libccd = 1.3-3.fc19
libccd(x86-64) = 1.3-3.fc19
libccd.so.1()(64bit)

Requires:
/sbin/ldconfig
/sbin/ldconfig
libc.so.6()(64bit)
libc.so.6(GLIBC_2.2.5)(64bit)
libc.so.6(GLIBC_2.3.4)(64bit)
libm.so.6()(64bit)
libm.so.6(GLIBC_2.2.5)(64bit)
rtld(GNU_HASH)

== libccd-debuginfo-1.3-3.fc19.x86_64.rpm ==
Provides:
libccd-debuginfo = 1.3-3.fc19
libccd-debuginfo(x86-64) = 1.3-3.fc19

Requires:

== libccd-devel-1.3-3.fc19.x86_64.rpm ==
Provides:
libccd-devel = 1.3-3.fc19
libccd-devel(x86-64) = 1.3-3.fc19
pkgconfig(ccd) = 1.3

Requires:
/usr/bin/pkg-config
libccd(x86-64) = 1.3-3.fc19
libccd.so.1()(64bit)

[ankur@ankur result]$

SHOULD Items:

[+] Should build in mock.
[-] Should build on all supported archs
[-] Should function as described.
[+] Should have sane scriptlets.
[+] Should have subpackages require base package with fully versioned depend.
[+] Should have dist tag
[+] Should package latest version

Issues:

1. Please check the license


Everything else looks okay. Almost ready for approval too!

Thanks,
Warm regards,
Ankur
Comment 13 Rich Mattes 2012-09-26 23:31:46 EDT
Looks like keeping the license as BSD is ok:

http://lists.fedoraproject.org/pipermail/legal/2012-September/001969.html
Comment 14 Ankur Sinha (FranciscoD) 2012-10-18 19:09:28 EDT
Hi Rich,

Great. Nothing to change then :)

XXX APPROVED XXX

Thanks,
Warm regards,
Ankur
Comment 15 Rich Mattes 2012-10-19 09:27:23 EDT
New Package SCM Request
=======================
Package Name: libccd
Short Description: Library for collision detection between convex shapes
Owners: rmattes
Branches: f17 f18 el6
InitialCC:
Comment 16 Jon Ciesla 2012-10-19 09:42:35 EDT
Git done (by process-git-requests).
Comment 17 Fedora Update System 2012-10-22 19:00:01 EDT
libccd-1.4-1.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/libccd-1.4-1.fc17
Comment 18 Fedora Update System 2012-10-23 22:49:05 EDT
libccd-1.4-1.fc17 has been pushed to the Fedora 17 testing repository.
Comment 19 Fedora Update System 2012-12-20 10:01:20 EST
libccd-1.4-1.fc17 has been pushed to the Fedora 17 stable repository.