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

wrapGAppsHook: use it for some more packages #32210

Merged
merged 3 commits into from Dec 5, 2017
Merged

wrapGAppsHook: use it for some more packages #32210

merged 3 commits into from Dec 5, 2017

Conversation

ghost
Copy link

@ghost ghost commented Nov 30, 2017

Motivation for this change

For #31293 (comment) after #31891

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-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/)
  • Fits CONTRIBUTING.md.

@ghost
Copy link
Author

ghost commented Nov 30, 2017

/cc @jtojnar

@@ -43,8 +44,6 @@ stdenv.mkDerivation rec {

preFixup = optionalString enableGTK3 /* gsettings schemas for file dialogues */ ''
Copy link
Member

Choose a reason for hiding this comment

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

This comment can be also removed.

Copy link
Author

Choose a reason for hiding this comment

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

@jtojnar Done, thanks!

@jtojnar
Copy link
Member

jtojnar commented Dec 1, 2017

Transmission and dconf-editor look fine, I am not familiar with libreoffice packaging, though.

@jtojnar
Copy link
Member

jtojnar commented Dec 4, 2017

I pushed the dconf-editor and transmission as cb6065f and a9746ab. Cannot test the libreoffice one.

@ghost
Copy link
Author

ghost commented Dec 4, 2017

@jtojnar Thank you!

I am not familiar with libreoffice packaging, though.
Cannot test the libreoffice one.

/cc @viric @7c6f434c
Notes for libreoffice changes:

@7c6f434c
Copy link
Member

7c6f434c commented Dec 5, 2017

LibreOffice change makes enough sense that I am willing to take the claims about testing at the face value.

@7c6f434c 7c6f434c merged commit a32b941 into NixOS:master Dec 5, 2017
@jtojnar
Copy link
Member

jtojnar commented Dec 5, 2017

I was more worried about wrapGAppsHook introducing unnecessary things into the libreoffice closure.

@7c6f434c
Copy link
Member

7c6f434c commented Dec 5, 2017 via email

@ghost ghost deleted the gapps branch December 5, 2017 21:37
@ghost
Copy link
Author

ghost commented Dec 6, 2017

Guys, I'm confused >_>

@jtojnar

I was more worried about wrapGAppsHook introducing unnecessary things into the libreoffice closure.

But you said before that programs using wrapProgram --prefix XDG_DATA_DIRS $GSETTINGS_SCHEMAS_PATH

are most likely broken outside of GNOME (usually missing icon theme) anyway and should be changed to wrapGAppsHook.

I use LibreOffice but not that often and extensively to notice if something will become "subtly broken"

This is how everything behaves after #31891 under wayland compositors (except Gnome of course):

  • program with or without GSETTINGS_SCHEMAS_PATH and without wrapGAppsHook that launched via menu doesn't inherit settings, though it inherits settings when launched from terminal
  • program with wrapGAppsHook inherits settings when launched both ways

@7c6f434c

It looks like there is now dconf in the
closure, but I am not even sure if something is subtly broken without
it.

Where? Dconf is not required, but gsettings_desktop_schemas is required for one particular case: Gtk3 programs working under wayland compositors (except Gnome which prepares proper environment). Though Gnome guys switched from old settings formats to dconf settings far ago, they just not broke things for X11 backend, but haven't added similar legacy stuff to Wayland backend, so things appear to be broken only under Wayland. If you're interested see #31293

@jtojnar
Copy link
Member

jtojnar commented Dec 6, 2017

But you said before that programs using wrapProgram --prefix XDG_DATA_DIRS $GSETTINGS_SCHEMAS_PATH

Yes, the wrapGAppsHook will add everything necessary – but it will also add, for example, everything in build inputs that contains a share directory, even the things that are not needed.

  • program with or without GSETTINGS_SCHEMAS_PATH and without wrapGAppsHook that launched via menu doesn't inherit settings, though it inherits settings when launched from terminal

GSETTINGS_SCHEMAS_PATH does not really do anything. It is only a helper variable created by glib to be picked up by a wrapProgram invocation or wrapGAppsHook.

Where? Dconf is not required, but gsettings_desktop_schemas is required for one particular case.

dconf library is a propagated dependency of wrapGAppsHook. We would like to remove it in favour of an impure solution (#11239) but it will require some work.

@ghost
Copy link
Author

ghost commented Dec 7, 2017

@jtojnar

GSETTINGS_SCHEMAS_PATH ... to be picked up by a wrapProgram

Meant exactly that (wrapProgram --prefix XDG_DATA_DIRS $GSETTINGS_SCHEMAS_PATH).

dconf library is a propagated dependency of wrapGAppsHook. We would like to remove it in favour of an impure solution (#11239) but it will require some work.

Oops, indeed. Termite shell messed tests on my side. Its a gtk program wrapped with wrapGAppsHook and it propagates wrapper context to the childs. Dconf library is indeed needed as well as gsettings_desktop_schemas to make programs use dconf settings.

Yes, the wrapGAppsHook will add everything necessary – but it will also add, for example, everything in build inputs that contains a share directory, even the things that are not needed.

So did we go a right route or there is a better way without possibility to break anything? I don't know one except a module with setting like ...useDconf = true; which will just add dconf library's gio/modules to GIO_EXTRA_MODULES and gsettings-desktop-schemas to XDG_DATA_DIRS environment variables.

@jtojnar
Copy link
Member

jtojnar commented Dec 7, 2017

So did we go a right route or there is a better way without possibility to break anything?

Making sure the resulting derivation does not contain unnecessary dependencies and splitting them would work for some (gobjectIntrospection #32293). Other, unfortunately, contain share directory that is only needed during make (appstream-glib) – for those, in order not to be included in the derivation, we would need to teach wrapsGAppsHook to distinguish between build-time and run-time dependencies. That could be accomplished either by adding a new attribute for explicitly listed the dependencies to add to the wrapper, or by excluding those in nativeBuildInputs.

I don't know one except a module with setting like ...useDconf = true; which will just add dconf library's gio/modules to GIO_EXTRA_MODULES and gsettings-desktop-schemas to XDG_DATA_DIRS environment variables

The schemas might be useful even without dconf backend, for now I would keep them.
dconf.nix module would append to GIO_EXTRA_MODULES as you describe, see example

environment.variables.XDG_DATA_DIRS =
[ "/run/opengl-driver/share" ] ++ optional cfg.driSupport32Bit "/run/opengl-driver-32/share";
and add dconf to systemPackages.

zimbatm pushed a commit that referenced this pull request Dec 21, 2017
* bemenu: init at 2017-02-14

* velox: 2015-11-03 -> 2017-07-04

* orbment, velox: don't expose subprojects

the development of orbment and velox got stuck
their subprojects (bemenu, dmenu-wayland, st-wayland) don't work correctly outside of parent projects
so hide them to not confuse people
swc and wld libraries are unpopular and unlike wlc are not used by anything except velox

* pythonPackages.pydbus: init at 0.6.0

* way-cooler: 0.5.2 -> 0.6.2

* nixos/way-cooler: add module

* dconf module: use for wayland

non-invasive approach for #31293
see discussion at #32210

* sway: embed LD_LIBRARY_PATH for #32755

* way-cooler: switch from buildRustPackage to buildRustCrate #31150
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.

3 participants