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

[#201] Deduce profile directory during activation #231

Merged
merged 1 commit into from
Sep 12, 2023

Conversation

rvem
Copy link
Member

@rvem rvem commented Sep 6, 2023

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

@rvem rvem requested a review from PhilTaken September 6, 2023 13:39
Copy link
Collaborator

@PhilTaken PhilTaken left a 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,
Copy link
Collaborator

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

@rvem rvem force-pushed the rvem/#201-dont-hardcode-profile-directory branch 2 times, most recently from 24de285 to 44b1b90 Compare September 11, 2023 09:39
@rvem
Copy link
Member Author

rvem commented Sep 11, 2023

@PhilTaken, I updated root user profiles handling a bit to match the Nix documentation, could you please re-review?

Copy link
Member

@Sereja313 Sereja313 left a 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()
Copy link
Collaborator

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()?

Copy link
Collaborator

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

Copy link
Member Author

@rvem rvem Sep 12, 2023

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

Copy link
Member Author

@rvem rvem Sep 12, 2023

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

Copy link
Member Author

@rvem rvem Sep 12, 2023

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

Copy link
Collaborator

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.
@rvem rvem force-pushed the rvem/#201-dont-hardcode-profile-directory branch from 55ff56d to f26e888 Compare September 12, 2023 10:00
@rvem rvem merged commit 31c32fb into master Sep 12, 2023
1 check passed
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.

Update default profiles location
3 participants