[staging] setup.sh: Support XDG_DATA_DIRS (bash completion in nix-shell) by roberth · Pull Request #103501 · NixOS/nixpkgs

roberth · 2020-11-11T21:36:58Z

Motivation for this change

Add bash completion support for packages in nix-shells.

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

one more test locally
check buildInputs vs nativeBuildInputs behavior (not expecting any surprises though)
- ~~either of which works with bash completion because strictDeps is disabled~~
- buildInputs should not be loaded because those will be for the wrong system
test a significant portion of Nixpkgs builds
- gnome3.eog
- konsole

Things done

pkgs/stdenv/generic/setup.sh

matthewbauer · 2020-11-12T21:54:48Z

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.

matthewbauer · 2020-11-12T22:02:07Z

pkgs/stdenv/generic/setup.sh

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.

Since it's not exported, this might not be as much of a problem actually.

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.

roberth · 2020-11-13T00:30:36Z

Ok, XDG_DATA_DIRS now follows strictDeps-like behavior. No mixing with cross-compilation outputs will occur.
(If some hypothetical build saves XDG_DATA_DIRS, that's equivalent to saving the builder's PATH; just one out of a thousand potential bad build ideas)

Also XDG_DATA_DIRS is now always exported as one will expect it to. Previous behavior was potentially confusing.

Ericson2314

This looks good to me!

worldofpeace · 2020-11-18T23:36:16Z

pkgs/stdenv/generic/setup.sh

Is this going to affect wrapGAppsHook or wrapQtAppsHook in any way?

It shouldn't affect those.
This variable is set in the build environment, not in the wrapper scripts.

cc @NixOS/freedesktop

Oh, so being set in the build environment, but during fixup it shouldn't be?

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?

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.

nixos-discourse · 2020-11-19T00:20:34Z

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

nixos-discourse · 2020-11-19T00:21:19Z

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

jonringer · 2020-11-19T00:44:25Z

My understanding is that only nativeBuildInputs have shell completion from this, however, most examples use buildInputs.

nix manual: https://nixos.org/manual/nix/unstable/command-ref/nix-shell.html
wiki: https://nixos.wiki/wiki/Development_environment_with_nix-shell
wiki / cachix: https://nixos.wiki/wiki/Caching_nix_shell_build_inputs

I don't think this is a blocker, but we should probably update the documentation examples so that people get expected behavior

roberth · 2020-11-19T09:43:08Z

My understanding is that only nativeBuildInputs have shell completion from this, however, most examples use buildInputs.

That is correct. buildInputs shouldn't be loaded on PATH either. That is a legacy behavior.

The idea is that XDG_DATA_DIRS supports apps that run in the "current" context, build in our cross compilation terminology. buildInputs can, in general, only run in the host context. Loading these by default will cause issues with cross compilation.

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 XDG_DATA_DIRS during checkPhase and installCheckPhase.

I don't think this is a blocker, but we should probably update the documentation examples so that people get expected behavior

💯. Should have been done with the introduction of cross compilation.

Ericson2314 · 2020-11-19T16:07:51Z

@jonringer Yeah, we've never found a sane way to flip the strictDeps switch without breaking the world. This PR to me says "fine, but new functionality should be strictDeps-style". I think this is a rather clever way to "nudge not shove" the ecosystem in the right direction --- nothing will be broken, per se, but the incentive to switch will compound over time.

I like it a lot!

jonringer

LGTM

zimbatm · 2020-11-22T09:40:09Z

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.

roberth · 2020-11-22T11:27:45Z

@zimbatm

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?

No black magic, just a shell script in the bash-completion package.

https://github.com/scop/bash-completion/blob/f7475cf2c0d60f690a0bd758e3fa84976017b99a/bash_completion#L2185-L2187

Incidentally, I recently worked on unifying the bash completions outputs: #103421

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 · 2020-11-22T13:22:01Z

Fixed up the first commit to unset the "private" _XDG_DATA_DIRS (note the underscore)
Tested build and execution of
- gnome3.eog
- konsole (KDE)

roberth · 2020-11-22T13:38:46Z

Speaking of testing, on non-cross we should probably change or unset XDG_DATA_DIRS during checkPhase and installCheckPhase.

We don't do this for PATH either, so let's keep it simple.
Another reason not to automagically change it for the check phases is that we want the package author to figure out that the data dependency exists, so that they are aware that XDG_DATA_DIRS must be set when the package is used.

UPDATE: ofborg fails for other reason; seems to want to build too much

jonringer · 2020-11-24T14:34:50Z

@GrahamcOfBorg eval

Ericson2314 · 2020-11-24T21:15:03Z

wiki: https://nixos.wiki/wiki/Development_environment_with_nix-shell
wiki / cachix: https://nixos.wiki/wiki/Caching_nix_shell_build_inputs

BTW I just updated those to use nativeBuildInputs where appropriate. Perhaps is a hoop users won't choose to jump through, and that's fine, but at least those docs won't be misleading them.

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.

roberth · 2020-11-25T11:56:31Z

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.

roberth changed the title ~~setup.sh: Support XDG_DATA_DIRS~~ setup.sh: Support XDG_DATA_DIRS (bash completion in nix-shell) Nov 11, 2020

roberth marked this pull request as ready for review November 12, 2020 00:56

roberth requested review from Ericson2314 and matthewbauer as code owners November 12, 2020 00:56

matthewbauer reviewed Nov 12, 2020

View reviewed changes

pkgs/stdenv/generic/setup.sh Outdated Show resolved Hide resolved

matthewbauer reviewed Nov 12, 2020

View reviewed changes

roberth requested a review from matthewbauer November 13, 2020 08:29

roberth mentioned this pull request Nov 13, 2020

Bash completion in nix-shell -p (also solving support for strictDeps by default) NixOS/nix#4254

Open

Ericson2314 approved these changes Nov 15, 2020

View reviewed changes

zowoq mentioned this pull request Nov 18, 2020

sourceInputCompletionHook: init #104225

Closed

10 tasks

worldofpeace reviewed Nov 18, 2020

View reviewed changes

jonringer approved these changes Nov 20, 2020

View reviewed changes

zimbatm mentioned this pull request Nov 22, 2020

support for bash-completions numtide/devshell#48

Merged

roberth added 3 commits November 22, 2020 14:19

setup.sh: export XDG_DATA_DIRS for consistency

be76099

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 November 22, 2020 13:19

roberth changed the title ~~setup.sh: Support XDG_DATA_DIRS (bash completion in nix-shell)~~ [staging] setup.sh: Support XDG_DATA_DIRS (bash completion in nix-shell) Nov 22, 2020

zimbatm mentioned this pull request Dec 10, 2020

fix bash-completion outputs NixOS/cabal2nix#474

Merged

This was referenced Jan 11, 2021

Completion support in nix-shell #26031

Closed

bash autocompletion in nix shell (nix command) NixOS/nix#4453

Open

rvl added a commit to cardano-foundation/cardano-wallet that referenced this pull request Jul 19, 2021

shell.nix: Better way to add bash completions to shell

b2375e8

We can use nativeBuildInputs now, since NixOS/nixpkgs#103501

rvl mentioned this pull request Jul 20, 2021

Misc dev env fixes cardano-foundation/cardano-wallet#2766

Merged

roberth mentioned this pull request Aug 4, 2021

mkShellMinimal: Create an ultra minimal nix-shell #132617

Draft

14 tasks

roberth mentioned this pull request Nov 11, 2021

prepend XDG_{CONFIG,DATA}_DIRS nix-community/lorri#61

Merged

1 task

Uh oh!

Comments

Conversation

roberth commented Nov 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation for this change

Things done

Uh oh!

Uh oh!

matthewbauer commented Nov 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roberth commented Nov 13, 2020

Uh oh!

Ericson2314 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nixos-discourse commented Nov 19, 2020

Uh oh!

nixos-discourse commented Nov 19, 2020

Uh oh!

jonringer commented Nov 19, 2020

Uh oh!

roberth commented Nov 19, 2020

Uh oh!

Ericson2314 commented Nov 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonringer left a comment

Choose a reason for hiding this comment

Uh oh!

zimbatm commented Nov 22, 2020

Uh oh!

roberth commented Nov 22, 2020

Uh oh!

roberth commented Nov 22, 2020

Uh oh!

roberth commented Nov 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonringer commented Nov 24, 2020

Uh oh!

Ericson2314 commented Nov 24, 2020

Uh oh!

roberth commented Nov 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

roberth commented Nov 11, 2020 •

edited

Loading

Ericson2314 commented Nov 19, 2020 •

edited

Loading

roberth commented Nov 22, 2020 •

edited

Loading