wrapGAppsHook: use makeBinaryWrapper #164163

ncfavier · 2022-03-14T20:26:14Z

WIP to make the GApps wrapper use makeBinaryWrapper instead of makeWrapper. cc @bergkvist @doronbehar @tfc

The basic motivation is that some applications like thunar are wrapped twice, once by wrapGAppsHook and once manually to add support for plugins, and because of the way shell scripts work this results in thunar's argv0 being set to .thunar-wrapped_, which is reflected in the X11 class name for all thunar windows. I already mentioned this issue at #126248 (comment) and this was one of the motivations for makeBinaryWrapper (#124556). Making the outer wrapper binary isn't enough to solve this, hence this PR.

This isn't as easy as it sounds for at least two reasons (more may arise!):

makeBinaryWrapper uses a wrapped GCC and will hence respect NIX_CFLAGS_COMPILE, which resulted in an error when trying to build libreoffice because a flag in its NIX_CFLAGS_COMPILE was for C++ only. I believe the wrapper shouldn't be affected by whatever CFLAGS happen to be set in the calling derivation, so I unset those variables in d399e701aa79815148ce08b41ed7263f122f1d24 (more unsetting may be needed). (I also tried to use gcc-unwrapped but got more errors)
some programs have both makeWrapper and wrapGAppsHook in their build inputs, and because setup hooks are propagated, both versions of the wrapping functions are installed and it seems like the binary ones win. This made the build of minecraft fail because the binary version doesn't implement --run, so I replaced every instance of --run "cd $foo" I could find with --chdir "$foo" in a4b32633e8c6429d2f2a02f70373d20e5d966ee6 and made makeWrapper implement --chdir in 2e3ae88a9c830ba18163c778ade179e7cc820cd3. I am not sure how exactly we want to resolve the issue of conflicting versions of wrapper hooks; maybe it's a good thing that all wrappers around a program are of the same "type".

I tried building a few things but I don't have a build farm handy and my server ran out of disk space 😬. Can I request a dedicated hydra jobset for this branch or something? cc @vcunat

nixos/modules/services/misc/gitlab.nix

pkgs/games/starsector/default.nix

pkgs/games/assaultcube/default.nix

pkgs/tools/cd-dvd/ventoy-bin/default.nix

SuperSandro2000 · 2022-03-14T22:50:15Z

The basic motivation is that some applications like thunar are wrapped twice, once by wrapGAppsHook and once manually to add support for plugins

This can be easily fixed with makeWrapperArgs and has nothing to do with makeBinaryWrapper and needs to be fixed independently.

ncfavier · 2022-03-14T22:52:25Z

The basic motivation is that some applications like thunar are wrapped twice, once by wrapGAppsHook and once manually to add support for plugins

This can be easily fixed with wrapperArgs and has nothing to do with makeBinaryWrapper and needs to be fixed independently.

That would work if the two wrappings happened in the same derivation, but there's a separate wrapper.nix stage.

ncfavier · 2022-03-14T22:53:41Z

Technically the wrapper could override the gapps wrapper's flags but then you'd need to rebuild thunar every time you add a plugin, which defeats the point of having the wrapper separate

SuperSandro2000 · 2022-03-14T22:53:50Z

That would work if the two wrappings happened in the same derivation, but there's a separate wrapper.nix stage.

We could override that but it would require a recompile or we clone the build derivation and dirty patch the wrapper.

SuperSandro2000 · 2022-03-14T22:55:05Z

pkgs/build-support/setup-hooks/make-wrapper.sh

@@ -18,6 +18,7 @@ assertExecutable() {
 # --set-default VAR VAL : like --set, but only adds VAR if not already set in
 #                         the environment
 # --unset       VAR     : remove VAR from the environment
+# --chdir       DIR     : change working directory (use instead of --run "cd DIR")


Should we log a warning or error when this is done?

Anything short of parsing shell syntax would have to be an ugly heuristic like "the command starts with cd" which is bound to miss some legitimate use cases, so I vote no

pkgs/build-support/setup-hooks/make-binary-wrapper.sh

SuperSandro2000 · 2022-03-14T22:57:45Z

Technically the wrapper could override the gapps wrapper's flags but then you'd need to rebuild thunar every time you add a plugin, which defeats the point of having the wrapper separate

We could also move the wrapping entirely to the wrapper or use a totally unwrapped program for plugin support and add the required variables only later. There are multiple ways to fix this independently of binary wrapper.

ncfavier · 2022-03-14T23:00:11Z

We could also move the wrapping entirely to the wrapper or use a totally unwrapped program for plugin support and add the required variables only later. There are multiple ways to fix this independently of binary wrapper.

Sounds like working around a technical limitation that has no raison d'être ("can't have more than one layer of wrappers")

SuperSandro2000 · 2022-03-15T18:48:42Z

If the goal is to totally switch most things to binary wrappers we should also not layer them.

pkgs/build-support/setup-hooks/wrap-gapps-hook/tests/lib.nix

ncfavier · 2022-03-15T19:05:15Z

If the goal is to totally switch most things to binary wrappers we should also not layer them.

I'm not seeing the connection, I think these are independent questions.

To be clear, when I said "the basic motivation" I meant what motivated me to make this PR. I'm not saying we shouldn't unlayer the wrappers, or that this is the only way to fix thunar, but it seemed like a logical step forward.

LunNova · 2022-03-15T19:15:35Z

Would it be possible to have makeWrapper detect an existing wrapper script, and copy and then edit it instead of double wrapping it?

That seems like it'd be a simpler change and would likely also solve this problem everywhere with minimal changes needed treewide.

ncfavier · 2022-03-15T19:50:09Z

Would it be possible to have makeWrapper detect an existing wrapper script, and copy and then edit it instead of double wrapping it?

I thought about this some time ago and eventually dropped the idea, but I can't remember why precisely. I think it felt like a layer violation because a wrapper (much less a wrapper builder) shouldn't have to know about the thing it's wrapping (imagine that the wrapped program is a mutable path like /run/current-system/sw/bin/foo, or somehow hasn't been built yet).

It would probably fix argv0 issues if done correctly, but doing it correctly doesn't seem significantly easier than this PR, and I think binary wrappers are a cleaner solution for now.

(Also note there's NixOS/rfcs#75 roughly going in that direction)

xaverdh · 2022-05-10T08:55:21Z

I think this broke signal-desktop.

Recently I get
[5036:0510/101840.222112:FATAL:platform_selection.cc(45)] Invalid ozone platform: wayland}} fish: Job 1, 'signal-desktop' terminated by signal SIGTRAP (Trace or breakpoint trap)
when trying to start signal-desktop. I am on xmonad (Xorg).

Originally posted by @xaverdh in #158175 (comment)

This

nixpkgs/pkgs/applications/networking/instant-messengers/signal-desktop/default.nix

Line 126 in f419dc5

    
                 --add-flags "\''${NIXOS_OZONE_WL:+\''${WAYLAND_DISPLAY:+--enable-features=UseOzonePlatform --ozone-platform=wayland}}"

does not work as it used to with the binary wrapper?

xaverdh · 2022-05-10T09:10:04Z

So the right solution for signal-desktop would be to double wrap it with a non-binary wrapper, that sets the appropriate flag?

ncfavier · 2022-05-10T09:52:30Z

So the right solution for signal-desktop would be to double wrap it with a non-binary wrapper, that sets the appropriate flag?

That would be a workaround, though it's slightly complicated to implement right now because wrapGAppsHook forces makeWrapper to be the binary version. I'm making a cleanup PR in which I'll add explicit makeShellWrapper and makeBinaryWrapper functions.

Another option would be to somehow support this pattern in binary wrappers, but that seems a bit awkward.

xaverdh · 2022-05-10T09:53:26Z

Well #172315 appears to be working actually.
Ah I see what you mean.. it does not double wrap, but only creates a single (non-binary) wrapper..

ncfavier · 2022-05-10T10:15:54Z

Nope (see discussion there).

Apparently whichever of makeWrapper and wrapGAppsHook is last in nativeBuildInputs wins... good to know.

When wrapGAppsHook switched to a binary wrapper (NixOS#164163), relying on environment variables being evaluated in gappsWrapperArgs at runtime no longer works.

The use of --add-flags in this derivation does not work with binary wrappers, which wrapGAppsHook uses since NixOS#164163.

The use of --add-flags in this derivation assumed quotes to be expanded, which the binary wrapper (which wrapGAppsHook uses since NixOS#164163) will not do.

ncfavier requested review from mmahut, RaghavSood, marsam and Ericson2314 as code owners March 14, 2022 20:26

ncfavier marked this pull request as draft March 14, 2022 20:26

github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: ruby 8.has: module (update) This PR changes an existing module in `nixos/` labels Mar 14, 2022

ncfavier mentioned this pull request Mar 14, 2022

ruby-modules/bundler-env: Replace makeWrapper with makeBinaryWrapper to make bundled ruby apps work on macOS #161298

Merged

13 tasks

ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ labels Mar 14, 2022

bergkvist reviewed Mar 14, 2022

View reviewed changes

SuperSandro2000 reviewed Mar 14, 2022

View reviewed changes

pkgs/build-support/setup-hooks/make-binary-wrapper.sh Show resolved Hide resolved

SuperSandro2000 reviewed Mar 15, 2022

View reviewed changes

pkgs/build-support/setup-hooks/wrap-gapps-hook/tests/lib.nix Outdated Show resolved Hide resolved

SuperSandro2000 reviewed Mar 15, 2022

View reviewed changes

pkgs/build-support/setup-hooks/wrap-gapps-hook/tests/lib.nix Outdated Show resolved Hide resolved

ncfavier mentioned this pull request Mar 18, 2022

makeWrapper: add --chdir and migrate most uses of --run cd #164746

Merged

ncfavier mentioned this pull request May 6, 2022

Fix wrapper arguments escaping #171836

Merged

OPNA2608 mentioned this pull request May 9, 2022

wrapGAppsHook taints build environment with wrong/its stdenv #172249

Closed

xaverdh mentioned this pull request May 10, 2022

electron,google-chrome: Ozone/Wayland is broken #158175

Open

xaverdh mentioned this pull request May 10, 2022

signal-desktop: fix usage of gappsWrapperArgs with binary wrapper #172315

Closed

13 tasks

ncfavier mentioned this pull request May 10, 2022

teams: 1.4.00.26453 -> 1.5.00.10453 #170863

Merged

13 tasks

xaverdh added a commit to xaverdh/nixpkgs that referenced this pull request May 10, 2022

signal-desktop: use non-binary wrapper for wrapGAppsHook

70d7d6e

The use of --add-flags in this derivation does not work with binary wrappers, which wrapGAppsHook uses since NixOS#164163.

xaverdh mentioned this pull request May 10, 2022

treewide: use non-binary wrapper for wrapGAppsHook where appropriate #172320

Merged

13 tasks

xaverdh added a commit to xaverdh/nixpkgs that referenced this pull request May 10, 2022

brave: use non-binary wrapper for wrapGAppsHook

6d06f47

The use of --add-flags in this derivation does not work with binary wrappers, which wrapGAppsHook uses since NixOS#164163.

xaverdh added a commit to xaverdh/nixpkgs that referenced this pull request May 10, 2022

discord: use non-binary wrapper for wrapGAppsHook

a697777

The use of --add-flags in this derivation does not work with binary wrappers, which wrapGAppsHook uses since NixOS#164163.

xaverdh added a commit to xaverdh/nixpkgs that referenced this pull request May 10, 2022

vscode: use non-binary wrapper for wrapGAppsHook

a2ee3ed

The use of --add-flags in this derivation does not work with binary wrappers, which wrapGAppsHook uses since NixOS#164163.

xaverdh added a commit to xaverdh/nixpkgs that referenced this pull request May 10, 2022

tlaplusToolbox: use non-binary wrapper for wrapGAppsHook

86e7fa9

The use of --add-flags in this derivation does not work with binary wrappers, which wrapGAppsHook uses since NixOS#164163.

xaverdh added a commit to xaverdh/nixpkgs that referenced this pull request May 10, 2022

cryptomator: use non-binary wrapper for wrapGAppsHook

9709c5f

The use of --add-flags in this derivation does not work with binary wrappers, which wrapGAppsHook uses since NixOS#164163.

bergkvist mentioned this pull request May 11, 2022

makeBinaryWrapper: Variable expansion at runtime #172583

Open

LunNova mentioned this pull request May 11, 2022

makeBinaryWrapper: breaks downstream derivations which edit wrappers #172592

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wrapGAppsHook: use makeBinaryWrapper #164163

wrapGAppsHook: use makeBinaryWrapper #164163

ncfavier commented Mar 14, 2022 •

edited

Loading

SuperSandro2000 commented Mar 14, 2022 •

edited

Loading

ncfavier commented Mar 14, 2022

ncfavier commented Mar 14, 2022

SuperSandro2000 commented Mar 14, 2022

SuperSandro2000 Mar 14, 2022

ncfavier Mar 14, 2022

SuperSandro2000 commented Mar 14, 2022

ncfavier commented Mar 14, 2022 •

edited

Loading

SuperSandro2000 commented Mar 15, 2022

ncfavier commented Mar 15, 2022

LunNova commented Mar 15, 2022

ncfavier commented Mar 15, 2022 •

edited

Loading

xaverdh commented May 10, 2022 •

edited

Loading

xaverdh commented May 10, 2022

ncfavier commented May 10, 2022

xaverdh commented May 10, 2022 •

edited

Loading

ncfavier commented May 10, 2022

wrapGAppsHook: use makeBinaryWrapper #164163

wrapGAppsHook: use makeBinaryWrapper #164163

Conversation

ncfavier commented Mar 14, 2022 • edited Loading

SuperSandro2000 commented Mar 14, 2022 • edited Loading

ncfavier commented Mar 14, 2022

ncfavier commented Mar 14, 2022

SuperSandro2000 commented Mar 14, 2022

SuperSandro2000 Mar 14, 2022

Choose a reason for hiding this comment

ncfavier Mar 14, 2022

Choose a reason for hiding this comment

SuperSandro2000 commented Mar 14, 2022

ncfavier commented Mar 14, 2022 • edited Loading

SuperSandro2000 commented Mar 15, 2022

ncfavier commented Mar 15, 2022

LunNova commented Mar 15, 2022

ncfavier commented Mar 15, 2022 • edited Loading

xaverdh commented May 10, 2022 • edited Loading

xaverdh commented May 10, 2022

ncfavier commented May 10, 2022

xaverdh commented May 10, 2022 • edited Loading

ncfavier commented May 10, 2022

ncfavier commented Mar 14, 2022 •

edited

Loading

SuperSandro2000 commented Mar 14, 2022 •

edited

Loading

ncfavier commented Mar 14, 2022 •

edited

Loading

ncfavier commented Mar 15, 2022 •

edited

Loading

xaverdh commented May 10, 2022 •

edited

Loading

xaverdh commented May 10, 2022 •

edited

Loading