Bug 186979 - [PATCH] graphics/darktable: update to 1.4.1
Summary: [PATCH] graphics/darktable: update to 1.4.1
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: Normal Affects Only Me
Assignee: Alexey Dokuchaev
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-02-23 15:00 UTC by Jean-Sébastien Pédron
Modified: 2014-06-04 15:04 UTC (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jean-Sébastien Pédron freebsd_committer freebsd_triage 2014-02-23 15:00:00 UTC
The following patch stages and updates graphics/darktable to 1.4.1:
http://people.freebsd.org/~dumbbell/darktable/graphics-darktable-update-141-staging-openmp.b.patch

The following changes were made to options:
- Remove KWALLET option (unused)
- Rename FACEBOOK option to FB_PICASA, because both export features depend on json-glib.
- Add COLORD option (color management)
- Add GRAPHMAGICK option (support GraphicsMagick's image formats)
- Add LUA option (embed Lua to add scripting)
- Add SQUISH option (use libsquish to compress thumbnails)

The patch enables more options by default, because I think it's more sensible for a photographer using binary packages:
- COLORD
- FB_PICASA
- FLICKR
- GEO
- GRAPHMAGICK
- LUA
- NLS
- OPENJPEG
- SQUISH

Regarding the GCC option (enabled by default to provide OpenMP support), USES=compiler:features allows to check if libc++ is the C++ standard library. In this case, devel/libc++ is pulled as a build dependency: this allows GCC to be used even when other libraries (eg. exiv2) were built with Clang.

COLORD adds a LIB_DEPENDS to graphics/colord, but darktable provides it's own libcolord.so internally (statically linked). The comment in src/CMakeLists.txt explains that libcolord.so in many [Linux] distros is linked against Gtk+3.

GRAPHMAGICK adds a LIB_DEPENDS to graphics/GraphicsMagick13: I used library name with version (libGraphicsMagick.so.14), because it doesn't build with graphics/GraphicsMagick. I didn't try with graphics/GraphicsMagick12.

graphics/webp is added as a mandatory option: CMakeLists.txt defines a "USE_WEBP" CMake option, but that option is never used.

patch-cmake_modules_FindFreetype.cmake is removed because the corresponding CMake module is gone.

patch-src-common-imageio_exr.hh is removed because a better fix (one which works for both FreeBSD and Mac OS X) is committed upstream.

pkg-plist's diff is large because I preferred to keep the order of "make makeplist". Future updates will be easier. Otherwise, changes are small.

The patch was tested using advices from StageDir wiki page, except the poudriere/tinderbox step.
Comment 1 Edwin Groothuis freebsd_committer freebsd_triage 2014-02-23 15:00:08 UTC
Responsible Changed
From-To: freebsd-ports-bugs->danfe

Over to maintainer (via the GNATS Auto Assign Tool)
Comment 2 Matthieu Volat 2014-03-11 22:17:19 UTC
I tried the patch, here's my experience:

* The first line of the post-patch target (reinplace __APPLE__ in a header with __${OPSYS}__) was not needed since a while

* Stage fails if NLS is disabled:
sed: /usr/ports/graphics/darktable/work/stage/usr/ports/graphics/darktable/work/.PLIST.mktmp:
No such file or directory
  I think ${TMPPLIST} should not be prefixed by ${STAGEDIR} in post-install, this is valable for the "without RAWSPEED" section too.

* For what it's worth, I don't think Graphmagick merits to be enabled by default, it places an additional dependency on a not so used package which brings little to the equation (it's used only for importing images which are not raw/jpeg/j2k/tif/webp... well unless bmp producing cameras are common...)

* The package seems to force the usage of cmake ninja generator (and thus add ninja as dependency) for no reason (build and install fine with make).

-- 
Matthieu Volat <mazhe@alkumuna.eu>
tel: 06 84 54 39 43
www: <http://500px.com/Mazhe>
Comment 3 Jean-Sébastien Pédron freebsd_committer freebsd_triage 2014-03-15 14:41:12 UTC
On 11.03.2014 23:17, Matthieu Volat wrote:
> * The first line of the post-patch target (reinplace __APPLE__ in a 
> header with __${OPSYS}__) was not needed since a while


You're right, I updated the patch (see below).

> * Stage fails if NLS is disabled:
> sed: /usr/ports/graphics/darktable/work/stage/usr/ports/graphics/darktable/work/.PLIST.mktmp:
> No such file or directory
> I think ${TMPPLIST} should not be prefixed by ${STAGEDIR} in 
> post-install, this is valable for the "without RAWSPEED" section
> too.


Fixed in the new patch too.

> * For what it's worth, I don't think Graphmagick merits to be 
> enabled by default, it places an additional dependency on a not so
> used package which brings little to the equation (it's used only for
> importing images which are not raw/jpeg/j2k/tif/webp... well unless
> bmp producing cameras are common...)


Yeah, I was not sure what to do and don't know what users expect. Fedora
has it enabled by default, not Debian for instance. I followed your
advice and disabled it by default.

> * The package seems to force the usage of cmake ninja generator (and 
> thus add ninja as dependency) for no reason (build and install fine
> with make).


Ninja isn't mandatory, but it reduces build time by 33% here. It's a
really small build dependency which helps when working on darktable.

Here's the new patch:
http://people.freebsd.org/~dumbbell/darktable/graphics-darktable-update-141-staging-openmp.c.patch

Thank you very much Matthieu for testing!

-- 
Jean-Sébastien Pédron
Comment 4 Matthieu Volat 2014-03-15 16:03:08 UTC
On Sat, 15 Mar 2014 15:41:12 +0100
Jean-Sébastien Pédron <dumbbell@FreeBSD.org> wrote:

> On 11.03.2014 23:17, Matthieu Volat wrote:
> > * For what it's worth, I don't think Graphmagick merits to be 
> > enabled by default, it places an additional dependency on a not so
> > used package which brings little to the equation (it's used only for
> > importing images which are not raw/jpeg/j2k/tif/webp... well unless
> > bmp producing cameras are common...)
> 
> Yeah, I was not sure what to do and don't know what users expect. Fedora
> has it enabled by default, not Debian for instance. I followed your
> advice and disabled it by default.


This one may be tricky, I hoped bsdstat could give a rational argument whether people activate this option, but it does not do port option stats.

> 
> > * The package seems to force the usage of cmake ninja generator (and 
> > thus add ninja as dependency) for no reason (build and install fine
> > with make).
> 
> Ninja isn't mandatory, but it reduces build time by 33% here. It's a
> really small build dependency which helps when working on darktable.


Leaving aside that it is a bit quick to ignore the time to build (and update) ninja versus darktable and others that may use it, I feel it as somewhat arbitrary enforcement, can't this be left at the user discretion /etc/make.conf? It has the advantage of allowing to enable it globaly (or selectively using conditionals).

(but thanks to bring it, I forgot that I wanted to have a look at it at one point)

> 
> Here's the new patch:
> http://people.freebsd.org/~dumbbell/darktable/graphics-darktable-update-141-staging-openmp.c.patch
> 
> Thank you very much Matthieu for testing!


No prob, this is a very important piece of soft for me :)

Here's a bonus: some files are optional to the LUA option, and should be moved from pkg-plist to an concatenation of the PLIST_FILE variable in the Makefile, here's the part in the Makefile:
.if ${PORT_OPTIONS:MLUA}
USES+=      lua
PLIST_FILES+= \
    %%DATADIR%%/lua/darktable/debug.lua \
    %%DATADIR%%/luarc \
    @dirrmtry %%DATADIR%%/lua/darktable \
    @dirrmtry %%DATADIR%%/lua
.else
CMAKE_ARGS+=    -DUSE_LUA:BOOL=OFF
.endif

And another pedantic bonus : the version in GCC_DESC can be either dropped or updated, 4.7 is the new port default :)

> 
> -- 
> Jean-Sébastien Pédron
> 



-- 
Matthieu Volat <mazhe@alkumuna.eu>
tel: 06 84 54 39 43
www: <http://500px.com/Mazhe>
Comment 5 Jean-Sébastien Pédron freebsd_committer freebsd_triage 2014-03-24 13:32:28 UTC
On 15.03.2014 17:03, Matthieu Volat wrote:
>> Ninja isn't mandatory, but it reduces build time by 33% here. It's a
>> really small build dependency which helps when working on darktable.
> 
> Leaving aside that it is a bit quick to ignore the time to build 
> (and update) ninja versus darktable and others that may use it, I
> feel it as somewhat arbitrary enforcement, can't this be left at the
> user discretion /etc/make.conf? It has the advantage of allowing to
> enable it globaly (or selectively using conditionals).


In fact, I looked at what was done in other ports and the use of ninja
is never conditional. Of course, that doesn't mean it should be the same
in darktable. I don't mind if this becomes an option but I believe
darktable already has a ton of options. ninja is fast to install/update
and this would bring more complexity to this port. And it should be
handled in the framework as you said, not in this specific port.

But that's my opinion and the port maintainer may choose otherwise :)

> No prob, this is a very important piece of soft for me :)
> 
> Here's a bonus: some files are optional to the LUA option, and 
> should be moved from pkg-plist to an concatenation of the PLIST_FILE
> variable in the Makefile, here's the part in the Makefile:


Thanks, this is fixed in the new patch. I just let the @dirrmtry
directives in the plist (I can't remember why though, I think there was
an error when putting them in the PLIST_FILES variable).

> And another pedantic bonus : the version in GCC_DESC can be either 
> dropped or updated, 4.7 is the new port default :)


Fixed too :)

So, the new patch:
http://people.freebsd.org/~dumbbell/darktable/graphics-darktable-update-141-staging-openmp.d.patch

Again, thank you for the extensive review!

-- 
Jean-Sébastien Pédron
Comment 6 Jean-Sébastien Pédron freebsd_committer freebsd_triage 2014-04-03 16:35:50 UTC
Here's a new patch using the DOCS option, instead of NOPORTDOCS:
http://people.freebsd.org/~dumbbell/darktable/graphics-darktable-update-141-staging-openmp.e.patch

-- 
Jean-Sébastien Pédron
Comment 7 jean-sebastien.pedron 2014-04-03 16:36:46 UTC
On 03.04.2014 17:35, Jean-Sébastien Pédron wrote:
> Here's a new patch using the DOCS option, instead of NOPORTDOCS:
> http://people.freebsd.org/~dumbbell/darktable/graphics-darktable-update-141-staging-openmp.e.patch


Forgot to mention: this was spotted by antoine@.

-- 
Jean-Sébastien Pédron
Comment 8 jean-sebastien.pedron 2014-04-07 17:05:22 UTC
Here's a refresh of the patch, following exiv2 shlib bump in r350164:
http://people.freebsd.org/~dumbbell/darktable/graphics-darktable-update-141-staging-openmp.f.patch

Otherwise, this is the same patch as the '.e' version.

-- 
Jean-Sébastien Pédron
Comment 9 Jean-Sébastien Pédron freebsd_committer freebsd_triage 2014-04-25 10:01:19 UTC
Here's a new patch, now that darktable 1.4.2 is released:
http://people.freebsd.org/~dumbbell/darktable/graphics-darktable-update-142-staging-openmp.g.patch

Compared to the previous patch:
    o  Update graphics/darktable to 1.4.2 instead of 1.4.1.
    o  WebP is now an option (enabled by default).
    o  OpenEXR is an option too (enabled by default).
    o  GCC is disabled, as it doesn't build anymore on my -CURRENT and
       darktable crashes on 10.0-RELEASE.
    o  The patch to FindLua52.cmake was upstreamed and is part of 1.4.2.

-- 
Jean-Sébastien Pédron
Comment 10 Matthieu Volat 2014-05-11 18:32:24 UTC
Please note that the webp option is a empty stub in darktable: when I
sent a fix to the upstream
(https://github.com/darktable-org/darktable/pull/453), it was decided
that it would be rejected in the spirit of making codec dependencies
non-optional in the future (quoting
https://github.com/darktable-org/darktable/pull/377).

There are also those two lines that must be moved from pkg-plist to the
conditional expansion of PLIST_FILES with PORT_OPTIONS:MLUA, or else there will be packaging problems if LUA is disabled:
@dirrmtry %%DATADIR%%/lua/darktable
@dirrmtry %%DATADIR%%/lua

-- 
Matthieu Volat <mazhe@alkumuna.eu>
Comment 11 martin 2014-05-14 00:31:31 UTC
Hi,

small notice: Darktable 1.4.2 is available. It compiles fine and works
without problems. I haven't seen that you are working at this port and
I haven't tested your patches (I made simpler, quick'n'dirty changes).

Compiling with GCC won't work anymore, in my opinion because there are
dependencies on libs like exiv2 and it is usually compiled with CLANG.

I am not sure, if this is also the problem why there is not threading
on JPEG export (only available with OPENMP??). It is really bad that my
CPU is at 12% load exporting images one by one.

Thank you for supporting this port.

--
Martin
Comment 12 Jean-Sébastien Pédron freebsd_committer freebsd_triage 2014-05-22 09:08:49 UTC
On 11.05.2014 19:32, Matthieu Volat wrote:
> Please note that the webp option is a empty stub in darktable: when
> I sent a fix to the upstream 
> (https://github.com/darktable-org/darktable/pull/453), it was
> decided that it would be rejected in the spirit of making codec
> dependencies non-optional in the future (quoting 
> https://github.com/darktable-org/darktable/pull/377).

Apparently, they changed their mind in revision 1a02bfb8, where the
USE_WEBP flag was introduced, based on #377.

> There are also those two lines that must be moved from pkg-plist to
> the conditional expansion of PLIST_FILES with PORT_OPTIONS:MLUA, or
> else there will be packaging problems if LUA is disabled:
> @dirrmtry %%DATADIR%%/lua/darktable
> @dirrmtry %%DATADIR%%/lua

You previously mentionned that problem, but I'm unable to move those
@dirrmtry entries to the Makefile: I get an error if I do so (but can't
remember the message).

-- 
Jean-Sébastien Pédron
Comment 13 Jean-Sébastien Pédron freebsd_committer freebsd_triage 2014-05-22 09:14:30 UTC
On 14.05.2014 01:31, Martin Sugioarto wrote:
> small notice: Darktable 1.4.2 is available. It compiles fine and works
> without problems. I haven't seen that you are working at this port and
> I haven't tested your patches (I made simpler, quick'n'dirty changes).

My last patch updates the port to 1.4.2. I just didn't changed the PR's
title.

> Compiling with GCC won't work anymore, in my opinion because there are
> dependencies on libs like exiv2 and it is usually compiled with CLANG.

Yes. At some point, I could use a workaround suggested by bapt@ which
allowed to build with GCC but still use libc++ as the standard library
(instead of libstdc++). But lately, this doesn't work anymore for me.
The option is still there (but disabled by default) for others to play
with it and report.

> I am not sure, if this is also the problem why there is not threading
> on JPEG export (only available with OPENMP??). It is really bad that my
> CPU is at 12% load exporting images one by one.

I don't know if the JPEG export code can use OpenMP, but I'm looking
forward to have OpenMP support in Clang to avoid the current solution! :)

-- 
Jean-Sébastien Pédron
Comment 14 Jean-Sébastien Pédron freebsd_committer freebsd_triage 2014-06-04 15:04:38 UTC
Committed in ports in r356162.