gdk-pixbuf: multiple module files

[cwyc]

Motivation for this change

The gdk-pixbuf library can search for "modules" or "loaders" to extend its ability to load image formats. The modules available on a system are gathered and listed in a loaders.cache file, which gdk-pixbuf looks for in $GDK_PIXBUF_MODULE_FILE.

Gdk-pixbuf recognizes only one loaders.cache file. This isn't a problem on other distributions, which call gdk-pixbuf-query-loaders after installation to update the systemwide loaders.cache that contains every installed loader.

But nix can't do that, so 1) a program can't be packaged with loaders from multiple packages, and 2) if a program is packaged with a loader, the user can't extend it with other loaders in their environment.

We see the first issue with pretty much all GTK apps, which need loaders from librsvg as well as gdk-pixbuf. Our solution is to manually merge all of gdk-pixbuf's loaders.cache entries in to librsvg's, and use only the latter. But that doesn't support any other package providing loaders.

Things done

A better way could be for gdk-pixbuf to accept multiple loaders.cache. (see #42562 (review) )

First, module loading code in gdk-pixbuf is patched to read multiple paths from $GDK_PIXBUF_MODULE_FILE, separated by a colon.

Second, wrapGappsHook, which sets $GDK_PIXBUF_MODULE_FILE for most executables using gdk-pixbuf, is patched so that it appends to it rather than replaces it. This will let user-set loaders in services.xserver.gdk-pixbuf.modulePackages be recognized.

Third, the setup hook by which gdk-pixbuf includes its own loaders is updated to take advantage of this feature. (see #102189 (comment) )

Currently, packages include the hybrid-librsvg-gdk-pixbuf-loaders.cache either by setting $GDK_PIXBUF_MODULE_FILE for wrapGappsHook to pick up, or by manually supplying it to makeWrapper.
Even though it's possible to supply librsvg and gdk-pixbuf separately, I've left librsvg as is for compatibility's sake.
And while this patches the wrapGappsHook packages to append rather than replace the environment variable, the manually-wrapped packages are unchanged.

Warning: this triggers a lot of compilation.

Documentation hasn't been updated.

Tagging @jtojnar

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• TODO Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 22.05 Release Notes (or backporting 21.11 Release notes)
  • TODO (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
  • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
• Fits CONTRIBUTING.md.

[jtojnar]

Sorry for the late reply, this got lost in my queue. I think this is reasonable compromise between performance and convenience for packaging.

Primary concerns would be that gdk-pixbuf does not normally allow this so I am not sure how well does gdk_pixbuf_io_init_modules function handle being called multiple times, potentially with overlapping cache files.

Some basic tests along the line of

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

Line 40 in b77494b

passthru.tests = let

would go a long way in making sure this does not bring any unfortunate breakage, without having to rebuild the world.

[jtojnar]

I wonder if --prefix might be safer here. E.g. in case user has GDK_PIXBUF_MODULE_FILE in their environment containing ABI incompatible loaders (assuming the cache format is stable, first registered module for given file takes precedence, and modules are loaded lazily). But it is not clear to me what handles matching the image to a module, so 🤷‍♀️

[cwyc]

I share your concern about the robustness of the patch. With some tests, hopefully we'll have a better idea of its vulnerabilities.
Though it seems to me like the code is somewhat resilient to loading multiple files. The module loading procedure loads a number of builtin modules before reading loaders.cache. And in gdk_pixbuf_io_init_modules, it expects modules to possibly already exist in file_formats prior to invocation.
Of course, upstream still assumes a single loaders.cache, so the internal interfaces can change at any time.

---

About --suffix, I think (?) later entries in $GDK_PIXBUF_MODULE_FILE have precedence.
The loop calls gdk_pixbuf_io_init_modules for each file in order, which then prepends loaders from the file to file_formats. So the order in file_formats is reversed from that of $GDK_PIXBUF_MODULE_FILE.
_gdk_pixbuf_get_module then selects which module to load by iterating through file_formats in order.
If it's built with GDK_PIXBUF_USE_GIO_MIME, it selects the first module in file_formats with a matching MIME type.
If it's built without MIME support, then it searches the entire list for the module with the best "score". Comparison is done with a > rather than a >=, so if multiple modules provide the highest score, the earliest encountered will remain the selected maximum.

In any case, the opposite precedence behavior seems to be the standard, and I agree modules included with the package should have precedence. So I'll change it to --prefix, and edit the patch to process the path in reverse order.

---

I wonder if packaging procedures will have to change.
I can think of these behaviours that packages rely on:

• when gdk-pixbuf or librsvg is added to buildInputs, its setupHook loads its loaders.cache to the build environment $GDK_PIXBUF_MODULE_FILE
• when both are added to buildInputs, their setupHooks work out the conflict, and the functionality of both appears
• wrapGAppsHook takes $GDK_PIXBUF_MODULE_FILE in the build environment as what it should include with the executable.
  • There may be an expectation that including the package in buildInputs is all that's needed to gain loader support, disregarding the role of wrapGAppsHook
• the librsvg loaders.cache contains the standard gdk-pixbuf loaders as well.
  • Though apps typically include both as dependencies, when manually constructing a wrapper, only librsvg's loaders.cache is set.
• obviously, the library should work with only one loaders.cache in $GDK_PIXBUF_MODULE_FILE

Seeing how many gtk programs there are, I imagine they behaviours have to stay. But it doesn't really indicate how we should design the interface for packaging more-than-one loaders.cache.

Should we make all loader packages have setupHooks that append to $GDK_PIXBUF_MODULE_FILE, just like shared libraries do to $LD_LOADERS_PATH? But unlike shared libraries, modules often overlap: do we then need a way to specify their order?

Though the vast majority of programs just need the standard librsvg/gdk-pixbuf to render GTK icons. Perhaps wrapGAppsHook can include them automatically, packages not directly calling gdk-pixbuf no longer need to explicitly include them in buildInputs (but it doesn't hurt), packages needing other loaders can manually wrap them (maintaining the librsvg-containing-gdk-pixbuf behaviour), and setupHooks can be eliminated. In any case, users will benefit from the ability to specify their own loaders.

[jtojnar]

• There may be an expectation that including the package in buildInputs is all that's needed to gain loader support, disregarding the role of wrapGAppsHook

It is ¿well? documented that wrappers are always required.

• Though apps typically include both as dependencies, when manually constructing a wrapper, only librsvg's loaders.cache is set.

Again, I would not worry about that. It is documented what wrapGAppsHook does and people are strongly encourage to use that. If they want to roll their own, they can, but then they are on their own.

obviously, the library should work with only one loaders.cache in $GDK_PIXBUF_MODULE_FILE

I think wrapGAppsHook should have gdk-pixbuf in deps, like it has librsvg.

(Alternately, we might compile the gdk-pixbuf’s loaders.cache path into the gdk-pixbuf library – not sure if that is a good idea since it would not be possible to disable them then.)

Should we make all loader packages have setupHooks that append to $GDK_PIXBUF_MODULE_FILE, just like shared libraries do to $LD_LOADERS_PATH? But unlike shared libraries, modules often overlap: do we then need a way to specify their order?

Yeah, with the ordering you mention, that will be unpredictable, especially since loaders can get into inputs via propagatedBuildInputs of dependencies. Though I wonder how common overlaps actually are, most of the modules focus on a single format/family. And when there are some overlaps, we do not package those modules anyway.

Though the vast majority of programs just need the standard librsvg/gdk-pixbuf to render GTK icons. Perhaps wrapGAppsHook can include them automatically, packages not directly calling gdk-pixbuf no longer need to explicitly include them in buildInputs (but it doesn't hurt), packages needing other loaders can manually wrap them (maintaining the librsvg-containing-gdk-pixbuf behaviour), and setupHooks can be eliminated.

Actually, that sounds reasonable too. For convenience, wrapGAppsHook could pick up packages from extraGdkPixbufModules attribute and completely ignore the inputs. The issue would occur when one would want to use the loaders outside the context of a wrapper (e.g. in Nix shell) so we would need to expose the variable to avoid the need for hacks like https://discourse.nixos.org/t/python-qt-woes/11808/2?u=jtojnar.

[cwyc]

Sorry for the delay. I've made some corrections, and added a simple test for gdk-pixbuf.

[cwyc]

Still need to modify services.xserver.gdk-pixbuf.modulePackages module, as it no longer needs to generate an amalgamated loaders.cache, and add tests for it.

[jtojnar]

Maybe add []{#setup-hook-gdk-pixbuf} so that the deleted setup hook section points here as well.

[jtojnar]

I think it is desirable to use fake modules here even after they have been packaged to allow testing wrapGAppsHook changes with minimal changes.

[jtojnar]

To clean this up, it might be nice to move this to the sample-project and then use installFlags = [ "loaders.cache" ];.

[cwyc]

I think they should be separate, since the sample-project is wrapped by the setup hook and the mock loader is an input for the wrapper.

[jtojnar]

Sorry, I meant move it to the Makefile in sample-project, then you will be able to use src & installFlags.

[jtojnar]

One use case that still is ¿not covered? is the use of loaders during build/checkPhase. Previously findGdkPixbufLoaders had us covered but now we need to set the environment variable manually. But maybe it is okay? Edit: Actually, that makes using nix-shell inconvenient compared to just nix-shell -I nixpkgs=$PWD -p gdk-pixbuf -p librsvg --run 'gdk-pixbuf-thumbnailer test.svg test.png'. So perhaps we might actually retain this setup hook just doing addToSearchPath.

[cwyc]

If we set GDK_PIXBUF_MODULE_FILE for the user, then should we also return to wrapGAppsHook drawing from the build-time environment variable, so that what works in the build environment also works in the final package?

[cwyc]

Also, would not being able to control order be an issue?

[jtojnar]

I can imagine some use cases for wanting specific order but they probably be rare so I would not bother with that. People can always juggle with the variable contents if they need it.

Though I am split on having a separate attribute vs. using buildInputs. On one hand, separate attribute offers a finer control and the behaviour is more clear by the fact of being explicit. On the other hand all other wrappers just pick stuff up from inputs so not doing that for loaders might be confusing (largely mitigated by docs?) and deciding where to put each dependency might be confusing as well. 🤷‍♀️

[lovesegfault]

@cwyc Could you resolve the conflicts?

[jtojnar]

Sorry I have not managed to review this early enough for this to make it into 2022.11. I have created a workaround for now #197029