Page MenuHomeFreeBSD

graphics/darktable: Update to 1.6.9
ClosedPublic

Authored by dumbbell on Oct 16 2015, 5:50 PM.

Details

Summary

Add a new option, OPENMP, to build darktable with OpenMP. This requires LLVM 3.7 as a runtime dependency when GCC is not the default compiler. The option is enabled by default because it brings an important performance improvement.

Diff Detail

Repository
rP FreeBSD ports repository
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 787
Build 787: arc lint + arc unit

Event Timeline

dumbbell updated this revision to Diff 9454.Oct 16 2015, 5:50 PM
dumbbell retitled this revision from to graphics/darktable: Update to 1.6.9.
dumbbell updated this object.
dumbbell edited the test plan for this revision. (Show Details)
dumbbell added a reviewer: kwm.
kwm added inline comments.Oct 17 2015, 10:27 AM
graphics/darktable/Makefile
99

Not sure this is needed.

104

This does also need a BUILD_DEPEND

109

Can darktable find the omp library after linking? Also please check if openMP is actually enabled, ImageMagick and inkscape don't by default when using clang 3.7.

dumbbell added inline comments.Oct 17 2015, 11:02 AM
graphics/darktable/Makefile
99

It didn't work with bsd.port.options.mk: make(1) complained with a syntax error in the .if below, usually meaning a variable is undefined.

104

Ok, I will add it.

About RUN_DEPENDS, the correct dependency is ${LOCALBASE}/llvm37/lib/libomp.so, not clang37. Is there a way to specify that instead?

109

Darktable works fine on my laptop and lld(1) finds libomp.so.

Darktable cmake conf properly detects -fopenmp with Clang 3.7 and enables OpenMP support.

dumbbell updated this revision to Diff 9464.Oct 17 2015, 11:04 AM

Add devel/llvm37 as a BUILD_DEPENDS

dumbbell updated this revision to Diff 9527.Oct 20 2015, 8:21 AM

Depend on Lua 5.2 explicitly (not any version)

darktable 1.6.9 was officially released, so the archive can be downloaded now with make fetch.

There is no need to update distinfo as it is the exact same archive provided to package managers.

danfe added a subscriber: danfe.Nov 5 2015, 8:18 AM
danfe added inline comments.
graphics/darktable/Makefile
104

I'm not sure if it would correctly define all required dependencies (both build- and run-time), but you might consider using USES=compiler:openmp.

117

I think this target is not needed (when building docless package, DOCSDIR is ignored).

dumbbell updated this revision to Diff 10029.Nov 8 2015, 1:03 PM
dumbbell marked 4 inline comments as done.

Address @danfe's comment + patch to src/CMakeLists.txt

The patch to src/CMakeLists.txt was prepared for the upcoming darktable 2.0 and backported to darktable 1.6. When darktable 2.0 is compiled with Clang and -ffast-math (which is enabled by default), isnan() always returns false, even if -fno-finite-math-only is specified. darktable 2.0 crashes on startup without this patch. darktable 1.6 still starts but the patch appears to improve stability in lighttable (I get more freezes/lockups without it). So let's apply it now to darktable 1.6.

dumbbell added inline comments.Nov 8 2015, 1:06 PM
graphics/darktable/Makefile
104

USES=compiler:openmp always pulls in GCC. This can't work with darktable because g++ and libstdc++ are used to build the C++ code and it's not compatible with libc++.

The trick used by graphics/rawtherapee is at least as ugly and darktable crashes with it anyway.

dumbbell updated this revision to Diff 10113.Nov 11 2015, 12:15 PM

Do not check ALT_COMPILER_TYPE

kwm accepted this revision.Nov 11 2015, 12:18 PM
kwm edited edge metadata.

I have seen the patch-src_CMakeLists.txt and it correct, even if phabricator doesn't show it...

This revision is now accepted and ready to land.Nov 11 2015, 12:18 PM
danfe added inline comments.Nov 11 2015, 12:29 PM
graphics/darktable/Makefile
104

USES=compiler:openmp always pulls in GCC.

Oh, I see. Reading the above comment (Enable OpenMP support with Clang 3.7) it looks like that we'd probably want to relax compiler:openmp dependency on GCC, but that's out of scope of this patch today.

This revision was automatically updated to reflect the committed changes.