Bug 403741 - Review Request: OpenEXR_CTL - A simplified OpenEXR interface to CTL
Summary: Review Request: OpenEXR_CTL - A simplified OpenEXR interface to CTL
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Rex Dieter
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 428228
TreeView+ depends on / blocked
 
Reported: 2007-11-29 01:50 UTC by Nicolas Chauvet (kwizart)
Modified: 2008-02-02 10:16 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-02-02 10:16:27 UTC
Type: ---
Embargoed:
rdieter: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Nicolas Chauvet (kwizart) 2007-11-29 01:50:45 UTC
Spec URL:
http://kwizart.fedorapeople.org/SPECS/OpenEXR_CTL.spec
SRPM URL:
http://kwizart.fedorapeople.org/SRPMS/OpenEXR_CTL-1.0.1-1.kwizart.fc8.src.rpm
Description: A simplified OpenEXR interface to CTL

Targeted for F-8 and beyond...

Comment 1 Nicolas Chauvet (kwizart) 2008-01-08 19:46:23 UTC
Spec URL:
http://kwizart.fedorapeople.org/SPECS/OpenEXR_CTL.spec
SRPM URL:
http://kwizart.fedorapeople.org/SRPMS/OpenEXR_CTL-1.0.1-2.kwizart.fc8.src.rpm
Description: A simplified OpenEXR interface to CTL

Fixed rpath and gcc43 ready

Comment 2 Rex Dieter 2008-01-09 15:03:19 UTC
I'd highly recommend leaving out the capitalization, stick with openexr_ctl 
here.  In a perfect world, I'd also rename OpenEXR -> openexr , but I've been 
to lazy to do that (yet).

Hmm... I thought there was some non-free license issue (non commercial?) with 
this, but that's only going off of foggy/vague recollection from past openexr 
mailing list dicussions.  ??

Comment 3 Rex Dieter 2008-01-09 15:20:42 UTC
ok, nevermind, I think it was only gpl-incompatible, but still free 
(enough). :)

Comment 4 Nicolas Chauvet (kwizart) 2008-01-09 15:31:11 UTC
About the Capitalization problem, I would say it's strange but it have some sense...
I think the real package names are OpenEXR but because it can also be compiled
on win32 they left the capitalization...
Actually i'm close to have OpenEXR_Viewers ready for review and the package name
(defined in configure.ac) is OpenEXR_Viewers which means they install doc in
%{_datadir}/doc/OpenEXR_Viewers-%{version}.
same are the pkgconfig files that are named OpenEXR.pc CTL.pc and OpenEXR_CTL.pc
here.

About the pkgconfig problem (do you have the pkgconfig patch I've added later?):
Because not all libs are used when linking there is lot of
unused-direct-shlib-dependencies (rpmlint OpenEXR-libs on installed files ). I
would say it would be interesting to clean them. 
There were also a undefined-non-weak symbol (fixed by the pkgconfig patch) that
prevent it to be linked from OpenEXR_Viewers...(CTL libs was missing).

I first thought it was due to the CTL 1.4.1 been used with OpenEXR 1.6.0
(instead of 1.6.1). But it seems to work. So I wonder if it worth the case to
have OpenEXR updated to 1.6.1 in F-8 (seems bugfixes), What do you think about
updating OpenEXR for F-8 ?!

The FE-LEGAL has been discussed in http://bugzilla.redhat.com/357461
Tom said it was "Free, but GPL incompatible".



Comment 5 Rex Dieter 2008-01-09 15:44:12 UTC
good point about the pkg name vs pkgconfig, I hadn't considered that.  either
way works.

your pkgconfig patch here looks like a very good start, send that upstream. :)

re: unused-direct-shlib-dependencies  : good point, I could/should hack in
-Wl,-unused like you did here.

Re: OpenEXR-1.6.x for F-8, I'm definitely ok with that, just need to be careful
about the soname changes.



Comment 6 Nicolas Chauvet (kwizart) 2008-01-09 16:00:56 UTC
Rebuilt failed in mock at the OpenEXR test (with gcc43 but went fine with gcc
4.1.2)... 
I Will investigate this: I can disable the OpenEXR test (like i did for fixing
OpenEXR_CTL for gcc43) or fix the test, but maybe that's related to
OpenEXR-devel not fixed for gcc43... I will have a look...



Comment 7 Nicolas Chauvet (kwizart) 2008-01-09 16:25:10 UTC
Ok that's fixed with a configure test fix
conftest.cc:32: error: 'system' was not declared in this scope
added %patch2 (without bumping release)...

Comment 8 Rex Dieter 2008-01-16 14:45:31 UTC
Scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=352922

$ rpmlint *.rpm
OpenEXR_CTL.x86_64: W: no-documentation
OpenEXR_CTL.x86_64: W: invalid-license AMPAS BSD
OpenEXR_CTL.src: W: invalid-license AMPAS BSD
OpenEXR_CTL-devel.x86_64: W: no-documentation
OpenEXR_CTL-devel.x86_64: W: no-dependency-on OpenEXR_CTL
OpenEXR_CTL-devel.x86_64: W: invalid-license AMPAS BSD
OpenEXR_CTL-libs.x86_64: W: invalid-license AMPAS BSD
(these are all harmless/bogus).

upstream source matches:
035a68db3b1cc40fe99a7c4012d7f024  openexr_ctl-1.0.1.tar.gz

Most everything looks real good.  I have only a couple issues:

SHOULD: (re)consider using reautoconf instead of manually hacking libtool (to
remove rpath).

SHOULD: (re)evaluate if
BuildRequires: OpenEXR
Requires: OpenEXR
are really needed.  To my naive eyes, I would guess no, but maybe you have
reason to include these.  If so, please document that in the specfile.

SHOULD: work to upstream your patches (gcc43, pkgconfig).


But I don't consider these blockers, APPROVED.

Comment 9 Nicolas Chauvet (kwizart) 2008-01-16 15:33:04 UTC
Thx for your review!

About regenerating autotools. It is related to the automake version used
(automake-1.9 - we don't have it yet as an alternative). using automake-1.10 it
will produce some strange result. So i will probably work with upstream to
improve either configure.in or Makefile.am to be automake-1.10 compliant. 
Until then, patching libtool seems safer to me.

About BR/R OpenEXR: you are right. Only libs are checked at configure and no
binaries are mentioned nor required for the program to run. I will remove them
before import.

Patches have been submitted upstream:
https://sourceforge.net/tracker/index.php?func=detail&aid=1872980&group_id=184265&atid=908566

I will request EPEL branch but this will not make sense to have it for now...
(only if we decide to have ilmbase and update OpenEXR to 1.6.x later)





Comment 10 Nicolas Chauvet (kwizart) 2008-01-16 15:38:11 UTC
New Package CVS Request
=======================
Package Name: OpenEXR_CTL
Short Description: A simplified OpenEXR interface to CTL
Owners: kwizart
Branches: F-7 F-8 EL-5
Cvsextras Commits: yes

Comment 11 Kevin Fenzi 2008-01-16 17:28:32 UTC
cvs done.


Note You need to log in before you can comment on or make changes to this bug.