installable: handle invalid installable resolution more gracefully#486
Conversation
|
Warning Rate limit exceeded@NotAShelf has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 45 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughResolves a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/installable.rs (1)
477-498: Consider extracting shared logic for default flake discovery.The three
try_find_default_for_*functions share significant structural similarity. You could extract a helper to reduce duplication:fn try_find_default_flake( flake_path: &str, fallback_dir: &str, context: &str, // e.g., "os", "home", "darwin" env_vars: &[&str], // e.g., ["NH_FLAKE", "NH_OS_FLAKE"] ) -> color_eyre::Result<Self>This would centralize the file-existence check, warning emission, and error message construction. However, the current implementation is clear and each function is relatively short, so this is optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/darwin.rs(3 hunks)src/home.rs(3 hunks)src/installable.rs(5 hunks)src/nixos.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/nixos.rs (1)
src/installable.rs (1)
try_find_default_for_os(392-414)
src/home.rs (1)
src/installable.rs (1)
try_find_default_for_home(428-466)
src/darwin.rs (1)
src/installable.rs (1)
try_find_default_for_darwin(477-499)
⏰ 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). (7)
- GitHub Check: profile
- GitHub Check: Test NH on Linux
- GitHub Check: Build NH on Linux
- GitHub Check: treewide-checks
- GitHub Check: Test NH on Linux
- GitHub Check: Build NH on Linux
- GitHub Check: Build NH on Darwin
🔇 Additional comments (14)
src/home.rs (3)
86-89: LGTM!The
Unspecifiedresolution is correctly placed after theNH_HOME_FLAKEenvironment variable check but before thetoplevel_forcall, ensuring the installable is always concrete when passed to downstream processing.
371-376: LGTM!The
unreachable!guard with a descriptive message is appropriate here. This ensures that ifUnspecifiedsomehow reachestoplevel_for, it will panic with a clear explanation rather than silently misbehave.
406-409: LGTM!Consistent with the rebuild path—
Unspecifiedis resolved to a default before proceeding totoplevel_for.src/darwin.rs (2)
105-108: LGTM!The
Unspecifiedresolution is correctly placed after theNH_DARWIN_FLAKEcheck and before attribute manipulation, ensuring a concrete installable is used for subsequent processing.
207-232: LGTM!The pattern of first binding
target_installablefrom the env-var check (immutable), then shadowing with a mutable binding for theUnspecifiedresolution is consistent with the approach insrc/nixos.rs. This maintains clarity about when the installable becomes mutable (only after resolution).src/nixos.rs (3)
335-338: LGTM!The
Unspecifiedresolution correctly follows theNH_OS_FLAKEenvironment variable check and precedes thetoplevel_forcall, maintaining the invariant thattoplevel_foralways receives a concrete installable.
924-930: LGTM!The
unreachable!guard with the descriptive panic message is consistent with the guards insrc/home.rsand ensures any unexpectedUnspecifiedreaching this point is caught immediately.
939-949: LGTM!The repl path correctly resolves
Unspecifiedafter checking the environment variable, consistent with the pattern established insrc/darwin.rsandsrc/home.rs.src/installable.rs (6)
26-29: LGTM!The new
Unspecifiedvariant is well-documented, clearly indicating its purpose as a deferred resolution placeholder that must be resolved before use.
144-144: LGTM!Returning
Ok(Self::Unspecified)instead of an error when no installable is found allows the resolution to be deferred to the appropriate command handler, which can then provide context-specific defaults and error messages.
305-311: LGTM!The
unreachable!with a descriptive message correctly enforces the invariant thatUnspecifiedmust be resolved before serialization to CLI arguments.
379-380: LGTM!Returning
"unspecified"for the string representation is appropriate for debugging and logging purposes.
392-413: LGTM!The
try_find_default_for_osfunction correctly checks for the existence of/etc/nixos/flake.nix, warns the user about the fallback, and provides a helpful error message with remediation steps when the default flake is not found.
428-465: LGTM!The
try_find_default_for_homefunction correctly handles theHOMEenvironment variable, checks for the default Home Manager flake location, and provides appropriate error handling for missingHOMEand invalid UTF-8 paths.
Performance Comparison
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
README.md (1)
395-414: Minor style suggestions from static analysis.The attribution section reads well. Static analysis tools suggest optional style improvements:
- Line 400: Consider replacing "grown a lot" with "grown considerably" or "grown significantly"
- Line 411: Consider replacing "very good utility" with "excellent utility" or similar
These are optional refinements and don't affect clarity.
src/darwin.rs (1)
247-316: Consider refactoring to reduce duplication with nixos.rs.The
toplevel_forimplementation is nearly identical to the one innixos.rs(lines 891-952), differing primarily in the configuration namespace (darwinConfigurationsvsnixosConfigurations). The logic is correct and works as intended.To reduce maintenance burden and improve consistency, consider extracting the common logic into a generic helper function that accepts the configuration type as a parameter.
Additionally, note that Store installable handling differs between implementations:
darwin.rs(line 303-305): Returns an errornixos.rs(line 949): Empty block (no-op)Ensure this difference is intentional and documented, or standardize the behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(3 hunks)src/darwin.rs(5 hunks)src/home.rs(4 hunks)src/nixos.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/nixos.rs
🧰 Additional context used
🧬 Code graph analysis (2)
src/home.rs (2)
src/installable.rs (1)
try_find_default_for_home(428-466)src/json.rs (1)
get(41-54)
src/darwin.rs (2)
src/nixos.rs (2)
info(998-1038)toplevel_for(892-953)src/installable.rs (1)
try_find_default_for_darwin(477-499)
🪛 GitHub Actions: Check formating & lints
src/darwin.rs
[error] 272-272: cargo fmt --check failed. Diff indicates: Attribute path is too specific: {}. Please either:\n 1. Use the flake reference without attributes (e.g., '.')\n 2. Specify only the configuration name (e.g., '.#{}')
🪛 LanguageTool
README.md
[style] ~400-~400: Consider a different phrase to strengthen your wording.
Context: ...as had a long history, and it has grown a lot over the years. I, NotAShelf, would fir...
(A_LOT_CONSIDERABLY)
[style] ~411-~411: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...ank you! [nix-output-monitor], is also a very good utility worth a mention, which NH uses ...
(EN_WEAK_ADJECTIVE)
⏰ 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). (9)
- 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: profile
- GitHub Check: Build NH on Darwin
- GitHub Check: Build NH on Linux
- GitHub Check: Test NH on Linux
- GitHub Check: Test NH on Darwin
🔇 Additional comments (9)
README.md (3)
253-257: LGTM! Helpful user guidance.The TIP block provides clear pointers to help resources, improving discoverability of NH's features.
264-274: LGTM! Clear migration examples.The comparison table effectively demonstrates NH's command syntax and helps users understand the migration path.
276-284: LGTM! Documentation aligns with code behavior.The autodiscovery explanation correctly describes the default resolution behavior implemented in the codebase via
Installable::Unspecifiedand thetry_find_default_for_*methods.src/darwin.rs (2)
104-107: LGTM! Consistent default resolution pattern.The default installable resolution for Darwin follows the same pattern as Home Manager and NixOS, ensuring consistent behavior across platforms.
215-218: LGTM! Consistent repl resolution.The Unspecified resolution in the repl path mirrors the rebuild path, ensuring consistent behavior across Darwin subcommands.
src/home.rs (4)
86-89: LGTM! Consistent default resolution.The default installable resolution for Home Manager follows the established pattern, providing a unified experience across all NH subcommands.
224-250: LGTM! Robust attribute path validation.The enhanced validation logic correctly prevents overly specific attribute paths and provides clear, actionable error messages. The double validation (before and after prepending
homeConfigurations) ensures consistent enforcement of the path length constraint.
397-402: LGTM! Proper invariant enforcement.The
unreachable!correctly enforces the invariant thatUnspecifiedinstallables must be resolved before callingtoplevel_for. The descriptive message aids debugging if this assumption is ever violated.
432-435: LGTM! Consistent repl resolution.The Unspecified resolution in the repl path matches the rebuild path, ensuring uniform behavior across Home Manager subcommands.
e2899a6 to
7d38ecc
Compare
19e5be4 to
74a73b0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/util.rs (1)
288-307: Consider usingget_or_initfor consistency with other caches.The current implementation works correctly, but unlike
NIX_VERSION_OUTPUTandNIX_VARIANTwhich useget_or_init, this cache uses a manual check-then-set pattern. Under concurrent access, multiple threads could execute thenix config showcommand before any thread sets the cache.#[cfg_attr(feature = "hotpath", hotpath::measure)] pub fn get_nix_experimental_features() -> Result<HashSet<String>> { - // Try to get cached features first - if let Some(features) = NIX_EXPERIMENTAL_FEATURES.get() { - return Ok(features.clone()); - } - - // Not cached, fetch them - let output = Command::new("nix") - .args(["config", "show", "experimental-features"]) - .run_capture()?; - - // If running with dry=true, output might be None - let enabled_features = output.map_or_else(HashSet::new, |output| { - output.split_whitespace().map(String::from).collect() - }); - - // Cache the result and return - let _ = NIX_EXPERIMENTAL_FEATURES.set(enabled_features.clone()); - Ok(enabled_features) + Ok( + NIX_EXPERIMENTAL_FEATURES + .get_or_init(|| { + Command::new("nix") + .args(["config", "show", "experimental-features"]) + .run_capture() + .ok() + .flatten() + .map_or_else(HashSet::new, |output| { + output.split_whitespace().map(String::from).collect() + }) + }) + .clone(), + ) }Note: This changes the error semantics slightly - errors from
run_capture()would result in an empty set rather than propagating. If error propagation is important, the current approach is acceptable.src/nixos.rs (3)
332-345: Unspecified resolution beforetoplevel_forlooks good; consider aligning withupdate()behaviorResolving
Installable::UnspecifiedviaInstallable::try_find_default_for_os()?before callingtoplevel_forcleanly enforces the “no Unspecified beyond CLI parsing” invariant and matches the PR’s intent to centralize default resolution.One thing to double‑check:
setup_build_contextstill callsupdate(&self.common.installable, …)earlier, which may seeInstallable::Unspecifiedeven whenNH_OS_FLAKEor the OS default would later be used. Ifupdatedoesn’t already handleUnspecifiedgracefully, you might want to either:
- resolve the effective installable once (env → CLI → default) and pass that into both
updateandresolve_installable_and_toplevel, or- explicitly document/ensure that
updatenever receivesUnspecified.
892-953:toplevel_forattribute shaping matches other backends; consider extra validation for multi‑segment non-nixosConfigurationsattributesThe new
Result<Installable>return and theInstallable::Unspecifiedunreachable!arm look correct and enforce the “resolve before use” contract.The
Installable::Flakeattribute logic mirrors the darwin/home patterns and adds a nice guard for over‑specificnixosConfigurations.<host>.<...>paths, with clear remediation text. For attributes that don’t start withnixosConfigurations, you currently:
- treat empty attributes as default (
nixosConfigurations.<hostname>), which is good,- treat a single element as a configuration name (
".#myhost"), which is also good,- but for attributes with multiple segments and a non‑
nixosConfigurationsfirst element, you just prependnixosConfigurationsand don’t re‑validate.That last case can produce slightly surprising paths (e.g., user‑supplied
config.system.build.toplevelwould becomenixosConfigurations.config.system.build.toplevel.config.system.build.<final_attr>). If you want the UX here to match the stricter “only configuration names” story you have for thenixosConfigurationsbranch, you could:
- after
attribute.insert(0, "nixosConfigurations".into()), re‑run a small validation that bails whenattribute.len() > 2, with a message similar to the existing “too specific” error.This would prevent odd composed paths while still supporting the simple
".#myhost"case.
958-969: REPL Unspecified/default handling is correct; consider extracting shared OS-resolution helperResolving
Installable::UnspecifiedviaInstallable::try_find_default_for_os()?inOsReplArgs::run, after checkingNH_OS_FLAKE, aligns the REPL path with the rebuild path and ensuresnh os replbehaves sensibly when no installable is provided.The env → CLI → default logic here closely mirrors
resolve_installable_and_toplevel. To keep behavior in lockstep going forward, it may be worth extracting a small helper like:fn resolve_os_installable(base: Installable) -> Result<Installable> { match base { Installable::Unspecified => Installable::try_find_default_for_os(), other => Ok(other), } }and using it in both places, so future changes to default resolution semantics can be made once.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Cargo.toml(1 hunks)README.md(3 hunks)src/darwin.rs(5 hunks)src/home.rs(4 hunks)src/installable.rs(5 hunks)src/nixos.rs(4 hunks)src/util.rs(12 hunks)
✅ Files skipped from review due to trivial changes (1)
- Cargo.toml
🧰 Additional context used
🧬 Code graph analysis (2)
src/home.rs (1)
src/installable.rs (1)
try_find_default_for_home(428-466)
src/nixos.rs (3)
src/installable.rs (1)
try_find_default_for_os(392-414)src/darwin.rs (3)
toplevel_for(247-316)run(32-44)run(191-244)src/home.rs (3)
toplevel_for(195-406)run(25-37)run(409-452)
🪛 LanguageTool
README.md
[style] ~400-~400: Consider a different phrase to strengthen your wording.
Context: ...as had a long history, and it has grown a lot over the years. I, NotAShelf, would fir...
(A_LOT_CONSIDERABLY)
[style] ~411-~411: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...ank you! [nix-output-monitor], is also a very good utility worth a mention, which NH uses ...
(EN_WEAK_ADJECTIVE)
⏰ 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). (4)
- GitHub Check: profile
- GitHub Check: treewide-checks
- GitHub Check: Test NH on Linux
- GitHub Check: Build NH on Linux
🔇 Additional comments (17)
README.md (1)
253-284: LGTM! Clear and helpful documentation updates.The new TIP block and Command Examples table provide excellent guidance for users transitioning to NH. The side-by-side comparison of old vs. new commands is particularly useful for onboarding.
src/util.rs (3)
25-41: LGTM! Well-designed caching for Nix version output.The
OnceLockpattern correctly ensures thread-safe, one-time initialization. The function gracefully handles command failures by returningNone, which is properly handled by consumers.
56-85: LGTM! Robust variant detection with graceful fallback.The function correctly handles the case where Nix version output is unavailable (dry runs, missing Nix) by logging a warning and defaulting to the mainline Nix variant. The
expectis safe sinceget_or_initguarantees initialization.
181-192: LGTM! Clean integration with cached version output.The function correctly leverages the shared cache and provides clear error messages when the cache is empty or lacks a version string.
src/darwin.rs (3)
104-109: LGTM! Clean resolution of Unspecified installable.The match expression clearly handles the
Unspecifiedvariant by resolving it to a platform-specific default before proceeding with the build.
215-218: LGTM! Consistent Unspecified resolution for REPL.The resolution pattern matches the rebuild path, ensuring consistent behavior across Darwin subcommands.
247-316: LGTM! Well-structured toplevel path construction with comprehensive validation.The function handles all
Installablevariants appropriately:
- Validates and extends Flake paths with proper error messages for overly specific attributes
- Extends File/Expression with toplevel path components
- Correctly rejects Store installables
- Uses
unreachable!for Unspecified, which is correct given the resolution contractThe formatting issue from the previous review has been addressed.
src/home.rs (4)
86-89: LGTM! Consistent Unspecified resolution for Home Manager.The resolution pattern using
try_find_default_for_home()matches the Darwin implementation and correctly falls back to~/.config/home-managerwhen available.
223-256: LGTM! Thorough attribute path validation.The validation correctly handles multiple scenarios:
- Paths already starting with
homeConfigurationsare validated for depth- Other paths get
homeConfigurationsprepended and re-validated- Error messages provide clear remediation steps
The early return at line 256 preserves explicitly provided paths, which is the expected behavior.
397-402: LGTM! Correct enforcement of resolution contract.The
unreachable!correctly documents thatUnspecifiedmust be resolved before reachingtoplevel_for, matching the Darwin implementation.
432-435: LGTM! Consistent Unspecified resolution for Home REPL.The pattern matches the rebuild path, ensuring uniform behavior across Home Manager subcommands.
src/installable.rs (6)
26-30: LGTM! Clean design for deferred installable resolution.The
Unspecifiedvariant with its documentation clearly communicates the intent that it represents a deferred resolution that must be handled before use. This is a good pattern for separating CLI parsing from resolution logic.
144-145: LGTM! Enables platform-specific default resolution.Returning
Ok(Self::Unspecified)instead of an error allows each platform (NixOS, Home Manager, Darwin) to provide its own default resolution logic, which is the core improvement of this PR.
305-311: LGTM! Enforces resolution contract at serialization boundary.Using
unreachable!here correctly enforces thatUnspecifiedmust be resolved before attempting to convert to command arguments, catching programming errors early.
392-414: LGTM! Robust default resolution for NixOS.The function correctly:
- Checks for
/etc/nixos/flake.nixexistence as a file (not directory)- Emits a warning when falling back to the default
- Provides a clear error with actionable remediation steps when no default is available
428-466: LGTM! Comprehensive default resolution for Home Manager.The function properly handles edge cases:
- Missing
HOMEenvironment variable- Invalid UTF-8 in the home directory path
- Non-existent or non-file at the expected location
The error messages are consistent with the NixOS variant.
477-499: LGTM! Consistent default resolution for Darwin.The implementation follows the same pattern as
try_find_default_for_os, checking the standard nix-darwin configuration location at/etc/nix-darwin/flake.nix.
I don't remember why I made this a Clap error but I regret it. Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: I711ba8438fbc9818bb12e0ae2ea79e0e6a6a6964
Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: I93a8a5b255b1b06e86fb7067f9200a1b6a6a6964
…atforms Makes `toplevel_for` return `Result` types for all platforms to properly handle and report invalid attribute paths. This also means that we get to infer correct installables from previously "invalid" installables such as `flake.nix#myHost` where this is normalized to `path/to/flake.nix` and the hostname `myHost` is automatically passed on to the toplevel handler. Additionally: - Fix Darwin validation to catch all invalid paths, not just those with "config" at position 2 - Add re-validation in Home Manager after prepending homeConfigurations to catch invalid paths - Move Darwin's toplevel_for from NixOS import to platform-specific implementation - Remove unnecessary attribute preprocessing in Darwin rebuild and update call site Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: Icb31f494e4734611d704ffbdea300c256a6a6964
Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: Idab3cc18f2efbbdcc11b2219d8b061816a6a6964
Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: Icbed80e22af51bfdb17c971881bb83a06a6a6964
Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: Icd940d9e17ed607fa8e44920b8fe40056a6a6964
Supersedes #471
Since standard tooling uses
/etc/nixos, that is now also the fallback location fornh oswhen the installable is missing. Current implementation ONLY supports flakes, but non-flakes required--fileto be passed anyway, so no behavioral changes for the Nix2 interface.Before:
(lame, confusing)
After:
(fancy, informative)
And for when both main and fallback installables are missing:
The error messages, when not deferred to Nix are nice(r) and informative after the changes. The default paths are taken from the tooling NH replaces (nixos-rebuild, nix-darwin and home-manager CLIs) so the usage is now more consistent with what the users may be used to.
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.