Skip to content

treewide: cleanup #560

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

Closed
faukah wants to merge 5 commits into from
Closed

treewide: cleanup #560

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
edae9a3
nixos: consolidate OsRebuildArgs and OsRebuildActivateArgs
faukah Feb 10, 2026
25ed937
all: drop `message` field from Command
faukah Feb 10, 2026
46bdee9
home: simplifiy erroring in toplevel_for
faukah Feb 10, 2026
1289c12
home: clean up explicit configuration check in toplevel_for
faukah Feb 10, 2026
db343b7
home: clean up automatic configuration detection in toplevel_for
faukah Feb 10, 2026
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
15 changes: 0 additions & 15 deletions src/commands.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -780,7 +780,6 @@ impl Command {

#[derive(Debug)]
pub struct Build {
message: Option<String>,
installable: Installable,
extra_args: Vec<OsString>,
nom: bool,
Expand All @@ -790,19 +789,12 @@ impl Build {
#[must_use]
pub const fn new(installable: Installable) -> Self {
Self {
message: None,
installable,
extra_args: vec![],
nom: false,
}
}

#[must_use]
pub fn message<S: AsRef<str>>(mut self, message: S) -> Self {
self.message = Some(message.as_ref().to_string());
self
}

#[must_use]
pub fn extra_arg<S: AsRef<OsStr>>(mut self, arg: S) -> Self {
self.extra_args.push(arg.as_ref().to_os_string());
Expand Down Expand Up @@ -838,10 +830,6 @@ impl Build {
///
/// Returns an error if the build command fails to execute.
pub fn run(&self) -> Result<()> {
if let Some(m) = &self.message {
info!("{m}");
}

let installable_args = self.installable.to_args();

let base_command = Exec::cmd("nix")
Expand Down Expand Up @@ -1394,7 +1382,6 @@ mod tests {

let build = Build::new(installable.clone());

assert!(build.message.is_none());
assert_eq!(build.installable.to_args(), installable.to_args());
assert!(build.extra_args.is_empty());
assert!(!build.nom);
Expand All @@ -1408,12 +1395,10 @@ mod tests {
};

let build = Build::new(installable)
.message("Building package")
.extra_arg("--verbose")
.extra_args(["--option", "setting", "value"])
.nom(true);

assert_eq!(build.message, Some("Building package".to_string()));
assert_eq!(build.extra_args, vec![
OsString::from("--verbose"),
OsString::from("--option"),
Expand Down
5 changes: 2 additions & 3 deletions src/darwin.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,10 +110,10 @@ impl DarwinRebuildArgs {

let toplevel = toplevel_for(hostname, installable, "toplevel")?;

info!("Building Darwin configuration");

// If a build host is specified, use remote build semantics
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")?;

Expand Down Expand Up @@ -148,7 +148,6 @@ impl DarwinRebuildArgs {
.extra_arg(&out_path)
.extra_args(&self.extra_args)
.passthrough(&self.common.passthrough)
.message("Building Darwin configuration")
.nom(!self.common.no_nom)
.run()
.wrap_err("Failed to build Darwin configuration")?;
Expand Down
128 changes: 58 additions & 70 deletions src/home.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,10 +96,10 @@ impl HomeRebuildArgs {
self.configuration.clone(),
)?;

info!("Building Home-Manager configuration");

// If a build host is specified, use remote build semantics
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")?;

Expand Down Expand Up @@ -134,7 +134,6 @@ impl HomeRebuildArgs {
.extra_arg(&out_path)
.extra_args(&self.extra_args)
.passthrough(&self.common.passthrough)
.message("Building Home-Manager configuration")
.nom(!self.common.no_nom)
.run()
.wrap_err("Failed to build Home-Manager configuration")?;
Expand Down Expand Up @@ -267,35 +266,19 @@ where
if !attribute.is_empty() {
// Check if the path is too specific
if attribute[0] == "homeConfigurations" {
if attribute.len() > 2 {
bail!(
"Attribute path is too specific: {}. Home Manager only allows \
configuration names. Please either:\n 1. Use the flake \
reference without attributes (e.g., '.')\n 2. Specify only \
the configuration name (e.g., '.#{}')",
attribute.join("."),
attribute.get(1).unwrap_or(&"<unknown>".to_string())
);
}
} else if attribute.len() > 1 {
// User provided ".#myconfig" or similar - prepend homeConfigurations
attribute.insert(0, String::from("homeConfigurations"));
// Re-validate after prepending
if attribute.len() > 2 {
bail!(
"Attribute path is too specific: {}. Home Manager only allows \
configuration names. Please either:\n 1. Use the flake \
reference without attributes (e.g., '.')\n 2. Specify only \
the configuration name (e.g., '.#{}')",
attribute.join("."),
attribute.get(1).unwrap_or(&"<unknown>".to_string())
);
}
bail!(
"Attribute path is too specific: {}. Home Manager only allows \
configuration names. Please either:\n 1. Use the flake \
reference without attributes (e.g., '.')\n 2. Specify only the \
configuration name (e.g., '.#{}')",
attribute.join("."),
attribute.get(1).unwrap_or(&"<unknown>".to_string())
);
}

debug!(
"Using explicit attribute path from installable: {:?}",
attribute
"Using explicit attribute path from installable: {}",
attribute.join(".")
);
return Ok(res);
Comment on lines 266 to 283
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Feb 17, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cd /tmp && git log --oneline -n 20 -- src/home.rs 2>/dev/null || echo "Git not available or file not in repo"

Repository: nix-community/nh

Length of output: 97

🏁 Script executed:

# Search for attribute handling patterns in nixos.rs and darwin.rs
rg -n -A 20 'if attribute\[0\] == "nixosConfigurations"' src/nixos.rs

Repository: nix-community/nh

Length of output: 909

🏁 Script executed:

rg -n -A 20 'if attribute\[0\] == "darwinConfigurations"' src/darwin.rs

Repository: nix-community/nh

Length of output: 940

🏁 Script executed:

# Also check the context around line 279-283 in home.rs to see the alternative code path
sed -n '266,290p' src/home.rs

Repository: nix-community/nh

Length of output: 898

🏁 Script executed:

# Look for any condition checking attribute length for homeConfigurations
rg -n -B 5 -A 15 'homeConfigurations' src/home.rs

Repository: nix-community/nh

Length of output: 1581

Fix attribute length check for homeConfigurations paths to match nixosConfigurations and darwinConfigurations behavior.

The current code unconditionally rejects any path starting with homeConfigurations, but nixos.rs (lines 1215–1230) and darwin.rs (lines 284–304) both allow 2-element paths (e.g., ["nixosConfigurations", "hostname"]) and only reject longer ones. The home.rs code should do the same:

if attribute[0] == "homeConfigurations" {
  if attribute.len() == 2 {
    // homeConfigurations.configname - fine
  } else if attribute.len() > 2 {
    bail!(...)
  }
}

Currently, users passing .#homeConfigurations.myuser get rejected, while the nixos and darwin modules accept the equivalent .#nixosConfigurations.hostname.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/home.rs` around lines 266 - 283, The current check in src/home.rs rejects
any attribute path that starts with "homeConfigurations"; change the logic to
mirror nixos/darwin: when attribute[0] == "homeConfigurations" allow
attribute.len() == 2 (i.e., two-element paths like
["homeConfigurations","name"]) but bail only if attribute.len() > 2 with the
existing error message that uses attribute.join(".") and
attribute.get(1).unwrap_or(...); update the conditional around attribute[0] ==
"homeConfigurations" to check length and only reject overly-specific paths while
permitting exactly two-element paths.

}
Expand All @@ -307,53 +290,53 @@ where

// Check if an explicit configuration name was provided via the flag
if let Some(config_name) = configuration_name {
// Verify the provided configuration exists
let func = format!(r#" x: x ? "{config_name}" "#);
let installable = Installable::Flake {
reference: flake_reference.clone(),
attribute: attribute.clone(),
};

// Verify the provided configuration exists
let check_res = commands::Command::new("nix")
.with_required_env()
.arg("eval")
.args(&extra_args)
.arg("--apply")
.arg(func)
.args(
(Installable::Flake {
reference: flake_reference.clone(),
attribute: attribute.clone(),
})
.to_args(),
)
.args(installable.to_args())
.run_capture()
.wrap_err(format!(
"Failed running nix eval to check for explicit configuration \
'{config_name}'"
))?;
.wrap_err_with(|| {
format!(
"Failed running nix eval to check for explicit configuration \
'{config_name}'"
)
})?;

if check_res.map(|s| s.trim().to_owned()).as_deref() == Some("true") {
debug!("Using explicit configuration from flag: {config_name:?}");
let exists = check_res.as_deref().map(|s| s.trim()) == Some("true");

attribute.push(config_name);
if push_drv {
attribute.extend(toplevel.clone());
if !exists {
let mut error_path = attribute.clone();
error_path.push(config_name);
let full_path = Installable::Flake {
reference: flake_reference,
attribute: error_path,
}

found_config = true;
} else {
// Explicit config provided but not found
let tried_attr_path = {
let mut attr_path = attribute.clone();
attr_path.push(config_name);
Installable::Flake {
reference: flake_reference,
attribute: attr_path,
}
.to_args()
.join(" ")
};
.to_args()
.join(" ");
bail!(
"Explicitly specified home-manager configuration not found: \
{tried_attr_path}"
{full_path}"
);
}

debug!("Using explicit configuration from flag: {config_name:?}");

attribute.push(config_name);
if push_drv {
attribute.extend(toplevel.clone());
}

found_config = true;
}

// If no explicit config was found via flag, try automatic detection
Expand All @@ -365,19 +348,19 @@ where

for attr_name in [format!("{username}@{hostname}"), username] {
let func = format!(r#" x: x ? "{attr_name}" "#);

let installable = Installable::Flake {
reference: flake_reference.clone(),
attribute: attribute.clone(),
};

let check_res = commands::Command::new("nix")
.with_required_env()
.arg("eval")
.args(&extra_args)
.arg("--apply")
.arg(func)
.args(
(Installable::Flake {
reference: flake_reference.clone(),
attribute: attribute.clone(),
})
.to_args(),
)
.args(installable.to_args())
.run_capture()
.wrap_err(format!(
"Failed running nix eval to check for automatic configuration \
Expand All @@ -389,14 +372,19 @@ where
attr_path.push(attr_name.clone());
attr_path
};
tried.push(current_try_attr.clone());
tried.push(current_try_attr);

let exists = check_res.as_deref().map(|s| s.trim()) == Some("true");

if check_res.map(|s| s.trim().to_owned()).as_deref() == Some("true") {
if exists {
debug!("Using automatically detected configuration: {}", attr_name);

attribute.push(attr_name);

if push_drv {
attribute.extend(toplevel.clone());
}

found_config = true;
break;
}
Expand Down
14 changes: 4 additions & 10 deletions src/interface.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ impl OsArgs {
OsSubcommand::Switch(args)
| OsSubcommand::Boot(args)
| OsSubcommand::Test(args) => {
if args.rebuild.uses_flakes() {
if args.uses_flakes() {
Box::new(FlakeFeatures)
} else {
Box::new(LegacyFeatures)
Expand Down Expand Up @@ -167,13 +167,13 @@ impl OsArgs {
#[derive(Debug, Subcommand)]
pub enum OsSubcommand {
/// Build and activate the new configuration, and make it the boot default
Switch(OsRebuildActivateArgs),
Switch(OsRebuildArgs),

/// Build the new configuration and make it the boot default
Boot(OsRebuildActivateArgs),
Boot(OsRebuildArgs),

/// Build and activate the new configuration
Test(OsRebuildActivateArgs),
Test(OsRebuildArgs),

/// Build the new configuration
Build(OsRebuildArgs),
Expand Down Expand Up @@ -263,12 +263,6 @@ pub struct OsRebuildArgs {
/// Skip pre-activation system validation checks
#[arg(long, env = "NH_NO_VALIDATE")]
pub no_validate: bool,
}

#[derive(Debug, Args)]
pub struct OsRebuildActivateArgs {
#[command(flatten)]
pub rebuild: OsRebuildArgs,

/// Show activation logs
#[arg(long, env = "NH_SHOW_ACTIVATION_LOGS")]
Expand Down
Loading