nixos: cleanup #436
nixos: cleanup #436
Conversation
WalkthroughRefactors NixOS rebuild/VM flows in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as CLI User
participant CLI as nh (os rebuild)
participant Ctx as setup_build_context
participant Host as get_nh_os_flake_env / get_hostname
participant Out as determine_output_path
participant Build as execute_build_command
participant Diff as handle_dix_diff
participant Act as activate_rebuilt_config
participant VM as VM flow
User->>CLI: nh os rebuild [args]
CLI->>Ctx: setup_build_context(args)
Ctx-->>CLI: context (elevation, flags, SSH)
CLI->>Host: resolve flake/hostname
Host-->>CLI: installable / hostname
CLI->>Out: determine_output_path(context, installable, hostname)
Out-->>CLI: out_path, toplevel
alt Build VM requested
CLI->>VM: derive VM attr ("vm" / "vmWithBootLoader")
VM-->>CLI: find_vm_script / runner path
CLI-->>User: print_vm_instructions (or run_vm)
else Normal rebuild
CLI->>Build: execute_build_command(toplevel, out_path, context)
Build-->>CLI: build result
CLI->>Diff: handle_dix_diff(build result, context)
Diff-->>CLI: diff output / decisions
CLI->>Act: activate_rebuilt_config(result, elevation)
Act-->>CLI: activation status
end
CLI-->>User: completion / errors
sequenceDiagram
autonumber
participant User as CLI User
participant CLI as nh update
participant Checker as is Flake?
participant Cmd as nix flake update cmd
User->>CLI: nh update [installable] [--inputs ...]
CLI->>Checker: check installable type
alt Non-Flake
Checker-->>CLI: not flake
CLI-->>User: warn and return early
else Flake
Checker-->>CLI: flake
CLI->>Cmd: build single `nix flake update` command
CLI->>Cmd: append inputs if provided
Cmd-->>CLI: run update
CLI-->>User: update output
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Should be all fine now, this definitely needs a squash. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/nixos.rs (3)
105-115: Consider extracting the VM warning condition into a helper for clarity.The nested condition spanning multiple lines with
is_some_andcould be more readable if extracted into a well-named helper function or an early-return pattern.Consider this refactor:
+ let should_warn_vm_hostname = self.hostname.is_none() + && matches!(variant, OsRebuildVariant::BuildVm) + && final_attr.is_some_and(|attr| attr == "vm" || attr == "vmWithBootLoader"); + - if self.hostname.is_none() - && matches!(variant, OsRebuildVariant::BuildVm) - && final_attr - .is_some_and(|attr| attr == "vm" || attr == "vmWithBootLoader") - { + if should_warn_vm_hostname { tracing::warn!( "Guessing system is {} for a VM image. If this isn't intended, use \ --hostname to change.", target_hostname ); }
763-776: Verify if caching generations list would be beneficial.
find_previous_generation()callslist_generations()at line 764, and thenget_current_generation_number()at line 769 also callslist_generations(). Each call reads the filesystem, which could be inefficient. Based on learningsConsider whether passing the generations list as a parameter would improve performance:
fn find_previous_generation() -> Result<generations::GenerationInfo> { let generations = list_generations()?; if generations.is_empty() { bail!("No generations found"); } let current_idx = get_current_generation_number_from_list(&generations)? as usize; if current_idx == 0 { bail!("No generation older than the current one exists"); } Ok(generations[current_idx - 1].clone()) } fn get_current_generation_number_from_list(generations: &[generations::GenerationInfo]) -> Result<u64> { let current_gen = generations .iter() .find(|g| g.current) .ok_or_else(|| eyre!("Current generation not found"))?; current_gen .number .parse::<u64>() .wrap_err("Invalid generation number") }
748-761: Simplify nested conditional with early return pattern.The nested
ifstatement inside theelseblock at line 753 can be flattened for better readability.Apply this diff to simplify the logic:
fn has_elevation_status(bypass_root_check: bool) -> Result<bool> { if bypass_root_check { warn!("Bypassing root check, now running nix as root"); Ok(false) } else { - if nix::unistd::Uid::effective().is_root() { - bail!( - "Don't run nh os as root. It will escalate its privileges internally \ - as needed." - ); - } - Ok(true) + if nix::unistd::Uid::effective().is_root() { + bail!( + "Don't run nh os as root. It will escalate its privileges internally \ + as needed." + ); + } + Ok(true) } }Or even more concisely:
fn has_elevation_status(bypass_root_check: bool) -> Result<bool> { if bypass_root_check { warn!("Bypassing root check, now running nix as root"); - Ok(false) - } else { - if nix::unistd::Uid::effective().is_root() { - bail!( - "Don't run nh os as root. It will escalate its privileges internally \ - as needed." - ); - } - Ok(true) + return Ok(false); } + + if nix::unistd::Uid::effective().is_root() { + bail!( + "Don't run nh os as root. It will escalate its privileges internally \ + as needed." + ); + } + + Ok(true) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/darwin.rs(2 hunks)src/home.rs(1 hunks)src/nixos.rs(18 hunks)src/util.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/home.rs (1)
src/util.rs (1)
get_hostname(188-221)
src/darwin.rs (1)
src/util.rs (1)
get_hostname(188-221)
src/nixos.rs (2)
src/commands.rs (5)
message(223-226)message(626-629)new(160-171)new(615-623)new(737-746)src/util.rs (3)
ensure_ssh_key_login(159-177)get_hostname(188-221)print_dix_diff(315-345)
⏰ 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: Test NH on Darwin
- GitHub Check: Build NH on Linux
- GitHub Check: Build NH on Darwin
- GitHub Check: Test NH on Linux
🔇 Additional comments (6)
src/home.rs (1)
286-286: LGTM!The call to
get_hostname(None)correctly uses the updated signature to retrieve the system hostname for automatic configuration detection.src/util.rs (1)
181-220: Excellent refactor consolidating hostname resolution.The addition of the
supplied_hostnameparameter makesget_hostnamemore flexible by allowing callers to provide an explicit hostname or fall back to system resolution. The documentation and error messages are properly updated to reflect this change.This refactor appears to address the previous review feedback questioning the need for multiple hostname resolution functions, consolidating the logic into a single, flexible API.
Based on past review comments from midischwarz12.
src/darwin.rs (2)
68-68: LGTM!The direct call to
get_hostname(self.hostname)?cleanly replaces the previous two-step fallback pattern, forwarding the user-supplied hostname (if any) or falling back to system hostname resolution within the helper function.
225-225: LGTM!Consistent usage of the updated
get_hostnamesignature, matching the pattern inDarwinRebuildArgs.src/nixos.rs (2)
643-659: LGTM! Simplified error handling for display-only function.The change from returning
Result<()>to no return type is appropriate here since this function only prints informational messages and any errors are already logged as warnings. The function gracefully degrades if the VM script cannot be found.
157-407: Excellent refactoring that improves code organization and maintainability.The introduction of helper functions (
setup_build_context,determine_output_path,resolve_installable_and_toplevel,execute_build_command,resolve_specialisation_and_profile,handle_dix_diff, andactivate_rebuilt_config) significantly improves the readability and maintainability of the rebuild flow. Each function has a clear, single responsibility and the overall flow is now much easier to understand.The refactoring also addresses previous review comments about naming (e.g.,
get_nh_os_flake_envandhas_elevation_status). Based on learnings
| self, | ||
| variant: &OsRebuildVariant, | ||
| final_attr: Option<String>, | ||
| final_attr: Option<&String>, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Prefer Option<&str> over Option<&String> for the parameter type.
Using &String as a parameter type is less flexible than &str, as it forces callers to own a String. The &str type can accept both String references and string slices, making the API more ergonomic.
Apply this diff to improve the parameter type:
fn rebuild(
self,
variant: &OsRebuildVariant,
- final_attr: Option<&String>,
+ final_attr: Option<&str>,
elevation: ElevationStrategy,
) -> Result<()> {You'll also need to update the call site at line 80:
- .rebuild(&OsRebuildVariant::BuildVm, Some(&attr), elevation)?;
+ .rebuild(&OsRebuildVariant::BuildVm, Some(&attr.as_str()), elevation)?;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/nixos.rs around line 96 change the parameter type final_attr:
Option<&String> to final_attr: Option<&str> to make the API accept both &String
and &str; then update the call site at line 80 to pass a &str (for example
convert an Option<String> with .as_deref() or take a &string[..] / &*string) so
the signature and all usages match the new Option<&str> type and lifetimes
remain correct.
faukah
force-pushed
the
faukah/cleanup
branch
from
4ef4ff4 to
6a6dc0a
Compare
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 (2)
src/nixos.rs (2)
379-386: Critical: wrong target passed tonix build --profileYou’re setting the system profile to the switch-to-configuration binary path, not the configuration output directory. The profile should point to the configuration output (target_profile), otherwise /nix/var/nix/profiles/system won’t be a valid system root.
Apply this fix:
Command::new("nix") .elevate(elevate.then_some(elevation.clone())) .args(["build", "--no-link", "--profile", SYSTEM_PROFILE]) - .arg(canonical_out_path) + .arg(target_profile) .ssh(self.target_host.clone()) .with_required_env() .run() .wrap_err("Failed to set system profile")?;
763-776: Critical: previous generation computation is incorrect (uses generation number as index)get_current_generation_number() returns the generation number, not an index into the vector. This will panic or return the wrong generation.
Apply this fix:
fn find_previous_generation() -> Result<generations::GenerationInfo> { let generations = list_generations()?; if generations.is_empty() { bail!("No generations found"); } - - let current_idx = get_current_generation_number()? as usize; - - if current_idx == 0 { - bail!("No generation older than the current one exists"); - } - - Ok(generations[current_idx - 1].clone()) + let current_number = get_current_generation_number()?; + let pos = generations + .iter() + .position(|g| g.number == current_number.to_string()) + .ok_or_else(|| eyre!("Current generation not found in generations list"))?; + if pos == 0 { + bail!("No generation older than the current one exists"); + } + Ok(generations[pos - 1].clone()) }
♻️ Duplicate comments (2)
src/nixos.rs (2)
96-98: UseOption<&str>instead ofOption<&String>forfinal_attrThis improves ergonomics and avoids unnecessary String lifetimes. It was noted previously as well.
fn rebuild( self, variant: &OsRebuildVariant, - final_attr: Option<&String>, + final_attr: Option<&str>, elevation: ElevationStrategy, ) -> Result<()> {
207-222: PropagateOption<&str>through helper; simplify unwrapFollow through with
Option<&str>here and avoid coercions.fn resolve_installable_and_toplevel( &self, target_hostname: &str, - final_attr: Option<&String>, + final_attr: Option<&str>, ) -> Result<Installable> { @@ - Ok(toplevel_for( - target_hostname, - installable, - final_attr.map_or("toplevel", |v| v), - )) + Ok(toplevel_for( + target_hostname, + installable, + final_attr.unwrap_or("toplevel"), + )) }
🧹 Nitpick comments (3)
src/nixos.rs (3)
65-81: Avoid temporary String for VM attribute; pass&strdirectlyOnce
final_attracceptsOption<&str>, you can drop allocations and pass slices.- let attr = if self.with_bootloader { - "vmWithBootLoader".to_owned() - } else { - "vm".to_owned() - }; + let attr: &str = + if self.with_bootloader { "vmWithBootLoader" } else { "vm" }; @@ - debug!("Building VM with attribute: {}", attr); + debug!("Building VM with attribute: {attr}"); @@ - .rebuild(&OsRebuildVariant::BuildVm, Some(&attr), elevation)?; + .rebuild(&OsRebuildVariant::BuildVm, Some(attr), elevation)?;
287-299: DIX diff scope: clarify local vs remote/explicit-hostname behaviorNice touch to limit diff to local implicit builds. Consider logging the resolved hostname when skipping due to explicit/remote criteria for traceability.
Also applies to: 300-304
642-659: Minor: function docs vs signature forprint_vm_instructionsThe docstring mentions returning Result, but the function returns unit and internally logs errors. Either update docs or return Result<()> and bubble up.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/darwin.rs(2 hunks)src/home.rs(1 hunks)src/nixos.rs(18 hunks)src/update.rs(1 hunks)src/util.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/update.rs
- src/util.rs
🧰 Additional context used
🧬 Code graph analysis (3)
src/darwin.rs (1)
src/util.rs (1)
get_hostname(188-221)
src/home.rs (1)
src/util.rs (1)
get_hostname(188-221)
src/nixos.rs (5)
src/commands.rs (6)
elevate(175-178)message(223-226)message(626-629)new(160-171)new(615-623)new(737-746)src/installable.rs (1)
parse_attribute(208-244)src/util.rs (3)
ensure_ssh_key_login(159-177)get_hostname(188-221)print_dix_diff(315-345)src/update.rs (1)
update(5-35)src/generations.rs (1)
describe(170-315)
⏰ 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: Test NH on Darwin
- GitHub Check: Test NH on Linux
- GitHub Check: Build NH on Linux
- GitHub Check: Build NH on Darwin
🔇 Additional comments (3)
src/home.rs (1)
286-286: LGTM: hostname resolution aligned with new APIUsing get_hostname(None)? here is correct and consistent with util::get_hostname.
src/darwin.rs (2)
68-68: LGTM: unified hostname resolutionget_hostname(self.hostname)? correctly routes both explicit and auto resolution through a single helper.
225-234: LGTM: REPL hostname handling updatedConsistent with the new util::get_hostname signature.
3 participants
General cleanup. More or less ready to review, I'd recommend looking at the individual commits to see the changes I made.
Sanity Checking
nix fmtto format my Nix codecargo fmtto format my Rust codecargo clippyand fixed any new linter warnings.logic
description.
x86_64-linuxaarch64-linuxx86_64-darwinaarch64-darwinAdd a 👍 reaction to pull requests you find important.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes