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
Contributor

@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

Works like a charm! ❤️

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

Choose a reason for hiding this comment

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

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

Comment on lines +36 to +41
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}" \
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@worldofpeace
Copy link
Contributor

We could probably backport this bugfix to 20.03

@flokli
Copy link
Contributor

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
Contributor

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 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
Contributor

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 commented Mar 29, 2020

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

@worldofpeace
Copy link
Contributor

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.

@picnoir
Copy link
Member

picnoir commented Mar 29, 2020 via email

@worldofpeace
Copy link
Contributor

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.

@picnoir
Copy link
Member

picnoir commented Mar 29, 2020 via email

@ehmry
Copy link
Contributor Author

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.

@Artturin
Copy link
Member

#161739

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

7 participants