-
Notifications
You must be signed in to change notification settings - Fork 101
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
[#201] Deduce profile directory during activation #231
Conversation
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
@@ -157,7 +174,7 @@ fn test_wait_command_builder() { | |||
struct RevokeCommandData<'a> { | |||
sudo: &'a Option<String>, | |||
closure: &'a str, | |||
profile_path: &'a str, |
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.
Love expressive types, glad this is gone
24de285
to
44b1b90
Compare
@PhilTaken, I updated |
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 didnt notice anything suspicious
} else { | ||
// https://github.com/NixOS/nix/blob/2.17.0/src/libstore/profiles.cc#L308 | ||
let state_dir = env::var("XDG_STATE_HOME").or_else(|_| { | ||
dirs::home_dir() |
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.
why not just use dirs::state_dir()
?
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.
also, all of the Paths in this function are handled as strings, I think explicit Path types would be preferable
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.
also, all of the Paths in this function are handled as strings, I think explicit Path types would be preferable
Agree, but looks like there is much more than just a profile path expressed as a String
(e.g. closure
thing), so I'd rather do this in a separate PR
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.
why not just use
dirs::state_dir()
?
Nice catch. However, AFAICS, it will return None
on macOS, while nix
will fallback to $HOME/.local/state
on both Linux and macOS. I think it's better to mock the nix behaviour here and avoid using state_dir
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.
Another possible issue with macOS is that 😕sudo
there doesn't change the HOME
directory
So non-root profiles on macOS deployment sounds like a huge PITA
Please disregard, this is only the case for the sudoing the root
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.
Nice catch. However, AFAICS, it will return None on macOS, while nix will fallback to $HOME/.local/state on both Linux and macOS. I think it's better to mock the nix behaviour here and avoid using state_dir
good point, lgtm then :)
Problem: Since NixOS/nix#5226 nix profiles for users are stored in 'XDG_STATE_HOME' or 'HOME' directory. However, 'deploy-rs' still expects profiles to be present in '/nix/var/nix/profiles/per-user'. As a result, an attempt to deploy a profile with newer nix may fail with an error about non-existing files. Solution: Instead of deducing the profile path prior to ssh'ing and actual activation, deduce the path to the profile during as a part of 'activate-rs' invocation. Now if the profile path is not specified explicitly as an attribute in profile within the deploy flake, the path to the profile is determined based on the user to which the profile belongs and on the values of 'XDG_STATE_HOME' and 'HOME' variables. Additionally, if the old profile directory (in '/nix/var/nix/profiles/per-user') for a given user already exists, it is used instead for the sake of backward compatibility.
55ff56d
to
f26e888
Compare
Problem: Since NixOS/nix#5226 nix profiles for users are stored in 'XDG_STATE_HOME' or 'HOME' directory. However, 'deploy-rs' still expects profiles to be present in '/nix/var/nix/profiles/per-user'. As a result, an attempt to deploy a profile with newer nix may fail with an error about non-existing files.
Solution: Instead of deducing the profile path prior to ssh'ing and actual activation, deduce the path to the profile as a part of 'activate-rs' invocation.
Now if the profile path is not specified explicitly as an attribute in profile within the deploy flake, the path to the profile is determined based on the user to which the profile belongs and on the values of 'XDG_STATE_HOME' and 'HOME' variables.
Additionally, if the old profile directory (in
'/nix/var/nix/profiles/per-user') for a given user already exists, it is used instead for the sake of backward compatibility.
Resolves #201