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

Use a NixOS module for generating the gdk-pixbuf loaders cache. #42562

Merged
merged 1 commit into from Oct 12, 2018

Conversation

ambrop72
Copy link
Contributor

Fixes issue #33231 and makes it possible to enable Plasma and KDE at the same time.

Previously, this worked like this:

  • The gdk-pixbuf package comes with a cache file covering the modules bundled
    with gdk-pixbuf.
  • The librsvg package comes with a cache covering modules from gdk-pixbuf as
    well as librsvg.
  • plasma5 and xfce modules set the environment variable GDK_PIXBUF_MODULE_FILE
    to the one from librsvg, so that SVG was supported in addition to the
    formats supported by gdk-pixbuf. However if both were enabled a configuration
    conflict would result (despite setting to the same value).

While this sort of worked (ignoring the conflict which perhaps could be hacked
around), it is unscalable and a hack, as there would be a real problem when one
wanted to add a third package that supports additional image formats.

A new NixOS module (gdk-pixbuf) is added with a configuration option
(modulePackages) that other modules use to request specific packages to be
included in the loaders cache. When any package is present in the list, the
module generates a system-wide loaders cache which includes the requested
packages (and always gdk-pixbuf itself), and sets the environment variable
GDK_PIXBUF_MODULE_FILE to point to the generated cache file.

The plasma5 and xfce modules are updated to add librsvg to modulePackages
instead of setting GDK_PIXBUF_MODULE_FILE.

Note that many packages create wrappers that set GDK_PIXBUF_MODULE_FILE,
some directly to the one from librsvg. Therefore this change does not
change the existing hack in the librsvg package which ensures that
file is generated. This change aims only to solve the conflict in the
global environent variable configuration.

Motivation for this change

Could not enable XFCE and Plasma at the same time. I confirm that with this change, I can enable both and then use Thunar from a Plasma session. I also confirm that the GDK_PIXBUF_MODULE_FILE enavironment variable is set correctly to the generated cache file and that the latter appears to be correct, listing image formats from both xdg-pixbuf modules and SVG from librsvg.

I suggest that this also be backported to 18.03 because the inability to use XFCE apps in KDE is pretty annoying and a regression from 17.09. Note that XFCE apps do not work in KDE correctly when the XFCE desktop is not enabled.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • 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/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Fits CONTRIBUTING.md.

@xeji
Copy link
Contributor

xeji commented Jun 25, 2018

cc @jtojnar

Copy link
Contributor

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely nicer than the current solution.

In the future, we might want to consider something like #42176 (comment), though that would require patching gdk-pixbuf to support multiple files in the environment variable.

nixos/modules/services/x11/gdk-pixbuf.nix Outdated Show resolved Hide resolved
nixos/modules/services/x11/gdk-pixbuf.nix Outdated Show resolved Hide resolved
Fixes issue NixOS#33231 and makes it possible to enable Plasma and KDE at the same time.

Previously, this worked like this:
- The gdk-pixbuf package comes with a cache file covering the modules bundled
  with gdk-pixbuf.
- The librsvg package comes with a cache covering modules from gdk-pixbuf as
  well as librsvg.
- plasma5 and xfce modules set the environment variable GDK_PIXBUF_MODULE_FILE
  to the one from librsvg, so that SVG was supported in addition to the
  formats supported by gdk-pixbuf. However if both were enabled a configuration
  conflict would result (despite setting to the same value).

While this sort of worked (ignoring the conflict which perhaps could be hacked
around), it is unscalable and a hack, as there would be a real problem when one
wanted to add a third package that supports additional image formats.

A new NixOS module (gdk-pixbuf) is added with a configuration option
(modulePackages) that other modules use to request specific packages to be
included in the loaders cache. When any package is present in the list, the
module generates a system-wide loaders cache which includes the requested
packages (and always gdk-pixbuf itself), and sets the environment variable
GDK_PIXBUF_MODULE_FILE to point to the generated cache file.

The plasma5 and xfce modules are updated to add librsvg to modulePackages
instead of setting GDK_PIXBUF_MODULE_FILE.

Note that many packages create wrappers that set GDK_PIXBUF_MODULE_FILE,
some directly to the one from librsvg. Therefore this change does not
change the existing hack in the librsvg package which ensures that
file is generated. This change aims only to solve the conflict in the
global environent variable configuration.
@ambrop72
Copy link
Contributor Author

I have updated this according to comments from @jtojnar and tested on nixos-unstable. Note that if this is backported to 18.03, moduleDir will need to be added to gdk_pixbuf or hardcoded in the service like it used to be.

@ambrop72
Copy link
Contributor Author

Why is this not being merged? I have been using this for months and see no problems, and it solves the very real problem of not being able to use KDE apps from Xfce.

@jtojnar jtojnar merged commit a112f16 into NixOS:master Oct 12, 2018
@jtojnar jtojnar mentioned this pull request Oct 17, 2018
6 tasks
@s9gf4ult
Copy link
Contributor

Have the same problem. How can I use this solution under

nixos-version 
18.09.1420.5d4a1a3897e (Jellyfish)

?

@s9gf4ult
Copy link
Contributor

@ambrop72 ?

@ambrop72
Copy link
Contributor Author

@s9gf4ult You need to checkout the nixos-18.09 branch from the nixpkgs-channels repository, cherry-pick the patch (which was merged to master already) and then do nixos-rebuild using that nixpkgs repository. Sorry I can't provide specific commands right now, if you ask in the #nixos channel on IRC someone should be able to help you out.

@nixos-discourse
Copy link

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/curious-gtk-issue-with-anydesk-svg-pixbuf-loader/1736/2

@nixos-discourse
Copy link

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/curious-gtk-issue-with-anydesk-svg-pixbuf-loader/1736/3

@jtojnar
Copy link
Contributor

jtojnar commented Apr 1, 2019

We might want to filter variables like these from environment.variables unless some option like environment.enableImpureClusterfuck is true.

@ambrop72
Copy link
Contributor Author

ambrop72 commented Apr 2, 2019

Interesting. This must be a general problem, surely we have other cases of environment variables leading to architecture-specific libraries (one just needs to look at environment variables in a desktop session, anything that contains PLUGIN is especially suspect), but I guess in many cases architecture mismatch is handled gracefully. In this case GDK really has no way to know the entire cache file is useless, it would only get an error when it tries to load a plugin.

We might want to filter variables like these from environment.variables unless some option like environment.enableImpureClusterfuck is true.

I don't get your point about filtering, if we don't want it, we don't set it (but reintroduce the bugs that it fixed).

@jtojnar
Copy link
Contributor

jtojnar commented Apr 2, 2019

The main issue with the variables is that they are problematic even on the same architecture. QT is notorious for this issue.

There is simply no guarantee that the variable will not crash your system. Projects like GStreamer solve it by maintaining a stable ABI and gdk_pixbuf did the same but it still broke when a bug was fixed in 2.38.

I do not think anybody ever considered multiarch, I certainly did not. Perhaps we could patch gdk_pixbuf to parse the elf files and skip them when they are different architecture but since this is such a niche issue I recommend just wrapping the 32 bit apps with their own loader variables.

I don't get your point about filtering, if we don't want it, we don't set it (but reintroduce the bugs that it fixed).

Yes that is what I meant.

@ambrop72
Copy link
Contributor Author

ambrop72 commented Apr 2, 2019

Actually what I would do is to patch only 32-bit builds of the gdk_pixbuf library to use GDK_PIXBUF_MODULE_FILE32, and change the new nixos module introduced in this PR to set that instead on 32-bit NixOS. Without any wrapping (I hate wrapping).

@jtojnar
Copy link
Contributor

jtojnar commented Apr 2, 2019

That is another possibility but it might be confusing. Wrapping is also the only way to prevent impurities like #54278

@ambrop72
Copy link
Contributor Author

ambrop72 commented Apr 2, 2019

But with wrapping there are other issues:

  • Every application using gdk_pixbuf (including indirectly) should generally be wrapped, and inevitably some will not be.
  • Wrapping to set/change environment variables results in those variables also being set/changed in child processes, which is usually undesirable. This could particularly be an issue for programs that are or include a terminal emulator.
  • For variables that contain a list, I think the current wrappers do not check if the variable already has the desired elements, so with additional child process executions the variable could get longer and longer with many instances of the same entry.

@ambrop72
Copy link
Contributor Author

Lots of apps are already wrapped to set GDK_PIXBUF_MODULE_FILE to the one from librsvg. And XFCE and Plasma NixOS modules want to ensure that all GTK apps are able to load SVG icons (presumably so they can load desktop-provided SVG icons to look more integrated).

Maybe we should somehow include the SVG loader inside the gdk_pixbuf package (or a wrapped package that everything uses) so that anything that uses gdk_pixbuf automatically gets SVG support and this mess with GDK_PIXBUF_MODULE_FILE no longer exists?

@cwyc cwyc mentioned this pull request Jan 2, 2022
13 tasks
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

6 participants