treewide: inject xdg-open into wrappers as $PATH suffix #181171

ehmry · 2022-07-11T23:34:24Z

Description of changes

The xdg-open utility is only ever a runtime dependency and its
dependents only expect that it accept a URI as a command line
argument and do something with it that the user would expect.
For such as a trivial relationship it should be possible for
users to override xdg-open with something else in their PATH.

I don't know anyone that knows what xdg-open does but hasn't also replaced it with a shell script.

Things done

nixos-discourse · 2022-08-07T18:53:21Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/1072

pkgs/applications/audio/bitwig-studio/bitwig-studio4.nix

LunNova · 2022-08-16T16:22:03Z

This is great! It makes a fix for #160923 a lot less intrusive as you can just provide a better xdg-open on the path instead of rebuilding everything.

kira-bruneau

This looks great! There are some merge conflicts to deal with since this was submitted, but I think it would be ok to merge after that.

nixos-discourse · 2022-08-16T18:48:44Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/should-we-support-encoding-runtime-dependencies-without-requiring-a-specific-implementation/21031/6

SuperSandro2000 · 2022-08-17T13:27:32Z

I think we should accompany this by a contributing guide change to prefix such tools.

Merge conflicts need to be resolved & contributing guide should be updated

ehmry · 2022-08-18T00:02:22Z

(FWIW, my xdg-open replacement.)

doc/stdenv/stdenv.chapter.md

kira-bruneau

I think the docs should be good with @fricklerhandwerk's suggestions. I also noticed though that there are still a few other places where we should suffix xdg-utils:

pkgs/applications/audio/bitwig-studio/bitwig-studio1.nix
pkgs/applications/audio/bitwig-studio/bitwig-studio3.nix
pkgs/applications/misc/avrdudess/default.nix
pkgs/applications/misc/fff/default.nix
pkgs/applications/networking/browsers/brave/default.nix
pkgs/applications/networking/browsers/google-chrome/default.nix
pkgs/applications/networking/browsers/offpunk/default.nix
pkgs/applications/networking/instant-messengers/armcord/default.nix
pkgs/applications/office/jabref/default.nix
pkgs/applications/version-management/sparkleshare/default.nix
pkgs/tools/admin/aws-sso-cli/default.nix
pkgs/tools/bluetooth/blueman/default.nix

Most of these are places where the wrapper is created indirectly through some other variable that includes xdg-utils, but some of them are just new updates to packages since this was started.

kira-bruneau · 2022-08-18T17:51:01Z

Also to fit CONTRIBUTING.md, the first commit should be renamed to: treewide: inject xdg-open into wrappers as a suffix to PATH

ehmry · 2022-08-18T18:20:09Z

I've fixed the packages that @kira-bruneau found.

pkgs/applications/audio/bitwig-studio/bitwig-studio1.nix

LunNova · 2022-08-18T21:11:22Z

Steam FHS does this too but it's fine to leave it out of this change as it has some indirection, probably better to be in a separate PR.

ehmry · 2022-08-18T23:40:42Z

I don't want to touch the FHS or appimage stuff, that should be a different PR.

LunNova · 2022-08-18T23:47:38Z

I'll try to do it for steam.

pkgs/applications/misc/fff/default.nix

kira-bruneau · 2022-08-19T00:03:44Z

pkgs/tools/graphics/mangohud/default.nix

@@ -102,7 +102,6 @@ in stdenv.mkDerivation rec {
        glxinfo
        gnugrep
        gnused
-        xdg-utils


The ./hardcode-dependencies.patch file will have to be updated to account for this change. I can take a look at this since I maintain mangohud.

Hmm wait, yeah the reason I hard-code these dependencies is because the library needs them, and mangohud can be enabled independently from the executable with MANGOHUD=1.

I think for now we should just leave this unchanged, I'll try to figure out a better solution for another PR.

pkgs/applications/version-management/sparkleshare/default.nix

kira-bruneau · 2022-08-19T00:05:32Z

pkgs/applications/networking/browsers/offpunk/default.nix

@@ -50,7 +50,7 @@ stdenv.mkDerivation (finalAttrs: {

    wrapProgram $out/bin/offpunk \
        --set PYTHONPATH "$PYTHONPATH" \
-        --set PATH ${lib.makeBinPath otherDependencies}
+        --suffix PATH ${lib.makeBinPath otherDependencies}


We should only suffix xdg-utils here.

[ less timg xdg-utils xsel ]

pkgs/applications/misc/qdirstat/default.nix

kira-bruneau · 2022-08-19T00:13:57Z

pkgs/applications/audio/faust/faust2.nix

@@ -221,7 +221,7 @@ let

      postFixup = ''
        for script in "$out"/bin/*; do
-          wrapProgram "$script" --prefix PATH : "${runtimePath}"
+          wrapProgram "$script" --suffix PATH : "${runtimePath}"


I don't think we should change this. It seems like runtimePath may include xdg-utils (I can't tell because it's populated by a function argument, runtimeInputs), but we shouldn't be suffixing any other dependencies.

The maintainer made this too complicated to begin with.

pkgs/applications/misc/taskwarrior/default.nix

pkgs/applications/terminal-emulators/alacritty/default.nix

pkgs/applications/version-management/sparkleshare/default.nix

The xdg-open utility is only ever a runtime dependency and its dependents only expect that it accept a URI as a command line argument and do something with it that the user would expect. For such as a trivial relationship it should be possible for users to override xdg-open with something else in their PATH.

ehmry · 2022-08-19T18:11:40Z

Merged what was reviewed.

jtrees · 2022-08-24T12:32:39Z

I used to have the following in my config:

nixpkgs.overlays = [(
  self: super: {
    xdg-utils = super.xdg-utils.override { mimiSupport = true; };
  }
)];

Unfortunately this resulted in having to build Chromium from source whenever there was a new version so I had to remove it again.

Does this PR mean I can add the overlay again without having to re-compile the world on every update?

fricklerhandwerk · 2022-08-24T13:05:03Z

@jtrees As far as I can see, no. Rebuilding a derivation because of an override is in the nature of nixpkgs; it's not an addition to an existing package, but a new package with different parameters.

jtrees · 2022-08-24T13:35:40Z

@jtrees As far as I can see, no. Rebuilding a derivation because of an override is in the nature of nixpkgs; it's not an addition to an existing package, but a new package with different parameters.

Ah shame. I thought I saw that xdg-utils was removed from the Chrome dependencies in this PR. But now I see that it's still an input so that will still trigger the build.

It's literally only used to add it to the PATH though, so it's essentially a runtime dependency. Is there no better way to deal with this kind of scenario in nix?

ehmry · 2022-08-24T15:49:31Z

@jtrees You do not want to override xdg-utils in an overlay, I think what you want is to add your modified xdg-utils to systemPackages and that will override the Chrome PATH at runtime.

jtrees · 2022-08-24T15:56:50Z

@jtrees You do not want to override xdg-utils in an overlay, I think what you want is to add your modified xdg-utils to systemPackages and that will override the Chrome PATH at runtime.

Ah, that makes sense! I'll try it. Thanks.

See NixOS#181171

See #181171

ehmry requested a review from mweinelt as a code owner July 11, 2022 23:34

ehmry requested a review from Profpatsch July 11, 2022 23:34

ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100 labels Jul 11, 2022

SuperSandro2000 reviewed Aug 7, 2022

View reviewed changes

pkgs/applications/audio/bitwig-studio/bitwig-studio4.nix Show resolved Hide resolved

ehmry force-pushed the xdg-nope branch from be4e281 to d43591d Compare August 7, 2022 22:55

ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 7, 2022

kira-bruneau mentioned this pull request Aug 16, 2022

newsflash: fix adding xdg-open to PATH #186982

Merged

13 tasks

LunNova approved these changes Aug 16, 2022

View reviewed changes

kira-bruneau previously approved these changes Aug 16, 2022

View reviewed changes

bobby285271 added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Aug 17, 2022

kira-bruneau removed the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Aug 17, 2022

ehmry force-pushed the xdg-nope branch from d43591d to 44fbec6 Compare August 17, 2022 23:56

ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 18, 2022

ehmry requested a review from fricklerhandwerk as a code owner August 18, 2022 13:29

github-actions bot added the 8.has: documentation This PR adds or changes documentation label Aug 18, 2022

ehmry requested a review from kira-bruneau August 18, 2022 13:31

LunNova mentioned this pull request Aug 18, 2022

nixos/xdg/xdg-open: init #187293

Closed

13 tasks

fricklerhandwerk reviewed Aug 18, 2022

View reviewed changes

doc/stdenv/stdenv.chapter.md Outdated Show resolved Hide resolved

ehmry force-pushed the xdg-nope branch from c94dab0 to 5fb87f9 Compare August 18, 2022 17:48

kira-bruneau reviewed Aug 18, 2022

View reviewed changes

ehmry force-pushed the xdg-nope branch from 5fb87f9 to e169004 Compare August 18, 2022 18:19

kira-bruneau reviewed Aug 18, 2022

View reviewed changes

pkgs/applications/audio/bitwig-studio/bitwig-studio1.nix Outdated Show resolved Hide resolved

ehmry force-pushed the xdg-nope branch from e169004 to a932511 Compare August 18, 2022 23:37

ehmry changed the title ~~Inject xdg-open into wrappers as a suffix to PATH~~ treewide: inject xdg-open into wrappers as $PATH suffix Aug 18, 2022

ofborg bot added 10.rebuild-darwin: 11-100 and removed 10.rebuild-darwin: 1-10 labels Aug 18, 2022

kira-bruneau reviewed Aug 19, 2022

View reviewed changes

ehmry force-pushed the xdg-nope branch from a932511 to dd5e48c Compare August 19, 2022 18:04

ehmry and others added 2 commits August 19, 2022 13:10

doc: add note about makeWrapper and PATH modification

f26987b

ehmry force-pushed the xdg-nope branch from dd5e48c to f26987b Compare August 19, 2022 18:10

ehmry merged commit 16eb45c into NixOS:master Aug 19, 2022

huantianad mentioned this pull request Mar 17, 2023

webcord: name -> pname, suffix xdg-utils to PATH #221581

Merged

12 tasks

ehmry deleted the xdg-nope branch May 29, 2023 08:23

ehmry mentioned this pull request May 29, 2023

BeatSaberModManager: wrap with xdg-utils #234637

Merged

12 tasks

Atemu added a commit to Atemu/nixpkgs that referenced this pull request May 29, 2023

BeatSaberModManager: suffix PATH with xdg-utils rather than prefix

ce203b1

See NixOS#181171

Atemu mentioned this pull request May 29, 2023

BeatSaberModManager: suffix PATH with xdg-utils rather than prefix #234817

Merged

12 tasks

ehmry pushed a commit that referenced this pull request May 29, 2023

BeatSaberModManager: suffix PATH with xdg-utils rather than prefix

0ad8dd6

See #181171

This was referenced May 29, 2023

Update sources nix-ocaml/nix-overlays#849

Closed

Update sources nix-ocaml/nix-overlays#850

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

treewide: inject xdg-open into wrappers as $PATH suffix #181171

treewide: inject xdg-open into wrappers as $PATH suffix #181171

ehmry commented Jul 11, 2022

nixos-discourse commented Aug 7, 2022

LunNova commented Aug 16, 2022

kira-bruneau left a comment

nixos-discourse commented Aug 16, 2022

SuperSandro2000 commented Aug 17, 2022

ehmry commented Aug 18, 2022

kira-bruneau left a comment •

edited

Loading

kira-bruneau commented Aug 18, 2022 •

edited

Loading

ehmry commented Aug 18, 2022

LunNova commented Aug 18, 2022

ehmry commented Aug 18, 2022

LunNova commented Aug 18, 2022

kira-bruneau Aug 19, 2022 •

edited

Loading

kira-bruneau Aug 19, 2022 •

edited

Loading

kira-bruneau Aug 19, 2022

ehmry Aug 19, 2022

kira-bruneau Aug 19, 2022 •

edited

Loading

ehmry Aug 19, 2022

ehmry commented Aug 19, 2022

jtrees commented Aug 24, 2022

fricklerhandwerk commented Aug 24, 2022

jtrees commented Aug 24, 2022

ehmry commented Aug 24, 2022

jtrees commented Aug 24, 2022

treewide: inject xdg-open into wrappers as $PATH suffix #181171

treewide: inject xdg-open into wrappers as $PATH suffix #181171

Conversation

ehmry commented Jul 11, 2022

Description of changes

Things done

nixos-discourse commented Aug 7, 2022

LunNova commented Aug 16, 2022

kira-bruneau left a comment

Choose a reason for hiding this comment

nixos-discourse commented Aug 16, 2022

SuperSandro2000 commented Aug 17, 2022

ehmry commented Aug 18, 2022

kira-bruneau left a comment • edited Loading

Choose a reason for hiding this comment

kira-bruneau commented Aug 18, 2022 • edited Loading

ehmry commented Aug 18, 2022

LunNova commented Aug 18, 2022

ehmry commented Aug 18, 2022

LunNova commented Aug 18, 2022

kira-bruneau Aug 19, 2022 • edited Loading

Choose a reason for hiding this comment

kira-bruneau Aug 19, 2022 • edited Loading

Choose a reason for hiding this comment

kira-bruneau Aug 19, 2022

Choose a reason for hiding this comment

ehmry Aug 19, 2022

Choose a reason for hiding this comment

kira-bruneau Aug 19, 2022 • edited Loading

Choose a reason for hiding this comment

ehmry Aug 19, 2022

Choose a reason for hiding this comment

ehmry commented Aug 19, 2022

jtrees commented Aug 24, 2022

fricklerhandwerk commented Aug 24, 2022

jtrees commented Aug 24, 2022

ehmry commented Aug 24, 2022

jtrees commented Aug 24, 2022

kira-bruneau left a comment •

edited

Loading

kira-bruneau commented Aug 18, 2022 •

edited

Loading

kira-bruneau Aug 19, 2022 •

edited

Loading

kira-bruneau Aug 19, 2022 •

edited

Loading

kira-bruneau Aug 19, 2022 •

edited

Loading