Bug 569204 - Review Request: rawtherapee - Raw image processing software
Summary: Review Request: rawtherapee - Raw image processing software
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Sebastian Dziallas
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-02-28 19:38 UTC by Thibault North
Modified: 2010-03-23 02:20 UTC (History)
7 users (show)

Fixed In Version: rawtherapee-3.0-0.11.a1.fc13
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-03-23 02:15:13 UTC
Type: ---
Embargoed:
sebastian: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Thibault North 2010-02-28 19:38:32 UTC
Spec URL: http://tnorth.ch/fedora/rawtherapee.spec
SRPM URL: http://tnorth.ch/fedora/rawtherapee-3.0-0.5.a1.fc12.src.rpm
Description: 

Hi there, Rawtherapee packaging is done a review would be appreciated to get it into Fedora.

Rawtherapee is a RAW image processing software. It gives full control over
many parameters to enhance the raw picture before finally exporting it
to some common image format.
(This description certainly sucks, so feel free to update it as you wish)

Comment 1 Thibault North 2010-02-28 21:34:22 UTC
Updated spec and new SRPM here:
http://tnorth.ch/fedora/rawtherapee-3.0-0.6.a1.fc12.src.rpm

- Remove rawzor from source archive (thanks thm)
- Add icons for desktop entry
- Preserve timestamps (thanks hicham)

Comment 2 Jan Klepek 2010-02-28 22:57:48 UTC
(feel free to add me as co-maintainer)

how do you address following issues?

1] licensing issues [1]
2] there is no need for shared libraries, they are not intended to be used outside of rt afaik.
3] patches lacks comment if it was sent to upstream / or what is id in upstream bugtracker, those patches are not fedora specific. [3]
4] did you consider using patches from ( or as source for tarfile ) following repo which address (almost) all building issues? [2]
5] options file is configuration file it has to reside in /etc (otherwise it breaks FHS) [4]
6] could you please point to koji build for rt? I remember there was issue with debuginfo packages for rawtherapee. Are they created correctly now?
7] why binaries are renamed? Is there any conflict in filenames with any other existing package? If there is no conflict, it could create confusion for users of older version of this software (where my rtstart (or rt) command disappeared?)

[1] http://code.google.com/p/rawtherapee/issues/detail?id=16
[2] http://repo.or.cz/w/rawtherapee-fixes.git
[3] http://fedoraproject.org/wiki/Packaging/Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment
[4] http://fedoraproject.org/wiki/Packaging/Guidelines#Filesystem_Layout

Comment 3 Thibault North 2010-03-01 07:45:43 UTC
To answer quickly:
- licensing issues seem to concern the translations and themes, and can be removed if it is a problem (isn't the global gpl licence enough?)
- upstream released rawtherapee under the gpl only a few month ago and lots of patches were reported but not included in svn yet.

I've just seen that rawtherapee has been forked, and a new repo containing many useful patches has been set up [1].

As a first try for inclusion, we aimed at providing a minimal patchset against svn. We are aware that a lot of things are to be changed in a near future, with new commits to svn.

Anyway, most of your points require fixes and we'll work on them. Maybe we should now rely on that fork before it gets merged or whatever.

Thanks for your comments.

[1] http://github.com/thebiguno/RawTherapee-Fork

Comment 4 Thibault North 2010-03-01 08:21:50 UTC
Just to mention an updated build here, which includes fixes but *not* your suggestions yet.

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

Comment 5 Sebastian Dziallas 2010-03-04 21:10:38 UTC
* MUST: rpmlint must be run on every package. The output should be posted in the review. -- OK

[sebastian@localhost rawtherapee]$ rpmlint rawtherapee-3.0-0.7.a1.fc12.i686.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
[sebastian@localhost rawtherapee]$ rpmlint rawtherapee-debuginfo-3.0-0.7.a1.fc12.i686.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

* MUST: The package must be named according to the Package Naming Guidelines. -- OK
* MUST: The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption. -- OK
* MUST: The package must meet the Packaging Guidelines. -- NEEDSWORK

I found a createicon.exe which we might need to remove from the source as well.
Also, please check whether the requires tag on argyllcms and lcms is really needed to be listed.
The vendor tag in desktop-file-install shouldn't be necessary.

* MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines. -- OK
* MUST: The License field in the package spec file must match the actual license. -- NEEDSWORK

The winclude directory contains all kinds of interesting licenses - I read that we don't really need this directory for the application to work. Can we make sure none of the files there get inherited into the final .rpm and if necessary remove the lib and winclude folders?

Apparently, also these files have some interesting headers:

rawtherapee-3.0/rtengine/iccjpeg.c: *No copyright* UNKNOWN
rawtherapee-3.0/rtengine/jdatasrc.c: UNKNOWN
rawtherapee-3.0/rtengine/dcraw.c: UNKNOWN
rawtherapee-3.0/rtengine/iccjpeg.h: *No copyright* UNKNOWN

Blocking FE-LEGAL for now; Spot, can you clarify whether...?

* the lib and winclude directories need to be removed from the source bundle,
* these four files from above are alright,
* we need to act according to this: http://code.google.com/p/rawtherapee/issues/detail?id=16

* MUST: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc. -- OK
* MUST: The spec file must be written in American English. -- OK
* MUST: The spec file for the package MUST be legible. -- OK
* MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use md5sum for this task. If no upstream URL can be specified for this package, please see the Source URL Guidelines for how to deal with this. -- N/A
* MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture. -- OK
* MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in bugzilla, describing the reason that the package does not compile/build/work on that architecture. The bug number MUST be placed in a comment, next to the corresponding ExcludeArch line. -- N/A
* MUST: All build dependencies must be listed in BuildRequires, except for any that are listed in the exceptions section of the Packaging Guidelines ; inclusion of those as BuildRequires is optional. Apply common sense. -- OK
* MUST: The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden. -- OK
* MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun. -- NEEDSWORK

I guess it'd be good to add this to %post & %postun using the common scriptlets.

* MUST: Packages must NOT bundle copies of system libraries. -- OK
* MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package. Without this, use of Prefix: /usr is considered a blocker. -- N/A
* MUST: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory. -- OK
* MUST: A Fedora package must not list a file more than once in the spec file's %files listings. -- OK
* MUST: Permissions on files must be set properly. Executables should be set with executable permissions, for example. Every %files section must include a %defattr(...) line. -- OK
* MUST: Each package must have a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT). -- OK
* MUST: Each package must consistently use macros. -- NEEDSWORK

Please make sure to either use %{buildroot} or $RPM_BUILD_ROOT consistently.

* MUST: The package must contain code, or permissable content. -- OK
* MUST: Large documentation files must go in a -doc subpackage. (The definition of large is left up to the packager's best judgement, but is not restricted to size. Large can refer to either size or quantity). -- N/A
* MUST: If a package includes something as %doc, it must not affect the runtime of the application. To summarize: If it is in %doc, the program must run properly if it is not present. -- OK
* MUST: Header files must be in a -devel package. -- OK
* MUST: Static libraries must be in a -static package. -- N/A
* MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for directory ownership and usability). -- N/A
* MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package. -- N/A
* MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release} -- N/A
* MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built. -- OK
* MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. If you feel that your packaged GUI application does not need a .desktop file, you must put a comment in the spec file with your explanation. -- OK
* MUST: Packages must not own files or directories already owned by other packages. The rule of thumb here is that the first package to be installed should own the files or directories that other packages may rely upon. This means, for example, that no package in Fedora should ever share ownership with any of the files or directories owned by the filesystem or man package. If you feel that you have a good reason to own a file or directory that another package owns, then please present that at package review time. -- OK
* MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot} (or $RPM_BUILD_ROOT). -- OK
* MUST: All filenames in rpm packages must be valid UTF-8. -- OK

* SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. -- N/A
* SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available. -- N/A
* SHOULD: The reviewer should test that the package builds in mock. -- OK
* SHOULD: The reviewer should test that the package functions as described. A package should not segfault instead of running, for example. -- OK
* SHOULD: If scriptlets are used, those scriptlets must be sane. This is vague, and left up to the reviewers judgement to determine sanity. -- OK
* SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency. -- N/A
* SHOULD: The placement of pkgconfig(.pc) files depends on their usecase, and this is usually for development purposes, so should be placed in a -devel pkg. A reasonable exception is that the main pkg itself is a devel tool not installed in a user runtime, e.g. gcc or gdb. -- N/A
* SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself. -- N/A

Comment 6 Thibault North 2010-03-05 21:27:33 UTC
>I found a createicon.exe which we might need to remove from the source as well.
Also, please check whether the requires tag on argyllcms and lcms is really
needed to be listed.
The vendor tag in desktop-file-install shouldn't be necessary.

Fixed.

>* MUST: Every binary RPM package (or subpackage) which stores shared library
files (not just symlinks) in any of the dynamic linker's default paths, must
call ldconfig in %post and %postun. -- NEEDSWORK

Getting rid of the shared libraries seems to be the best think to do, according to Jan.

> * MUST: Each package must consistently use macros. -- NEEDSWORK
*Should* be ok. Some people seem to be always macros when possible, some other not. What do you think ?

>7] why binaries are renamed? Is there any conflict in filenames with any other
existing package? If there is no conflict, it could create confusion for users
of older version of this software (where my rtstart (or rt) command
disappeared?)

Fixed. Well, now calling rtstart actually starts it, but rt fails (requiring shared libs and paths). This should be fixed as soon as rawtherapee will properly find its own path.

Updated koji build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=2033824

Comment 7 Sebastian Dziallas 2010-03-07 15:46:45 UTC
I'm having trouble finding the latest .spec file - could you upload that?

Comment 9 Tom "spot" Callaway 2010-03-08 21:54:51 UTC
There look to be several licensing questions in here, please let me know if I missed any:

1. There is not clear licensing on the included translations and themes. If upstream cannot clarify the licensing of those files, they need to be removed.

2. Several files have unclear licensing:

rawtherapee-3.0/rtengine/iccjpeg.c: Copied 99% intact from lcms (jpegicc/iccjpeg.c)
rawtherapee-3.0/rtengine/jdatasrc.c: Modified copy from libjpeg (
rawtherapee-3.0/rtengine/dcraw.c: Exact copy from dcraw (dcraw/dcraw.c)
rawtherapee-3.0/rtengine/iccjpeg.h: Copied 99% intact from lcms (jpegicc/iccjpeg.h)

The files copied from lcms are MIT. (I'm not crazy about the file copying, but this code chunk isn't in the lcms library.)
The file copied from dcraw is GPLv2+. There is no dcraw library, because upstream thinks it is better for people to either copy the dcraw code into their application or fork off dcraw processes. I can't wait for the first major dcraw exploit to come out. :P
The file copied from libjpeg is IJG. The rawtherapee upstream went to some trouble to fork this code and obfuscate the function names so it could link to libjpeg without conflicting. I have no idea why, but I suspect this is a security exploit waiting to happen. If I were the maintainer here, I'd try to patch this to use the system libjpeg instead.

*****
Leaving FE-Legal in place until the translations/theme licensing is either clarified or the files are clearly removed.

Nevertheless, from a strict licensing perspective, these files are all acceptably licensed, if poorly marked. License tag on this package should be:

License: GPLv3 and MIT and IJG

Comment 10 Sebastian Dziallas 2010-03-14 15:15:43 UTC
Thibault, I'm not sure, but it might make sense to remove the files in question to unblock us here until upstream takes action. Just suggesting that, though - it's your call! ;)

Comment 11 Thibault North 2010-03-14 15:31:53 UTC
Hi Sebastian,
Sure, sorry I've been pretty busy these days. Oh, and don't hesitate to also apply fixes and post new SPECS here, that is always welcome!

We can safely remove all languages which are not english-us, and also drop all themes.

@Tom: thanks for your work to clarify what is to be fixed. Most of these issues are discussed on upstream's forum, and we will try to have this done soon.
english-us translation file contains no copyright. I guess that is the only one we can keep for now.

I will fix that asap (but sebastian if you have time, just do it :).

Comment 12 Thibault North 2010-03-17 17:05:34 UTC
Hi again,

New spec and source rpm here:

http://tnorth.ch/fedora/rawtherapee.spec
http://tnorth.ch/fedora/rawtherapee-3.0-0.9.a1.fc12.src.rpm

Koji build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=2059329

Hope it will fix our legal issues :-)

Comment 13 Sebastian Dziallas 2010-03-17 17:36:53 UTC
Spot, thanks for wading through this!

Oh, two things - otherwise this looks good from my side: The source tarball still seems to contain the files in question. Can you make sure to rebuild the tarball following the steps at the top of the .spec file and rebuild the .rpm?

You should presumably also run this, since I see two .so files in the .rpm [1].

As soon as we can make sure that the files in question got really excluded, we'll need to ask Spot to lift FE-LEGAL, after which I can approve this. :)

[1] http://fedoraproject.org/wiki/PackagingGuidelines#Shared_Libraries

Comment 14 Thibault North 2010-03-17 17:59:13 UTC
Ahem! Sorry for that. Will fix it ASAP.

Comment 15 Thibault North 2010-03-17 18:58:47 UTC
Ok. Actually removed these files, plus windows libs. (lib/*.a)
Now calls ldconfig at %post and %postun. In the future, we will certainly drop these shared libs that aren't use by other programs.

http://tnorth.ch/fedora/rawtherapee.spec
http://tnorth.ch/fedora/rawtherapee-3.0-0.10.a1.fc12.src.rpm

Source archive is now less than 900KiB.

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

Comment 16 Sebastian Dziallas 2010-03-17 19:18:03 UTC
Awesome, thanks - Spot, your call! ;)

Comment 17 Tom "spot" Callaway 2010-03-17 19:26:46 UTC
Perhaps I'm not being clear. The translations and themes are still present, and they do not have licensing. This is not acceptable. They either need to have their license clarified or they need to be removed.

Comment 18 Sebastian Dziallas 2010-03-17 19:38:12 UTC
Uh? Perhaps I'm wrong, but I can't see them anywhere, neither in the source rpm nor in the built one in 3.0-0.10.a1. Just to make sure I get you correctly, you're talking about the files in /release/languages and /release/themes, right? The languages folder seems to contain only the english-us locale and the themes folder is apparently empty. 3.0-0.9.a1 still had those in, but I thought Thibault had fixed this with the new update - am I missing anything?

Comment 19 Sebastian Dziallas 2010-03-17 19:41:36 UTC
Or wait, are you referring to the english-us file? If that's the blocker, Thibault, you should probably nag upstream... ;)

/me is sorry for all this confusion

Comment 20 Tom "spot" Callaway 2010-03-17 19:47:33 UTC
No, this was my mistake, I accidentally installed the older SRPM.

Lifting FE-Legal

Comment 21 Sebastian Dziallas 2010-03-17 20:03:27 UTC
Alright, thanks! Since all other issues have been fixed, this package is APPROVED.

Comment 22 Thibault North 2010-03-18 19:06:53 UTC
New Package CVS Request
=======================
Package Name: rawtherapee
Short Description: RAW image processing software
Owners: tnorth sdziallas hpejakle
Branches: F-11 F-12 F-13
InitialCC:

Comment 23 Sebastian Dziallas 2010-03-18 19:22:45 UTC
I'm pretty sure you want sdz instead of sdziallas - the former one is me, whereas the latter one account doesn't exist... :) - thanks for adding me!

Comment 24 Thibault North 2010-03-18 19:32:03 UTC
Yes sorry. I did a quick wiki search to get your FAS account and I believed it :-)

Thanks for having spotted it quickly :)

New Package CVS Request
=======================
Package Name: rawtherapee
Short Description: RAW image processing software
Owners: tnorth sdz hpejakle
Branches: F-11 F-12 F-13
InitialCC:

Comment 25 Thomas Spura 2010-03-19 10:31:22 UTC
You might want "fedora-cvs ?" and not "fedora-cvs +" as currently set...

Comment 26 Thibault North 2010-03-19 12:47:34 UTC
Fixed. Sorry for the typo...

Comment 27 Kevin Fenzi 2010-03-19 19:50:33 UTC
CVS done (by process-cvs-requests.py).

Comment 28 Fedora Update System 2010-03-20 14:50:10 UTC
rawtherapee-3.0-0.11.a1.fc13 has been submitted as an update for Fedora 13.
http://admin.fedoraproject.org/updates/rawtherapee-3.0-0.11.a1.fc13

Comment 29 Fedora Update System 2010-03-20 14:50:18 UTC
rawtherapee-3.0-0.11.a1.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/rawtherapee-3.0-0.11.a1.fc12

Comment 30 Fedora Update System 2010-03-23 02:15:07 UTC
rawtherapee-3.0-0.11.a1.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 31 Fedora Update System 2010-03-23 02:20:24 UTC
rawtherapee-3.0-0.11.a1.fc13 has been pushed to the Fedora 13 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.