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

Handle use-xdg-base-directories for profile link #7929

Closed
wants to merge 1 commit into from

Conversation

balsoft
Copy link
Member

@balsoft balsoft commented Mar 1, 2023

Motivation

#5588 broke the PATH setting logic in profile.sh: #5588 (comment)

This PR attempts to fix that by checking the use-xdg-base-directories config option and deciding whether to use the XDG-compliant location or the old one based on that.

Context

#5588 introduced a use-xdg-base-directories option which, when enabled, makes Nix use the new locations instead of ~/.nix-profile, ~/.nix-channel and ~/.nix-defexpr.
However, the accompanying profile.sh change was not quite correct: it chose the new location if the old one didn't exist.
Checking the use-xdg-base-directories should enable us to handle this properly (only adding the relevant profile link to $PATH).

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
  • agreed on implementation strategy
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests - tests/nixos/*
  • documentation in the manual
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or incompatible change: updated release notes

printf "$warning Profiles do not match. You should manually migrate from %s to %s.\n" "$NIX_LINK" "$NIX_LINK_NEW" 1>&2
fi
NIX_LINK="$HOME/.nix-profile"
if [[ $(PATH="$PATH:@localstatedir@/nix/profiles/default/bin" nix show-config use-xdg-base-directories) == true ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

Ideally we shouldn't call nix (or any non-shell builtin) inside profile scripts since it slows down every shell invocation...

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, true. Each invocation of nix show-config use-xdg-base-directories takes about 17 ms, which is quite a lot :(

Copy link
Member Author

@balsoft balsoft Mar 1, 2023

Choose a reason for hiding this comment

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

Would it be an acceptable compromise to just add both locations to $PATH then?

Copy link
Member

Choose a reason for hiding this comment

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

I guess 17 ms is fine for login shells. That's probably preferrable over polluting $PATH with non-existent paths.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think it is fine for login shells.

Copy link
Member

@cole-h cole-h left a comment

Choose a reason for hiding this comment

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

I'm slightly meh on the idea of needing to invoke nix on shell startup (even if it's "only" for login shells), but if this is decided as the accepted solution, we need to make sure to pass --extra-experimental-features (or use a legacy CLI invocation, if one exists...?).

scripts/nix-profile-daemon.sh.in Outdated Show resolved Hide resolved
scripts/nix-profile.sh.in Outdated Show resolved Hide resolved
@fricklerhandwerk fricklerhandwerk added the regression Something doesn't work anymore label Mar 3, 2023
@Ericson2314 Ericson2314 removed the regression Something doesn't work anymore label Mar 31, 2023
@roberth roberth added the profiles Versioned gc root symlinks; nix profile, nix-env label Mar 31, 2023
@fricklerhandwerk
Copy link
Contributor

Triaged in the Nix team meeting 2023-03-31:

  • @Ericson2314: we need a plan to make XDG the default
    • it's a lot of work maintaining both
  • 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-03-31-nix-team-meeting-minutes-45/27002/1

@fricklerhandwerk
Copy link
Contributor

Discussed in the Nix team meeting 2023-04-03:

  • The team agrees we don't want to invoke Nix to check the config option, but no other decision was recorded in the meeting notes

  • @Ericson2314 thinks the full solution will be getting rid of this config option altogether

    • then we are clearly going towards one version (e.g. check per XDG dir if they exist, or neither exists)
    • concretely, this would mean that just checking the state of the filesystem is actually correct, rather than mostly correct.

@fricklerhandwerk fricklerhandwerk added the regression Something doesn't work anymore label Apr 5, 2023
@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-04-03-nix-team-meeting-minutes-46/27008/1

@balsoft balsoft mentioned this pull request May 23, 2023
12 tasks
@thufschmitt
Copy link
Member

Re-discussed during the Nix team meeting. Since this isn't urgent any more (a less-correct but acceptable fix having being merged) @Ericson2314 will try an alternative approach that doesn't involve calling nix (with --extra-experimental-features 😱 ) in the init script. Closing that one in the meantime.

Discussion log
Not that urgent since another fix for the original issue landed
    That one is more principled, though
    But involves calling Nix in the shell startup script, which is meh
    @regnat: Also uses xp features in the init script, which is a no-go
    @ericson2314: We could make the usage of XDG dirs dependant on whether the corresponding files exist or not, which would make the current (master) solution totally correct, and would sidestep the problem altogether
        @ericson2314 will try doing that
        Will also make sure the default value indicates this.
    Will close that PR in the meantime

@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-07-24-nix-team-meeting-minutes-74/31116/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
profiles Versioned gc root symlinks; nix profile, nix-env regression Something doesn't work anymore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants