Skip to content
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

Populate $XDG_DATA_DIRS with appropriate folder from Nix profile #8985

Merged
merged 3 commits into from
Nov 6, 2023

Conversation

Hoverbear
Copy link
Contributor

@Hoverbear Hoverbear commented Sep 15, 2023

Motivation

On non-NixOS systems, the default nix install does not populate the $XDG_DATA_DIRS.

This populates it and enables things like bash-completion and .desktop file detection for nix profile installed packages.

With this PR:

  • Bash completions like nix r<tab> and nix build .#pro<tab>, or nix profile add nixpkgs#ripgrep then rg -<tab> should show the expected output.
  • nix profile install nixpkgs#firefox should result in firefox appearing in the user's Desktop environment menus via the .desktop file. (This may require a relog in some environments)

This PR sets XDG_DATA_DIRS appropriately in currently supported shells, ensuring to include the spec-defined defaults should the variable not exist.

This variable is a PATH style variable which is : separated, with a spec defined default of /usr/local/share:/usr/share.

I do not believe the implementation in this PR risks clobbering existing system settings or breaking things for users. We add the nix provided directories to the end of the XDG_DATA_DIRS, making them the lowest priority, meaning we won't break anything already existing on the user's system. We ensure the spec-defined default is set, meaning we won't break the system if the value is unset. Nix already manipulates the PATH variable and this PR manipulates XDG_DATA_DIRS in roughly the same way.

Testing:

I have tested this on:

  • Ubuntu 22.04 multi-user install
  • Ubuntu 22.04 single-user install
  • The full nix repo's installer test suite (via nix --extra-experimental-features 'nix-command flakes' run .#hydraJobs.installTests.x86_64-linux -L -j 8)

If you'd like to test, you can add the line yourself to the file /nix/var/nix/profiles/default/etc/profile.d/nix-daemon.sh, or build the appropriate github:nixpkgs/nix#hydraJobs.binaryTarball.$SYSTEM output and try it on a VM.

Doing that should result in new shells being able to complete nix r<tab> like so:

ana@ephemeral-ubuntu:~$ nix r<tab>
realisation  registry     repl         run

Or nix build github:nixos/nix#hydraJobs.<tab> (you may need to be patient here -- takes a sec to do the network):

ana@ephemeral-ubuntu:~$ nix build github:nixos/nix#hydraJobs.<tab>
nixos/nix#hydraJobs.binaryTarball          nixos/nix#hydraJobs.installerScript
nixos/nix#hydraJobs.binaryTarballCross     nixos/nix#hydraJobs.installerScriptForGHA
nixos/nix#hydraJobs.build                  nixos/nix#hydraJobs.installerTests
nixos/nix#hydraJobs.buildCross             nixos/nix#hydraJobs.installTests
nixos/nix#hydraJobs.buildNoGc              nixos/nix#hydraJobs.internal-api-docs
nixos/nix#hydraJobs.buildNoTests           nixos/nix#hydraJobs.metrics
nixos/nix#hydraJobs.buildStatic            nixos/nix#hydraJobs.perlBindings
nixos/nix#hydraJobs.coverage               nixos/nix#hydraJobs.tests
nixos/nix#hydraJobs.dockerImage

You can also test .desktop files:

ana@ephemeral-ubuntu:~$ nix profile install nixpkgs#xfce.thunar

Reload your DE (eg, relog) and observe:

image

Context

Previously, #2443 suggested this change, however it was years ago and initially intermingled with another man related change (not present here). In #2443 (comment) @edolstra offered some insight into why this was not merged, it was focused on the MANPATH related changes.

In NixOS/nixpkgs#78822, this issue was once again surfaced and referenced #2443 (comment).

In DeterminateSystems/nix-installer#614, it was suggested that this be done in order to allow users to do something like nix profile install nixpkgs#blender and see blender in their desktop environment's menus.

In DeterminateSystems/nix-installer#486, it was suggested that this be done in order to enable shell completion on an installed Nix.

In #6098, someone mentioned a gotcha around setting this value such that you don't overwrite the XDG spec defined defaults. (Spec here: https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html)

In #4453 someone requested this feature as well, offering an alternative of doing this only inside of a nix shell, but that was a more complicated solution.

Despite the PR from several years ago not being merged, I believe it's worth revisiting the discussion in light of the numerous related issues as well as rather substantial potential benefits it can bring to users.

Priorities

Add 👍 to pull requests you find important.

On non-NixOS systems, the default `nix` install does not populate the
`$XDG_DATA_DIRS`. This populates it and enables things like bash-completion
and `.desktop` file detection for `nix` profile installed packages.

Signed-off-by: Ana Hobden <operator@hoverbear.org>
@Hoverbear Hoverbear changed the title Populate $XDG_DATA_DIRS with appropriate folder from Nix profile Populate $XDG_DATA_DIRS with appropriate folder from Nix profile Sep 15, 2023
scripts/nix-profile.fish.in Outdated Show resolved Hide resolved
@fricklerhandwerk
Copy link
Contributor

Triaged in Nix team meeting:

  • @edolstra: ideally we'd split the part responsible for Nix completions from stuff that's installed in profiles
    • @fricklerhandwerk: since all declarative frameworks do that kind of things on top, changes made by Nix should be minimal and only do the completions part
    • @roberth: agreed since the package management aspect is unbounded, Nix should not take care of that aspect
      • otherwise we'd be hardcoding a part of the problem domain we should not
      • propose to add a generic mechanism for packages to export environments
  • @thufschmitt: another option is for the installer to put things in the default locations
    • @edolstra: we probably don't want this and mess with the existing system as little as possible
      • could install Nix into its own profile
  • @edolstra: the real risk here is the environment being polluted by stuff in your profile
    • @Ericson2314: even Home Manager does not override these scripts
      • the policy introduced by these low-level mechanisms is forboding
      • we rather want very small, reliable foundations
      • adding more things there is not very sustainable
      • we should do either a Home-Manager-like thing or an ad hoc profile thing, not both
  • @fricklerhandwerk: these are two separate issues to discuss:
    • should we focus on profiles to be first-class citizens xor hand off that authority to other tools?
    • how should Nix install its own things into the host system?
  • to discuss

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2023-09-22-nix-team-meeting-minutes-88/33343/1

@kolloch
Copy link
Contributor

kolloch commented Nov 4, 2023

Note that in my own fix I also needed to extend the fpath for zsh:

https://gitlab.com/nexxiot-labs/nix-bootstrap/-/blob/main/src/install.sh#L96-126

Copy link
Contributor

@fricklerhandwerk fricklerhandwerk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about it a bit more, the direction of this change seems perfectly fine to me. We may have went a bit astray in the triage discussion:

  1. @edolstra How Nix deploys its own completions is somewhat of an orthogonal question here.

    This PR is aiming to deploy things provided by packages, and opens one possibility for Nix to plug itself in the same way.

  2. @roberth How packages expose environments is also orthogonal.

    While offering a minimal (or even no dedicated) interface in Nix to coordinate with however Nixpkgs developers want it to work would be good, it doesn't matter right here. Software builds put things into their share output directory, profiles merge those together, so exposing those where one would expect them is appropriate.

  3. My own concern about Nix being too heavyweight on package management and that I find profiles somewhat awkward is also unrelated to this change.

    Profiles exist, nix profile is still experimental so we can still change some of that, the goal to have some imperative interface to declarative configurations seems not to be contested by anyone (and @bobvanderlinden and @iFreilicht have ideas on how to actually implement that which I would subscribe to), and the problems I see are not in the outward interface of wiring up environment variables.

This PR appears to me like a strict improvement, especially because it solves an obvious end-user problem and opens up more options to solve deeper technical issues, and does not preclude making decisions in the future.

scripts/nix-profile-daemon.fish.in Outdated Show resolved Hide resolved
@iFreilicht
Copy link
Contributor

This PR appears to me like a strict improvement, especially because it solves an obvious end-user problem and opens up more options to solve deeper technical issues, and does not preclude making decisions in the future.

100% agree. This PR solves a huge pain that actually made me not use Nix for any GUI programs at all on my Linux installs, because this basic integration is broken.

I was not aware of this PR but support its merge wholeheartedly. Will happily test it on my own Manjaro and Parabola installations later today.

@fricklerhandwerk fricklerhandwerk self-assigned this Nov 6, 2023
@fricklerhandwerk fricklerhandwerk added the idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. label Nov 6, 2023
@fricklerhandwerk
Copy link
Contributor

Discussed in Nix team meeting:

  • @edolstra: the main objection is that we could be setting any of literally hundreds of environment variables
  • @Ericson2314: we brought up that packages should do more of that
  • (some consideration of Home Manager, which does something similar)
  • we agree we want the general architecture to be better, but that issue is unrelated to this particular change
  • @Ericson2314: it's not at the same level of involvement as changing nix-shell, since this is only how the installer places Nix on the host
  • @fricklerhandwerk: and it doesn't preclude re-negotiating how Nix presents package builds to the surrounding environment
  • @thufschmitt: incidentally even NixOS sets the variable to the same value; every Nix consumer seems to be setting it
    • @edolstra: this is because on NixOS we're in charge of the environment and don't have to worry messing up the host distribution
  • decision: idea approved, assigend to @fricklerhandwerk

Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>
@fricklerhandwerk fricklerhandwerk merged commit 150b5ab into NixOS:master Nov 6, 2023
8 checks passed
@bjornfor
Copy link
Contributor

bjornfor commented Nov 6, 2023

Thanks! This will be a productivity boost for all Nix on non-NixOS users! 🚀

iFreilicht added a commit to iFreilicht/nix that referenced this pull request Nov 6, 2023
@iFreilicht
Copy link
Contributor

So, I've tested this now, and while it seems to not require a re-installation as I initially assumed, the change does have an undesirable side-effect on my machine:

$ nix --version
nix (Nix) 2.17.0
$ echo $XDG_DATA_DIRS
/home/felix/.local/share/flatpak/exports/share:/var/lib/flatpak/exports/share:/usr/local/share:/usr/share:/var/lib/snapd/desktop
$ nix build --print-out-paths
/nix/store/38apmbr5j9z950m83yzqfsm18f39w38i-nix-2.19.0pre20231106_150b5ab
$ echo '{ x86_64-linux = "/nix/store/38apmbr5j9z950m83yzqfsm18f39w38i-nix-2.19.0pre20231106_150b5ab"; }' > x.nix
$ sudo nix upgrade-nix --nix-store-paths-url file:///home/felix/repos-new/nix/x.nix
[sudo] password for felix: 
replacing old 'nix-2.17.0'
installing 'nix-2.19.0pre20231106_150b5ab'

Open a new terminal:

echo $XDG_DATA_DIRS
/usr/local/share:/usr/share:/home/felix/.nix-profile/share:/nix/var/nix/profiles/default/share

And while the reference to .nix-profile/share is now there, the references to snap's and flatpak's share are gone.

I'll have to find out why this is happening exactly. This might very well be something about my local setup, but either way, it's suboptimal (and unexpected) that this can happen at all. Shell is zsh.

@fricklerhandwerk
Copy link
Contributor

fricklerhandwerk commented Nov 6, 2023

Wait, how does nix upgrade-nix manipulate the shell initialisation? Supposedly something would need to touch your nix-profile.sh.

iFreilicht added a commit to iFreilicht/nix that referenced this pull request Nov 6, 2023
iFreilicht added a commit to iFreilicht/nix that referenced this pull request Nov 6, 2023
The [POSIX test manpage](https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html)
as well as the [fish test manpage](https://fishshell.com/docs/current/cmds/test.html#operators-for-text-strings)
specify that `-z` will be "True if the length of string string is zero;
otherwise, false."

The `-n` was likely a mixup and not caught during testing of
NixOS#8985 due to a lack of missing
conflicting entries in `XDG_DATA_DIRS`.
@iFreilicht
Copy link
Contributor

Ah, I added this line to my user's .zshrc as a workaround for the macOS update issue, but as my dotfiles are synced via git, it ended up on my Linux machines as well:

[[ -e "/nix/var/nix/profiles/default/etc/profile.d/nix-daemon.sh" ]] && source "/nix/var/nix/profiles/default/etc/profile.d/nix-daemon.sh"

Either way, the issue I found was real and I already fixed it, so I'll open a PR for that shortly.

fricklerhandwerk pushed a commit that referenced this pull request Nov 7, 2023
The [POSIX test manpage](https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html)
as well as the [fish test manpage](https://fishshell.com/docs/current/cmds/test.html#operators-for-text-strings)
specify that `-z` will be "True if the length of string string is zero;
otherwise, false."

The `-n` was likely a mixup and not caught during testing of
#8985 due to a lack of missing
conflicting entries in `XDG_DATA_DIRS`.
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/nix-installer-workgroup/21495/27

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2023-11-06-nix-team-meeting-minutes-101/35247/1

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/should-nix-shell-make-bash-completions-available-for-the-package/39383/2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants