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

gdk-pixbuf: Use a different GDK_PIXBUF_MODULE_FILE environment variable on 32-bit and 64-bit systems. #60254

Draft
wants to merge 1 commit into
base: staging
Choose a base branch
from

Conversation

ambrop72
Copy link
Contributor

Pull request #42562 caused a regression that running a 32-bit program that uses
gdk-pixbuf would result in a crash because it would try to load a 64-bit
library and fail. This change fixes this issue by patching gdk-pixbuf and many
places where this variable is defined to use different variable names on
32-bit and 64-bit systems.

Motivation for this change

Fix regression running 32-bit programs.

This only intends to fix the 32-bit program issue. The situation is still a mess. The way I understand the situation is:

  • Desktops want to set GDK_PIXBUF_MODULE_FILE such that all GTK apps will be able to load SVG icons. I presume this is because the desktop comes with SVG icons and the apps should be able to use the desktop-provided icons to integrate visually.
  • Some apps need to specifically support SVG icons, probably because they ship with them. Those set GDK_PIXBUF_MODULE_FILE in the wrapper (the value comes from the gdk-pixbuf setup hook). In these cases the wrapper sets the variable to point to the cache file in librsvg.
  • Some apps set GDK_PIXBUF_MODULE_FILE in the wrapper without depending on librsvg, which results in the wraper setting the variable to the cache file from gdk-pixbuf. This wrapping might be to ensure the app works even if GDK_PIXBUF_MODULE_FILE is set incorrectly (e.g. when NixOS is updated, if running outside NixOS) (if it is not set gdk-pixbuf will use its default cache file). Note that these apps will never support SVG icons even if the desktop sets the variable, because the wrapper overrides it! That's why I added a librsvg dependency to some apps, but I didn't check this for the many apps that get wrapped indirectly via wrapGAppsHook.

My suggestion going forward would be to try to ensure that (almost?) all GTK apps are wrapped to set GDK_PIXBUF_MODULE_FILE to the one from librsvg (yes, it results in a larger closure in some cases), and then remove the code in desktops to set the environment variable together with the gdk-pixbuf NixOS module.

Things done

I have testes this on 19.03 branch and it seems to work as intended. I have checked that the correct variable is set by the gdk_pixbuf NixOS module and that the apps which set it in the wrapper set the correct variable. I have observed no breakage on my desktop system (KDE-based). I have tried running a 32-bit application (pavucontrol) and it does start and run (though there are errors loading 64-bit GVFS plugins).

  • 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 nix-review --run "nix-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

…le on 32-bit and 64-bit systems.

Pull request NixOS#42562 caused a regression that running a 32-bit program that uses
gdk-pixbuf would result in a crash because it would try to load a 64-bit
library and fail. This change fixes this issue by patching gdk-pixbuf and many
places where this variable is defined to use different variable names on
32-bit and 64-bit systems.
@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation 8.has: module (update) This PR changes an existing module in `nixos/` labels Apr 26, 2019
@ambrop72
Copy link
Contributor Author

Note that in the referenced pull request #42562 there was a suggestion from @jtojnar that this could be solved by making sure librsvg included in wrappers and removing the variable setting from desktops. As already stated I agree with this approach, however the 32-bit program issue would still exist for any child processes of a wrapped program (e.g. if a terminal is a GTK app that is wrapped in this way), hence this pull request.

@jtojnar
Copy link
Member

jtojnar commented May 4, 2019

however the 32-bit program issue would still exist for any child processes of a wrapped program (e.g. if a terminal is a GTK app that is wrapped in this way), hence this pull request.

Would not the child process be wrapped too with platform-correct modules?

@ambrop72
Copy link
Contributor Author

ambrop72 commented May 4, 2019

however the 32-bit program issue would still exist for any child processes of a wrapped program (e.g. if a terminal is a GTK app that is wrapped in this way), hence this pull request.

Would not the child process be wrapped too with platform-correct modules?

Certainly not all apps that use gdk-pixbuf are wrapped to set the variable. Until someone goes chasing all apps that are not and wrapping them, the issue still exists.

Anyway I am currently working on an idea to solve the problem of children inheriting variables set by wrappers, and it seems like there is a way with only minor modifications of programs. So I think we should just apply this as an immediate fix until we have a proper approach to such problems.

@stale
Copy link

stale bot commented Jun 2, 2020

Thank you for your contributions.
This has been automatically marked as stale because it has had no activity for 180 days.
If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.
Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the
    related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse. 3. Ask on the #nixos channel on
    irc.freenode.net.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 2, 2020
@Ericson2314
Copy link
Member

IMO we should do something like cc-wrapper's "suffix salt" to support multiple platforms more thoroughly than just 64 vs 32 CC @matthewbauer

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Sep 17, 2020
@ryantm ryantm added the 2.status: merge conflict This PR has merge conflicts with the target branch label Oct 3, 2020
@jtojnar jtojnar mentioned this pull request Nov 2, 2020
10 tasks
@stale
Copy link

stale bot commented Jun 9, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 9, 2021
@wegank wegank marked this pull request as draft March 20, 2024 15:03
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 501-1000 10.rebuild-darwin: 501+ 10.rebuild-linux: 501+ 10.rebuild-linux: 2501-5000
Projects
Status: Needs review
Development

Successfully merging this pull request may close these issues.

5 participants