Bug 545045 - Review Request: CQRlib - ANSI C API for quaternion arithmetic and rotation
Summary: Review Request: CQRlib - ANSI C API for quaternion arithmetic and rotation
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: 12
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Chen Lei
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 541462
TreeView+ depends on / blocked
 
Reported: 2009-12-07 12:40 UTC by Takanori MATSUURA
Modified: 2010-11-01 16:35 UTC (History)
6 users (show)

Fixed In Version: CQRlib-1.1.1-1.el5
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-10-28 05:45:32 UTC
supercyper1: fedora-review+
tcallawa: fedora-cvs+


Attachments (Terms of Use)

Description Takanori MATSUURA 2009-12-07 12:40:24 UTC
Spec URL: http://t-matsuu.sakura.ne.jp/mock/cqrlib/cqrlib.spec
SRPM URL: http://t-matsuu.sakura.ne.jp/mock/cqrlib/cqrlib-1.0.3-1.20090805.fc12.src.rpm
Description: 
CQRlib is an ANSI C implementation of a utility library for quaternion
arithmetic and quaternion rotation math.

Comment 1 Ralf Corsepius 2009-12-07 14:49:27 UTC
Some comments:

* Wrt. this construct:
%ifarch x86_64 ppc64
sed -e '/^LIB/s/lib/lib64/g' -i Makefile
%endif

- Such sed calls can be generalized by applying %{_lib} (expands to "lib" on 32bit and to "lib64" on 64bit targets)

- Such sed calls are equivalent to "configuring" a package. They belong into %build

* static libs
%files devel
...
%{_libdir}/*.a

Feel strongly encouraged not to ship static libs. If you really can't avoid shipping them, move the *.a's to a separate *-static package.


* Upstream's tarball name is CQRlib. Fedora convention is to keep upstream's
package name => Your package should be named "CQRlib", not cqrlib.

* Your patch adds changes the shared libs from --shared to --export-dynamic and removes --release - Why? This is a significant behavioral change to upstream, I don't see any sense in.

Comment 2 Takanori MATSUURA 2009-12-08 12:40:48 UTC
(In reply to comment #1)

Spec URL: http://t-matsuu.sakura.ne.jp/mock/CQRlib.spec
SRPM URL: http://t-matsuu.sakura.ne.jp/mock/CQRlib-1.0.3-2.20090805.fc12.src.rpm

Thank you for your reviewing.

I removed the patch file because all of the modification for Makefile is now in spec file.


> * Wrt. this construct:
> %ifarch x86_64 ppc64
> sed -e '/^LIB/s/lib/lib64/g' -i Makefile
> %endif
> 
> - Such sed calls can be generalized by applying %{_lib} (expands to "lib" on
> 32bit and to "lib64" on 64bit targets)
> 
> - Such sed calls are equivalent to "configuring" a package. They belong into
> %build

Use %{_lib} and the sed call is moved to %build section.


> * static libs
> %files devel
> ...
> %{_libdir}/*.a
> 
> Feel strongly encouraged not to ship static libs. If you really can't avoid
> shipping them, move the *.a's to a separate *-static package.

Static library is separated to static subpackage.


> * Upstream's tarball name is CQRlib. Fedora convention is to keep upstream's
> package name => Your package should be named "CQRlib", not cqrlib.

Renamed to CQRlib.


> * Your patch adds changes the shared libs from --shared to --export-dynamic and
> removes --release - Why? This is a significant behavioral change to upstream, I
> don't see any sense in.  

Probably "--shared" you said means "-dynamic".
I found that "-dynamic" should be not "--export-dynamic" but "-rdynamic".  So I fix it to use "-rdynamic".

For "--release" issue, I keep the option in Makefile

Comment 4 Takanori MATSUURA 2009-12-09 12:14:32 UTC
Spec URL: http://t-matsuu.sakura.ne.jp/mock/CQRlib/CQRlib.spec
SRPM URL: http://t-matsuu.sakura.ne.jp/mock/CQRlib/CQRlib-1.0.3-3.20090805.fc12.src.rpm

* Update based on the comment for another in-parallel reviewing package.
* remove static lirary

Comment 5 Takanori MATSUURA 2009-12-09 12:16:41 UTC
* The issue for the option in generating dynamic library has been reported to the upstream developer.

Comment 6 Takanori MATSUURA 2009-12-09 13:27:26 UTC
Uah, the file name of the shared library include full version number. :-(

It's not good.
If this package is updated, we have to recompile all the software which link to this library.

Now I'm contacting the upstream developer.

Comment 7 Ralf Corsepius 2009-12-10 13:52:31 UTC
(In reply to comment #6)
> Uah, the file name of the shared library include full version number. :-(
> 
> It's not good.
Yep, but - this can be intentional. It's a matter of design.

If, for some reasons, upstream wants it this way, you have to stick with what upstream wants - In most cases, however --release is a matter of upstreams not being familiar with Linux shared libs.

Please also read 
info libtool 'Versioning'

Comment 8 Takanori MATSUURA 2009-12-10 15:02:59 UTC
After the discussion with upstream developer, he has no objection to remove -release.

I'll prepare a patch to remove -release and propose to upstream.

Comment 9 Chen Lei 2010-05-05 08:13:28 UTC
(In reply to comment #8)
> After the discussion with upstream developer, he has no objection to remove
> -release.
> I'll prepare a patch to remove -release and propose to upstream.    

No need to do that if upstream wants it this way.

I cannot download your spec and SRPM, the server is down?

Comment 10 Takanori MATSUURA 2010-05-06 04:05:54 UTC
(In reply to comment #9)
I believe you can download from the URL in the comment #5.
Please try again.

Comment 12 Chen Lei 2010-08-25 02:50:33 UTC
IMHO, sed -e 's,-dynamic,-rdynamic,g' -i Makefile should be replaced by sed -e 's,-dynamic,-shared,g' , also you may also need to add -fPIC to CFLAGS since CQRlib is a shared lib.


Do you mind to report those issues to upstream?

Comment 13 Chen Lei 2010-08-25 03:38:36 UTC
After taking a look at Makefile, the -dynamic argument is irrelevant to the shared lib and static lib, so the above hack is not needed at all. BUILD_COMMAND_DYNAMIC is for example files only.

Comment 14 Takanori MATSUURA 2010-08-25 11:08:17 UTC
(In reply to comment #12)
> Do you mind to report those issues to upstream?

I've already reported last year.
I'll try again later.


(In reply to comment #13)
> After taking a look at Makefile, the -dynamic argument is irrelevant to the
> shared lib and static lib, so the above hack is not needed at all.
> BUILD_COMMAND_DYNAMIC is for example files only.

Do you mean your hack in comment #12 is not needed?

Comment 15 Carl Byington 2010-09-03 19:33:39 UTC
sourceforge now has CQRlib 1.0.6

I put together a quick autoconf'd version at 

http://www.five-ten-sg.com/cqrlib.spec
http://www.five-ten-sg.com/autoconf-files.tar.gz
http://www.five-ten-sg.com/CQRlib-1.0.6.tar.gz
http://www.five-ten-sg.com/CQRlib-1.0.6-1.fc12.src.rpm

That might be easier than extensive hacking on the upstream makefile, and it might also be applicable to the other parts needed for rasmol (cvector, neartree, cbflib).

Comment 16 Takanori MATSUURA 2010-09-03 23:40:17 UTC
Now I'm contacting upstream developer for /-dynamic/-rdynamic/ issue.
I'll back here next week.

Comment 18 Takanori MATSUURA 2010-09-16 17:04:35 UTC
Compile failed on i[36]86 architecture.
I've reported upstream.

Waiting for fix.

Comment 20 Chen Lei 2010-09-30 12:39:01 UTC
rpmlint CQRlib*rpm

CQRlib.src: W: %ifarch-applied-patch Patch1: CQRlib-1.0.6-lib64.patch

It'll better to avoid of using %inarch in spec, actually there are some other 64bit archs(e.g. sparc64 mipsel64).

%ifarch x86_64 ppc64 s390x
%patch1 -p1 -b .lib64
%endif
->
% %{_lib}==lib64
%patch1 -p1 -b .lib64
%endif

CQRlib.x86_64: W: non-coherent-filename getfile?taskID=2503499&name=CQRlib-1.1.1-0.fc15.x86_64.rpm CQRlib-1.1.1-0.fc15.x86_64.rpm

Comment 21 Chen Lei 2010-10-07 19:45:59 UTC
ping?

Comment 22 Takanori MATSUURA 2010-10-09 05:43:29 UTC
Sorry for my late response.

I'll back next week because my machine for working this bug is out of network now.

Comment 23 Takanori MATSUURA 2010-10-12 10:45:47 UTC
Updated:
Spec URL: http://t-matsuu.sakura.ne.jp/mock/CQRlib/CQRlib.spec
SRPM URL:
http://t-matsuu.sakura.ne.jp/mock/CQRlib/CQRlib-1.1.1-0.1.fc13.src.rpm

Issue mentioned in comment #20 is now fixed.

Comment 24 Chen Lei 2010-10-12 11:56:43 UTC
Approved!

Comment 25 Takanori MATSUURA 2010-10-12 12:07:40 UTC
New Package SCM Request
=======================
Package Name: CQRlib
Short Description: ANSI C API for quaternion arithmetic and rotation
Owners: tmatsuu
Branches: f12 f13 f14 el5 el6
InitialCC:

Comment 26 Tom "spot" Callaway 2010-10-13 15:09:09 UTC
Git done (by process-git-requests).

Comment 27 Fedora Update System 2010-10-14 14:17:48 UTC
CQRlib-1.1.1-1.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/CQRlib-1.1.1-1.el5

Comment 28 Fedora Update System 2010-10-14 14:17:50 UTC
CQRlib-1.1.1-1.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/CQRlib-1.1.1-1.fc14

Comment 29 Fedora Update System 2010-10-14 18:23:25 UTC
CQRlib-1.1.1-1.el5 has been pushed to the Fedora EPEL 5 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update CQRlib'.  You can provide feedback for this update here: https://admin.fedoraproject.org/updates/CQRlib-1.1.1-1.el5

Comment 30 Fedora Update System 2010-10-28 05:44:42 UTC
CQRlib-1.1.1-1.fc14 has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 31 Fedora Update System 2010-11-01 16:35:03 UTC
CQRlib-1.1.1-1.el5 has been pushed to the Fedora EPEL 5 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.