[staging] setup.sh: Support XDG_DATA_DIRS (bash completion in nix-shell) #103501
Conversation
roberth
changed the title
ofborg
bot
added
6.topic: stdenv
|
While I think this is a good idea, I just wanted to note something similar was previously discussed in NixOS/nix#2443 and @edolstra had some objections. It may be less controversial in Nixpkgs though since we rely on XDG_DATA_DIRS in a lot of places already. |
pkgs/stdenv/generic/setup.sh
Outdated
| @@ -602,9 +603,11 @@ fi | |||
|
|
|||
| PATH="${_PATH-}${_PATH:+${PATH:+:}}$PATH" | |||
| HOST_PATH="${_HOST_PATH-}${_HOST_PATH:+${HOST_PATH:+:}}$HOST_PATH" | |||
| XDG_DATA_DIRS="${_XDG_DATA_DIRS-}${_XDG_DATA_DIRS:+${XDG_DATA_DIRS:+:}}${XDG_DATA_DIRS-}" | |||
There was a problem hiding this comment.
I spent a couple minutes looking through Nixpkgs to see if this could interfere with anything. I don't think it will, but setting something like this can always have unintended consequences. The main concern would be if native XDG_DATA_DIRS gets mixed in with target XDG_DATA_DIRS in cross-compilation. For things like bash completion and man pages, it's not such a big deal, but XDG_DATA_DIRS is also used to locate qt5 plugins.
There was a problem hiding this comment.
Since it's not exported, this might not be as much of a problem actually.
There was a problem hiding this comment.
It is implicitly exported when XDG_DATA_DIRS already exists as an environment variable (and --pure isn't used).
Always exporting it seems more consistent. If mixing with the nix-shell host environment is problematic, that can be avoided with nix-shell --pure.
In cross compilation without strictDeps, mixing will occur. Perhaps XDG_DATA_DIRS should follow "strictDeps" behavior regardless. That will require development shells to put tools in nativeBuildInputs instead of buildInputs for bash completion to work. nix-shell -p gets that wrong, so it won't work there yet, but can be fixed.
|
Ok, Also |
| @@ -602,9 +606,11 @@ fi | |||
|
|
|||
| PATH="${_PATH-}${_PATH:+${PATH:+:}}$PATH" | |||
| HOST_PATH="${_HOST_PATH-}${_HOST_PATH:+${HOST_PATH:+:}}$HOST_PATH" | |||
| export XDG_DATA_DIRS="${_XDG_DATA_DIRS-}${_XDG_DATA_DIRS:+${XDG_DATA_DIRS:+:}}${XDG_DATA_DIRS-}" | |||
There was a problem hiding this comment.
Is this going to affect wrapGAppsHook or wrapQtAppsHook in any way?
There was a problem hiding this comment.
It shouldn't affect those.
This variable is set in the build environment, not in the wrapper scripts.
There was a problem hiding this comment.
cc @NixOS/freedesktop
There was a problem hiding this comment.
Oh, so being set in the build environment, but during fixup it shouldn't be?
There was a problem hiding this comment.
Well it's still set during fixupPhase but it doesn't affect the wrapper writing functions. Although, it can affect call sites if they are like wrapProgram --set XDG_DATA_DIRS $XDG_DATA_DIRS, so expanding $XDG_DATA_DIRS before invoking wrapProgram, but I don't think we have that?
There was a problem hiding this comment.
Yeah, wrapGAppsHook uses different environment variables for populating XDG_DATA_DIRS for wrapper. So unless one of those variables is populated from XDG_DATA_DIRS (do not see any such thing when greping sh files for XDG_DATA_DIRS so it would have to be indirectly through some program).
The only possible issue that comes to mind is that it further distances the build environment from the runtime one, possibly obscuring some missing dependencies until runtime (i.e. (install)CheckPhase passes but program will crash at runtime). But perhaps the difference is huge enough already so this might not be so much worse in proportion.
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/setup-bash-completion-when-running-under-nix-shell-on-macosx/5216/6 |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/shell-nix-load-completions/8803/15 |
|
My understanding is that only nativeBuildInputs have shell completion from this, however, most examples use nix manual: https://nixos.org/manual/nix/unstable/command-ref/nix-shell.html I don't think this is a blocker, but we should probably update the documentation examples so that people get expected behavior |
That is correct. The idea is that This change does affect all derivations after all, for better or for worse. I'm hopeful but this needs testing. Speaking of testing, on non-cross we should probably change or unset
💯. Should have been done with the introduction of cross compilation. |
|
@jonringer Yeah, we've never found a sane way to flip the I like it a lot! |
|
One thing I didn't understand is; what kind of black magic auto-loads the completion? Is it part of bash itself to look for those files? Incidentally, I recently worked on unifying the bash completions outputs: #103421 |
It looks like bash automatically finds the right completion scripts by looking into XDG_DATA_DIRS. Probably other programs respecting XDG specs can also benefit from setting that environment variable. Thanks to NixOS/nixpkgs#103501 for finding that gem. I don't understand how this works, the Bash source code doesn't mention XDG at all. Maybe it's a NixOS-specific thing.
No black magic, just a shell script in the bash-completion package.
That's great! |
XDG_DATA_DIRS is to /share as PATH is to /bin. It was defined as part of the XDG basedir specification. https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html While it originated from the X Desktop Group, it is not limited to the X11 ecosystem, as evidenced by its use in bash-completion. The removal of ` && -d "$pkg/bin"` is ok, because this optimization is already performed by `addToSearchPath`.
This avoids the scenario where strictDeps is off and cross-compiled XDG_DATA_DIRS content is brought into the environment. While probably harmless for data like manpages and completion scripts, this would cause issues when XDG_DATA_DIRS is used to find executables or plugins. The Qt framework is known to behave like this and might have run into incompatibilities.
By exporting it, we always make the new directories available to subprocesses, regardless of whether the environment variable existed before `nix-shell` was invoked.
roberth
force-pushed
the
stdenv-xdg-data-dirs
branch
from
50b736d
to
be76099
Compare
|
We don't do this for UPDATE: ofborg fails for other reason; seems to want to build too much |
roberth
changed the title
|
@GrahamcOfBorg eval |
BTW I just updated those to use |
It looks like bash automatically finds the right completion scripts by looking into XDG_DATA_DIRS. Probably other programs respecting XDG specs can also benefit from setting that environment variable. Thanks to NixOS/nixpkgs#103501 for finding that gem. I don't understand how this works, the Bash source code doesn't mention XDG at all. Maybe it's a NixOS-specific thing.
The bash-completions package automatically finds the right completion scripts by looking into XDG_DATA_DIRS. Probably other programs respecting XDG specs can also benefit from setting that environment variable. Thanks to NixOS/nixpkgs#103501 for finding that gem.
The bash-completions package automatically finds the right completion scripts by looking into XDG_DATA_DIRS. Probably other programs respecting XDG specs can also benefit from setting that environment variable. Thanks to NixOS/nixpkgs#103501 for finding that gem.
|
Any objections to testing this on staging? |
Use "$out/share/bash-completion/completions" because it gets automatically loaded by the bash-completions package if "$out/share" is listed in the XDG_DATA_DIRS environment variable. This combined with NixOS/nixpkgs#103501 will mean that nix-shell environments can have completion available.
We can use nativeBuildInputs now, since NixOS/nixpkgs#103501
Use "$out/share/bash-completion/completions" because it gets automatically loaded by the bash-completions package if "$out/share" is listed in the XDG_DATA_DIRS environment variable. This combined with NixOS/nixpkgs#103501 will mean that nix-shell environments can have completion available.
The bash-completions package automatically finds the right completion scripts by looking into XDG_DATA_DIRS. Probably other programs respecting XDG specs can also benefit from setting that environment variable. Thanks to NixOS/nixpkgs#103501 for finding that gem.
Motivation for this change
Add bash completion support for packages in nix-shells.
Closes #26031
XDG_DATA_DIRS is to /share as PATH is to /bin.
It was defined as part of the XDG basedir specification.
https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html
While it originated from the X Desktop Group, it is not limited to
the X11 ecosystem, as evidenced by its use in bash-completion.
TODO
either of which works with bash completion because strictDeps is disabledThings done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)