-
-
Notifications
You must be signed in to change notification settings - Fork 13.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[staging] setup.sh: Support XDG_DATA_DIRS (bash completion in nix-shell) #103501
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it's not exported, this might not be as much of a problem actually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me!
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this going to affect wrapGAppsHook or wrapQtAppsHook in any way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @NixOS/freedesktop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, so being set in the build environment, but during fixup it shouldn't be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 grep
ing 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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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.
50b736d
to
be76099
Compare
|
We don't do this for UPDATE: ofborg fails for other reason; seems to want to build too much |
@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
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)