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

fix profile backwards compat #8009

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

thufschmitt
Copy link
Member

Motivation

Fix the backwards-compat hack that was butchered as part of #5226

/cc @rycee @ncfavier

Context

Upon initializing a local or daemon store, create (if it doesn't exist) a link from ${NIX_STATE_DIR}/profiles/per-user/${currentUser} to the actual profiles dir (under the user's XDG state dir).

This is a bit involved for the daemon as

  1. We need the client to pass the path to its profile directory (since the daemon can't guess it)
  2. We don't want the client to be able to impersonate another user as that would give it the ability to create a link from ${NIX_STATE_DIR}/profiles/per-user/${otherUser} to any place in the file system, effectively controlling this other user's environment for anything that doesn't use the new profiles directory.

So the way we do that is that is that:

  1. The userName is taken from the Unix socket used for the connection (if there's one. Otherwise this doesn't make sense any way)
  2. The creation of the symlink is triggered by a dedicated protocol command which also passes the path to the user's profile directory.

Using a custom command just for that is a bit nasty, but that also seems to be the most reasonable solution.

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

Priorities

Add 👍 to pull requests you find important.

Upon initializing a local or daemon store, create (if it doesn't exist)
a link from `${NIX_STATE_DIR}/profiles/per-user/${currentUser}` to the
actual profiles dir (under the user's XDG state dir).

This is a bit involved for the daemon as

1. We need the client to pass the path to its profile directory (since
   the daemon can't guess it)
2. We don't want the client to be able to impersonate another user as
   that would give it the ability to create a link from
   `${NIX_STATE_DIR}/profiles/per-user/${otherUser}` to any place in the
   file system, effectively controlling this other user's environment
   for anything that doesn't use the new profiles directory.

So the way we do that is that is that
1. The `userName` is taken from the Unix socket used for the connection
   (if there's one. Otherwise this doesn't make sense any way)
2. The creation of the symlink is triggered by a dedicated protocol
   command which also passes the path to the user's profile directory.

Using a custom command just for that is a bit nasty, but that also seems
to be the most reasonable solution.
@edolstra
Copy link
Member

edolstra commented Mar 8, 2023

Hm, it seems to me that this introduces a lot of complexity, and the daemon is back to creating stuff on behalf of the user. So we're basically back to where we started, only with an additional protocol call. So maybe it's better to revert #5226 after all...

@Ericson2314
Copy link
Member

I still think we should just change the default so:

  1. ${NIX_STATE_DIR}/profiles/per-user/${otherUser} is preferred, and will be created if it doesn't exist
  2. ${XDG_STATE_HOME}/profiles/ is only used if
    • it exists
    • ${NIX_STATE_DIR}/profiles/per-user/${otherUser} doesn't exist

This is a "soft revert" that will allow us to continue fixing issues with the home directory channels while not effecting non-adventurous users.


Or, just combine with use-xdg-base-directories and make it experimental.

@thufschmitt
Copy link
Member Author

we're basically back to where we started

Not exactly, because the daemon doesn't have to impersonate the user. But yeah, this adds a lot of complexity indeed. We could possibly avoid the extra protocol call by bundling that into sendOption or a similar call, although that feels a bit hacky

@Ericson2314 that's not really feasible (not with an equivalent level of complexity at least) because that still means that the client must send it's profile directory to the daemon if it wants it to be used.

@rycee
Copy link
Member

rycee commented Mar 8, 2023

FWIW Home Manager now should work OK with Nix 2.14 (see nix-community/home-manager#3742). So the new behavior is not a blocker for us.

The main issue for me, is that it is unclear whether nix-collect-garbage will handle these new profile locations (when using --delete-old or --delete-older-than) and I think it would be pedagogically troublesome to force the user to use a HM specific command to remove old profiles. We do have home-manager remove-generations ID... and home-manager expire-generations TIMESTAMP but I imagine quite a few people rely on nix-collect-garbage to also work with the HM profile.

The second main issue is that the documentation still seems to refer to the per-user directory in a number of places, which I think would be out-of-date now? And I really would have liked to see a description of this change in the release notes. If you add a release note, personally I would like to see a mention of whether the ~/.nix-profile link will stay or the intention is to remove that as well and also a mention of whether third party tools should use their own profile directory or they should put their profiles inside ~/.local/state/nix/profiles.

@Ericson2314
Copy link
Member

@thufschmitt Sorry, I completely missed that the original PR didn't continue to support the old location on an ongoing basis, but instead migrated it to the new location (though I don't fully understand how it does that yet).

@thufschmitt
Copy link
Member Author

Sorry, I completely missed that the original PR didn't continue to support the old location on an ongoing basis, but instead migrated it to the new location (though I don't fully understand how it does that yet).

It doesn't. The logic is mostly the same as the old one (well, the one of getDefaultProfile, there were a few places doing things differently), namely that it looks at the target of ~/.nix-profile. The only change is what happens if it doesn't exist.

The main issue for me, is that it is unclear whether nix-collect-garbage will handle these new profile locations (when using --delete-old or --delete-older-than)

After #7993 it will. And since this PR makes the old profile directory a symlink to the new one, both should be usable interchangeably.

@thufschmitt
Copy link
Member Author

@Ericson2314 that's not really feasible (not with an equivalent level of complexity at least) because that still means that the client must send it's profile directory to the daemon if it wants it to be used.

Just expanding a bit on that: The problem with using the client-side profile only if it exists is that for this to be useful at all, the daemon must know whether it exists or not to know whether it should create the one under /nix/var or not. Which means that the client must send it (or at least send a bit telling whether it exists or not). So the level of complexity is similar to the one here I think.

@Ericson2314
Copy link
Member

@thufschmitt

After #7993 it will. And since this PR makes the old profile directory a symlink to the new one, both should be usable interchangeably.

Err but @rycee made it use ${XDG_STATE_HOME:-$HOME/.local/state}/home-manager/profiles. not .../nix/profiles. I don't think that will work with nix-collect-garbage and that PR, no?

@thufschmitt
Copy link
Member Author

@thufschmitt

After #7993 it will. And since this PR makes the old profile directory a symlink to the new one, both should be usable interchangeably.

Err but @rycee made it use ${XDG_STATE_HOME:-$HOME/.local/state}/home-manager/profiles. not .../nix/profiles. I don't think that will work with nix-collect-garbage and that PR, no?

I'll have to double-check (just have 1min between two meetings right now), but I think this will work just fine since ~/.nix-profile will point to ${XDG_STATE_HOME:-$HOME/.local/state}/home-manager/profiles

@rycee
Copy link
Member

rycee commented Mar 9, 2023

@thufschmitt Ah, I see the confusion now. Unfortunately, Home Manager does not completely take over the user's profile since we still want the user to be able to use the regular nix-env -i or nix profile install commands. So, instead Home Manager creates a separate profile and then does nix-env -i ${hm-built-profile} in that generation's activation script. See the code.

So, ~/.nix-profile will still point to the user's default profile and even if some default profile generation gets garbage collected, there is still a (more or less) corresponding Home Manager generation that is still a GC root.

Ideally, it would be nice if it were possible to set up HM to have its profile show up in NIX_PROFILES or something so we wouldn't have to do this installation into the default profile at all. But I haven't investigated this alternative in any depth.

@Ericson2314
Copy link
Member

@rycee Yes what you say matched my understanding. Absent a new way to tell nix-collect-garbage where to look (indirect profile roots? indirect weak roots?) I think you will want to move from ${XDG_STATE_HOME:-$HOME/.local/state}/home-manager/profiles to ${XDG_STATE_HOME:-$HOME/.local/state}/nix/profiles.


I am leaning towards not having a backwards compat trick but either one of:

  1. Just continue fixing bugs, "rip off the band-aid".
  2. Support the old and new way side by side, but completely separately, with a configuration option switching between them.

@Ericson2314
Copy link
Member

Discussed in the Nix team Meeting:

  • @edolstra: Does the new daemon protocol operation help? Old clients don't know about it
  • no indirect roots when /nix/var/nix/profiles origianlly concieved
  • @Ericson2314: Likes ripping off the band-aid: hard
  • new behavior? written/release-notes
  • @thufschmitt: Will document the new state of things

@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-10-nix-team-meeting-minutes-39/26279/1

Comment on lines +311 to +317
void createLegacyProfileLink(Path oldProfile, Path newProfile)
{
if (pathExists(oldProfile))
return;
createDirs(dirOf(oldProfile));
replaceSymlink(newProfile, oldProfile);
}
Copy link
Member

Choose a reason for hiding this comment

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

Make naming consistent with the rest of PR, and less confusing

Suggested change
void createLegacyProfileLink(Path oldProfile, Path newProfile)
{
if (pathExists(oldProfile))
return;
createDirs(dirOf(oldProfile));
replaceSymlink(newProfile, oldProfile);
}
void createLegacyProfilesLink(Path oldProfilesDir, Path newProfilesDir)
{
if (pathExists(oldProfilesDir))
return;
createDirs(dirOf(oldProfilesDir));
replaceSymlink(newProfilesDir, oldProfilesDir);
}

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/tweag-nix-dev-update-46/26872/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants