Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

darktable: Removed unneeded dependencies, update Lua to 5.3 #25133

Merged
merged 1 commit into from Feb 9, 2018

Conversation

Hodapp87
Copy link
Contributor

Motivation for this change

Based on what LebedevRI told me on IRC and in darktable-org/darktable#1474, darktable's buildInputs in nixpkgs contain many unnecessary things. Additionally, the version of Lua supplied is too old for the build to use it, so it was updated to 5.3.

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@mention-bot
Copy link

@Hodapp87, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @lsix and @cillianderoiste to be potential reviewers.

@Mic92
Copy link
Member

Mic92 commented Apr 23, 2017

According to the autotools configure script the following libraries were requested:

  • libXdmcp, epoxy, udev, pcre, libpthreadstubs

These are mostley transitive dependencies from other pkgconfig files like glib or xcb.

diff --git a/pkgs/applications/graphics/darktable/default.nix b/pkgs/applications/graphics/darktable/default.nix
index 5c1ea345d9..4fc6856e32 100644
--- a/pkgs/applications/graphics/darktable/default.nix
+++ b/pkgs/applications/graphics/darktable/default.nix
@@ -1,10 +1,10 @@
 { stdenv, fetchurl, libsoup, graphicsmagick, json_glib
-, atk, cairo, cmake, curl, dbus_glib, exiv2, glib
+, atk, cairo, cmake, curl, dbus_glib, exiv2, glib, pcre, libpthreadstubs
 , gtk3, ilmbase, intltool, lcms2
 , lensfun, libX11, libexif, libgphoto2, libjpeg
 , libpng, librsvg, libtiff, libxcb
 , openexr, osm-gps-map, pixman, pkgconfig, sqlite, bash, libxslt, openjpeg
-, lua5_3, pugixml, colord, colord-gtk, libxshmfence, libxkbcommon
+, lua5_3, pugixml, colord, colord-gtk, libxshmfence, libxkbcommon, xlibs, epoxy, udev
 , at_spi2_core, libwebp, libsecret, wrapGAppsHook, gnome3
 }:

@@ -20,13 +20,13 @@ stdenv.mkDerivation rec {
   };

   buildInputs =
-    [ atk cairo cmake curl dbus_glib exiv2 glib gtk3
-      ilmbase intltool lcms2 lensfun libX11 libexif
+    [ atk cairo cmake curl dbus_glib exiv2 glib gtk3 pcre libpthreadstubs
+      ilmbase intltool lcms2 lensfun libX11 libexif xlibs.libXdmcp
       libgphoto2 libjpeg libpng
       librsvg libtiff libxcb openexr pixman pkgconfig sqlite libxslt
       libsoup graphicsmagick json_glib openjpeg lua5_3 pugixml
       colord colord-gtk libxshmfence libxkbcommon at_spi2_core
-      libwebp libsecret wrapGAppsHook gnome3.adwaita-icon-theme
+      libwebp libsecret wrapGAppsHook gnome3.adwaita-icon-theme epoxy udev
       osm-gps-map
     ];

@Hodapp87
Copy link
Contributor Author

darktable's build doesn't use autotools though; it uses CMake. What autotools configure script are you talking about?

@Mic92
Copy link
Member

Mic92 commented Apr 23, 2017

I meant cmake.

@Hodapp87
Copy link
Contributor Author

The build seems to proceed just fine without these (without disabling any optional features), and being transitive dependencies, I don't see why they'd need to be declared. @LebedevRI, want to weigh in on this one?

@LebedevRI
Copy link

@Mic92
Hi.

As darktable dev, i can say that i'm quite sure that darktable itself does not directly use any of those deps which are being suggested in #25133 (comment), namely: pcre, libpthreadstubs, epoxy, udev
I'm not too sure about libpthreadstubs, we do hard-require pthreads...

Is the default.nix supposed to contain only the application's direct dependencies, or all the transitive deps too?

@7c6f434c
Copy link
Member

7c6f434c commented May 1, 2017

Not really clear-cut: when some dependency of a library is needed to build anything dependent on that library, the dependency is usually added to propagatedBuildInputs and doesn't need explicit passing (but mistakes happen). If CMake builds require discoverability of a dependency and its library files but Autotools don't, it is most likely that CMake-using packages will just get an extra build input.

@7c6f434c
Copy link
Member

7c6f434c commented May 1, 2017

Hm, build log looks like darktable would prefer Lua 5.2 and we pass Lua 5.3

@LebedevRI
Copy link

dt git master uses lua 5.3, all current stable releases still use 5.2

@Hodapp87
Copy link
Contributor Author

Hodapp87 commented May 1, 2017

Are they referring to https://travis-ci.org/NixOS/nixpkgs/jobs/224812233#L2364?

@Hodapp87
Copy link
Contributor Author

Hodapp87 commented May 1, 2017

Ah... I must have confused this with darktable-org/darktable#1474 which was on git master while this is on a stable release. I'll update back.

@7c6f434c
Copy link
Member

7c6f434c commented May 1, 2017

Well, the PR is about improving the packaging of the release.

@Hodapp87: yes; I wonder if this actually disables Lua support.

@c0bw3b
Copy link
Contributor

c0bw3b commented Nov 13, 2017

v2.2.5 has landed in the meantime.
@Hodapp87 do you still want to cleanup this package definition?

@jtojnar jtojnar force-pushed the darktable_removedeps branch 2 times, most recently from efff8ad to 17cb693 Compare February 4, 2018 00:19
@jtojnar
Copy link
Contributor

jtojnar commented Feb 4, 2018

@GrahamcOfBorg eval

@jtojnar
Copy link
Contributor

jtojnar commented Feb 4, 2018

Let’s wrap it up, I have rebased this and removed further dependencies.

@jtojnar
Copy link
Contributor

jtojnar commented Feb 5, 2018

@GrahamcOfBorg build darktable

@GrahamcOfBorg
Copy link

Failure on x86_64-darwin (full log)

Partial log (click to expand)

Package ‘darktable-2.4.1’ in /private/var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/lnl7-mac/pkgs/applications/graphics/darktable/default.nix:42 is not supported on ‘x86_64-darwin’, refusing to evaluate.

a) For `nixos-rebuild` you can set
  { nixpkgs.config.allowBroken = true; }
in configuration.nix to override this.

b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
  { allowBroken = true; }
to ~/.config/nixpkgs/config.nix.

@GrahamcOfBorg
Copy link

Failure on aarch64-linux (full log)

Partial log (click to expand)

g++: error: unrecognized command line option '-msse2'
make[2]: *** [libs/lensfun/CMakeFiles/lensfun.dir/modifier.cpp.o] Error 1
g++: error: unrecognized command line option '-msse'; did you mean '-fdse'?
g++: error: unrecognized command line option '-msse2'
make[2]: *** [libs/lensfun/CMakeFiles/lensfun.dir/auxfun.cpp.o] Error 1
make[1]: *** [libs/lensfun/CMakeFiles/lensfun.dir/all] Error 2
make: *** [all] Error 2
builder for '/nix/store/c4acz90smx69s8arcksvaa38ygnsafyy-lensfun-0.3.2.drv' failed with exit code 2
cannot build derivation '/nix/store/wacll2zw76335cgc9ibndkrj6cvwm225-darktable-2.4.1.drv': 1 dependencies couldn't be built
error: build of '/nix/store/wacll2zw76335cgc9ibndkrj6cvwm225-darktable-2.4.1.drv' failed

@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Partial log (click to expand)

shrinking /nix/store/zrxspsi99frpizj0pd9py36d14f2fgrg-darktable-2.4.1/lib/darktable/plugins/libwatermark.so
shrinking /nix/store/zrxspsi99frpizj0pd9py36d14f2fgrg-darktable-2.4.1/lib/darktable/plugins/libgamma.so
shrinking /nix/store/zrxspsi99frpizj0pd9py36d14f2fgrg-darktable-2.4.1/lib/darktable/plugins/librawdenoise.so
shrinking /nix/store/zrxspsi99frpizj0pd9py36d14f2fgrg-darktable-2.4.1/lib/darktable/plugins/libtonemap.so
gzipping man pages under /nix/store/zrxspsi99frpizj0pd9py36d14f2fgrg-darktable-2.4.1/share/man/
strip is /nix/store/5qj61lcvzlap87rf6blvf8p577d482bv-binutils-2.28.1/bin/strip
stripping (with command strip and flags -S) in /nix/store/zrxspsi99frpizj0pd9py36d14f2fgrg-darktable-2.4.1/lib  /nix/store/zrxspsi99frpizj0pd9py36d14f2fgrg-darktable-2.4.1/bin 
patching script interpreter paths in /nix/store/zrxspsi99frpizj0pd9py36d14f2fgrg-darktable-2.4.1
checking for references to /tmp/nix-build-darktable-2.4.1.drv-0 in /nix/store/zrxspsi99frpizj0pd9py36d14f2fgrg-darktable-2.4.1...
/nix/store/zrxspsi99frpizj0pd9py36d14f2fgrg-darktable-2.4.1

Based on what LebedevRI told me on IRC and in
darktable-org/darktable#1474
@jtojnar jtojnar merged commit c6bd327 into NixOS:master Feb 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants