treewide: refactor --build-host to use remote build semantics
#497
Conversation
WalkthroughAdds a public SSH-based remote build subsystem, integrates a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Local as nh (local host)
participant Build as Build Host (ssh)
participant Target as Target Host (optional)
User->>Local: run nh ... --build-host <host> [installable]
Local->>Local: evaluate installable -> drvPath (nix eval / prepare)
Local->>Build: copy closure -> build host (scp/ssh, may use substitutes)
Build->>Build: run nix build (with or without nom)
alt target_host specified
Build->>Target: copy build output to target host
Target-->>User: final path or error
else
Build->>Local: copy build output back to local
Local->>Local: create out-link if requested
Local-->>User: final path or error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (3)src/darwin.rs (1)
src/remote.rs (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 (20)
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: 5
🧹 Nitpick comments (2)
src/remote.rs (2)
18-37: Consider cleanup of SSH control directory on exit.The SSH control socket directory (
nh-ssh-{pid}) persists after the program terminates. While theControlPersist=60setting ensures connections close after 60 seconds of inactivity, the empty directory remains. This is a minor resource leak.You could implement cleanup using a
Dropguard oratexithandler, though this is low priority since the impact is minimal (just empty directories in/tmporXDG_RUNTIME_DIR).
368-409: Consider extracting common attribute-appending logic.The pattern of cloning the installable and appending
"drvPath"to the attribute is repeated forFlake,File, andExpressionvariants. This could be simplified with a helper method onInstallable.This is a minor refactor opportunity for future cleanup - the current implementation is correct and readable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/commands.rs(1 hunks)src/home.rs(1 hunks)src/interface.rs(2 hunks)src/lib.rs(1 hunks)src/main.rs(1 hunks)src/nixos.rs(2 hunks)src/remote.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/interface.rs (1)
src/commands.rs (1)
arg(248-251)
src/remote.rs (1)
src/util.rs (1)
get_nix_variant(56-85)
src/nixos.rs (1)
src/remote.rs (2)
parse(67-107)build_remote(474-517)
⏰ 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: profile
- GitHub Check: Test NH on Linux
- GitHub Check: Test NH on Darwin
- 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 (10)
src/main.rs (1)
12-12: LGTM!Module declaration is correctly placed and follows the existing naming conventions.
src/lib.rs (1)
13-13: LGTM!Public module declaration is consistent with other modules in this file. The internal-use comment at line 1 applies to the entire library.
src/nixos.rs (2)
25-25: LGTM!Import of remote module types is correctly placed and all imported items are used in the
execute_build_commandmethod.
354-407: Well-structured remote build implementation matchingnixos-rebuildsemantics.The implementation correctly:
- Parses host specifications with proper error context
- Constructs the
RemoteBuildConfigwith all necessary parameters- Preserves the local build path as a fallback
- Passes extra args from both explicit args and passthrough options
The inline documentation clearly explains the workflow, which helps maintainability.
src/commands.rs (1)
654-752: LGTM! Clean removal of builder field from Build struct.The
Buildstruct and its tests have been correctly updated to remove thebuilderfield and related method. The test at line 1200 properly removes the.builder("user@host")call, and the struct now cleanly delegates remote build concerns to the newremotemodule.src/remote.rs (5)
67-107: LGTM! Robust host specification parsing with helpful error messages.The
RemoteHost::parsefunction handles various input formats correctly and provides actionable error messages (e.g., suggestingNIX_SSHOPTSfor port configuration). The validation of edge cases like empty usernames, hostnames, and invalid characters is comprehensive.
206-254: LGTM! Secure remote command execution.The function properly:
- Shell-quotes all arguments to prevent injection
- Uses
--separator to prevent argument injection to SSH- Includes stderr in error messages for debugging
- Wraps errors with context
256-362: LGTM! Well-structured closure copying functions.The three copy functions handle the different scenarios correctly:
copy_closure_toandcopy_closure_fromusenix-copy-closurefor localhost ↔ remote transferscopy_closure_between_remotesusesnix copywith SSH URIs for remote-to-remote transfers- Substitutes flags are correctly mapped (
--use-substitutesvs--substitute-on-destination)
577-653: LGTM! Clever approach for nom integration.The double-invocation pattern (build with nom consuming output, then query for output path) is a reasonable workaround since nom consumes the JSON output. The second build is a no-op due to Nix caching, so the overhead is minimal.
655-762: LGTM! Comprehensive unit tests for parsing and quoting logic.The tests provide good coverage for:
- Various
RemoteHostinput formats and error cases- Shell quoting edge cases including the critical
drv^*syntax- SSH options default values
The
unsafeblock at lines 753-755 for environment variable manipulation is acceptable in test code.
| // Collect extra args that are valid strings | ||
| let extra_args_strings: Vec<String> = config | ||
| .extra_args | ||
| .iter() | ||
| .filter_map(|s| s.to_str().map(String::from)) | ||
| .collect(); | ||
| for arg in &extra_args_strings { | ||
| args.push(arg); | ||
| } |
There was a problem hiding this comment.
Non-UTF-8 extra arguments are silently dropped.
The filter_map(|s| s.to_str().map(String::from)) silently discards any OsString arguments that aren't valid UTF-8. While rare, this could cause unexpected behavior if such arguments are passed.
Consider logging a warning when arguments are dropped:
let extra_args_strings: Vec<String> = config
.extra_args
.iter()
- .filter_map(|s| s.to_str().map(String::from))
+ .filter_map(|s| {
+ match s.to_str() {
+ Some(str) => Some(str.to_string()),
+ None => {
+ tracing::warn!("Dropping non-UTF-8 argument: {:?}", s);
+ None
+ }
+ }
+ })
.collect();The same pattern exists at lines 596-600.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Collect extra args that are valid strings | |
| let extra_args_strings: Vec<String> = config | |
| .extra_args | |
| .iter() | |
| .filter_map(|s| s.to_str().map(String::from)) | |
| .collect(); | |
| for arg in &extra_args_strings { | |
| args.push(arg); | |
| } | |
| // Collect extra args that are valid strings | |
| let extra_args_strings: Vec<String> = config | |
| .extra_args | |
| .iter() | |
| .filter_map(|s| { | |
| match s.to_str() { | |
| Some(str) => Some(str.to_string()), | |
| None => { | |
| tracing::warn!("Dropping non-UTF-8 argument: {:?}", s); | |
| None | |
| } | |
| } | |
| }) | |
| .collect(); | |
| for arg in &extra_args_strings { | |
| args.push(arg); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/home.rs (1)
106-124: Consider extracting shared remote build configuration logic.The
RemoteBuildConfigconstruction here is nearly identical to the one insrc/darwin.rs(lines 119-137). A shared helper function could reduce duplication:// In a shared location (e.g., remote.rs or a new util) fn build_config_from_args( build_host: RemoteHost, no_nom: bool, use_substitutes: bool, extra_args: &[String], passthrough: &Passthrough, ) -> RemoteBuildConfig { ... }This is a minor refactoring opportunity that can be deferred.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/darwin.rs(2 hunks)src/home.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/home.rs (1)
src/remote.rs (2)
parse(67-107)build_remote(474-517)
src/darwin.rs (1)
src/remote.rs (2)
parse(67-107)build_remote(474-517)
⏰ 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). (11)
- GitHub Check: Test NH on Darwin
- GitHub Check: Test NH on Linux
- GitHub Check: Build NH on Linux
- GitHub Check: Build NH on Darwin
- GitHub Check: treewide-checks
- GitHub Check: profile
- GitHub Check: Test NH on Linux
- GitHub Check: Test NH on Darwin
- GitHub Check: Build NH on Darwin
- GitHub Check: Build NH on Linux
- GitHub Check: treewide-checks
🔇 Additional comments (4)
src/darwin.rs (2)
18-18: LGTM!The new imports for the remote build subsystem are correctly added and all three symbols (
remote,RemoteBuildConfig,RemoteHost) are used in the remote build logic below.
112-151: Well-structured remote build integration.The conditional remote build path correctly:
- Parses the host specification with proper error context
- Constructs the
RemoteBuildConfigwith appropriate field mappings- Passes
out_pathas the out-link sobuild_remotecreates the symlink there- Falls back to the local build flow when no
build_hostis specifiedThe subsequent code (line 153 onwards) correctly uses
out_pathwhich will be a valid symlink after either build path completes.src/home.rs (2)
14-14: Imports are now properly used.The previously flagged unused imports (
remote,RemoteBuildConfig,RemoteHost) are now correctly used in the remote build logic at lines 103, 106, and 126 respectively.
99-138: Remote build integration looks correct.The conditional remote build path properly:
- Parses the host specification with error context
- Builds
RemoteBuildConfigwith the correct field mappings- Invokes
build_remotewith the out-link for symlink creation- Falls back to local build when no
build_hostis providedThe post-build steps (activation, diffs, etc.) work correctly since
out_pathwill be a valid symlink after either path.
Fixes #428 This is a large architectural change to NH, which lead to me extracting the remote build logic to its own file so that we may implement it for Darwin and Home-Manager as well. The `--builders` flag was dropped from `nh::commands`, and it was replaced with the new and shiny logic that hopefully avoids previous pitfalls. The new `nh::remote` module handles remote builds, including: - Parsing remote host specifications. - Copying derivations to remote hosts using `nix-copy-closure`. - Building derivations on remote hosts via `nix build`. - Copying results back to localhost or directly to a target host. Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: I236eb1e35dd645f2169462d207bc82e76a6a6964
Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: I946d8e54261e9136c83f6dfe38b046106a6a6964
Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: I0b82e84223c3df61cfa23464bd3d4bcc6a6a6964
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/remote.rs (1)
604-612: Non-UTF-8 extra arguments are silently dropped.As flagged in a previous review,
filter_map(|s| s.to_str().map(String::from))silently discards anyOsStringarguments that aren't valid UTF-8. Consider logging a warning when this occurs:let extra_args_strings: Vec<String> = config .extra_args .iter() - .filter_map(|s| s.to_str().map(String::from)) + .filter_map(|s| { + match s.to_str() { + Some(str) => Some(str.to_string()), + None => { + tracing::warn!("Dropping non-UTF-8 argument: {:?}", s); + None + } + } + }) .collect();The same pattern exists at lines 649-656.
🧹 Nitpick comments (5)
src/commands.rs (1)
44-46: Silent failure on malformed input may hide user errors.Using
unwrap_or_default()silently returns an empty vector whenshlex::splitfails (e.g., unclosed quotes). While the test at line 1421-1424 documents this behavior, returning an empty vector could mask user input errors in contexts likeself_elevate_cmd(line 444), where an empty parse leads to a bail at line 447.Consider logging a warning when parsing fails to aid debugging:
fn parse_cmdline_with_quotes(cmdline: &str) -> Vec<String> { - shlex::split(cmdline).unwrap_or_default() + shlex::split(cmdline).unwrap_or_else(|| { + tracing::warn!("Failed to parse command line (unclosed quote?): {cmdline}"); + Vec::new() + }) }src/darwin.rs (1)
112-140: Consider adding SSH reachability check for consistency with NixOS module.The NixOS implementation (src/nixos.rs lines 374-385) performs an SSH reachability check before starting expensive evaluation. This provides early feedback if the build host is unreachable. The Darwin implementation omits this check.
if let Some(ref build_host_str) = self.build_host { info!("Building Darwin configuration"); let build_host = RemoteHost::parse(build_host_str) .wrap_err("Invalid build host specification")?; + // Check SSH connectivity before expensive evaluation + info!("Checking SSH connectivity to build host..."); + remote::check_ssh_reachability(&build_host) + .wrap_err(format!("Build host ({build_host}) is not reachable"))?; + let config = RemoteBuildConfig {src/home.rs (1)
99-127: Consider adding SSH reachability check for consistency.Similar to the Darwin module, the Home-Manager implementation omits the SSH reachability pre-check that exists in the NixOS module (src/nixos.rs lines 374-385). For consistency and better user experience, consider adding the check:
if let Some(ref build_host_str) = self.build_host { info!("Building Home-Manager configuration"); let build_host = RemoteHost::parse(build_host_str) .wrap_err("Invalid build host specification")?; + info!("Checking SSH connectivity to build host..."); + remote::check_ssh_reachability(&build_host) + .wrap_err(format!("Build host ({build_host}) is not reachable"))?; + let config = RemoteBuildConfig {src/nixos.rs (1)
286-291: Minor: Documentation formatting could be improved.The
Returnssection has inconsistent line breaks that affect readability:/// # Returns - /// - /// `Result` containing a tuple: - /// + /// + /// Returns a `Result` containing a tuple: + /// /// - `bool`: `true` if elevation is required, `false` otherwise. /// - `String`: The resolved target hostname.src/remote.rs (1)
18-48: SSH control directory is not cleaned up on exit.The SSH control directory created at
get_ssh_control_dir()persists after the program exits. While the directory itself is small, the SSH control sockets inside may linger. Consider documenting this behavior or implementing cleanup:+// Note: The SSH control directory and sockets are intentionally not cleaned up +// on exit to allow connection multiplexing across multiple nh invocations. +// SSH's ControlPersist=60 handles socket cleanup after idle timeout. fn get_ssh_control_dir() -> &'static PathBuf {Alternatively, you could register an atexit handler to clean up the directory if this is undesirable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/commands.rs(4 hunks)src/darwin.rs(3 hunks)src/home.rs(3 hunks)src/nixos.rs(6 hunks)src/remote.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/home.rs (1)
src/remote.rs (2)
parse(78-118)build_remote(522-570)
src/nixos.rs (1)
src/remote.rs (3)
check_ssh_reachability(143-173)parse(78-118)build_remote(522-570)
src/darwin.rs (1)
src/remote.rs (2)
parse(78-118)build_remote(522-570)
src/remote.rs (1)
src/util.rs (1)
get_nix_variant(56-85)
⏰ 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: Build NH on Linux
- GitHub Check: Build NH on Darwin
- GitHub Check: treewide-checks
- GitHub Check: Test NH on Darwin
- GitHub Check: Test NH on Linux
- GitHub Check: Build NH on Darwin
- GitHub Check: Build NH on Linux
- GitHub Check: treewide-checks
- GitHub Check: Test NH on Linux
- GitHub Check: Test NH on Darwin
🔇 Additional comments (9)
src/commands.rs (2)
690-733: LGTM! Correct handling of pipeline exit status.Using
popen()to access individual processes and checking the first process's (nix build) exit status is the right approach. This ensures build failures are properly detected even when nom always succeeds.
1377-1442: LGTM! Comprehensive test coverage for shell parsing.The new tests thoroughly cover edge cases including escaped quotes, backslash escapes, nix store paths, environment variables in quotes, unclosed quotes, and complex sudo commands. Good coverage of the shlex delegation.
src/darwin.rs (1)
141-151: LGTM! Clean conditional branching between remote and local builds.The remote/local build branching is well-structured. The local build path correctly preserves the existing behavior with
--out-link,extra_args, passthrough, and nom support.src/home.rs (1)
128-138: LGTM! Local build fallback is correctly preserved.The existing local build path is properly maintained as a fallback when no
build_hostis specified.src/nixos.rs (1)
349-421: LGTM! Well-structured remote build implementation.The
execute_buildmethod properly implements the remote build workflow:
- Validates both
build_hostandtarget_hostspecifications- Performs SSH reachability checks before expensive evaluation
- Constructs
RemoteBuildConfigwith all necessary fields- Falls back to local build when no
build_hostis specifiedThe early SSH connectivity check provides good user feedback before starting the evaluation.
src/remote.rs (4)
191-197: LGTM! Shell quoting uses shlex with appropriate fallback.The implementation correctly uses
shlex::try_quotewith a sensible fallback for edge cases (NUL bytes). The fallback uses proper single-quote escaping.
704-722: Re-running nix build to get output path is a reasonable workaround.Since nom consumes the build output, re-querying with
--print-out-pathsis a pragmatic solution. The second invocation is effectively a no-op since the derivation is already built.
522-569: LGTM! Well-structured remote build workflow.The
build_remotefunction correctly implements the four-step workflow:
- Evaluate drvPath locally
- Copy derivation to build host
- Build on remote
- Copy result to destination (target_host or localhost)
The logic correctly handles the case where both
out_linkis requested andtarget_hostis set, ensuring the closure is available locally for specialisation resolution.
725-1036: LGTM! Comprehensive test coverage.The test suite thoroughly covers:
- Host parsing (bare, user@host, URI schemes, error cases)
- Shell quoting (simple strings, special characters, roundtrip verification)
- SSH options (default, with NIX_SSHOPTS, quoted values)
- Control directory creation
The roundtrip tests at lines 894-926 are particularly valuable for ensuring shell quoting correctness.
Fixes a minor issue in how commands that are invalid or improperly handled are forwarded to the Nix command. Replaces `join()` with `popen()` to access individual processes in the pipeline. This way we can better check the exist status of the `nix build` process and properly propagate them. Also improves naming a little bit because why not? Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: I8a44abf924f9c9a1c06d102e5a3f40aa6a6a6964
Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: Ia8506cad3352243001a281e99b8162c26a6a6964
Tiny improvement to how remote connections are made. We now check BEFORE the connection is made, so that we can avoid all that expensive eval if it's not reachable. This is not infallible, but it is better. To fix some target-host quirks, we also have to deal with local symlinks so we enforce it locally either way. Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: I65fd7258828459ea82fe6739383567556a6a6964
Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: I2366fac6ca7a72fc73eecfc0b07bd2d76a6a6964
Fixes #428
This is a large architectural change to NH, which lead to me extracting the remote build logic to its own file so that we may implement it for Darwin and Home-Manager as well. The
--buildersflag was dropped fromnh::commands, and it was replaced with the new and shiny logic that hopefully avoids previous pitfalls.The new
nh::remotemodule handles remote builds, including:nix-copy-closure.nix build.The legacy builder logic is removed from the
Buildcommand and re-introduced in a new, more robust remote build implementation for NixOS rebuildsChange-Id: I236eb1e35dd645f2169462d207bc82e76a6a6964
Summary by CodeRabbit
New Features
Refactor
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.