various: allow specifying profiles for platform commands #415
Conversation
Signed-off-by: NotAShelf <[email protected]> Change-Id: I6a6a6964ededbfb54041c1c3e7c98467d08b5fd3
WalkthroughAdds a new optional --profile/-P option to OS, Home, and Darwin rebuild/rollback commands with CLI-time symlink validation, threads the provided profile through build/rollback/diff logic, adjusts mutex poison handling and sudo/env formatting in command helpers, and updates nix invocations to propagate env and wrap errors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as CLI Parser
participant Cmd as Rebuild/Rollback Logic
participant Validator as Symlink Validator
participant Nix as Nix Runner
User->>CLI: Invoke command [--profile /path]?
CLI->>Validator: validate --profile (symlink exists)
alt valid profile provided
Validator-->>CLI: Ok(profile PathBuf)
else invalid/missing provided profile
Validator-->>CLI: Err -> CLI returns error
end
CLI->>Cmd: pass args (profile: Option<PathBuf>)
Cmd->>Cmd: derive system_profile_path (provided or default)
Cmd->>Nix: build/run with ["build","--no-link","--profile", <system_profile_path>]
Note right of Nix: env formatted as key=value\n.with_required_env() → .run()\nerrors wrapped with context
Nix-->>Cmd: result
Cmd-->>User: outcome / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks (2 passed, 1 inconclusive)❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches
🧪 Generate unit tests
Comment |
NotAShelf
force-pushed
the
notashelf/push-lmlmokouvzvy
branch
from
0202446 to
814d921
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/commands.rs (1)
494-505: Honor NH_PRESERVE_ENV in remote-elevation env propagation (parity with local).Remote sudo path currently preserves envs unconditionally, diverging from build_sudo_cmd. Align behavior to avoid leaking envs when NH_PRESERVE_ENV=0.
- // Add env command to handle environment variables + // Add env command to handle environment variables elev_cmd = elev_cmd.arg("env"); + let preserve_env = std::env::var("NH_PRESERVE_ENV") + .as_deref() + .map(|x| !matches!(x, "0")) + .unwrap_or(true); for (key, action) in &self.env_vars { match action { EnvAction::Set(value) => { elev_cmd = elev_cmd.arg(format!("{key}={value}")); }, - EnvAction::Preserve => { - if let Ok(value) = std::env::var(key) { - elev_cmd = elev_cmd.arg(format!("{key}={value}")); - } - }, + EnvAction::Preserve if preserve_env => { + if let Ok(value) = std::env::var(key) { + elev_cmd = elev_cmd.arg(format!("{key}={value}")); + } + } _ => {}, } }src/nixos.rs (2)
399-418: Rollback ignores custom --profile when selecting generations.find_previous_generation/find_generation_by_number still scan the default profiles dir, then you later write to a possibly different profile_dir. This can select a non-existent generation for the chosen profile, risking a broken system profile symlink.
Minimal fix: derive system_profile_path/profile_dir first, then pass profile_dir into the generation lookups.
- // Find previous generation or specific generation - let target_generation = if let Some(gen_number) = self.to { - find_generation_by_number(gen_number)? - } else { - find_previous_generation()? - }; + // Resolve profile_dir from --profile (or default) first + let system_profile_path = self + .profile + .as_deref() + .unwrap_or_else(|| Path::new(SYSTEM_PROFILE)); + let profile_dir = system_profile_path.parent().unwrap_or_else(|| { + tracing::warn!("system profile has no parent, defaulting to /nix/var/nix/profiles"); + Path::new("/nix/var/nix/profiles") + }); + // Find previous generation or specific generation within that directory + let target_generation = if let Some(gen_number) = self.to { + find_generation_by_number(gen_number, profile_dir)? + } else { + find_previous_generation(profile_dir)? + }; - - // Construct path to the generation - let system_profile_path = self - .profile - .as_deref() - .unwrap_or_else(|| Path::new(SYSTEM_PROFILE)); - let profile_dir = system_profile_path.parent().unwrap_or_else(|| { - tracing::warn!( - "SYSTEM_PROFILE has no parent, defaulting to /nix/var/nix/profiles" - ); - Path::new("/nix/var/nix/profiles") - }); + // Construct path to the generationAnd update current-gen lookup later:
- let current_gen_number = match get_current_generation_number() { + let current_gen_number = match get_current_generation_number(profile_dir) {Additional changes required outside this hunk are provided below.
574-619: Refactor helpers to accept profile_dir (supporting custom --profile).Apply this change outside the modified hunks to keep behavior consistent:
// Replace these signatures and bodies: fn find_previous_generation() -> Result<generations::GenerationInfo> { /* ... */ } fn find_generation_by_number(number: u64) -> Result<generations::GenerationInfo> { /* ... */ } fn get_current_generation_number() -> Result<u64> { /* ... */ } // With: fn find_previous_generation(profile_dir: &Path) -> Result<generations::GenerationInfo> { let mut generations: Vec<_> = std::fs::read_dir(profile_dir)? .filter_map(|e| { e.ok().and_then(|e| { let path = e.path(); path.file_name() .and_then(|n| n.to_str()) .filter(|n| n.starts_with("system-") && n.ends_with("-link")) .and_then(|_| generations::describe(&path)) }) }) .collect(); if generations.is_empty() { bail!("No generations found in {}", profile_dir.display()); } generations.sort_by(|a, b| a.number.parse::<u64>().unwrap_or(0).cmp(&b.number.parse::<u64>().unwrap_or(0))); let current_idx = generations.iter().position(|g| g.current).ok_or_else(|| eyre!("Current generation not found"))?; if current_idx == 0 { bail!("No generation older than the current one exists"); } Ok(generations[current_idx - 1].clone()) } fn find_generation_by_number(number: u64, profile_dir: &Path) -> Result<generations::GenerationInfo> { let generations: Vec<_> = std::fs::read_dir(profile_dir)? .filter_map(|e| { e.ok().and_then(|e| { let path = e.path(); path.file_name() .and_then(|n| n.to_str()) .filter(|n| n.starts_with("system-") && n.ends_with("-link")) .and_then(|_| generations::describe(&path)) }) }) .filter(|g| g.number == number.to_string()) .collect(); if generations.is_empty() { bail!("Generation {} not found in {}", number, profile_dir.display()); } Ok(generations[0].clone()) } fn get_current_generation_number(profile_dir: &Path) -> Result<u64> { let generations: Vec<_> = std::fs::read_dir(profile_dir)? .filter_map(|e| e.ok().and_then(|e| generations::describe(&e.path()))) .collect(); let current = generations.iter().find(|g| g.current).ok_or_else(|| eyre!("Current generation not found"))?; current.number.parse::<u64>().wrap_err("Invalid generation number") }
🧹 Nitpick comments (4)
src/interface.rs (1)
232-235: Add path value hints to improve CLI UX and shell completion.Attach a file-path hint to each new --profile flag so clap-driven shells offer correct completion.
- #[arg(long, short = 'P')] + #[arg(long, short = 'P', value_hint = clap::ValueHint::FilePath)] pub profile: Option<std::path::PathBuf>,Apply to OsRebuildArgs, OsRollbackArgs, HomeRebuildArgs, and DarwinRebuildArgs.
Also applies to: 293-296, 526-529, 639-642
src/commands.rs (1)
299-313: Mask sensitive env values in debug logs.Debug logs currently print full values (incl. NH_*). Safer to redact likely secrets.
- debug!( - "Configured envs: {}", - self - .env_vars - .iter() - .map(|(key, action)| { - match action { - EnvAction::Set(value) => format!("{key}={value}"), - EnvAction::Preserve => format!("{key}=<preserved>"), - EnvAction::Remove => format!("{key}=<removed>"), - } - }) - .collect::<Vec<_>>() - .join(", ") - ); + let redact = |k: &str, v: &str| { + static SUSPICIOUS: &[&str] = &["TOKEN", "PASSWORD", "PASS", "SECRET", "KEY", "CREDENTIAL"]; + if SUSPICIOUS.iter().any(|s| k.contains(s)) { "<redacted>" } else { v } + }; + debug!( + "Configured envs: {}", + self.env_vars.iter().map(|(key, action)| match action { + EnvAction::Set(value) => format!("{key}={}", redact(key, value)), + EnvAction::Preserve => format!("{key}=<preserved>"), + EnvAction::Remove => format!("{key}=<removed>"), + }).collect::<Vec<_>>().join(", ") + );src/darwin.rs (1)
158-165: Create parent directory for custom --profile before nix writes it.If a user-provided profile path’s parent doesn’t exist, nix build will fail. Ensure (or error with context) before invoking nix.
- let profile_path = self.profile.as_ref().map_or_else( - || std::ffi::OsStr::new(SYSTEM_PROFILE), - |p| p.as_os_str(), - ); + let profile_path: std::path::PathBuf = self + .profile + .clone() + .unwrap_or_else(|| std::path::PathBuf::from(SYSTEM_PROFILE)); + if let Some(parent) = profile_path.parent() { + if !parent.exists() { + std::fs::create_dir_all(parent) + .wrap_err("Failed to create parent directory for --profile")?; + } + } Command::new("nix") - .args(["build", "--no-link", "--profile"]) - .arg(profile_path) + .args(["build", "--no-link", "--profile"]) + .arg(&profile_path)src/home.rs (1)
102-123: Don’t silently disable diff when --profile doesn’t exist; warn and fall back.Current behavior sets prev_generation=None if the provided path is missing, skipping diffs. Prefer warning and using default discovery, optionally validating it’s a symlink.
- let prev_generation: Option<PathBuf> = if let Some(ref profile) = - self.profile - { - if profile.exists() { - Some(profile.clone()) - } else { - None - } - } else { - [ - PathBuf::from("/nix/var/nix/profiles/per-user") - .join(env::var("USER").map_err(|_| eyre!("Couldn't get username"))?) - .join("home-manager"), - PathBuf::from( - env::var("HOME").map_err(|_| eyre!("Couldn't get home directory"))?, - ) - .join(".local/state/nix/profiles/home-manager"), - ] - .into_iter() - .find(|next| next.exists()) - }; + let candidates = || -> color_eyre::Result<[PathBuf; 2]> { + Ok([ + PathBuf::from("/nix/var/nix/profiles/per-user") + .join(env::var("USER").map_err(|_| eyre!("Couldn't get username"))?) + .join("home-manager"), + PathBuf::from( + env::var("HOME").map_err(|_| eyre!("Couldn't get home directory"))?, + ) + .join(".local/state/nix/profiles/home-manager"), + ]) + }; + let prev_generation: Option<PathBuf> = if let Some(ref profile) = self.profile { + if profile.exists() { + Some(profile.clone()) + } else { + warn!("--profile '{}' not found; falling back to default detection", profile.display()); + candidates()?.into_iter().find(|p| p.exists()) + } + } else { + candidates()?.into_iter().find(|p| p.exists()) + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/commands.rs(3 hunks)src/darwin.rs(1 hunks)src/home.rs(1 hunks)src/interface.rs(4 hunks)src/nixos.rs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/darwin.rs (1)
src/commands.rs (3)
new(160-171)new(601-609)new(723-732)
src/nixos.rs (1)
src/commands.rs (4)
new(160-171)new(601-609)new(723-732)elevate(175-178)
src/commands.rs (1)
src/checks.rs (1)
new(338-347)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build NH on Linux
- GitHub Check: Test NH on Linux
- GitHub Check: Test NH on Linux
- GitHub Check: Build NH on Linux
🔇 Additional comments (4)
src/commands.rs (2)
25-27: Good: resilient recovery from poisoned password-cache mutex.Switching to PoisonError::into_inner keeps the cache usable after a panic.
Also applies to: 33-35
456-457: Prompt tweak LGTM.More natural sudo prompt with host interpolation.
src/nixos.rs (2)
333-341: Nice: thread --profile path through build/switch.Passing the path as a separate arg is correct and avoids accidental concatenation bugs.
480-489: Follow-ups for the same bug: use the resolved system_profile_path consistently.After parameterizing generation discovery by profile_dir, these call sites are correct for writing the link, but the discovery functions must also use profile_dir (see below).
Would you like me to push a patch PR branch with the refactor wired through all helpers and tests?
Also applies to: 550-558
Signed-off-by: NotAShelf <[email protected]> Change-Id: I6a6a69644cca0780985012b18715ffaec0eb76d5
Signed-off-by: NotAShelf <[email protected]> Change-Id: I6a6a69640fea6789de7fd14d88a130fa0c488933
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/home.rs (1)
3-5: Document intent and XDG nuance for profile path constantsAdd brief docs so future readers know HOME_PROFILE_PATH is a HOME-based fallback and that XDG_STATE_HOME may take precedence elsewhere.
-const USER_PROFILE_PATH: &str = "/nix/var/nix/profiles/per-user"; -const HOME_PROFILE_PATH: &str = ".local/state/nix/profiles/home-manager"; +/// Base dir for per-user profiles (multi-user Nix). +const USER_PROFILE_PATH: &str = "/nix/var/nix/profiles/per-user"; +/// HOME-based fallback path for HM profile; prefer XDG_STATE_HOME when set. +const HOME_PROFILE_PATH: &str = ".local/state/nix/profiles/home-manager";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/darwin.rs(2 hunks)src/home.rs(2 hunks)src/interface.rs(4 hunks)src/nixos.rs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/interface.rs
- src/nixos.rs
- src/darwin.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Test NH on Darwin
- GitHub Check: Test NH on Linux
- GitHub Check: treewide-checks
- GitHub Check: Build NH on Linux
- GitHub Check: Build NH on Darwin
- GitHub Check: Test NH on Darwin
- GitHub Check: Build NH on Darwin
- GitHub Check: Test NH on Linux
- GitHub Check: Build NH on Linux
- GitHub Check: treewide-checks
🔇 Additional comments (1)
src/home.rs (1)
105-131: Drop XDG_STATE_HOME/nix/profiles/home-manager fallback
Home Manager’s profile link remains~/.nix-profileby default and, when Nix’suse-xdg-base-directoriesis enabled, becomes$XDG_STATE_HOME/nix/profile; it does not use$XDG_STATE_HOME/nix/profiles/home-manager.Likely an incorrect or invalid review comment.
Signed-off-by: NotAShelf <[email protected]> Change-Id: I6a6a69646f8083bc0869647ef1e8130b9497f545
Signed-off-by: NotAShelf <[email protected]> Change-Id: I6a6a6964c952f3e7c68a3d5933de3f6616b0c031
NotAShelf
force-pushed
the
notashelf/push-lmlmokouvzvy
branch
from
ff4cc37 to
ea51c44
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/nixos.rs (2)
604-705: Parameterize generation discovery by profile path; current impl ignores --profile
find_previous_generation,find_generation_by_number, andget_current_generation_numberhard-codeSYSTEM_PROFILEand look for entries starting withsystem-. With--profile, rollback will search the wrong directory/prefix.Proposed implementation (outside the changed hunk) to make these helpers profile-aware:
// Change signatures fn find_previous_generation(profile: &Path) -> Result<generations::GenerationInfo> { /* ... */ } fn find_generation_by_number(profile: &Path, number: u64) -> Result<generations::GenerationInfo> { /* ... */ } fn get_current_generation_number(profile: &Path) -> Result<u64> { /* ... */ }Core logic sketch:
fn profile_dir_and_prefix(profile: &Path) -> (PathBuf, String) { let dir = profile.parent().unwrap_or_else(|| Path::new("/nix/var/nix/profiles")); let prefix = profile.file_name() .and_then(|s| s.to_str()) .unwrap_or("system") .to_owned(); (dir.to_path_buf(), prefix) }Use
starts_with(format!("{prefix}-"))when scanning, and passprofilefrom rollback:let profile_path = self.profile.as_deref().unwrap_or(Path::new(SYSTEM_PROFILE)); let target_generation = if let Some(gen) = self.to { find_generation_by_number(profile_path, gen)? } else { find_previous_generation(profile_path)? }; let current_gen_number = get_current_generation_number(profile_path).unwrap_or(0);I can provide a complete patch if helpful.
Also applies to: 650-681, 683-703
419-425: find_previous_generation and find_generation_by_number ignore custom profile
Both functions insrc/nixos.rs(starting around lines 603 and 651) unconditionally use the hard-codedSYSTEM_PROFILEconstant and never take the user’sprofileinto account. This will break rollbacks when a custom profile is specified. Refactor these functions to accept the configured profile path and use it instead ofSYSTEM_PROFILE.
🧹 Nitpick comments (4)
src/nixos.rs (4)
241-247: Avoid cloning PathBuf for diff ‘lhs’ and simplify to &PathUse
as_deref()to pass an&Pathdirectly and remove duplication at both call sites.Apply this diff to both places:
- let _ = print_dix_diff( - &self - .profile - .as_ref() - .map_or_else(|| PathBuf::from(CURRENT_PROFILE), PathBuf::from), - &target_profile, - ); + let _ = print_dix_diff( + self.profile + .as_deref() + .unwrap_or_else(|| Path::new(CURRENT_PROFILE)), + &target_profile, + );And similarly below:
- let _ = print_dix_diff( - &self - .profile - .as_ref() - .map_or_else(|| PathBuf::from(CURRENT_PROFILE), PathBuf::from), - &target_profile, - ); + let _ = print_dix_diff( + self.profile + .as_deref() + .unwrap_or_else(|| Path::new(CURRENT_PROFILE)), + &target_profile, + );Also applies to: 261-267
239-275: Consider suppressing local diffs when acting on a remote hostEven with
--diff always, comparing against a localCURRENT_PROFILEor local--profileis misleading for--target-host. Either skip diffs or print a clear note when targeting remote hosts.
429-435: Minor: warning text mentions SYSTEM_PROFILE constantThe message should be generic (it’s not necessarily
SYSTEM_PROFILEanymore).Already fixed in the earlier diff; calling it out so it isn’t missed.
352-361: Optional: name for OsStr binding
system_profile_pathis an&OsStrhere; consider renaming tosystem_profile_osstr(or use&Path) for clarity and type consistency with other uses.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)src/darwin.rs(4 hunks)src/home.rs(2 hunks)src/nixos.rs(8 hunks)
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/home.rs
- src/darwin.rs
🧰 Additional context used
🧬 Code graph analysis (1)
src/nixos.rs (3)
src/commands.rs (4)
new(160-171)new(601-609)new(723-732)elevate(175-178)src/util.rs (1)
print_dix_diff(308-338)src/interface.rs (3)
OsGenerationsArgs(344-348)OsRebuildArgs(196-231)CommonRebuildArgs(291-317)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build NH on Linux
- GitHub Check: Test NH on Linux
🔇 Additional comments (3)
src/nixos.rs (3)
352-361: LGTM: correct use of nix --profile path for Boot/SwitchUsing
--profilewith the derived path is a clean way to set the system profile, and keeping--no-linkavoids the extraresultsymlink.
508-513: LGTM: switching to elevated ln -sfn with the derived profile pathUsing
ln -sfnvia the command wrapper (with elevation) is robust and avoids partial state when retargeting the profile symlink.
578-583: LGTM: correct restoration path on activation failureRestoring the previous profile with the same derived
--profilepath keeps rollback consistent with custom profiles.
Signed-off-by: NotAShelf <[email protected]> Change-Id: I6a6a696402d1085e6699ec5eed6ecf0084df54b6
Signed-off-by: NotAShelf <[email protected]> Change-Id: I6a6a6964db21018c13af7ffc0cee613e01ea24b4
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/nixos.rs (1)
410-417: find_previous_generation/find_generation_by_number ignore --profileBoth helpers scan only “system-*-link” under the default directory. With --profile, rollback may select the wrong generation or fail.
Proposed approach (new helpers; call them when self.profile is Some):
// New helpers (add near existing find_* fns) fn find_previous_generation_for_profile(profile: &Path) -> Result<generations::GenerationInfo> { let profile_dir = profile.parent().unwrap_or(Path::new("/nix/var/nix/profiles")); let base = profile.file_name().and_then(|s| s.to_str()).unwrap_or("system"); let mut gens: Vec<_> = fs::read_dir(profile_dir)? .filter_map(|e| e.ok()) .map(|e| e.path()) .filter(|p| { p.file_name() .and_then(|n| n.to_str()) .map(|n| n.starts_with(&format!("{base}-")) && n.ends_with("-link")) .unwrap_or(false) }) .filter_map(|p| generations::describe(&p)) .collect(); if gens.is_empty() { bail!("No generations found"); } gens.sort_by_key(|g| g.number.parse::<u64>().unwrap_or(0)); let current_idx = gens.iter().position(|g| g.current) .ok_or_else(|| eyre!("Current generation not found"))?; if current_idx == 0 { bail!("No generation older than the current one exists"); } Ok(gens[current_idx - 1].clone()) } fn find_generation_by_number_for_profile(profile: &Path, number: u64) -> Result<generations::GenerationInfo> { let profile_dir = profile.parent().unwrap_or(Path::new("/nix/var/nix/profiles")); let base = profile.file_name().and_then(|s| s.to_str()).unwrap_or("system"); let gens: Vec<_> = fs::read_dir(profile_dir)? .filter_map(|e| e.ok()) .map(|e| e.path()) .filter(|p| { p.file_name() .and_then(|n| n.to_str()) .map(|n| n.starts_with(&format!("{base}-")) && n.ends_with("-link")) .unwrap_or(false) }) .filter_map(|p| generations::describe(&p)) .filter(|g| g.number == number.to_string()) .collect(); if gens.is_empty() { bail!("Generation {} not found", number); } Ok(gens[0].clone()) }And in rollback():
let target_generation = if let Some(gen_number) = self.to { if let Some(profile) = self.profile.as_ref() { find_generation_by_number_for_profile(profile, gen_number)? } else { find_generation_by_number(gen_number)? } } else { if let Some(profile) = self.profile.as_ref() { find_previous_generation_for_profile(profile)? } else { find_previous_generation()? } };I can prepare a full patch if you want this wired in now.
♻️ Duplicate comments (2)
src/nixos.rs (2)
419-433: Rollback uses hard-coded “system-” prefix; must derive from the selected profile basenameIf --profile is /nix/var/nix/profiles/foo, generation symlinks are foo--link. Current code will point to non-existent “system--link”.
Apply:
- let profile_dir = system_profile_path.parent().unwrap_or_else(|| { - tracing::warn!( - "SYSTEM_PROFILE has no parent, defaulting to /nix/var/nix/profiles" - ); + let profile_dir = system_profile_path.parent().unwrap_or_else(|| { + tracing::warn!( + "Profile path has no parent, defaulting to /nix/var/nix/profiles" + ); Path::new("/nix/var/nix/profiles") }); - let generation_link = - profile_dir.join(format!("system-{}-link", target_generation.number)); + let profile_base = system_profile_path + .file_name() + .and_then(|s| s.to_str()) + .unwrap_or("system"); + let generation_link = + profile_dir.join(format!("{profile_base}-{}-link", target_generation.number));Follow-up: the “restore previous” path below must use the same profile_base; see further comment.
569-574: Restore-on-failure must also use dynamic basenameUse the same profile_base computed above to build current_gen_link.
Apply:
- let current_gen_link = - profile_dir.join(format!("system-{current_gen_number}-link")); + let current_gen_link = + profile_dir.join(format!("{profile_base}-{current_gen_number}-link"));Also applies to: 578-579
🧹 Nitpick comments (11)
src/home.rs (2)
105-121: Profile resolution: handle missing USER more gracefullyIf $USER is unset, this errors even though we could fall back to the $HOME-based profile. Prefer optional username + existence check, then fall back.
Apply:
- let user_profile = PathBuf::from(USER_PROFILE_PATH) - .join(env::var("USER").map_err(|_| eyre!("Couldn't get username"))?) - .join("home-manager"); - let home_profile = PathBuf::from( + let home_profile = PathBuf::from( env::var("HOME").map_err(|_| eyre!("Couldn't get home directory"))?, ) .join(HOME_PROFILE_PATH); - if user_profile.exists() { - user_profile - } else { - home_profile - } + let user_profile = env::var("USER").ok().map(|u| { + PathBuf::from(USER_PROFILE_PATH).join(u).join("home-manager") + }); + if let Some(up) = user_profile.as_ref().filter(|p| p.exists()) { + up.clone() + } else { + home_profile + }
122-127: Warn when an explicit --profile doesn’t exist (skip diff visibly)Currently we silently skip diff if the override path is missing.
Apply:
-let prev_generation: Option<PathBuf> = if profile_path.exists() { - Some(profile_path.clone()) -} else { - None -}; +let prev_generation: Option<PathBuf> = profile_path.exists().then(|| profile_path.clone()); +if prev_generation.is_none() && self.profile.is_some() { + warn!( + "--profile path provided but does not exist: {}. Skipping diff.", + profile_path.display() + ); +}src/darwin.rs (1)
162-166: Skip diff if the “current” profile path doesn’t existOn Darwin, the default CURRENT_PROFILE may not exist. Guard to avoid noisy errors.
Apply:
- let _ = print_dix_diff( - &get_current_profile_pathbuf(&self.profile), - &target_profile, - ); + let current = get_current_profile_pathbuf(&self.profile); + if current.exists() { + let _ = print_dix_diff(¤t, &target_profile); + } else { + debug!("Skipping diff: current profile not found at {}", current.display()); + }src/nixos.rs (2)
232-238: Nit: avoid PathBuf::from on a PathBufUse clone/unwrap_or_else to prevent redundant construction.
Apply:
- let _ = print_dix_diff( - &self - .profile - .as_ref() - .map_or_else(|| PathBuf::from(CURRENT_PROFILE), PathBuf::from), - &target_profile, - ); + let lhs = self.profile.clone().unwrap_or_else(|| PathBuf::from(CURRENT_PROFILE)); + let _ = print_dix_diff(&lhs, &target_profile);
252-258: Nit: same simplification hereApply:
- let _ = print_dix_diff( - &self - .profile - .as_ref() - .map_or_else(|| PathBuf::from(CURRENT_PROFILE), PathBuf::from), - &target_profile, - ); + let lhs = self.profile.clone().unwrap_or_else(|| PathBuf::from(CURRENT_PROFILE)); + let _ = print_dix_diff(&lhs, &target_profile);src/interface.rs (6)
9-10: Drop eyre import for value parser errors.These errors are plain strings; eyre adds no value here.
Apply:
- use color_eyre::eyre::eyre;
11-29: Doc/behavior mismatch: do not claim to return a canonicalized path.The function returns the provided path, not a canonicalized PathBuf.
Apply:
-/// - `Ok(PathBuf)`: If the path exists and is a symlink, returns the -/// canonicalized `PathBuf`. +/// - `Ok(PathBuf)`: If the path exists and is a symlink, returns the provided +/// `PathBuf` unchanged. @@ -/// Returns an error if the path does not exist or is not a symbolic link. +/// Returns an error if the path does not exist (including broken symlinks) or is not a symbolic link.
29-49: Tighten diagnostics and avoid eyre in the validator.Simplify to format!-based strings and distinguish broken symlinks.
Apply:
fn symlink_path_validator(s: &str) -> Result<PathBuf, String> { let path = Path::new(s); - // `bail!` is for early returns in functions that return `Result<T, E>`, i.e., - // it immediately returns from the function with an error. Since this is a - // value parser and we need to return `Err(String)` `eyre!` is more - // appropriate. + // Clap value parsers must return Result<T, String>; build plain strings so clap can render them. - if !path.exists() { - return Err( - eyre!("--profile path provided but does not exist: {}", s).to_string(), - ); - } - - if !path.is_symlink() { - return Err( - eyre!("--profile path exists but is not a symlink: {}", s).to_string(), - ); - } + if path.is_symlink() && !path.exists() { + return Err(format!("--profile symlink is broken: {s}")); + } + if !path.exists() { + return Err(format!("--profile path does not exist: {s}")); + } + if !path.is_symlink() { + return Err(format!("--profile path exists but is not a symlink: {s}")); + } Ok(path.to_path_buf()) }Optional: If you want to accept ~ expansion for Home Manager paths, I can follow up with a minimal tilde-expansion (adds a tiny dependency) or a home-dir based helper.
276-279: Nit: possessive “Nix's”.Minor grammar fix in help text.
- /// Path to Nix' system profile + /// Path to Nix's system profileOptional: For consistency with OsGenerationsArgs, consider aligning on PathBuf + validator there too.
337-340: Nit: possessive “Nix's”.- /// Path to Nix' system profile for rollback + /// Path to Nix's system profile for rollback
570-573: Nit: project name styling.Prefer “Home Manager” (project name) over “Home-Manager”.
- /// Path to Home-Manager profile + /// Path to Home Manager profile
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/darwin.rs(3 hunks)src/home.rs(3 hunks)src/interface.rs(5 hunks)src/nixos.rs(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/nixos.rs (2)
src/util.rs (1)
print_dix_diff(308-338)src/commands.rs (4)
new(160-171)new(601-609)new(723-732)elevate(175-178)
src/darwin.rs (1)
src/util.rs (1)
print_dix_diff(308-338)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Test NH on Linux
- GitHub Check: Test NH on Darwin
- GitHub Check: Build NH on Linux
- GitHub Check: Build NH on Darwin
- GitHub Check: Build NH on Linux
- GitHub Check: Test NH on Linux
- GitHub Check: Build NH on Darwin
- GitHub Check: Test NH on Darwin
🔇 Additional comments (10)
src/home.rs (2)
3-5: Constants for default Home-Manager profiles: LGTMPaths look correct for per-user and XDG state locations.
154-164: Diff gate on prev_generation: LGTMNo diff on fresh installs or non-existent profile is sensible.
src/darwin.rs (3)
26-33: Helper get_system_profile(): LGTMReturns &OsStr without allocations; fits the nix invocation usage.
34-43: Helper get_current_profile_pathbuf(): LGTM (naming matches return type)Comment clarifies Darwin semantics; good.
179-183: Using derived profile in nix build: LGTMPassing --profile from user or default aligns with Nix expectations.
src/nixos.rs (3)
343-347: Using derived profile for nix --profile: LGTMOsStr handling avoids allocations and works locally/over SSH.
Also applies to: 350-352
456-462: Diff path for rollback: LGTMLeft-hand uses provided profile or CURRENT_PROFILE; good.
499-503: Setting system profile via ln with derived path: LGTMConsistent with earlier profile selection.
src/interface.rs (2)
1-4: Imports expansion looks good.Path is needed for the new validator; no issues.
683-686: Darwin profile flag wiring looks good.
Allows specifying a
--profilefor Nix commands in NixOS, Darwin, and Home-Manager subcommands (os,darwin, andhomerespectively). This fills in the gap of--profile-pathfromnixos-installand similar CLIs being missing in NH.Summary by CodeRabbit
New Features
Improvements
Chores
Documentation