Skip to content

cleanup installable #445

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
+83 −184
Draft

cleanup installable #445

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
190d690
installable: clean up augment_args
Oct 15, 2025
afc422b
installable: make installable attribute be a String
Oct 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/commands.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1124,7 +1124,7 @@ mod tests {
fn test_build_new() {
let installable = Installable::Flake {
reference: "github:user/repo".to_string(),
attribute: vec!["package".to_string()],
attribute: "package".to_string(),
};

let build = Build::new(installable.clone());
Expand All @@ -1140,7 +1140,7 @@ mod tests {
fn test_build_builder_pattern() {
let installable = Installable::Flake {
reference: "github:user/repo".to_string(),
attribute: vec!["package".to_string()],
attribute: "package".to_string(),
};

let build = Build::new(installable)
Expand Down
16 changes: 4 additions & 12 deletions src/darwin.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,10 +86,7 @@ impl DarwinRebuildArgs {
Some(r) => r.to_owned(),
None => return Err(eyre!("NH_DARWIN_FLAKE missing reference part")),
};
let attribute = elems
.next()
.map(crate::installable::parse_attribute)
.unwrap_or_default();
let attribute = elems.next().unwrap_or_default().to_string();

Installable::Flake {
reference,
Expand All @@ -107,8 +104,7 @@ impl DarwinRebuildArgs {
// 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}");
}
Comment on lines 104 to 108
Copy link

@coderabbitai coderabbitai bot Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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.

}

Expand Down Expand Up @@ -205,10 +201,7 @@ impl DarwinReplArgs {
Some(r) => r.to_owned(),
None => return Err(eyre!("NH_DARWIN_FLAKE missing reference part")),
};
let attribute = elems
.next()
.map(crate::installable::parse_attribute)
.unwrap_or_default();
let attribute = elems.next().unwrap_or_default().to_string();

Installable::Flake {
reference,
Expand All @@ -229,8 +222,7 @@ impl DarwinReplArgs {
} = target_installable
{
if attribute.is_empty() {
attribute.push(String::from("darwinConfigurations"));
attribute.push(hostname);
*attribute = format!("darwinConfigurations.{hostname}");
}
Comment on lines 222 to 226
Copy link

@coderabbitai coderabbitai bot Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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.

}

Expand Down
20 changes: 7 additions & 13 deletions src/home.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,7 @@ impl HomeRebuildArgs {
Some(r) => r.to_owned(),
None => return Err(eyre!("NH_HOME_FLAKE missing reference part")),
};
let attribute = elems
.next()
.map(crate::installable::parse_attribute)
.unwrap_or_default();
let attribute = elems.next().unwrap_or_default().to_string();

Installable::Flake {
reference,
Expand Down Expand Up @@ -223,7 +220,7 @@ where
return Ok(res);
}

attribute.push(String::from("homeConfigurations"));
*attribute = String::from("homeConfigurations");

Comment on lines +223 to 224
Copy link

@coderabbitai coderabbitai bot Oct 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Fix attrpath construction (compile errors + incorrect paths).

  • String::extend with iterator won’t compile for String.
  • Missing “.” separators create invalid paths.
  • Segments like username@host must 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.

let flake_reference = reference.clone();
let mut found_config = false;
Expand Down Expand Up @@ -254,7 +251,7 @@ where
if check_res.map(|s| s.trim().to_owned()).as_deref() == Some("true") {
debug!("Using explicit configuration from flag: {config_name:?}");

attribute.push(config_name);
attribute.push_str(&config_name);
if push_drv {
attribute.extend(toplevel.clone());
}
Expand All @@ -264,7 +261,7 @@ where
// Explicit config provided but not found
let tried_attr_path = {
let mut attr_path = attribute.clone();
attr_path.push(config_name);
attr_path.push_str(&config_name);
Installable::Flake {
reference: flake_reference,
attribute: attr_path,
Expand Down Expand Up @@ -309,7 +306,7 @@ where

let current_try_attr = {
let mut attr_path = attribute.clone();
attr_path.push(attr_name.clone());
attr_path.push_str(&attr_name);
attr_path
};
tried.push(current_try_attr.clone());
Expand All @@ -318,7 +315,7 @@ where
check_res.map(|s| s.trim().to_owned()).as_deref()
{
debug!("Using automatically detected configuration: {}", attr_name);
attribute.push(attr_name);
attribute.push_str(&attr_name);
if push_drv {
attribute.extend(toplevel.clone());
}
Expand Down Expand Up @@ -379,10 +376,7 @@ impl HomeReplArgs {
Some(r) => r.to_owned(),
None => return Err(eyre!("NH_HOME_FLAKE missing reference part")),
};
let attribute = elems
.next()
.map(crate::installable::parse_attribute)
.unwrap_or_default();
let attribute = elems.next().unwrap_or_default().to_string();

Installable::Flake {
reference,
Expand Down
Loading
Loading