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 (`wrapGAppsHook` is preferred), 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/from_md/release-notes/rl-2211.section.xml

@@ -31,6 +31,56 @@
           <literal>stdenv.buildPlatform.canExecute stdenv.hostPlatform</literal>.
         </para>
       </listitem>
+      <listitem>
+        <para>
+          How <literal>gdk-pixbuf</literal> modules are set has changed.
+        </para>
+        <itemizedlist spacing="compact">
+          <listitem>
+            <para>
+              <literal>gdk-pixbuf</literal> has been patched to accept
+              multiple <literal>loaders.cache</literal> files in
+              <literal>$GDK_PIXBUF_MODULE_FILE</literal>, separated by
+              <literal>:</literal>
+            </para>
+          </listitem>
+          <listitem>
+            <para>
+              Setting modules through <literal>wrapGAppsHook</literal>
+              (preferred method): <literal>wrapGAppsHook</literal> now
+              automatically includes modules from
+              <literal>gdk-pixbuf</literal> and
+              <literal>librsvg</literal>. This is backwards-compatible
+              for most packages. It no longer looks for a module file in
+              <literal>$GDK_PIXBUF_MODULE_FILE</literal> in the build
+              environment, <literal>gdk-pixbuf</literal> no longer sets
+              it when added to <literal>buildInputs</literal>, and you
+              no longer need to add <literal>gdk-pixbuf</literal> or
+              <literal>librsvg</literal> to
+              <literal>buildInputs</literal> unless the program directly
+              depends on it. To include additional gdk-pixbuf modules,
+              set <literal>extraGdkPixbufModules</literal> with an array
+              of packages.
+            </para>
+          </listitem>
+          <listitem>
+            <para>
+              Manually wrapping
+              <literal>$GDK_PIXBUF_MODULE_FILE</literal>: The
+              <literal>librsvg</literal> loaders.cache still contains
+              all of the loaders from <literal>gdk-pixbuf</literal>.
+              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 <literal>--prefix</literal> rather
+              than <literal>--set</literal>
+              <literal>$GDK_PIXBUF_MODULE_FILE</literal> with your
+              values. For example:
+              <literal>makeWrapper ... --prefix GDK_PIXBUF_MODULE_FILE : &quot;${librsvg}/lib/gdk-pixbuf-2.0/2.10.0/loaders.cache:${gdk-pixbuf}/lib/gdk-pixbuf-2.0/2.10.0/loaders.cache&quot;</literal>
+            </para>
+          </listitem>
+        </itemizedlist>
+      </listitem>
       <listitem>
         <para>
           PHP now defaults to PHP 8.1, updated from 8.0.

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

@@ -17,6 +17,17 @@ In addition to numerous new and upgraded packages, this release has the followin
   built for `stdenv.hostPlatform` (i.e. produced by `stdenv.cc`) by evaluating
   `stdenv.buildPlatform.canExecute stdenv.hostPlatform`.
 
+- 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` (preferred method):
+    `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"`
+
 - PHP now defaults to PHP 8.1, updated from 8.0.
 
 <!-- To avoid merge conflicts, consider adding your item at an arbitrary place in the list instead. -->

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

@@ -10,6 +10,7 @@
 , callPackage
 , wrapGAppsHook
 , writeTextFile
+, gdk-pixbuf
 }:
 
 makeSetupHook {
@@ -37,8 +38,12 @@ 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;
+      mock-pixbuf-module = callPackage ./tests/sample-module.nix {};
 
       testLib = callPackage ./tests/lib.nix { };
       inherit (testLib) expectSomeLineContainingYInFileXToMentionZ;
@@ -64,6 +69,19 @@ makeSetupHook {
         ''
       );
 
+      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/tests/sample-module.nix

@@ -0,0 +1,13 @@
+{stdenv, gdk-pixbuf}:
+stdenv.mkDerivation {
+  name = "mock-gdk-pixbuf-module";
+  outputs = ["out" "bin"];
+  dontUnpack = true;
+  buildPhase = ''
+    mkdir -p $bin
+    mkdir -p $out/$(dirname ${gdk-pixbuf.cacheFile})
+    touch $out/${gdk-pixbuf.cacheFile}
+  '';
+  dontInstall = true;
+  dontFixup = true;
+}

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,57 @@
+# Simple tests for multiple module files functionality.
+# gdk-pixbuf-thumbnailer is used as a simple, command-line consumer of loaders.
+# In the future, would like to test unexpected inputs/fuzzing.
+{ runCommand, gdk-pixbuf, librsvg, callPackage, imagemagick}:
+let
+  thumbnailer = "${gdk-pixbuf}/bin/gdk-pixbuf-thumbnailer";
+
+  pixbufloader = gdk-pixbuf + "/" + gdk-pixbuf.cacheFile;
+  svgloader = librsvg + "/" + gdk-pixbuf.cacheFile;
+  redloader = (callPackage ./test-loader.nix {outputColor="#FF0000";}) + "/" + gdk-pixbuf.cacheFile;
+  blueloader = (callPackage ./test-loader.nix {outputColor="#0000FF";}) + "/" + gdk-pixbuf.cacheFile;
+
+  svgsample = ./sample.svg;
+  gifsample = ./sample.gif;
+in runCommand "pixbuf-mmf-test" {} ''
+  mkdir -p $out
+  set +e
+
+  function svgtest {
+    modulepath=$1
+    GDK_PIXBUF_MODULE_FILE=$modulepath "${thumbnailer}" "${svgsample}" "$out/thumbnail.png"
+  }
+
+  function colortest {
+    modulepath=$1
+    checkcolor=$2
+    GDK_PIXBUF_MODULE_FILE=$modulepath "${thumbnailer}" "${gifsample}" "$out/thumbnail.png"
+    "${imagemagick}/bin/convert" "$out/thumbnail.png" -resize 1x1 "$out/summarized.txt"
+    grep "$checkcolor" "$out/summarized.txt"
+  }
+
+  # sanity check
+  if svgtest "${pixbufloader}"; then
+    >&2 echo "Test failed. SVG parsing succeeded even though librsvg loader wasn't provided. This shouldn't happen?"
+    exit 1
+  fi
+
+  # basic check for multiple file loading
+  if ! svgtest "${pixbufloader}:${svgloader}"; then
+    >&2 echo "Test failed. Second module file couldn't be loaded."
+    exit 2
+  fi
+
+  # check that priority goes to earlier entries in the list
+  if ! colortest "${redloader}:${pixbufloader}:${blueloader}" "#FF0000"; then
+    if colortest "${redloader}:${pixbufloader}:${blueloader}" "#0000FF"; then
+      >&2 echo "Test failed. Path order priority is incorrect."
+      exit 3
+    else
+      >&2 echo "Test failed. Unknown error with test loader code."
+      exit 4
+    fi
+  fi
+
+  echo "Tests succeeded for gdk-pixbuf multiple module files patch."
+  exit 0
+''

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

@@ -0,0 +1,18 @@
+{ stdenv
+, gdk-pixbuf
+, pkg-config
+, meson
+, ninja
+, outputColor ? "#ffffff"
+}:
+stdenv.mkDerivation {
+  name = "gdk-pixbuf-test-loader";
+  src = ./test-loader;
+  buildInputs = [gdk-pixbuf];
+  nativeBuildInputs = [pkg-config meson ninja];
+  postInstall = ''
+    mkdir -p $(dirname $out/${gdk-pixbuf.cacheFile})
+    ${gdk-pixbuf.dev}/bin/gdk-pixbuf-query-loaders $out/lib/libtest_loader.so > $out/${gdk-pixbuf.cacheFile}
+  '';
+  mesonFlags = ["-DoutputColor=${outputColor}"];
+}