gdk-pixbuf: multiple module files

doc/languages-frameworks/gnome.section.md

@@ -34,11 +34,11 @@ To allow software to use various virtual file systems, `gvfs` package can be als
 
 ### GdkPixbuf loaders {#ssec-gnome-gdk-pixbuf-loaders}
 
-GTK applications typically use [GdkPixbuf](https://developer.gnome.org/gdk-pixbuf/stable/) to load images. But `gdk-pixbuf` package only supports basic bitmap formats like JPEG, PNG or TIFF, requiring to use third-party loader modules for other formats. This is especially painful since GTK itself includes SVG icons, which cannot be rendered without a loader provided by `librsvg`.
+GTK applications typically use [GdkPixbuf](https://developer.gnome.org/gdk-pixbuf/stable/) to load images. But `gdk-pixbuf` package only supports basic bitmap formats like JPEG, PNG or TIFF, requiring third-party loader modules for other formats. GTK itself includes SVG icons, which cannot be rendered without a loader provided by `librsvg`.
 
-Unlike other libraries mentioned in this section, GdkPixbuf only supports a single value in its controlling environment variable `GDK_PIXBUF_MODULE_FILE`. It is supposed to point to a cache file containing information about the available loaders. Each loader package will contain a `lib/gdk-pixbuf-2.0/2.10.0/loaders.cache` file describing the default loaders in `gdk-pixbuf` package plus the loader contained in the package itself. If you want to use multiple third-party loaders, you will need to create your own cache file manually. Fortunately, this is pretty rare as [not many loaders exist](https://gitlab.gnome.org/federico/gdk-pixbuf-survey/blob/master/src/modules.md).
+Each loader package will contain a `lib/gdk-pixbuf-2.0/2.10.0/loaders.cache` file containing information about the available loaders. GdkPixbuf looks for these files in the `GDK_PIXBUF_MODULE_FILE` environment variable. Although upstream GdkPixbuf only supports a single file, in nixpkgs it is patched to accept multiple files, separated by `:`.
 
-`gdk-pixbuf` contains [a setup hook](#ssec-gnome-hooks-gdk-pixbuf) that sets `GDK_PIXBUF_MODULE_FILE` from dependencies but as mentioned in further section, it is pretty limited. Loaders should propagate this setup hook.
+[`wrapGAppsHook`](#ssec-gnome-hooks-wrapgappshook) handles setting `GDK_PIXBUF_MODULE_FILE` for GTK apps. If you're manually constructing a wrapper, consider `--prefix`ing the variable rather than overwriting it, so that users can extend your program's capabilities through `services.xserver.gdk-pixbuf.modulePackages`.
 
 ### Icons {#ssec-gnome-icons}
 
@@ -106,7 +106,7 @@ For convenience, it also adds `dconf.lib` for a GIO module implementing a GSetti
 
 - []{#ssec-gnome-hooks-glib} `glib` setup hook will populate `GSETTINGS_SCHEMAS_PATH` and then `wrapGAppsHook` will prepend it to `XDG_DATA_DIRS`.
 
-- []{#ssec-gnome-hooks-gdk-pixbuf} `gdk-pixbuf` setup hook will populate `GDK_PIXBUF_MODULE_FILE` with the path to biggest `loaders.cache` file from the dependencies containing [GdkPixbuf loaders](#ssec-gnome-gdk-pixbuf-loaders). This works fine when there are only two packages containing loaders (`gdk-pixbuf` and e.g. `librsvg`) – it will choose the second one, reasonably expecting that it will be bigger since it describes extra loader in addition to the default ones. But when there are more than two loader packages, this logic will break. One possible solution would be constructing a custom cache file for each package containing a program like `services/x11/gdk-pixbuf.nix` NixOS module does. `wrapGAppsHook` copies the `GDK_PIXBUF_MODULE_FILE` environment variable into the produced wrapper.
+- []{#ssec-gnome-hooks-gdk-pixbuf} `wrapGAppsHook` will automatically prepend `GDK_PIXBUF_MODULE_FILE` with modules from `gdk-pixbuf` and `librsvg`. To include additional gdk-pixbuf modules, set `extraGdkPixbufModules` with an array of packages.
 
 - []{#ssec-gnome-hooks-gtk-drop-icon-theme-cache} One of `gtk3`’s setup hooks will remove `icon-theme.cache` files from package’s icon theme directories to avoid conflicts. Icon theme packages should prevent this with `dontDropIconThemeCache = true;`.
 

doc/stdenv/stdenv.chapter.md

@@ -1077,10 +1077,6 @@ Adds the `share/texmf-nix` subdirectory of each build input to the `TEXINPUTS` e
 
 Sets the `QTDIR` environment variable to Qt’s path.
 
-### gdk-pixbuf {#setup-hook-gdk-pixbuf}
-
-Exports `GDK_PIXBUF_MODULE_FILE` environment variable to the builder. Add librsvg package to `buildInputs` to get svg support. See also the [setup hook description in GNOME platform docs](#ssec-gnome-hooks-gdk-pixbuf).
-
 ### GHC {#ghc}
 
 Creates a temporary package database and registers every Haskell build input in it (TODO: how?).

nixos/doc/manual/release-notes/rl-2205.section.md

@@ -621,6 +621,17 @@ In addition to numerous new and upgraded packages, this release has the followin
   To import that data after to the upgrade, run
   `sudo -u epgstation epgstation run v1migrate /tmp/epgstation.bak`
 
+- How `gdk-pixbuf` modules are set has changed.
+  - `gdk-pixbuf` has been patched to accept multiple `loaders.cache` files in `$GDK_PIXBUF_MODULE_FILE`, separated by `:`
+  - Setting modules through `wrapGAppsHook`: 
+    `wrapGAppsHook` now automatically includes modules from `gdk-pixbuf` and `librsvg`. This is backwards-compatible for most packages.
+    It no longer looks for a module file in `$GDK_PIXBUF_MODULE_FILE` in the build environment, `gdk-pixbuf` no longer sets it when added to `buildInputs`, and you no longer need to add `gdk-pixbuf` or `librsvg` to `buildInputs` unless the program directly depends on it.
+    To include additional gdk-pixbuf modules, set `extraGdkPixbufModules` with an array of packages.
+  - Manually wrapping `$GDK_PIXBUF_MODULE_FILE`:
+    The `librsvg` loaders.cache still contains all of the loaders from `gdk-pixbuf`. This might change in the future, so it is recommended that you explicitly include both.
+    As this change allows users to extend programs with their own loaders, it is recommended that you `--prefix` rather than `--set` `$GDK_PIXBUF_MODULE_FILE` with your values.
+    For example: `makeWrapper ... --prefix GDK_PIXBUF_MODULE_FILE : "${librsvg}/lib/gdk-pixbuf-2.0/2.10.0/loaders.cache:${gdk-pixbuf}/lib/gdk-pixbuf-2.0/2.10.0/loaders.cache"`
+
 - `switch-to-configuration` (the script that is run when running `nixos-rebuild switch` for example) has been reworked
     * The interface that allows activation scripts to restart units has been streamlined. Restarting and reloading is now done by a single file `/run/nixos/activation-restart-list` that honors `restartIfChanged` and `reloadIfChanged` of the units.
         * Preferring to reload instead of restarting can still be achieved using `/run/nixos/activation-reload-list`.

pkgs/build-support/setup-hooks/wrap-gapps-hook/default.nix

@@ -10,6 +10,7 @@
 , callPackage
 , wrapGAppsHook
 , writeTextFile
+, gdk-pixbuf
 }:
 
 makeSetupHook {
@@ -37,6 +38,9 @@ makeSetupHook {
     makeWrapper
   ];
   substitutions = {
+    standardGdkPixbufModules = lib.makeSearchPathOutput "lib" gdk-pixbuf.cacheFile [ gdk-pixbuf librsvg ];
+    gdkPixbufCacheFile = gdk-pixbuf.cacheFile;
+
     passthru.tests = let
       sample-project = ./tests/sample-project;
 
@@ -64,6 +68,33 @@ makeSetupHook {
         ''
       );
 
+      # Stopgap as no modules have been packaged yet
+      mock-pixbuf-module = stdenv.mkDerivation {
+        name = "mock-gdk-pixbuf-module";
+        outputs = ["out" "lib"];
+        dontUnpack = true;
+        buildPhase = ''
+          mkdir -p $out
+          mkdir -p $lib/$(dirname ${gdk-pixbuf.cacheFile})
+          touch $lib/${gdk-pixbuf.cacheFile}
+        '';
+        dontInstall = true;
+        dontFixup = true;
+      };
+
+      pixbuf-modules = basic.overrideAttrs (old: {extraGdkPixbufModules = [mock-pixbuf-module];});
+
+      pixbuf-modules-check = let
+        tested = pixbuf-modules;
+      in testLib.runTest "pixbuf-modules-check" ''
+        ${expectSomeLineContainingYInFileXToMentionZ "${tested}/bin/foo" "GDK_PIXBUF_MODULE_FILE" "${gdk-pixbuf}/${gdk-pixbuf.cacheFile}"}
+        ${expectSomeLineContainingYInFileXToMentionZ "${tested}/bin/foo" "GDK_PIXBUF_MODULE_FILE" "${librsvg}/${gdk-pixbuf.cacheFile}"}
+        ${expectSomeLineContainingYInFileXToMentionZ "${tested}/bin/foo" "GDK_PIXBUF_MODULE_FILE" "${mock-pixbuf-module}/${gdk-pixbuf.cacheFile}"}
+        ${expectSomeLineContainingYInFileXToMentionZ "${tested}/libexec/bar" "GDK_PIXBUF_MODULE_FILE" "${gdk-pixbuf}/${gdk-pixbuf.cacheFile}"}
+        ${expectSomeLineContainingYInFileXToMentionZ "${tested}/libexec/bar" "GDK_PIXBUF_MODULE_FILE" "${librsvg}/${gdk-pixbuf.cacheFile}"}
+        ${expectSomeLineContainingYInFileXToMentionZ "${tested}/libexec/bar" "GDK_PIXBUF_MODULE_FILE" "${mock-pixbuf-module}/${gdk-pixbuf.cacheFile}"}
+      '';
+
       # Simple derivation containing a gobject-introspection typelib.
       typelib-Mahjong = stdenv.mkDerivation {
         name = "typelib-Mahjong";

pkgs/build-support/setup-hooks/wrap-gapps-hook/wrap-gapps-hook.sh

@@ -10,8 +10,13 @@ find_gio_modules() {
 addEnvHooks "${targetOffset:?}" find_gio_modules
 
 gappsWrapperArgsHook() {
-    if [ -n "$GDK_PIXBUF_MODULE_FILE" ]; then
-        gappsWrapperArgs+=(--set GDK_PIXBUF_MODULE_FILE "$GDK_PIXBUF_MODULE_FILE")
+
+    gappsWrapperArgs+=(--prefix GDK_PIXBUF_MODULE_FILE : "@standardGdkPixbufModules@")
+
+    if [ -n "$extraGdkPixbufModules" ]; then
+        for pkg in $extraGdkPixbufModules; do
+            gappsWrapperArgs+=(--prefix GDK_PIXBUF_MODULE_FILE : "$pkg/@gdkPixbufCacheFile@")
+        done
     fi
 
     if [ -n "$GSETTINGS_SCHEMAS_PATH" ]; then

pkgs/development/libraries/gdk-pixbuf/default.nix

@@ -19,6 +19,7 @@
 , doCheck ? false
 , makeWrapper
 , lib
+, callPackage
 , withIntrospection ? (stdenv.buildPlatform == stdenv.hostPlatform)
 , gobject-introspection
 }:
@@ -42,6 +43,8 @@ stdenv.mkDerivation rec {
   patches = [
     # Move installed tests to a separate output
     ./installed-tests-path.patch
+    # Allow for multiple loaders.cache files specified in $GDK_PIXBUF_MODULE_FILE, delimited by ":"
+    ./multiple-module-files.patch
   ];
 
   # gdk-pixbuf-thumbnailer is not wrapped therefore strictDeps will work
@@ -128,8 +131,6 @@ stdenv.mkDerivation rec {
   # The tests take an excessive amount of time (> 1.5 hours) and memory (> 6 GB).
   inherit doCheck;
 
-  setupHook = ./setup-hook.sh;
-
   separateDebugInfo = stdenv.isLinux;
 
   passthru = {
@@ -140,10 +141,12 @@ stdenv.mkDerivation rec {
 
     tests = {
       installedTests = nixosTests.installed-tests.gdk-pixbuf;
+      multiple-module-files = callPackage ./multiple-module-files-test {};
     };
 
     # gdk_pixbuf_moduledir variable from gdk-pixbuf-2.0.pc
     moduleDir = "lib/gdk-pixbuf-2.0/2.10.0/loaders";
+    cacheFile = "lib/gdk-pixbuf-2.0/2.10.0/loaders.cache";
   };
 
   meta = with lib; {

pkgs/development/libraries/gdk-pixbuf/multiple-module-files-test/default.nix

@@ -0,0 +1,34 @@
+# Simple tests for multiple module files functionality.
+# In the future, would like to test precedence, more than two files, unexpected inputs/fuzzing.
+{ runCommand, gdk-pixbuf, librsvg}:
+runCommand "pixbuf-mmf-test" {} ''
+  mkdir -p $out
+  set +e
+
+  function test {
+    modulepath=$1
+    testfile=$2
+    GDK_PIXBUF_MODULE_FILE=$modulepath ${gdk-pixbuf}/bin/gdk-pixbuf-thumbnailer $testfile $out/$(basename $testfile) > /dev/null
+  }
+
+  test ${gdk-pixbuf}/${gdk-pixbuf.cacheFile} ${./sample.svg}
+  if ! (($?)); then
+    >&2 echo "This shouldn't happen? SVG parsing succeeded even though librsvg wasn't supplied."
+    exit 1
+  fi
+
+  test ${gdk-pixbuf}/${gdk-pixbuf.cacheFile}:${librsvg}/${gdk-pixbuf.cacheFile} ${./sample.svg}
+  if (($?)); then
+    >&2 echo "Test failed."
+    exit 1
+  fi
+
+  test ${librsvg}/${gdk-pixbuf.cacheFile}:${gdk-pixbuf}/${gdk-pixbuf.cacheFile} ${./sample.svg}
+  if (($?)); then
+    >&2 echo "Test failed."
+    exit 1
+  fi
+
+  set -e
+  exit 0
+''

pkgs/development/libraries/gdk-pixbuf/multiple-module-files.patch

@@ -0,0 +1,32 @@
+diff --git a/gdk-pixbuf/gdk-pixbuf-io.c b/gdk-pixbuf/gdk-pixbuf-io.c
+index 1827811..eda0c29 100644
+--- a/gdk-pixbuf/gdk-pixbuf-io.c
++++ b/gdk-pixbuf/gdk-pixbuf-io.c
+@@ -668,13 +668,24 @@ static gboolean
+ gdk_pixbuf_io_init (void)
+ {
+ 	char *module_file;
+-	gboolean ret;
++	gboolean ret = FALSE;
+
+ 	gdk_pixbuf_io_init_builtin ();
+ #ifdef USE_GMODULE
+ 	module_file = gdk_pixbuf_get_module_file ();
++	gchar **files = g_strsplit(module_file, ":", -1);
++
++	int num_files = 0;
++	while(files[num_files] != NULL) num_files++;
++
++	for(int i = num_files - 1; i >= 0; i--){ /* reverse iteration so earlier files have precedence */
++		if(files[i][0] != '\0'){ /* :: yields an empty split string, we should ignore */
++			ret |= gdk_pixbuf_io_init_modules (files[i], NULL);
++		}
++	}
++
++	g_strfreev(files);
+ #endif
+-	ret = gdk_pixbuf_io_init_modules (module_file, NULL);
+ 	g_free (module_file);
+ 	return ret;
+ }
+