nixos: implement --run for VM builders
#430
Conversation
WalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant CLI as OsBuildVmArgs CLI
participant Build as build_vm
participant Helpers as VM Helpers
participant Host as Host Shell
U->>CLI: invoke build command (optional --run / -r)
CLI->>Build: build_vm(args)
Build->>Build: build Nix derivation -> produce out_path
alt --run provided
Build->>Helpers: run_vm(out_path)
Helpers->>Helpers: find_vm_script(out_path)
alt script found
Helpers->>Host: exec run-*-vm (spawn)
Host-->>Helpers: exit status
else script not found
Helpers->>CLI: print_vm_instructions(out_path) (fallback)
end
else no --run
Build->>CLI: print_vm_instructions(out_path)
end
Build-->>CLI: return
CLI-->>U: display logs/instructions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)src/nixos.rs (1)
⏰ 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)
🔇 Additional comments (5)
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 |
![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
🧹 Nitpick comments (1)
src/nixos.rs (1)
616-634: Consider documenting that VM execution is synchronous.The VM is executed synchronously, blocking until the VM is shut down. This is likely the intended behavior for simplicity, but consider documenting this in the CLI help text so users understand the tool won't return until they shut down the VM.
If you decide the blocking behavior should be documented, add a note to the
--runflag's help text insrc/interface.rs:/// Run the VM immediately after building -#[arg(long, short = 'r')] +#[arg(long, short = 'r', help = "Run the VM immediately after building (blocks until VM is shut down)")] pub run: bool,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/interface.rs(1 hunks)src/nixos.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/interface.rs (1)
src/commands.rs (1)
arg(199-202)
src/nixos.rs (1)
src/commands.rs (3)
new(156-167)new(611-619)new(733-742)
⏰ 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: Build NH on Linux
- GitHub Check: Build NH on Darwin
- GitHub Check: Test NH on Darwin
- GitHub Check: Test NH on Linux
- GitHub Check: Test NH on Darwin
- GitHub Check: Test NH on Linux
- GitHub Check: Build NH on Darwin
- GitHub Check: Build NH on Linux
🔇 Additional comments (5)
src/interface.rs (1)
195-197: LGTM!The new
--run/-rflag is well-implemented and follows the existing patterns in the codebase. The boolean field appropriately defaults tofalse, preserving backward compatibility.src/nixos.rs (4)
67-87: LGTM!The implementation correctly captures the
--runflag, builds the VM, and conditionally executes it. The output path handling is consistent with the existing patterns in therebuildmethod.
282-285: LGTM!The placement of
print_vm_instructionsin the non-dry BuildVm path is correct. Even when the--runflag is used (which executes the VM after this returns), printing the instructions remains useful as it shows the user the script location for future manual runs.
596-614: LGTM!The function gracefully handles the case where the VM script can't be found, providing a helpful fallback message with the expected path pattern. This is good UX.
566-594: No duplicate VM helper function definitions Confirmed thatfind_vm_script,print_vm_instructions, andrun_vmeach appear only once insrc/nixos.rs.
Adds a `--run` flag to `nh os build-vm`, that can be used to automatically start the built VM once the build is complete. Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: I6a6a696460cbcf9422104fbdc062156a5a234227
NotAShelf
force-pushed
the
notashelf/push-tznonkqvxxyz
branch
from
c5b6392 to
1641518
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/nixos.rs (2)
616-634: Consider verifying executable permissions before running the script.The function attempts to execute the VM script without checking if it has executable permissions, which could lead to a less clear error message at runtime.
Add a permissions check:
fn run_vm(out_path: &Path) -> Result<()> { let vm_script = find_vm_script(out_path)?; + // Verify the script is executable + if let Ok(metadata) = fs::metadata(&vm_script) { + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + if metadata.permissions().mode() & 0o111 == 0 { + return Err(eyre!( + "VM script at {} is not executable. Try: chmod +x {}", + vm_script.display(), + vm_script.display() + )); + } + } + } + info!(
580-591: Ensure deterministic VM script selection (src/nixos.rs:580–587)
fs::read_diryields entries in arbitrary order; if multiplerun-*-vmscripts exist, the current.find()will pick one non-deterministically. Collect all matching entries, sort by file name (or error if more than one), then select the runner.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/commands.rs(1 hunks)src/generations.rs(2 hunks)src/interface.rs(1 hunks)src/nixos.rs(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/generations.rs
- src/commands.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/interface.rs
🧰 Additional context used
🧬 Code graph analysis (1)
src/nixos.rs (1)
src/commands.rs (3)
new(160-171)new(615-623)new(737-746)
⏰ 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: treewide-checks
- GitHub Check: Test NH on Linux
- 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
- GitHub Check: treewide-checks
🔇 Additional comments (4)
src/nixos.rs (4)
67-88: LGTM! Clean implementation of the--runflag.The logic correctly captures the run flag, builds the VM, and conditionally executes it. The error propagation and path handling are appropriate.
281-285: Good addition of user guidance for VM builds.Printing instructions on how to run the VM when not in dry-run mode improves the user experience.
596-614: LGTM! Graceful error handling with helpful fallback.The function properly handles both success and failure cases from
find_vm_script, providing clear guidance to users in either scenario.
566-634: Inconsistency: AI summary mentions duplication that isn't present.The AI-generated summary claims "VM helper utilities are declared in two separate locations within src/nixos.rs, effectively duplicating the utilities," but the provided code shows only one set of these helper functions (lines 566-634). This appears to be an inconsistency between the summary and the actual code changes.
Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: I6a6a69649dd90b33ab8b88126d30ed0bb5545671
Adds a
--runflag tonh os build-vm, that can be used to automatically start the built VM once the build is complete.Change-Id: I6a6a696460cbcf9422104fbdc062156a5a234227
Summary by CodeRabbit
New Features
Improvements
Style