Skip to content

Comments

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

Closed
xaverdh wants to merge 1 commit intoNixOS:masterfrom
xaverdh:signal-desktop-binary-wrapper
Closed

signal-desktop: fix usage of gappsWrapperArgs with binary wrapper#172315
xaverdh wants to merge 1 commit intoNixOS:masterfrom
xaverdh:signal-desktop-binary-wrapper

Conversation

@xaverdh
Copy link
Contributor

@xaverdh xaverdh commented May 10, 2022 •
edited
Loading

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

Indeed since the last stanging merge 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 (on xmonad; Xorg).

Description of changes

Double wrap instead with a non-binary wrapper, that sets the flags as appropriate.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@xaverdh
Copy link
Contributor Author

xaverdh commented May 10, 2022 •
edited
Loading

I successfully ran the nixosTests, and verified locally that this fixes the issue.

@ofborg ofborg bot requested review from Mic92 and equirosa May 10, 2022 09:52
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels May 10, 2022
@xaverdh xaverdh mentioned this pull request May 10, 2022
Merged
@xaverdh
Copy link
Contributor Author

xaverdh commented May 10, 2022 •
edited
Loading

ah ok, while this does work, it has the side effect of only creating a single non-binary wrapper (contrary to my intention), so should probably not be merged as is..

@ncfavier
Copy link
Member

ncfavier commented May 10, 2022 •
edited
Loading

You meant postFixup, not postFixupPhase.

Also this PR is based on a commit without #164163, which is why shell wrappers are winning. Try nixos-unstable-small (or nixos-unstable, which apparently just updated).

An actual fix would be to replace wrapGAppsHook with (wrapGAppsHook.override { makeBinaryWrapper = makeWrapper; }). That does create a single non-binary wrapper, which is fine. EDIT: or to have makeWrapper after wrapGAppsHook in nativeBuildInputs, apparently.

@ncfavier
Copy link
Member

ncfavier commented May 10, 2022

We'll also want to do this for vscode, discord and brave.

@srhb srhb mentioned this pull request May 10, 2022
Merged
13 tasks
@xaverdh
signal-desktop: fix usage of gappsWrapperArgs with binary wrapper
871bc12 
When wrapGAppsHook switched to a binary wrapper (NixOS#164163), relying on
environment variables being evaluated in gappsWrapperArgs at runtime
no longer works.
@xaverdh xaverdh force-pushed the signal-desktop-binary-wrapper branch from 44ce424 to 871bc12 Compare May 10, 2022 10:32
@xaverdh
Copy link
Contributor Author

xaverdh commented May 10, 2022 •
edited
Loading

You meant postFixup, not postFixupPhase.

Also this PR is based on a commit without #164163, which is why shell wrappers are winning. Try nixos-unstable-small (or nixos-unstable, which apparently just updated).

Sorry I pushed the pr from a branch different from the one I was testing (which indeed had the commit in it). The commit from #164163 should be included now here. It does not change the outcome though (a single non-binary wrapper)

An actual fix would be to replace wrapGAppsHook with (wrapGAppsHook.override { makeBinaryWrapper = makeWrapper; }). That does create a single non-binary wrapper, which is fine. EDIT: or to have makeWrapper after wrapGAppsHook in nativeBuildInputs, apparently.

^^ ah indeed that is what makes it work O.o

@xaverdh
Copy link
Contributor Author

xaverdh commented May 10, 2022

An actual fix would be to replace wrapGAppsHook with (wrapGAppsHook.override { makeBinaryWrapper = makeWrapper; }). That does create a single non-binary wrapper, which is fine.

That is certainly cleaner compared to relying on order in nativeBuildInputs. So do you suggest we do this for all affected packages, for the time being?

@ncfavier
Copy link
Member

ncfavier commented May 10, 2022

Yes, please.

@xaverdh xaverdh mentioned this pull request May 10, 2022
Merged
13 tasks
@xaverdh
Copy link
Contributor Author

xaverdh commented May 10, 2022

closing in favour of #172320

@xaverdh xaverdh closed this May 10, 2022
@xaverdh xaverdh deleted the signal-desktop-binary-wrapper branch May 10, 2022 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@equirosa equirosa Awaiting requested review from equirosa

@Mic92 Mic92 Awaiting requested review from Mic92

Assignees

No one assigned

Labels

10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@xaverdh @ncfavier