do you miss chromium (and ungoogle-chromium)?

Btw arch instead reads the flags from a file
https://github.com/archlinux/svntogit-community/blob/packages/electron/trunk/electron-launcher.sh

and for chrom* https://wiki.archlinux.org/title/Chromium#Making_flags_persistent

do you miss chromium (and ungoogle-chromium)?

Ah, added it now (but did not test)

Btw arch instead reads the flags from a file https://github.com/archlinux/svntogit-community/blob/packages/electron/trunk/electron-launcher.sh

and for chrom* https://wiki.archlinux.org/title/Chromium#Making_flags_persistent

Hmm that's a good idea, we can do that too. It would mean adding a --run to makeWrapper that sources a generic electron-loading script that defines extra flags.

However, instead of putting a single simple env var, users would have to put a complex line in all those config files. We could have a generic fallback config, but still.

(Coming from #147945)
Does anyone know if there is a way to make Chrom* compatible with Wayland and Xorg at the same time without needing different variants of packages?
For me that would be the preferred solution. 🤔

(Coming from #147945) Does anyone know if there is a way to make Chrom* compatible with Wayland and Xorg at the same time without needing different variants of packages? For me that would be the preferred solution. 🤔

Isn't this PR exactly that?

Bad wording on my part, it doesn't require the separate packages anymore, but you still need to do more than to install chrom*. It would be nice to just start chrom* with Wayland support without breaking Xorg support, I currently have no Xorg setup, so that is why I asked that "dumb" question.

@TilCreator ah - what you need to do is ensure that NIXOS_OZONE=y is in your environment. The reason for that is that ozone support is still not 100% so unless you choose it explicitly, we shouldn't force it for everyone.

Ah, explicitly enabling it then should be needed. I would prefer this then over #147945

I'm wondering if I should make this slightly more complex, and make all Cr* projects add a --run ${pkgs.cr-support/prepCr.sh} script that adds a bash array of chrome flags and then --flags "${crFlags[@]}" instead of the current one-liner.

It would allow more complex rules but OTOH it's more magic.

This should also be enabled for discord/discord-ptb/discord-canary.

added discord and the wrapper scripts for the 2 *-wayland packages

I think this is good to merge and backport to 21.11.

For the wrapping, it would be nice if there was a less error-prone way of providing the makeWrapper arguments. Perhaps a --plugin ${pkgs.electronWrapSupport} flag to makeWrapper that evaluates the given script at build time and lets it add extra configuration to the wrapper declaration. What do you think @abbradar? Note that that does not block this PR, just a thought.

so - anyone opposed to merging this?

I'm trying to adapt nixosTests.vscodium.wayland to use this, and it somehow dumps core D:

diff --git a/nixos/tests/vscodium.nix b/nixos/tests/vscodium.nix
index 43a0d61c856..f10effe2790 100644
--- a/nixos/tests/vscodium.nix
+++ b/nixos/tests/vscodium.nix
@@ -4,11 +4,17 @@ let
       imports = [ ./common/wayland-cage.nix ];
 
       services.cage.program = ''
-        ${pkgs.vscodium}/bin/codium \
-          --enable-features=UseOzonePlatform \
-          --ozone-platform=wayland
+        ${pkgs.vscodium}/bin/codium
       '';
 
+      environment.variables.NIXOS_OZONE = "y";
+
+      nixpkgs.overlays = [
+        (self: super: {
+          cage = super.cage.override { xwayland = null; };
+        })
+      ];
+
       fonts.fonts = with pkgs; [ dejavu_fonts ];
     };
     xorg = { pkgs, ... }: {

(finished: sending monitor command: screendump /build/tmpfgchet8s/ppm, in 0.01 seconds)
wayland # [   18.254489] systemd[1]: Created slice Slice /system/systemd-coredump.
wayland # [   18.261805] systemd[1]: Started Process Core Dump (PID 945/UID 0).
wayland # [   18.689134] systemd-coredump[946]: Process 927 (codium) of user 1000 dumped core.
wayland # [   18.732325] systemd[1]: [email protected]: Deactivated successfully.
wayland # [   18.849545] systemd-logind[657]: Session c1 logged out. Waiting for processes to exit.
wayland # [   18.865381] systemd[1]: cage-tty1.service: Deactivated successfully.

Any idea what could be going on here?

@Synthetica9 thanks for checking that! Are you sure the test doesn't just dump core without the change? Could you add a env to the program so we can see what the environment variables are?

I must say I'm mystified how this could cause a coredump, it should be the exact same behavior :-/

@Synthetica9 thanks for checking that! Are you sure the test doesn't just dump core without the change? Could you add a env to the program so we can see what the environment variables are?

I must say I'm mystified how this could cause a coredump, it should be the exact same behavior :-/

Yep, when I just run it unmodified (or with only the overlay) it works on this PR. I'm also quite flabbergasted on the how and why of this crash tbh

@Synthetica9 can you change the program to bash -xv ${pkgs.vscodium}/bin/codium? Then we should be able to see what command it ends up executing.

It might not be worth dropping --enable-features=UseOzonePlatform as some older Electron apps probably won't have this set by default.

Sure, that's a possibility, but then again it'd also be surprising if those Electron apps would still work on modern Wayland compositors. They won't work with wlroots 0.15 (https://bugs.chromium.org/p/chromium/issues/detail?id=1279574), other compositors will follow soon, and for such old Ozone/Wayland revisions there could be additional incompatibilities.

It feels like --ozone-platform-hint will supersede this in the near future?

https://chromium-review.googlesource.com/c/chromium/src/+/3188974

Sure, that's a possibility, but then again it'd also be surprising if those Electron apps would still work on modern Wayland compositors. They won't work with wlroots 0.15 (https://bugs.chromium.org/p/chromium/issues/detail?id=1279574), other compositors will follow soon, and for such old Ozone/Wayland revisions there could be additional incompatibilities.

I just tested VS Code and it requires both flags to render with Wayland natively and it seems to work fine without crashing.

Ok I renamed the flag to NIXOS_OZONE_WL=1 to keep it a little shorter.

Good to go?

@primeos there is a test and it passes, so merging. In case of trouble we can still revert :)

@wmertens ok, thanks for your work and seeing this through! :)

This doesn't work as intended. This syntax:

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

checks the variables are set at build time not runtime. You can see this behavior if you run the build manually:

$ cd $(mktemp -d)
$ nix-shell '<nixpkgs>' -A chromium --pure --run 'source $stdenv/setup; out=. sandbox=./sandbox NIXOS_OZONE_WL=1 WAYLAND_DISPLAY=1 genericBuild'

Builder called die: makeWrapper doesn't understand the arg --ozone-platform=wayland

And if those env vars are not set at build time (which they probably never would be), nothing gets appended.

I believe this needs to use the double-escaping used elsewhere:

--add-flags ${escapeShellArg (escapeShellArg commandLineArgs)}

in order to have a chance to work, that is.

EDIT: most of the above is wrong in the general case, it turns out that double-escaping is only needed for chromium because it's just a wrapper package and uses buildCommand does weird stuff idk (see #147557 (comment)).

Odd, I did test it. I'll investigate.

Oh, right, strange...
Not sure how the current situation looks but now that @eddyb mentions it (thanks!) I remember that I had this problem once too.

Back then I didn't find an elegant solution due to the way wrapProgram is implemented (not sure if that's still the case) and resorted to the following hack for Telegram-Desktop:

There's hopefully a better alternative available.

@eddyb are you seeing it not work somewhere? It's working correctly for me with vscode and google-chrome-stable.

Ah, interesting, I've only tested it with Chromium but e.g. Signal-Desktop works:

$ git rev-parse HEAD
5bb20f9dc70e9ee16e21cc404b6508654931ce41
$ tail -n1 $(nix-build -A chromium)/bin/chromium
gq7rrhd10df-chromium-unwrapped-97.0.4692.99/libexec/chromium/chromium"    "$@"
$ tail -n1 $(nix-build -A signal-desktop)/bin/signal-desktop
exec -a "$0" "/nix/store/k7cjv6sg0pqqrdqlm6abv6vyxw9kgp49-signal-desktop-5.29.1/bin/.signal-desktop-wrapped"  ${NIXOS_OZONE_WL:+${WAYLAND_DISPLAY:+--enable-features=UseOzonePlatform --ozone-platform=wayland}} "$@"

Looks like it works via $gappsWrapperArgs:

diff --git a/pkgs/applications/networking/browsers/google-chrome/default.nix b/pkgs/applications/networking/browsers/google-chrome/default.nix
index e93ea8ca66d..b0fc87a1801 100644
--- a/pkgs/applications/networking/browsers/google-chrome/default.nix
+++ b/pkgs/applications/networking/browsers/google-chrome/default.nix
@@ -142,8 +142,9 @@ in stdenv.mkDerivation {
     makeWrapper "$out/share/google/$appname/google-$appname" "$exe" \
       --prefix LD_LIBRARY_PATH : "$rpath" \
       --prefix PATH            : "$binpath" \
-      --prefix XDG_DATA_DIRS   : "$XDG_ICON_DIRS:$GSETTINGS_SCHEMAS_PATH:${addOpenGLRunpath.driverLink}/share" \
-      --add-flags ${escapeShellArg commandLineArgs}
+      --prefix XDG_DATA_DIRS   : "$XDG_ICON_DIRS:$GSETTINGS_SCHEMAS_PATH" \
+      --add-flags ${escapeShellArg commandLineArgs} \
+      --add-flags "\''${NIXOS_OZONE_WL:+\''${WAYLAND_DISPLAY:+--enable-features=UseOzonePlatform --ozone-platform=wayland}}"

     for elf in $out/share/google/$appname/{chrome,chrome-sandbox,${crashpadHandlerBinary},nacl_helper}; do
       patchelf --set-rpath $rpath $elf
diff --git a/pkgs/applications/networking/instant-messengers/signal-desktop/default.nix b/pkgs/applications/networking/instant-messengers/signal-desktop/default.nix
index f1bcd101279..22440d71805 100644
--- a/pkgs/applications/networking/instant-messengers/signal-desktop/default.nix
+++ b/pkgs/applications/networking/instant-messengers/signal-desktop/default.nix
@@ -123,6 +123,7 @@ in stdenv.mkDerivation rec {
     gappsWrapperArgs+=(
       --prefix LD_LIBRARY_PATH : "${lib.makeLibraryPath [ stdenv.cc.cc ] }"
       ${customLanguageWrapperArgs}
+      --add-flags "\''${NIXOS_OZONE_WL:+\''${WAYLAND_DISPLAY:+--enable-features=UseOzonePlatform --ozone-platform=wayland}}"
     )

     # Fix the desktop link

Seems like I also missed that you've dropped ${addOpenGLRunpath.driverLink}/share for google-chrome. That needs to be reverted. Edit: I've opened #157554.

@eddyb are you seeing it not work somewhere? It's working correctly for me with vscode and google-chrome-stable.

I was testing with the chromium package - since it seemed like the same line got copy-pasted everywhere, I didn't expect it to work anywhere, but maybe the context is subtly different?

EDIT: okay, yeah, rereading the comment above, gappsWrapperArgs+=( is very likely giving you one extra level of escapeShellArg to make it work. Ideally this would always be added to a Nix "wrapper args" variable instead of directly inline.

@wmertens Ugh, sorry for all the noise, it might just be chromium. In #147557 (comment) I noticed something I had missed before, and went back to check, and chromium is the only one that uses buildCommand, which could explain why it needs double-escaping.

(Frankly the entire buildCommand could probably be wrapped in a single escapeShellArg to make it equivalent to installPhase in other packages)

EDIT: wait, it's doing eval makeWrapper while other scripts are doing makeWrapper?
Is it not buildCommand vs installCommand but rather an unnecessary(?) eval??

@primeos Regarding reusing commandLineArgs, I was mostly just imagining something like this:

let commandLineArgs =
  "${commandLineArgs} \${NIXOS_OZONE_WL:+\${WAYLAND_DISPLAY:+--enable-features=UseOzonePlatform --ozone-platform=wayland}}";

(i.e. not changing commandLineArgs at its origin, but creating a longer string locally)

(not sure about the escaping syntax, but at least if it was always done in Nix, it should be possible to remain consistent and avoid weird shell escaping syntax)

@eddyb good catch on the eval! I removed it and things seem to work fine in #157835