Bug 817193

(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.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).

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.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.

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.

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.5
libccd.x86_64: W: shared-lib-calls-exit /usr/lib64/libccd.so.1.2 exit.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).

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.

- 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

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.

Rich, are you still interested in this package? Your SRPM is unavailable at the moment.

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.

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

Hi folks, 

I'll review this one. 

Thanks,
Warm regards,
Ankur

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

Looks like keeping the license as BSD is ok:

http://lists.fedoraproject.org/pipermail/legal/2012-September/001969.html

Hi Rich,

Great. Nothing to change then :)

XXX APPROVED XXX

Thanks,
Warm regards,
Ankur

New Package SCM Request
=======================
Package Name: libccd
Short Description: Library for collision detection between convex shapes
Owners: rmattes
Branches: f17 f18 el6
InitialCC:

Git done (by process-git-requests).

libccd-1.4-1.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/libccd-1.4-1.fc17

libccd-1.4-1.fc17 has been pushed to the Fedora 17 testing repository.

libccd-1.4-1.fc17 has been pushed to the Fedora 17 stable repository.