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

ssb-patchwork: fix GTK file selection dialog #83410

Closed
wants to merge 1 commit into from

Conversation

@ehmry
Copy link
Member

@ehmry ehmry commented Mar 26, 2020

Motivation for this change

Fix a bug with file selection dialogs.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

ping @asymmetric

@asymmetric
Copy link
Contributor

@asymmetric asymmetric commented Mar 26, 2020

Works like a charm! ❤️

symlinkJoin {
inherit name;
paths = [ binary ];
in stdenvNoCC.mkDerivation {

This comment has been minimized.

@worldofpeace

worldofpeace Mar 26, 2020
Member

I don't really get why we have to use the NoCC version here? It has little benefit.

nativeBuildInputs = [ wrapGAppsHook ];

buildCommand = ''
makeWrapper "${binary}/bin/ssb-patchwork" "$out/bin/ssb-patchwork" \
--prefix XDG_DATA_DIRS : "${gsettings-desktop-schemas}/share/gsettings-schemas/${gsettings-desktop-schemas.name}" \
--prefix XDG_DATA_DIRS : "${gtk3}/share/gsettings-schemas/${gtk3.name}" \
Comment on lines +36 to +41

This comment has been minimized.

@worldofpeace

worldofpeace Mar 26, 2020
Member

this is an improper use of wrapGAppsHook.
Please read https://nixos.org/nixpkgs/manual/#ssec-gnome-hooks, it exists so you don't have to do this.

It also should automatically wrap everything at $out/bin. If you make a symlink here for ${binary}/bin/ssb-patchwork $out/bin/ssb-patchwork you shouldn't have to do anything.

This comment has been minimized.

@ehmry

ehmry Mar 27, 2020
Author Member

I would prefer an automatic solution but it doesn't work here.

This comment has been minimized.

@worldofpeace

worldofpeace Mar 27, 2020
Member

Could you explain why? If you can't you can just use the makeWrapper with the arguments wrapGAppsHook collects "${gappsWrapperArgs[@]}".

@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Mar 26, 2020

We could probably backport this bugfix to 20.03

@flokli
Copy link
Contributor

@flokli flokli commented Mar 27, 2020

This uses a buildFHSUserEnv under the hood - I'd expect things to just work out of the box inside one ot these, so wrapGappsHook or other custom wrapping shouldn't be necessary at all.

@jtojnar, @tilpner, could you take a look?

@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Mar 27, 2020

This uses a buildFHSUserEnv under the hood - I'd expect things to just work out of the box inside one ot these, so wrapGappsHook or other custom wrapping shouldn't be necessary at all.

@jtojnar, @tilpner, could you take a look?

Gsettings-schemas are relocated in share/gsettings-schemas/${name}/glib-2.0, not the fhs location. I would obviously be very familiar with this 😕

@tilpner
Copy link
Member

@tilpner tilpner commented Mar 28, 2020

This uses a buildFHSUserEnv under the hood - I'd expect things to just work out of the box inside one ot these, so wrapGappsHook or other custom wrapping shouldn't be necessary at all.

@jtojnar, @tilpner, could you take a look?

I have little knowledge about how gsettings is supposed to work, and what this gschema.compiled is.
Here's how you can play with the FHS env interactively:

$ APPIMAGE_DEBUG_EXEC=bash /nix/store/md7rlyjkkzl84pimmwmh3qs986czs1rf-Patchwork-3.17.6/bin/ssb-patchwork
bash-4.4$ find -L /usr/share/gsettings-schemas
/usr/share/gsettings-schemas
/usr/share/gsettings-schemas/gtk+3-3.24.14
/usr/share/gsettings-schemas/gtk+3-3.24.14/glib-2.0
/usr/share/gsettings-schemas/gtk+3-3.24.14/glib-2.0/schemas
/usr/share/gsettings-schemas/gtk+3-3.24.14/glib-2.0/schemas/org.gtk.Settings.Debug.gschema.xml
/usr/share/gsettings-schemas/gtk+3-3.24.14/glib-2.0/schemas/org.gtk.Settings.EmojiChooser.gschema.xml
/usr/share/gsettings-schemas/gtk+3-3.24.14/glib-2.0/schemas/org.gtk.Demo.gschema.xml
/usr/share/gsettings-schemas/gtk+3-3.24.14/glib-2.0/schemas/gschemas.compiled
/usr/share/gsettings-schemas/gtk+3-3.24.14/glib-2.0/schemas/org.gtk.Settings.FileChooser.gschema.xml
/usr/share/gsettings-schemas/gtk+3-3.24.14/glib-2.0/schemas/org.gtk.Settings.ColorChooser.gschema.xml
bash-4.4$ echo $XDG_DATA_DIRS
/home/tilpner/.nix-profile/share:/etc/profiles/per-user/tilpner/share:/nix/var/nix/profiles/default/share:/run/current-system/sw/share

It seems like a version of gschemas.compiled is installed, but buildFHSUserEnv might not set XDG_DATA_DIRS at all. If it's supposed to, you can try adding

profile = ''
  export XDG_DATA_DIRS=/usr/share/...
'';

to

wrapAppImage = args@{ name, src, extraPkgs, ... }: buildFHSUserEnv (defaultFhsEnvArgs // {
inherit name;
targetPkgs = pkgs: [ appimage-exec ]
++ defaultFhsEnvArgs.targetPkgs pkgs ++ extraPkgs pkgs;
runScript = "appimage-exec.sh -w ${src}";
} // (removeAttrs args (builtins.attrNames (builtins.functionArgs wrapAppImage))));

@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Mar 28, 2020

This uses a buildFHSUserEnv under the hood - I'd expect things to just work out of the box inside one ot these, so wrapGappsHook or other custom wrapping shouldn't be necessary at all.
@jtojnar, @tilpner, could you take a look?

I have little knowledge about how gsettings is supposed to work, and what this gschema.compiled is.
Here's how you can play with the FHS env interactively:

$ APPIMAGE_DEBUG_EXEC=bash /nix/store/md7rlyjkkzl84pimmwmh3qs986czs1rf-Patchwork-3.17.6/bin/ssb-patchwork
bash-4.4$ find -L /usr/share/gsettings-schemas
/usr/share/gsettings-schemas
/usr/share/gsettings-schemas/gtk+3-3.24.14
/usr/share/gsettings-schemas/gtk+3-3.24.14/glib-2.0
/usr/share/gsettings-schemas/gtk+3-3.24.14/glib-2.0/schemas
/usr/share/gsettings-schemas/gtk+3-3.24.14/glib-2.0/schemas/org.gtk.Settings.Debug.gschema.xml
/usr/share/gsettings-schemas/gtk+3-3.24.14/glib-2.0/schemas/org.gtk.Settings.EmojiChooser.gschema.xml
/usr/share/gsettings-schemas/gtk+3-3.24.14/glib-2.0/schemas/org.gtk.Demo.gschema.xml
/usr/share/gsettings-schemas/gtk+3-3.24.14/glib-2.0/schemas/gschemas.compiled
/usr/share/gsettings-schemas/gtk+3-3.24.14/glib-2.0/schemas/org.gtk.Settings.FileChooser.gschema.xml
/usr/share/gsettings-schemas/gtk+3-3.24.14/glib-2.0/schemas/org.gtk.Settings.ColorChooser.gschema.xml
bash-4.4$ echo $XDG_DATA_DIRS
/home/tilpner/.nix-profile/share:/etc/profiles/per-user/tilpner/share:/nix/var/nix/profiles/default/share:/run/current-system/sw/share

It seems like a version of gschemas.compiled is installed, but buildFHSUserEnv might not set XDG_DATA_DIRS at all. If it's supposed to, you can try adding

profile = ''
  export XDG_DATA_DIRS=/usr/share/...
'';

to

wrapAppImage = args@{ name, src, extraPkgs, ... }: buildFHSUserEnv (defaultFhsEnvArgs // {
inherit name;
targetPkgs = pkgs: [ appimage-exec ]
++ defaultFhsEnvArgs.targetPkgs pkgs ++ extraPkgs pkgs;
runScript = "appimage-exec.sh -w ${src}";
} // (removeAttrs args (builtins.attrNames (builtins.functionArgs wrapAppImage))));

wrapGAppsHook will add the gsettings-schemas directories to XDG_DATA_DIRS via the glib setup-hook https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/libraries/glib/setup-hook.sh#L2.
You've confirmed what I've said here, we don't install gsettings-schemas into their default location, so we have to use wrapGAppsHook to include the gsettings-schemas.

@flokli
Copy link
Contributor

@flokli flokli commented Mar 29, 2020

But didn't this PR state that simply adding wrapGAppsHook didn't work?

@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Mar 29, 2020

But didn't this PR state that simply adding wrapGAppsHook didn't work?

wrapGAppsHook being added also just automatically wraps. Maybe manual wrapping is needed, but you still need wrapGAppsHook to collect gappsWrapperArgs.

@NinjaTrappeur
Copy link
Member

@NinjaTrappeur NinjaTrappeur commented Mar 29, 2020

@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Mar 29, 2020

Stupid question from a GTK noob here: why do we need to wrap the binary to add these schema in the first place? The appimageTools.wrapType2 function we're using in this package is in charge of creating an FHS env containing among other things GTK3. Ain't this FHS environment supposed to provide the ${gtk3}/share/gsettings-schemas/* schemas? Why are we missing them there? Is there a particular reason why we rather should wrap this particular binary instead of adding these schemas directly in the appimageTools.wrapType2 function, thus making sure we won't end up facing the same issue in other appImage bindists?

As I've said, in nixos we have a setup-hook that relocates all settings schemas to share/gsettings-schemas/${name}/glib-2.0/schemas. The FHS location would have been usr/share/glib-2.0/schemas. We can fix this problem by having the FHS chrootenv install them in that location #83705. That PR should obsolete this.

@NinjaTrappeur
Copy link
Member

@NinjaTrappeur NinjaTrappeur commented Mar 29, 2020

@ehmry
Copy link
Member Author

@ehmry ehmry commented Mar 30, 2020

While this patch may fix Patchwork in the short-time, I'm closing the PR in favor of the general fix, #83705.

@ehmry ehmry closed this Mar 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.