Bug 814804 - Review Request: OpenColorIO - Enables color transforms and image display across graphics apps
Summary: Review Request: OpenColorIO - Enables color transforms and image display acro...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Kalev Lember
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2012-04-20 17:51 UTC by Richard Shaw
Modified: 2014-05-20 18:35 UTC (History)
3 users (show)

Fixed In Version: OpenColorIO-1.0.7-4.fc17
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2012-05-04 20:32:18 UTC
Type: ---
Embargoed:
kalevlember: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Richard Shaw 2012-04-20 17:51:31 UTC
Spec URL: http://hobbes1069.fedorapeople.org/OpenColorIO/OpenColorIO.spec
SRPM URL: http://hobbes1069.fedorapeople.org/OpenColorIO/OpenColorIO-1.0.7-1.fc16.src.rpm
Description:
Enables color transforms and image display to be handled in a consistent manner across multiple graphics applications. Unlike other color management solutions,  OpenColorIO is geared towards motion-picture post production, with an emphasis on visual effects and animation color pipelines.

rpmlint output:
$ rpmlint mockbuild/16/OpenColorIO/*.rpm
OpenColorIO.src: W: invalid-url Source0: imageworks-OpenColorIO-v1.0.7-0-g87da508.tar.gz
Github source. Instructions provided in spec file.

OpenColorIO-devel.x86_64: W: no-documentation
Documentation is in a -doc sub-package.

Comment 1 Kalev Lember 2012-04-24 21:37:33 UTC
Taking for review.

Comment 2 Kalev Lember 2012-04-25 01:38:35 UTC
It doesn't seem to build in koji. Scratch builds for both F17 and rawhide fail with a self test error:
> The following tests FAILED:
> 	  1 - ocio_core_tests (Failed)

http://koji.fedoraproject.org/koji/taskinfo?taskID=4020499


> Patch0:         OpenColorIO-1.0.7-pylib_no_soname.patch
> Patch1:         OpenColorIO-1.0.7-docfix.patch

Have you sent these changes upstream?

The first patch rips out the code that sets the SOVERSION for the shared object; why is such a change needed? Would be great to have a comment explaining this in the spec file.


> %files
> [...]
> %{_datadir}/ocio/setup_ocio.sh

Should own the directory as well, to avoid leaving behind unowned directories when uninstalling the package.

Comment 3 Richard Shaw 2012-04-25 13:03:05 UTC
(In reply to comment #2)
> It doesn't seem to build in koji. Scratch builds for both F17 and rawhide fail
> with a self test error:
> > The following tests FAILED:
> > 	  1 - ocio_core_tests (Failed)
> 
> http://koji.fedoraproject.org/koji/taskinfo?taskID=4020499

I'll have to take a look. I'm not sure if all tests are expected to pass yet for all platforms...


> > Patch0:         OpenColorIO-1.0.7-pylib_no_soname.patch
> > Patch1:         OpenColorIO-1.0.7-docfix.patch
> 
> Have you sent these changes upstream?

Yes. I've been pretty active with upstream.


> The first patch rips out the code that sets the SOVERSION for the shared
> object; why is such a change needed? Would be great to have a comment
> explaining this in the spec file.

I can add a comment but basically since it's a python module it should not have a soanme. I'm not sure where or if that's stated in the guidelines. I based this on the fact that none of the other python modules in site-packages seem to have a soname.

We decided on a different approach for a permanent fix. In the commit I sent upstream, there's a cmake option to either set or not set the soname for the python module.

Patch1 has already been accepted upstream.


> > %files
> > [...]
> > %{_datadir}/ocio/setup_ocio.sh
> 
> Should own the directory as well, to avoid leaving behind unowned directories
> when uninstalling the package.

Whoops! Good catch.

Comment 5 Kalev Lember 2012-04-25 22:19:43 UTC
Thanks for the explanation, if you are working closely with upstream then it's all good.

The spec file changes look fine, but it is still failing to build in F17 / rawhide:
http://koji.fedoraproject.org/koji/taskinfo?taskID=4023252

If you have reasons to believe that the self test result isn't correct, could just disable it, I suppose. Or wrap the 'make test' in %if %endif and only conditionally run the tests.

Comment 6 Richard Shaw 2012-04-26 00:16:47 UTC
I guess we'll just have to disable it for now. I just checked (again as it turns out) that it builds fine for me in mock on both 17 and rawhide. I'm not sure what different about the build server that's causing it to fail...

SPEC: http://hobbes1069.fedorapeople.org/OpenColorIO/OpenColorIO.spec
SRPM:
http://hobbes1069.fedorapeople.org/OpenColorIO/OpenColorIO-1.0.7-3.fc16.src.rpm

Comment 7 Kalev Lember 2012-04-26 01:13:21 UTC
Fedora review OpenColorIO-1.0.7-3.fc16.src.rpm 2012-04-25

+ OK
! needs attention

rpmlint output:
$ rpmlint OpenColorIO \
          OpenColorIO-debuginfo \
          OpenColorIO-devel \
          OpenColorIO-doc \
          OpenColorIO-1.0.7-3.fc16.src.rpm
OpenColorIO-devel.x86_64: W: no-documentation
OpenColorIO.src: W: invalid-url Source0: imageworks-OpenColorIO-v1.0.7-0-g87da508.tar.gz
5 packages and 0 specfiles checked; 0 errors, 2 warnings.

+ Rpmlint warnings are harmless and can be ignored
+ The package is named according to Fedora packaging guidelines
+ The spec file name matches the base package name.
+ The package meets the Packaging Guidelines
+ The package is licensed with a Fedora approved license and meets the
  Licensing Guidelines.
+ The license field in the spec file matches the actual license
+ The package contains the license file (LICENSE)
+ Spec file is written in American English
+ Spec file is legible
+ Upstream sources match the sources in the srpm.
+ The package builds in koji
n/a ExcludeArch bugs filed
+ BuildRequires look sane
n/a The spec file handles locales properly
+ ldconfig in %post and %postun
+ Package does not bundle copies of system libraries
n/a Package isn't relocatable
+ Package owns all the directories it creates
+ No duplicate files in %files
+ Permissions are properly set
+ Consistent use of macros
+ The package must contain code or permissible content
+ Large documentation files should go in -doc subpackage
+ Files marked %doc should not affect package
+ Header files should be in -devel
n/a Static libraries should be in -static
+ Library files that end in .so must go in a -devel package
+ -devel must require the fully versioned base
+ Packages should not contain libtool .la files
n/a Proper .desktop file handling
+ Doesn't own files or directories already owned by other packages
+ Filenames are valid UTF-8

Only issue I see is that it is passing -msse2 to the compiler when building for the i686 architecture. Not all i686 processors have SSE support and as this architecture is for compatibility for older processors, we should avoid building binaries that don't run on all supported i686 processors.

From a quick look at the CMakeLists.txt file, looks like passing -DOCIO_USE_SSE=OFF should be enough to turn this off.

Usually it would be enough to unconditionally disable the -msse2 flag, as this is enabled by default for the x86-64 with gcc. But in this case, it looks like there's also some SSE-specific code in OpenColorIO itself, so just unconditionally turning off all SSE support with -DOCIO_USE_SSE=OFF wouldn't work, if we want to keep the SSE-specific code for x86-64.

Could perhaps use:
%ifnarch x86_64
  -DOCIO_USE_SSE=OFF
%endif

to disable SSE on all architectures where we don't want SSE support (including arm and other secondary arches without SSE)?

Comment 8 Richard Shaw 2012-04-26 14:50:37 UTC
I've sent a link to your comment to the ocio-dev mailing list yesterday evening. Hopefully someone will respond soon.

Comment 9 Richard Shaw 2012-04-26 15:08:54 UTC
I did some research (not that I didn't believe you!) to satisfy my curiosity and I went ahead with the %ifnarch since that will also disable it for PPC64 if I decide to go for an EPEL branch.

SPEC: http://hobbes1069.fedorapeople.org/OpenColorIO/OpenColorIO.spec
SRPM:
http://hobbes1069.fedorapeople.org/OpenColorIO/OpenColorIO-1.0.7-4.fc16.src.rpm

Comment 10 Kalev Lember 2012-04-26 15:22:38 UTC
Excellent. 

Regarding i686 processors, I don't think there's anything that OpenColorIO upstream should do here. It's not really their problem that we're supporting i686 processors without SSE. They've already added support for disabling SSE (the
-DOCIO_USE_SSE=OFF option), and now it's just the matter of using this in the
Fedora build.

Regarding PPC64, I guess it would be nice if OpenColorIO's cmake build system automatically disabled SSE support on architectures that don't have the SSE instruction set (like arm and ppc), but we've already got an acceptable workaround for that in the spec file.

Comment 11 Kalev Lember 2012-04-26 15:26:41 UTC
Looks all good now.

APPROVED

Comment 12 Richard Shaw 2012-04-26 15:34:06 UTC
Great! Thanks for the review.

Comment 13 Richard Shaw 2012-04-26 15:35:08 UTC
New Package SCM Request
=======================
Package Name: OpenColorIO
Short Description: Enables color transforms and image display across graphics apps
Owners: hobbes1069
Branches: f16 f17 el6
InitialCC:

Comment 14 Gwyn Ciesla 2012-04-26 15:54:13 UTC
Git done (by process-git-requests).

Comment 15 Fedora Update System 2012-04-26 16:39:44 UTC
OpenColorIO-1.0.7-4.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/OpenColorIO-1.0.7-4.fc16

Comment 16 Fedora Update System 2012-04-26 16:39:52 UTC
OpenColorIO-1.0.7-4.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/OpenColorIO-1.0.7-4.fc17

Comment 17 Fedora Update System 2012-04-27 06:00:17 UTC
OpenColorIO-1.0.7-4.fc16 has been pushed to the Fedora 16 testing repository.

Comment 18 Fedora Update System 2012-05-04 20:32:18 UTC
OpenColorIO-1.0.7-4.fc16 has been pushed to the Fedora 16 stable repository.

Comment 19 Fedora Update System 2012-05-04 23:08:41 UTC
OpenColorIO-1.0.7-4.fc17 has been pushed to the Fedora 17 stable repository.

Comment 20 Richard Shaw 2014-05-20 16:46:45 UTC
Package Change Request
======================
Package Name: OpenColorIO
New Branches: epel7
Owners: hobbes1069
InitialCC:

Comment 21 Gwyn Ciesla 2014-05-20 18:35:43 UTC
Git done (by process-git-requests).


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