cleanup installable #445
cleanup installable #445
Conversation
WalkthroughThis PR refactors the attribute representation in Installable types from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes follow a consistent refactoring pattern across multiple files (Vec to String conversions, corresponding operation changes), but require verification of type propagation through various callsites and confirmation that the new string-based attribute handling produces equivalent behavior across different platforms and contexts. Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
faukah
force-pushed
the
faukah/cleanup-installable
branch
from
e4e1949 to
407d6c4
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: 3
🧹 Nitpick comments (2)
src/nixos.rs (2)
101-155: LGTM! Excellent refactoring.Breaking down the monolithic
rebuildfunction into focused helper methods significantly improves code organization, readability, and maintainability. Each helper has a single, well-defined responsibility, making the code easier to understand and test.
745-758: Consider simplifying the nested conditional.The nested
ifinside theelsebranch could be flattened for improved 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) + } else if nix::unistd::Uid::effective().is_root() { + bail!( + "Don't run nh os as root. It will escalate its privileges internally \ + as needed." + ); + } else { + Ok(true) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/commands.rs(2 hunks)src/darwin.rs(5 hunks)src/home.rs(8 hunks)src/installable.rs(8 hunks)src/nixos.rs(18 hunks)src/update.rs(1 hunks)src/util.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/darwin.rs (1)
src/util.rs (1)
get_hostname(188-221)
src/update.rs (2)
src/installable.rs (1)
str_kind(260-267)src/commands.rs (3)
new(160-171)new(615-623)new(737-746)
src/home.rs (1)
src/util.rs (1)
get_hostname(188-221)
src/nixos.rs (4)
src/commands.rs (6)
elevate(175-178)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)src/update.rs (1)
update(5-35)src/generations.rs (1)
describe(170-315)
🪛 GitHub Actions: Test NH
src/installable.rs
[error] 239-239: Test 'installable_to_args' failed: assertion mismatch. left: ["w#xy.z"] vs right: ["w#x."y.z""]
⏰ 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 Darwin
- GitHub Check: Build NH on Linux
🔇 Additional comments (11)
src/home.rs (4)
72-72: LGTM!The simplified attribute parsing using
unwrap_or_default().to_string()is clearer and handles the absence of an attribute fragment correctly by defaulting to an empty string.Also applies to: 379-379
223-223: LGTM!The change from
attribute.push(...)to*attribute = String::from(...)correctly replaces the entire attribute string rather than attempting to push a string as a single character. This aligns with the attribute type change fromVec<String>toString.
254-254: LGTM!Correctly changed from
push()topush_str()to append string slices. Thepush()method onStringonly accepts a singlechar, so these changes fix what would have been compilation errors.Also applies to: 264-264, 309-309, 318-318
283-283: LGTM!The updated call to
get_hostname(None)correctly uses the new API signature that accepts an optional hostname parameter. PassingNonetriggers automatic hostname detection, which is appropriate for this automatic configuration discovery path.src/nixos.rs (7)
65-80: LGTM!The explicit derivation of the VM attribute and passing it by reference to the rebuild function is clear and correct. The debug logging helps with troubleshooting.
169-187: LGTM!The
setup_build_contexthelper properly encapsulates the initialization logic for SSH, elevation, updates, and hostname resolution. The early SSH key prompt improves user experience for remote operations.
588-627: LGTM! Robust error handling.The VM script finding logic properly handles directory traversal errors with warnings while continuing to search, and provides clear error messages when the script cannot be found. The filter-map pattern is appropriate here.
695-706: LGTM!Extracting the error message into a dedicated function improves consistency and maintainability. The error message provides clear guidance to users on how to resolve the issue.
712-730: LGTM!The
get_nh_os_flake_envhelper properly parses the environment variable and provides clear error messages. The use ofunwrap_or_default()for the optional attribute fragment is appropriate.
797-830: LGTM! Improved error handling.The
list_generationsfunction properly handles directory traversal errors with warnings, filters for the correct generation links, and sorts the results. The error handling for empty results is appropriate.
838-851: LGTM!The attribute string construction correctly uses
push_strfor appending the toplevel path. The hostname formatting and conditional attribute assignment align with the refactored attribute handling.
| // If user explicitly selects some other attribute, don't push | ||
| // darwinConfigurations | ||
| if attribute.is_empty() { | ||
| attribute.push(String::from("darwinConfigurations")); | ||
| attribute.push(hostname.clone()); | ||
| *attribute = format!("darwinConfigurations.{hostname}"); | ||
| } |
There was a problem hiding this comment.
Quote hostname when building the attrpath.
format!("darwinConfigurations.{hostname}") breaks as soon as the hostname contains dots or other characters that need quoting (e.g. the default macOS *.local). We previously relied on join_attribute to emit darwinConfigurations."myhost.local". Without that quoting, Nix resolves the wrong attribute and the rebuild fails.
Please reintroduce the quoting helper (or escape the hostname) so that we emit a valid attrpath for all hostnames.
🤖 Prompt for AI Agents
In src/darwin.rs around lines 104 to 108, the code builds an attrpath with
format!("darwinConfigurations.{hostname}") which fails for hostnames containing
dots or special chars; instead use the existing attrpath-quoting helper (or
escape/quote the hostname) so the attribute path becomes
darwinConfigurations."hostname" (or equivalent escaped form). Replace the
format! call with a call to the helper that joins/quotes attributes (e.g.
join_attribute("darwinConfigurations", hostname) or
format!("darwinConfigurations.{}", quote_attr(hostname))) so the hostname is
properly quoted/escaped in all cases.
| } = target_installable | ||
| { | ||
| if attribute.is_empty() { | ||
| attribute.push(String::from("darwinConfigurations")); | ||
| attribute.push(hostname); | ||
| *attribute = format!("darwinConfigurations.{hostname}"); | ||
| } |
There was a problem hiding this comment.
Same quoting issue leaks into REPL.
The REPL path has the same format!("darwinConfigurations.{hostname}") problem and will fail for hostnames that include dots/hyphens. Fix alongside the rebuild path.
🤖 Prompt for AI Agents
In src/darwin.rs around lines 222 to 226, the REPL path uses
format!("darwinConfigurations.{hostname}") which relies on named-format parsing
and can mis-handle hostnames with dots/hyphens; replace this with positional
formatting for the hostname (e.g. format!("darwinConfigurations.{}", hostname))
and apply the same change to the rebuild path occurrence so hostnames are
inserted as literal strings rather than parsed format keys.
| Self::Flake { | ||
| reference, | ||
| attribute, | ||
| } => { | ||
| res.push(format!("{reference}#{}", join_attribute(attribute))); | ||
| vec![format!("{reference}#{attribute}")] | ||
| }, | ||
| Self::File { path, attribute } => { | ||
| res.push(String::from("--file")); | ||
| res.push(path.to_str().unwrap().to_string()); | ||
| res.push(join_attribute(attribute)); | ||
| vec![ | ||
| String::from("--file"), | ||
| path.to_str().unwrap().to_string(), | ||
| attribute.to_string(), | ||
| ] | ||
| }, | ||
| Self::Expression { | ||
| expression, | ||
| attribute, | ||
| } => { | ||
| res.push(String::from("--expr")); | ||
| res.push(expression.to_string()); | ||
| res.push(join_attribute(attribute)); | ||
| vec![ | ||
| String::from("--expr"), | ||
| expression.to_string(), | ||
| attribute.to_string(), | ||
| ] | ||
| }, | ||
| Self::Store { path } => res.push(path.to_str().unwrap().to_string()), | ||
| Self::Store { path } => vec![path.to_str().unwrap().to_string()], | ||
| } |
There was a problem hiding this comment.
Restore proper attrpath encoding in to_args.
Encoding the attribute path as a raw string regresses two behaviors:
- When
attributeis empty (common for--file/--exprwithout an attrpath), we now emit an extra empty argument or a trailing"#"(nix build --file file ''/flake#). This breaks vanilla invocations. - We dropped the quoting that
join_attributeused to insert for segments containing dots, spaces, etc. The failingtest_installable_to_argsshows the regression (w#xy.zinstead ofw#x."y.z"), and real hostnames likemacbook-pro.localwill now be misparsed.
Please reinstate the old join/quote helper (or equivalent) and only append the attribute when it is non-empty. For example:
- } => {
- vec![format!("{reference}#{attribute}")]
- },
+ } => {
+ if attribute.is_empty() {
+ vec![reference.clone()]
+ } else {
+ vec![format!("{reference}#{}", encode_attr(attribute))]
+ }
+ },(Apply the same guard in the File/Expression branches.)
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/installable.rs around lines 209 to 233, the current to_args
implementation encodes attribute paths as raw strings which causes an extra
empty arg or trailing "#" when attribute is empty and loses quoting for segments
with dots/spaces; restore the previous join/quote helper (or implement
equivalent logic) to produce properly quoted attrpaths (e.g. turn ["w","xy.z"]
into w#x."y.z"), and only append the attribute component when it is non-empty in
the Flake branch and likewise guard the File and Expression branches so they
don't emit an extra empty argument; leave Store as-is.
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 (1)
src/installable.rs (1)
5-5: Remove unusedColorimport
cargo clippyis already flagging this (see pipeline warning). Dropping the unused import keeps the build clean.-use yansi::{Color, Paint}; +use yansi::Paint;
🧹 Nitpick comments (3)
src/nixos.rs (3)
591-596: Check existence before testing directory status.Calling
is_dir()on a non-existent path can produce unexpected results. Usetry_exists()or verify existence first for more robust error handling.Apply this diff:
- if !bin_dir.is_dir() { + if !bin_dir.try_exists().unwrap_or(false) || !bin_dir.is_dir() { bail!( "VM build output missing bin directory at {}", bin_dir.display() ); }
598-624: Consider simplifying the filter_map chain.The current implementation is correct but verbose. For better readability, consider a more direct iteration pattern.
Example refactor:
- let vm_script = fs::read_dir(&bin_dir) - .wrap_err_with(|| { - format!("Failed to read directory {}", bin_dir.display()) - })? - .filter_map(|entry_result| { - match entry_result { - Ok(entry) => Some(entry), - Err(e) => { - warn!("Error reading entry in {}: {}", bin_dir.display(), e); - None - }, - } - }) - .find_map(|entry| { - let fname = entry.file_name(); - if fname - .to_str() - .is_some_and(|name| name.starts_with("run-") && name.ends_with("-vm")) - { - Some(entry.path()) - } else { - None - } - }) - .ok_or_else(|| { - eyre!("Could not find VM runner script in {}", bin_dir.display()) - })?; + let mut vm_script = None; + for entry_result in fs::read_dir(&bin_dir) + .wrap_err_with(|| format!("Failed to read directory {}", bin_dir.display()))? + { + let entry = match entry_result { + Ok(e) => e, + Err(e) => { + warn!("Error reading entry in {}: {}", bin_dir.display(), e); + continue; + } + }; + if let Some(name) = entry.file_name().to_str() { + if name.starts_with("run-") && name.ends_with("-vm") { + vm_script = Some(entry.path()); + break; + } + } + } + let vm_script = vm_script.ok_or_else(|| { + eyre!("Could not find VM runner script in {}", bin_dir.display()) + })?;
745-758: Consider flattening the nested conditional.The nested
ifin theelsebranch can be simplified for better readability.Apply this diff:
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 (7)
src/commands.rs(2 hunks)src/darwin.rs(5 hunks)src/home.rs(8 hunks)src/installable.rs(8 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 (1)
- src/home.rs
🧰 Additional context used
🧬 Code graph analysis (4)
src/installable.rs (1)
src/commands.rs (3)
new(160-171)new(615-623)new(737-746)
src/update.rs (2)
src/installable.rs (1)
str_kind(260-267)src/commands.rs (3)
new(160-171)new(615-623)new(737-746)
src/darwin.rs (1)
src/util.rs (1)
get_hostname(188-221)
src/nixos.rs (4)
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)src/update.rs (1)
update(5-35)src/generations.rs (1)
describe(170-315)
🪛 GitHub Actions: Test NH
src/installable.rs
[warning] 5-5: unused import: Color
[error] 239-239: installable::test_installable_to_args failed: assertion left == right failed (left: ["w#xy.z"]; right: ["w#x."y.z""])
⏰ 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 Darwin
- GitHub Check: Build NH on Linux
🔇 Additional comments (6)
src/nixos.rs (6)
65-89: LGTM! VM build refactor is clean.The inline attribute derivation and reference passing align well with the broader String-based attribute refactor. The
--runflag implementation is straightforward.
91-155: Excellent refactor! Much improved readability.Breaking down the monolithic rebuild function into focused helper phases (setup, resolve, build, activate) significantly improves maintainability and testability.
157-187: LGTM! Setup phase is well-structured.The setup phase appropriately handles side effects (SSH prompts, updates) before the build, and returns the necessary context (elevation flag, hostname).
797-830: LGTM! Robust generation listing implementation.The error handling with
warnandcontinueis appropriate for directory iteration, ensuring partial failures don't break the entire operation.
832-864: LGTM! Attribute construction aligns with String-based refactor.The transition from
Vec<String>toStringattributes is handled correctly withpush_strinstead ofpush, maintaining the expected installable format.
643-659: LGTM! Well-crafted helper functions.The VM instruction printing, execution, and environment parsing helpers are clean and provide appropriate error handling and user feedback.
Also applies to: 675-693, 695-730
| let nh_flake = env::var("NH_FLAKE").unwrap_or_default(); | ||
| let nh_os_flake = env::var("NH_OS_FLAKE").unwrap_or_default(); | ||
| let nh_home_flake = env::var("NH_HOME_FLAKE").unwrap_or_default(); | ||
| let nh_darwin_flake = env::var("NH_DARWIN_FLAKE").unwrap_or_default(); | ||
| let nh_file = env::var("NH_FILE").unwrap_or_default(); | ||
| let nh_attr = env::var("NH_ATTR").unwrap_or_default(); | ||
|
|
||
| let long_help = format!( | ||
| r"Which installable to use. | ||
| Nix accepts various kinds of installables: | ||
| [FLAKEREF[#ATTRPATH]] | ||
| Flake reference with an optional attribute path. | ||
| [env: NH_FLAKE={nh_flake}] | ||
| [env: NH_OS_FLAKE={nh_os_flake}] | ||
| [env: NH_HOME_FLAKE={nh_home_flake}] | ||
| [env: NH_DARWIN_FLAKE={nh_darwin_flake}] | ||
| {f_short}, {f_long} <FILE> [ATTRPATH] | ||
| Path to file with an optional attribute path. | ||
| [env: NH_FILE={nh_file}] | ||
| [env: NH_ATTRP={nh_attr}] | ||
| {e_short}, {e_long} <EXPR> [ATTRPATH] | ||
| Nix expression with an optional attribute path. | ||
| [PATH] | ||
| Path or symlink to a /nix/store path | ||
| ", | ||
| f_short = "-f".yellow(), | ||
| f_long = "--file".yellow(), | ||
| e_short = "-e".yellow(), | ||
| e_long = "--expr".yellow(), | ||
| ); |
There was a problem hiding this comment.
Read the same env var you advertise
The long help advertises [env: NH_ATTRP=…], but we populate it with the value from NH_ATTR. That makes the help misleading and hides the actual NH_ATTRP value users may have set. Please align the lookup with the documented variable.
- let nh_attr = env::var("NH_ATTR").unwrap_or_default();
+ let nh_attrp = env::var("NH_ATTRP").unwrap_or_default();
@@
- [env: NH_FILE={nh_file}]
- [env: NH_ATTRP={nh_attr}]
+ [env: NH_FILE={nh_file}]
+ [env: NH_ATTRP={nh_attrp}]📝 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.
| let nh_flake = env::var("NH_FLAKE").unwrap_or_default(); | |
| let nh_os_flake = env::var("NH_OS_FLAKE").unwrap_or_default(); | |
| let nh_home_flake = env::var("NH_HOME_FLAKE").unwrap_or_default(); | |
| let nh_darwin_flake = env::var("NH_DARWIN_FLAKE").unwrap_or_default(); | |
| let nh_file = env::var("NH_FILE").unwrap_or_default(); | |
| let nh_attr = env::var("NH_ATTR").unwrap_or_default(); | |
| let long_help = format!( | |
| r"Which installable to use. | |
| Nix accepts various kinds of installables: | |
| [FLAKEREF[#ATTRPATH]] | |
| Flake reference with an optional attribute path. | |
| [env: NH_FLAKE={nh_flake}] | |
| [env: NH_OS_FLAKE={nh_os_flake}] | |
| [env: NH_HOME_FLAKE={nh_home_flake}] | |
| [env: NH_DARWIN_FLAKE={nh_darwin_flake}] | |
| {f_short}, {f_long} <FILE> [ATTRPATH] | |
| Path to file with an optional attribute path. | |
| [env: NH_FILE={nh_file}] | |
| [env: NH_ATTRP={nh_attr}] | |
| {e_short}, {e_long} <EXPR> [ATTRPATH] | |
| Nix expression with an optional attribute path. | |
| [PATH] | |
| Path or symlink to a /nix/store path | |
| ", | |
| f_short = "-f".yellow(), | |
| f_long = "--file".yellow(), | |
| e_short = "-e".yellow(), | |
| e_long = "--expr".yellow(), | |
| ); | |
| let nh_flake = env::var("NH_FLAKE").unwrap_or_default(); | |
| let nh_os_flake = env::var("NH_OS_FLAKE").unwrap_or_default(); | |
| let nh_home_flake = env::var("NH_HOME_FLAKE").unwrap_or_default(); | |
| let nh_darwin_flake = env::var("NH_DARWIN_FLAKE").unwrap_or_default(); | |
| let nh_file = env::var("NH_FILE").unwrap_or_default(); | |
| let nh_attrp = env::var("NH_ATTRP").unwrap_or_default(); | |
| let long_help = format!( | |
| r"Which installable to use. | |
| Nix accepts various kinds of installables: | |
| [FLAKEREF[#ATTRPATH]] | |
| Flake reference with an optional attribute path. | |
| [env: NH_FLAKE={nh_flake}] | |
| [env: NH_OS_FLAKE={nh_os_flake}] | |
| [env: NH_HOME_FLAKE={nh_home_flake}] | |
| [env: NH_DARWIN_FLAKE={nh_darwin_flake}] | |
| {f_short}, {f_long} <FILE> [ATTRPATH] | |
| Path to file with an optional attribute path. | |
| [env: NH_FILE={nh_file}] | |
| [env: NH_ATTRP={nh_attrp}] | |
| {e_short}, {e_long} <EXPR> [ATTRPATH] | |
| Nix expression with an optional attribute path. | |
| [PATH] | |
| Path or symlink to a /nix/store path | |
| ", | |
| f_short = "-f".yellow(), | |
| f_long = "--file".yellow(), | |
| e_short = "-e".yellow(), | |
| e_long = "--expr".yellow(), | |
| ); |
🤖 Prompt for AI Agents
In src/installable.rs around lines 140 to 173, the long help text references the
environment variable NH_ATTRP but the code reads NH_ATTR, causing the help to be
misleading; change the env::var lookup to read "NH_ATTRP" (or change the help to
advertise NH_ATTR if that was intended) so the displayed value matches the
actual variable being read, and update the formatted long_help string
accordingly.
| Self::Flake { | ||
| reference, | ||
| attribute, | ||
| } => { | ||
| res.push(format!("{reference}#{}", join_attribute(attribute))); | ||
| vec![format!("{reference}#{attribute}")] | ||
| }, | ||
| Self::File { path, attribute } => { | ||
| res.push(String::from("--file")); | ||
| res.push(path.to_str().unwrap().to_string()); | ||
| res.push(join_attribute(attribute)); | ||
| vec![ | ||
| String::from("--file"), | ||
| path.to_str().unwrap().to_string(), | ||
| attribute.to_string(), | ||
| ] | ||
| }, | ||
| Self::Expression { | ||
| expression, | ||
| attribute, | ||
| } => { | ||
| res.push(String::from("--expr")); | ||
| res.push(expression.to_string()); | ||
| res.push(join_attribute(attribute)); | ||
| vec![ | ||
| String::from("--expr"), | ||
| expression.to_string(), | ||
| attribute.to_string(), | ||
| ] | ||
| }, | ||
| Self::Store { path } => res.push(path.to_str().unwrap().to_string()), | ||
| Self::Store { path } => vec![path.to_str().unwrap().to_string()], |
There was a problem hiding this comment.
Don’t emit empty attribute arguments
Installable::to_args now always appends an attribute. For flakes this produces reference# when no attribute was supplied, and for files/expressions we pass an empty string as the next CLI argument. Both break previously working invocations (and the updated unit test in the pipeline already fails on this). Please restore the old behaviour where attrpaths are only emitted when present.
Self::Flake {
reference,
attribute,
} => {
- vec![format!("{reference}#{attribute}")]
+ if attribute.is_empty() {
+ vec![reference.clone()]
+ } else {
+ vec![format!("{reference}#{attribute}")]
+ }
},
Self::File { path, attribute } => {
- vec![
- String::from("--file"),
- path.to_str().unwrap().to_string(),
- attribute.to_string(),
- ]
+ let mut args = vec![
+ String::from("--file"),
+ path.to_str().unwrap().to_string(),
+ ];
+ if !attribute.is_empty() {
+ args.push(attribute.clone());
+ }
+ args
},
Self::Expression {
expression,
attribute,
} => {
- vec![
- String::from("--expr"),
- expression.to_string(),
- attribute.to_string(),
- ]
+ let mut args =
+ vec![String::from("--expr"), expression.clone()];
+ if !attribute.is_empty() {
+ args.push(attribute.clone());
+ }
+ args
},📝 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.
| Self::Flake { | |
| reference, | |
| attribute, | |
| } => { | |
| res.push(format!("{reference}#{}", join_attribute(attribute))); | |
| vec![format!("{reference}#{attribute}")] | |
| }, | |
| Self::File { path, attribute } => { | |
| res.push(String::from("--file")); | |
| res.push(path.to_str().unwrap().to_string()); | |
| res.push(join_attribute(attribute)); | |
| vec![ | |
| String::from("--file"), | |
| path.to_str().unwrap().to_string(), | |
| attribute.to_string(), | |
| ] | |
| }, | |
| Self::Expression { | |
| expression, | |
| attribute, | |
| } => { | |
| res.push(String::from("--expr")); | |
| res.push(expression.to_string()); | |
| res.push(join_attribute(attribute)); | |
| vec![ | |
| String::from("--expr"), | |
| expression.to_string(), | |
| attribute.to_string(), | |
| ] | |
| }, | |
| Self::Store { path } => res.push(path.to_str().unwrap().to_string()), | |
| Self::Store { path } => vec![path.to_str().unwrap().to_string()], | |
| Self::Flake { | |
| reference, | |
| attribute, | |
| } => { | |
| if attribute.is_empty() { | |
| vec![reference.clone()] | |
| } else { | |
| vec![format!("{reference}#{attribute}")] | |
| } | |
| }, | |
| Self::File { path, attribute } => { | |
| let mut args = vec![ | |
| String::from("--file"), | |
| path.to_str().unwrap().to_string(), | |
| ]; | |
| if !attribute.is_empty() { | |
| args.push(attribute.clone()); | |
| } | |
| args | |
| }, | |
| Self::Expression { | |
| expression, | |
| attribute, | |
| } => { | |
| let mut args = | |
| vec![String::from("--expr"), expression.clone()]; | |
| if !attribute.is_empty() { | |
| args.push(attribute.clone()); | |
| } | |
| args | |
| }, | |
| Self::Store { path } => vec![path.to_str().unwrap().to_string()], |
🤖 Prompt for AI Agents
In src/installable.rs around lines 209 to 232, Installable::to_args currently
always appends an attribute which produces broken CLI args like "reference#" or
an empty argument for file/expr variants; change the logic so attributes are
only emitted when non-empty: for Flake emit "reference#attribute" only if
attribute is Some/non-empty (otherwise emit just "reference"); for File and
Expression push the "--file"/"--expr" and path/expression entries as before but
only push the attribute argument when it is present and not empty; leave Store
unchanged and ensure no empty string elements are added to the returned vec.
There is no point in making the attribute of an
Installable be a Vec. Attributes can are used in one of:
- Flake: FLAKEREF#ATTRIBUTE:
Here, we cannot pass multiple attributes anyways.
- File, Expression:
The Nix expression (in that file), or any selected attribute,
must evaluate to a derivation.
It is fine to only accept a single attribute here,
since no one is going to add several attributes corresponding
to {nixos,home,darwin}Configurations, here.
faukah
force-pushed
the
faukah/cleanup-installable
branch
from
03361e6 to
afc422b
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 (3)
src/darwin.rs (1)
60-62: Clarify subcommand name in root-check error.Message says “nh os” in a darwin command.
- bail!("Don't run nh os as root. I will call sudo internally as needed"); + bail!("Don't run nh darwin as root. I will call sudo internally as needed");src/home.rs (1)
166-171: Remove unnecessary unsafe block around env::set_var.
env::set_varis safe.- unsafe { - env::set_var("HOME_MANAGER_BACKUP_EXT", ext); - } + env::set_var("HOME_MANAGER_BACKUP_EXT", ext);src/installable.rs (1)
237-256: Update tests to String-based attributes and pass pre-quoted attrpaths.Current tests use
Vec<String>and won’t compile. Provide quoted segments in the single string.fn test_installable_to_args() { assert_eq!( (Installable::Flake { reference: String::from("w"), - attribute: ["x", "y.z"].into_iter().map(str::to_string).collect(), + attribute: r#"x."y.z""#.to_string(), }) .to_args(), vec![r#"w#x."y.z""#] ); assert_eq!( (Installable::File { path: PathBuf::from("w"), - attribute: ["x", "y.z"].into_iter().map(str::to_string).collect(), + attribute: r#"x."y.z""#.to_string(), }) .to_args(), vec!["--file", "w", r#"x."y.z""#] ); }
♻️ Duplicate comments (4)
src/darwin.rs (2)
104-108: Quote hostname in attrpath to support dots/hyphens/spaces.
format!("darwinConfigurations.{hostname}")breaks for*.localand other non-identifier hostnames. Quote the segment.Apply:
- *attribute = format!("darwinConfigurations.{hostname}"); + *attribute = format!("darwinConfigurations.{}", quote_attr_segment(&hostname));Add once in this module:
// helper (module-private) fn quote_attr_segment(s: &str) -> String { let needs_quotes = s.is_empty() || s.chars().any(|c| !(c.is_ascii_alphanumeric() || c == '_' || c == '-' || c == '\'')); if needs_quotes { let escaped = s.replace('"', "\\\""); format!(r#""{}""#, escaped) } else { s.to_string() } }
224-226: Same quoting bug in REPL path.Mirror the fix here.
- *attribute = format!("darwinConfigurations.{hostname}"); + *attribute = format!("darwinConfigurations.{}", quote_attr_segment(&hostname));src/installable.rs (2)
209-233: Don’t emit empty attribute arguments.Avoid
reference#and empty""CLI args.Self::Flake { reference, attribute, } => { - vec![format!("{reference}#{attribute}")] + if attribute.is_empty() { + vec![reference.clone()] + } else { + vec![format!("{reference}#{attribute}")] + } }, Self::File { path, attribute } => { - vec![ - String::from("--file"), - path.to_str().unwrap().to_string(), - attribute.to_string(), - ] + let mut args = vec![String::from("--file"), path.to_str().unwrap().to_string()]; + if !attribute.is_empty() { + args.push(attribute.clone()); + } + args }, Self::Expression { expression, attribute, } => { - vec![ - String::from("--expr"), - expression.to_string(), - attribute.to_string(), - ] + let mut args = vec![String::from("--expr"), expression.clone()]; + if !attribute.is_empty() { + args.push(attribute.clone()); + } + args },
140-146: Help advertises NH_ATTRP but code reads NH_ATTR. Align them.Read and display the same env var.
- let nh_attr = env::var("NH_ATTR").unwrap_or_default(); + let nh_attrp = env::var("NH_ATTRP").unwrap_or_default(); @@ - [env: NH_FILE={nh_file}] - [env: NH_ATTRP={nh_attr}] + [env: NH_FILE={nh_file}] + [env: NH_ATTRP={nh_attrp}]Also applies to: 147-174
🧹 Nitpick comments (1)
src/installable.rs (1)
41-49: Minor: fix typo in local variable name.
canonincal→canonical.- let canonincal = fs::canonicalize(i); + let canonical = fs::canonicalize(i); @@ - if let Ok(p) = canonincal { + if let Ok(p) = canonical {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/commands.rs(2 hunks)src/darwin.rs(4 hunks)src/home.rs(7 hunks)src/installable.rs(8 hunks)src/nixos.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/commands.rs
- src/nixos.rs
🧰 Additional context used
🧬 Code graph analysis (1)
src/installable.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). (5)
- GitHub Check: Build NH on Darwin
- GitHub Check: Build NH on Linux
- GitHub Check: Test NH on Darwin
- GitHub Check: Test NH on Linux
- GitHub Check: treewide-checks
| *attribute = String::from("homeConfigurations"); | ||
|
|
There was a problem hiding this comment.
Fix attrpath construction (compile errors + incorrect paths).
- String::extend with iterator won’t compile for
String. - Missing “.” separators create invalid paths.
- Segments like
username@hostmust be quoted.
Apply targeted changes:
@@
- *attribute = String::from("homeConfigurations");
+ *attribute = String::from("homeConfigurations");
@@
- attribute.push_str(&config_name);
- if push_drv {
- attribute.extend(toplevel.clone());
- }
+ attribute = format!("{attribute}.{}", quote_attr_segment(&config_name));
+ if push_drv {
+ attribute.push_str(".config.home.activationPackage");
+ }
@@
- let tried_attr_path = {
- let mut attr_path = attribute.clone();
- attr_path.push_str(&config_name);
- Installable::Flake {
- reference: flake_reference,
- attribute: attr_path,
- }
- .to_args()
- .join(" ")
- };
+ let tried_attr_path = {
+ let attr_path = format!("{attribute}.{}", quote_attr_segment(&config_name));
+ Installable::Flake {
+ reference: flake_reference,
+ attribute: attr_path,
+ }
+ .to_args()
+ .join(" ")
+ };
@@
- let current_try_attr = {
- let mut attr_path = attribute.clone();
- attr_path.push_str(&attr_name);
- attr_path
- };
+ let current_try_attr =
+ format!("{attribute}.{}", quote_attr_segment(&attr_name));
@@
- attribute.push_str(&attr_name);
- if push_drv {
- attribute.extend(toplevel.clone());
- }
+ attribute = format!("{attribute}.{}", quote_attr_segment(&attr_name));
+ if push_drv {
+ attribute.push_str(".config.home.activationPackage");
+ }
@@
- Installable::File {
- ref mut attribute, ..
- } => {
- if push_drv {
- attribute.extend(toplevel);
- }
- },
+ Installable::File { ref mut attribute, .. } => {
+ if push_drv {
+ attribute.push_str(".config.home.activationPackage");
+ }
+ },
@@
- Installable::Expression {
- ref mut attribute, ..
- } => {
- if push_drv {
- attribute.extend(toplevel);
- }
- },
+ Installable::Expression { ref mut attribute, .. } => {
+ if push_drv {
+ attribute.push_str(".config.home.activationPackage");
+ }
+ },Add once in this module:
fn quote_attr_segment(s: &str) -> String {
let needs_quotes = s.is_empty()
|| s.chars().any(|c| !(c.is_ascii_alphanumeric() || c == '_' || c == '-' || c == '\'')); // dot not allowed in bare names
if needs_quotes {
let escaped = s.replace('"', "\\\"");
format!(r#""{}""#, escaped)
} else {
s.to_string()
}
}Optional cleanup (outside ranges): drop the now-unused toplevel iterator defined at lines 204-207.
Also applies to: 254-257, 263-271, 309-311, 318-321, 351-361
🤖 Prompt for AI Agents
In src/home.rs around lines 223-224 (and similarly at 254-257, 263-271, 309-311,
318-321, 351-361), the attrpath construction is broken: String::extend with an
iterator won't compile, segments aren't joined with "." which yields invalid
paths, and segments containing characters like '@' or '.' (e.g., username@host)
must be quoted. Add the provided quote_attr_segment function once in this
module, replace the current String::extend/concatenation with building the path
by mapping each segment through quote_attr_segment and joining with "." (or
using push_str/format! per segment to form "seg1.seg2"), and ensure segments
that require quoting are wrapped accordingly; optionally remove the now-unused
toplevel iterator at lines 204-207.
| impl Installable { | ||
| #[must_use] | ||
| pub fn to_args(&self) -> Vec<String> { | ||
| let mut res = Vec::new(); | ||
| match self { | ||
| Self::Flake { | ||
| reference, | ||
| attribute, | ||
| } => { | ||
| res.push(format!("{reference}#{}", join_attribute(attribute))); | ||
| vec![format!("{reference}#{attribute}")] | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
Where to handle quoting now that attribute is a String.
With flattened strings, to_args cannot re-infer segment boundaries for quoting. Ensure all constructors (home/darwin/nixos paths) build pre-quoted attrpaths via a shared helper (e.g., quote_attr_segment + join_attr(base, seg)), and keep to_args a pass-through.
Would you like me to add a small shared attrpath module and wire usages in home.rs, darwin.rs, and nixos.rs?
I need to verify the review comment by examining the codebase structure and understanding the current implementation. Let me gather the necessary information.
🏁 Script executed:
#!/bin/bash
# 1. Examine the Installable enum structure and to_args implementation
echo "=== Examining src/installable.rs (Flake variant and to_args) ==="
rg -n -A 20 "enum Installable" src/installable.rs
echo -e "\n=== Checking to_args implementation ==="
rg -n -B 5 -A 30 "fn to_args" src/installable.rs
# 2. Search for Flake construction patterns in related files
echo -e "\n=== Flake construction in home.rs ==="
rg -n "Self::Flake\|Installable::Flake" src/home.rs -C 3
echo -e "\n=== Flake construction in darwin.rs ==="
rg -n "Self::Flake\|Installable::Flake" src/darwin.rs -C 3
echo -e "\n=== Flake construction in nixos.rs ==="
rg -n "Self::Flake\|Installable::Flake" src/nixos.rs -C 3
# 3. Check for existing quoting logic
echo -e "\n=== Searching for quoting-related functions/logic ==="
rg -n "quote\|escape" src/ --type rust
# 4. Verify Installable structure and field types
echo -e "\n=== Full Installable enum structure ==="
rg -n -A 50 "^pub enum Installable" src/installable.rsLength of output: 3025
I need to verify whether the files mentioned in the review comment actually construct Flake instances and understand the actual data flow.
🏁 Script executed:
#!/bin/bash
# 1. List all Rust source files to understand structure
echo "=== All .rs files in src/ ==="
fd -e rs src/
# 2. Search for ANY Flake construction across all files
echo -e "\n=== Searching for Flake construction anywhere ==="
rg -n "Flake\s*{" src/ --type rust -C 2
# 3. Check if home.rs, darwin.rs, nixos.rs exist
echo -e "\n=== Checking for specific files ==="
test -f src/home.rs && echo "home.rs exists" || echo "home.rs NOT FOUND"
test -f src/darwin.rs && echo "darwin.rs exists" || echo "darwin.rs NOT FOUND"
test -f src/nixos.rs && echo "nixos.rs exists" || echo "nixos.rs NOT FOUND"
# 4. Search for functions that might return Installable
echo -e "\n=== Functions returning Installable ==="
rg -n "-> Installable" src/ --type rust -A 5
# 5. Check for references to 'attribute' field being set
echo -e "\n=== Setting 'attribute' field ==="
rg -n "attribute\s*:" src/ --type rust -B 2 -A 1Length of output: 4996
I need to examine where the attribute data is actually constructed in home.rs to understand the concern better.
🏁 Script executed:
#!/bin/bash
# 1. Examine home.rs Flake construction contexts
echo "=== home.rs: Flake construction at lines 239-241 ==="
sed -n '230,250p' src/home.rs
echo -e "\n=== home.rs: Flake construction at lines 265-267 ==="
sed -n '256,275p' src/home.rs
echo -e "\n=== home.rs: Flake construction at lines 332-334 ==="
sed -n '325,340p' src/home.rs
# 2. Look for attribute building logic before construction
echo -e "\n=== home.rs: Search for attribute preparation ==="
rg -n "attribute.*=" src/home.rs -B 2 | head -50
# 3. Check if attribute contains dots or path separators
echo -e "\n=== Comments or hints about attribute format ==="
rg -n "attr|path|segment" src/home.rs -i | head -20
# 4. Verify the test case: is it outdated or revealing old structure?
echo -e "\n=== Installable test cases showing structure ==="
sed -n '237,260p' src/installable.rsLength of output: 4009
Now let me verify the test actually fails with the current code:
🏁 Script executed:
#!/bin/bash
# 1. Check if the test compiles/runs with current code
echo "=== Checking test compilation ==="
rg -n "test_installable_to_args" src/installable.rs -A 30
# 2. Verify current attribute type in all Flake constructions
echo -e "\n=== Current attribute values being passed (sampling) ==="
sed -n '72p' src/home.rs
sed -n '223p' src/home.rs
# 3. Check darwin.rs and nixos.rs for Flake usage
echo -e "\n=== Searching darwin.rs and nixos.rs for Flake ==="
rg -n "Installable::Flake\|Flake {" src/darwin.rs
rg -n "Installable::Flake\|Flake {" src/nixos.rs
# 4. Look for any existing quoting/escaping in attribute handling
echo -e "\n=== Any quote-related processing ==="
rg -n "quote|escape|\"" src/installable.rs | grep -i attrLength of output: 1716
Attributes with dots require quoting that is now impossible with flattened String type.
The test at line 242 confirms this is a real regression: it expects ["x", "y.z"] to produce w#x."y.z", but the current String-based implementation at line 213 has no way to know segment boundaries and will incorrectly output w#x.y.z (invalid Nix syntax).
All Flake construction sites must pre-quote attribute segments before passing to to_args. The quoting logic must move from to_args to constructors in home.rs (lines 72, 223), darwin.rs, and nixos.rs. The test at lines 238–256 is currently broken and must be fixed to match the new String type.
🤖 Prompt for AI Agents
In src/installable.rs around lines 205–214, the Flake arm of Installable
currently stores a flattened String for attribute paths which loses segment
boundaries and breaks quoting for segments containing dots; change the Flake
variant to store Vec<String> (segments) instead of a single String, update
to_args to join segments with '.' without attempting to quote, and remove any
quoting logic from to_args; then move the quoting logic into the Flake
constructors in home.rs (lines ~72 and ~223), darwin.rs and nixos.rs so each
attribute segment that requires quoting (contains dots or other special chars)
is wrapped/escaped at construction time; finally update the failing test around
lines 238–256 to pass and assert on the new Vec<String>-based behavior (e.g.,
input ["x","y.z"] -> constructor produces segments with "y.z" pre-quoted and
to_args emits w#x."y.z").
1 participant
depends on #436.
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