nixos: implement hints for failing units on activation#465
nixos: implement hints for failing units on activation#465
Conversation
Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: Iaeb37ecaf25b5084719ed7bc18e68da76a6a6964
|
Warning Rate limit exceeded@NotAShelf has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 18 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 (2)
WalkthroughReworks NixOS rebuild/activation into distinct build-only and rebuild-and-activate flows via a new public Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (OsArgs)
participant ActivateArgs as OsRebuildActivateArgs
participant Builder as Builder (build_only / rebuild)
participant Activator as activate_rebuilt_config
participant System as System (profiles / systemctl / bootloader)
CLI->>ActivateArgs: parse args (Switch/Boot/Test -> ActivateArgs)
alt Build-only (Build / BuildVm)
ActivateArgs->>Builder: build_only(...)
Builder-->>CLI: built artifacts + post-build instructions
else Activate path (Switch/Boot/Test)
ActivateArgs->>Builder: rebuild_and_activate (build phase)
Builder-->>ActivateArgs: artifacts (out_path, final_attr)
ActivateArgs->>Activator: activate_rebuilt_config(subcmd, final_attr, show_systemctl_hints)
Activator->>System: perform activation / bootloader steps
System-->>Activator: result (success / unit failures)
alt failure & show_systemctl_hints
Activator-->>CLI: failure + systemctl troubleshooting hints
else
Activator-->>CLI: success or standard error
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/interface.rs(1 hunks)src/nixos.rs(1 hunks)
⏰ 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: treewide-checks
- GitHub Check: Test NH on Linux
- GitHub Check: Build NH on Linux
- GitHub Check: treewide-checks
- GitHub Check: Build NH on Linux
- GitHub Check: Build NH on Darwin
- GitHub Check: Test NH on Linux
🔇 Additional comments (2)
src/interface.rs (1)
240-243: LGTM! Clean flag implementation.The flag is properly configured with both CLI and environment variable support, and defaults to opt-in behavior (false).
src/nixos.rs (1)
373-373: LGTM! Good addition for debugging visibility.Showing output during activation helps users understand what's happening and is consistent with other command executions in this file.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: I56bc6b3e8324c69ce04dec515d07817e6a6a6964
…functions Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: I6ac0a70f12d628c1930fcbb93068318d6a6a6964
Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: I93c4a9daace6cd381ae841c6a3c4e0ce6a6a6964
Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: I409f9f0dae95cd1f2613efff5ab0dd056a6a6964
Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: I77199506d2c8e002a0678c2c67c0369f6a6a6964
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/nixos.rs (1)
160-168: Fixnix build --profile SYSTEM_PROFILEto pass the system closure, not the binary pathThe code at line 263 passes
canonical_out_path(theswitch-to-configurationbinary) tonix build --profile, but--profileexpects a system closure directory, not a binary file. Profiles are versioned directories (normally symlinks) that should point to configuration roots in the store, not individual binaries.Currently:
- Line 221-224:
canonical_out_pathis set to the path of theswitch-to-configurationbinary- Line 263: This binary path is passed to
nix build --profile SYSTEM_PROFILEThis causes
/nix/var/nix/profiles/systemto point to the binary rather than the system closure, breaking generation handling and differing from standardnixos-rebuildbehavior.Fix: Pass
target_profile(the system closure) tonix build --profile, notcanonical_out_path:let canonical_profile = target_profile .canonicalize() .context("Failed to resolve output path")?; let switch_to_configuration = canonical_profile .join("bin") .join("switch-to-configuration") .canonicalize() .context("Failed to resolve switch-to-configuration path")?; // ... later, in the Boot | Switch branch: Command::new("nix") .elevate(...) .args(["build", "--no-link", "--profile", SYSTEM_PROFILE]) - .arg(canonical_out_path) + .arg(canonical_profile.to_str().ok_or_else(|| { + eyre!("canonical_profile path contains invalid UTF-8") + })?) .ssh(...)Also update line 227 to use
&switch_to_configurationdirectly instead ofcanonical_out_pathfor the Test/Switch activation command.
♻️ Duplicate comments (1)
src/nixos.rs (1)
181-188: Activation error handling and systemctl hints are much more robust nowThe new
map_errblock usesformat!("{e:#}")to preserve the full error chain and lowercases it before checking for service/unit failure patterns (e.g. “units failed”, “failed” + “service”/“unit”). Gating the additional systemd troubleshooting hints on both this heuristic andself.show_systemctl_hintsgives users an opt‑in hinting mode without losing detailed error context.Aside from the profile path issue noted separately, this activation error wrapping is a solid improvement over the previous plain‑string match.
Also applies to: 221-254
🧹 Nitpick comments (3)
README.md (1)
33-40: Minor wording/tone cleanups are possible but non-blockingThere are a few small style nits LanguageTool flagged (e.g. “3rd party” → “third‑party”, “cool looking UIs” → “cool‑looking UIs”, “Platform specific” → “Platform‑specific”, “grown a lot over the years”, “talked about NH”). None of these block understanding, but tightening phrasing and adding hyphens where appropriate would polish the README further if you’re touching this text again.
Also applies to: 45-52, 55-58, 71-82, 233-237, 372-399
src/interface.rs (1)
248-256: Confirm clap env parsing semantics for NH_SHOW_SYSTEMCTL_HINTS
show_systemctl_hints: boolwith#[arg(long, env = "NH_SHOW_SYSTEMCTL_HINTS")]will use clap’s default boolean parsing (accepting e.g.1/0,true/false, etc.). The README and changelog only mention"1".You might want to clarify accepted values in docs or explicitly use a
BoolishValueParserif you want stricter semantics; otherwise the implementation is fine as‑is.If you want to double‑check clap’s current bool/env behavior for your version, please consult the latest clap 4.x docs.
src/nixos.rs (1)
101-147: Rebuild/activate refactor is a nice separation, with a couple of small cleanups possibleThe new split between:
OsRebuildActivateArgs::rebuild_and_activate(Boot/Switch/Test),OsRebuildArgs::build_only(Build/BuildVm), and- helper methods (
setup_build_context,determine_output_path,resolve_installable_and_toplevel,resolve_specialisation_and_profile,handle_dix_diff)is a big improvement in readability and reusability. The build phase is clearly separated from activation and VM handling.
A couple of minor nits:
In
rebuild_and_activate, the VM‑specific warning branch:if self.rebuild.hostname.is_none() && matches!(variant, OsRebuildVariant::BuildVm) && final_attr.is_some_and(|attr| attr == "vm" || attr == "vmWithBootLoader") { ... }is currently unreachable because
rebuild_and_activateis only called withBoot,Switch, orTest(BuildVm usesOsRebuildArgs::build_only). This can be removed or moved alongside the BuildVm call site for clarity.The comment above
OsRebuildArgs::build_onlysays “Used by Build subcommand which doesn't activate,” but it is also used byBuildVm. Updating the comment to reflect both callers would avoid confusion.Otherwise the flow and flag handling (including
--askand--drysemantics for activation vs pure builds) look consistent.Also applies to: 324-376, 444-476
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.md(2 hunks)Cargo.toml(1 hunks)README.md(7 hunks)src/interface.rs(3 hunks)src/nixos.rs(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- Cargo.toml
🧰 Additional context used
🧬 Code graph analysis (1)
src/nixos.rs (1)
src/commands.rs (7)
args(255-264)elevate(220-223)message(268-271)message(676-679)new(205-216)new(665-673)new(788-797)
🪛 LanguageTool
README.md
[grammar] ~39-~39: Use a hyphen to join words.
Context: ..._ tool that brings together relevant 3rd party projects that you might be familia...
(QB_NEW_EN_HYPHEN)
[grammar] ~54-~54: Use a hyphen to join words.
Context: ...se. I mean who does not love some cool looking UIs? - **Build-tree Visualizat...
(QB_NEW_EN_HYPHEN)
[style] ~72-~72: This phrasing can be wordy. For improved clarity, try opting for something more concise.
Context: ...port into a single, convenient utility. For the time being, this appears in NH's interface as supp...
(FOR_THE_TIME_BEING)
[grammar] ~74-~74: Use a hyphen to join words.
Context: ...provide a familiar, convenient and good looking interface for users of any and ...
(QB_NEW_EN_HYPHEN)
[style] ~75-~75: ‘any and all’ might be wordy. Consider a shorter alternative.
Context: ...d good looking interface for users of any and all of those projects. The familiar interf...
(EN_WORDINESS_PREMIUM_ANY_AND_ALL)
[grammar] ~233-~233: Use a hyphen to join words.
Context: ...width="750px" >
(QB_NEW_EN_HYPHEN)
[grammar] ~235-~235: Use a hyphen to join words.
Context: ... Platform Specific Subcommands Platform specific subcommands are those that impl...
(QB_NEW_EN_HYPHEN)
[style] ~372-~372: 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] ~399-~399: This phrase can be considered informal. To elevate your writing, consider using a more professional alternative.
Context: ... to everyone who has contributed to NH, talked about NH or criticized NH to allow further im...
(TALK_ABOUT_DISCUSS)
⏰ 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: Test NH on Linux
- GitHub Check: Build NH on Darwin
- GitHub Check: Test NH on Linux
- GitHub Check: Test NH on Darwin
- GitHub Check: Build NH on Linux
🔇 Additional comments (3)
CHANGELOG.md (1)
39-40: Changelog otherwise aligns with interface changesThe nushell support and the whitespace‑splitting fix both look consistent with the code changes (Shell::Nushell variant and argument splitting changes elsewhere). No further action needed here.
Also applies to: 50-51
src/interface.rs (2)
127-151: OsRebuildActivateArgs wrapper and feature detection look soundSwitch/Boot/Test now carry
OsRebuildActivateArgs, andget_feature_requirementscorrectly defers toargs.rebuild.uses_flakes(), so feature probing should remain equivalent to the old behavior. The wrapper’s#[command(flatten)] pub rebuild: OsRebuildArgskeeps the existing flags (incl.--install-bootloader,--hostname, etc.) available while adding--show-systemctl-hints/NH_SHOW_SYSTEMCTL_HINTS.This API shape looks good and should be stable for future activation‑only flags.
Also applies to: 248-256
165-190: Nushell completion wiring appears correct; ensure generator side is updatedAdding
Shell::Nushellwith#[value(alias = "nu", name = "nushell")]matches the changelog entry and should letnh completions nushell/nuparse correctly. Assuming the completions generation code (whereCompletionArgs.shellis matched) has a corresponding arm forShell::Nushell, this change looks complete.Just confirm the completions handler covers the new variant and that nushell output matches expectations.
Also applies to: 597-612
Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: I1b8885d66d336c909649b2d1eeecc8c36a6a6964
Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: Ic13023a29ce685ef3e70aab9b2ac39296a6a6964
Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: Ifc1259e04dbd07f8e0a070c9c0ee9d966a6a6964
Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: I93ca9d405f9db21d1b568ee539dd17b86a6a6964
Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: Ibd9649bff189259e40f7f263f4a8ca606a6a6964
Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: Idf6731340294c154277cecb093df03c16a6a6964
Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: Id9a16ef19e691b929863631628e5080d6a6a6964
Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: I16eeefc1f66a0cd0f470e5b00bdb46956a6a6964
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
CHANGELOG.md (1)
39-47: Changelog entry overstates systemctl hints behavior.This entry claims the flag "displays failing systemd units at the end of the rebuild log," but the actual implementation provides generic systemd troubleshooting hints rather than parsing and listing specific failed units.
README.md (1)
307-311: Environment variable documentation overstates current behavior.The description claims NH will "parse activation logs and isolate specific unit failures to display them at the end," but the actual implementation provides generic systemd troubleshooting hints when activation fails, rather than parsing and listing specific failed units.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.md(2 hunks)README.md(7 hunks)xtask/Cargo.toml(1 hunks)xtask/src/main.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
xtask/src/main.rs (2)
xtask/src/comp.rs (1)
generate(40-67)xtask/src/man.rs (1)
generate(6-101)
🪛 LanguageTool
README.md
[grammar] ~39-~39: Use a hyphen to join words.
Context: ..._ tool that brings together relevant 3rd party projects that you might be familia...
(QB_NEW_EN_HYPHEN)
[grammar] ~53-~53: Use a hyphen to join words.
Context: ...se. I mean who does not love some cool looking UIs? - **Build-tree Visualizat...
(QB_NEW_EN_HYPHEN)
[style] ~71-~71: This phrasing can be wordy. For improved clarity, try opting for something more concise.
Context: ...port into a single, convenient utility. For the time being, this appears in NH's interface as supp...
(FOR_THE_TIME_BEING)
[grammar] ~73-~73: Use a hyphen to join words.
Context: ...provide a familiar, convenient and good looking interface for users of any and ...
(QB_NEW_EN_HYPHEN)
[style] ~74-~74: ‘any and all’ might be wordy. Consider a shorter alternative.
Context: ...d good looking interface for users of any and all of those projects. The familiar interf...
(EN_WORDINESS_PREMIUM_ANY_AND_ALL)
[grammar] ~232-~232: Use a hyphen to join words.
Context: ...width="750px" >
(QB_NEW_EN_HYPHEN)
[grammar] ~234-~234: Use a hyphen to join words.
Context: ... Platform Specific Subcommands Platform specific subcommands are those that impl...
(QB_NEW_EN_HYPHEN)
[grammar] ~275-~275: Ensure spelling is correct
Context: ...will call the necessary script to force and installation of your bootloader. Th...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~371-~371: 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] ~398-~398: This phrase can be considered informal. To elevate your writing, consider using a more professional alternative.
Context: ... to everyone who has contributed to NH, talked about NH or criticized NH to allow further im...
(TALK_ABOUT_DISCUSS)
⏰ 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: treewide-checks
- GitHub Check: Test NH on Linux
- GitHub Check: profile
- GitHub Check: Build NH on Linux
🔇 Additional comments (5)
xtask/Cargo.toml (2)
8-13: Dependency additions support the new completion generation flow.The additions of
clap_complete,clap_complete_nushell, and the version bump ofclap_mangenproperly support the migration of completion generation to the xtask workflow.
8-13: xtask/Cargo.toml appears correctly formatted per project rules; verify via CI or local Taplo run.The project's
.taplo.tomlspecifiesalign_entries = truewithcolumn_width = 100. The currentxtask/Cargo.tomlformatting shows properly aligned entries in both[package]and[dependencies]sections, consistent with these rules. However, since the Taplo tool is unavailable in this environment, runtaplo fmt --check xtask/Cargo.tomllocally or confirm the formatting passes CI checks.xtask/src/main.rs (3)
20-39: Command enum enhancements improve CLI usability.The updated Command enum with explicit defaults for output directories and the new Dist variant for generating both manpages and completions together provides a cleaner, more intuitive interface.
41-47: Improved error handling in main function.The change to return
Resultand the explicit error handling for directory changes with a clear error message is a significant improvement over previous panic-based error handling.
54-69: Parallel generation in Dist command is well-implemented.The parallel execution of manpage and completion generation is properly implemented with:
- Thread panic handling
- Proper error propagation
- No resource conflicts (separate output directories)
Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: I0944f48b5bf599e95c3edea5be84214e6a6a6964
Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: Id752500f053253475e013c0b0f6f449f6a6a6964
The commit message makes this sound cooler than it actually is... Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: Ib5b3ab53d313122aec242b01703313f46a6a6964
Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: I089c6facfb8ef8c77e7be162440a83ac6a6a6964
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
xtask/src/main.rs (1)
20-38: CLI wiring forMan/Completions/Distlooks good; consider de-duplicating default output dirsThe new subcommands and flags are consistent with
man::generate/comp::generate, andshell: Option<CompletionShell>maps cleanly to “all shells if omitted”. One small improvement:Command::Disthard-codes"man"and"comp"while theMan/Completionsvariants have their own default directories. If those defaults ever change,Distwill silently diverge.Consider centralizing the default paths (e.g., shared constants or a helper) and using them both in the clap attributes and in the
Distbranch to keep behavior in sync.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/darwin.rs(2 hunks)xtask/src/main.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
xtask/src/main.rs (2)
xtask/src/comp.rs (1)
generate(40-67)xtask/src/man.rs (1)
generate(6-101)
⏰ 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: treewide-checks
- GitHub Check: profile
- GitHub Check: Build NH on Linux
- GitHub Check: Test NH on Linux
- GitHub Check: Build NH on Linux
- GitHub Check: treewide-checks
- GitHub Check: Test NH on Linux
🔇 Additional comments (3)
src/darwin.rs (2)
62-64: LGTM! Improved error message accuracy.The updated error message correctly references "nh darwin" instead of "nh os", which provides clearer guidance to users running Darwin-specific commands.
77-77: LGTM! Platform-specific temp directory naming.The temp directory prefix now correctly uses "nh-darwin" instead of "nh-os", which improves debugging clarity and aligns with the Darwin-specific context of this module.
xtask/src/main.rs (1)
41-66: The review comment is based on an incorrect premise and should be ignoredThe core claim—that "String does not implement std::error::Error, so String → Box via .into() will not type-check"—is false. The Rust standard library implements From for Box by wrapping the String in a private StringError newtype, allowing String values to be converted into Box via .into().
The current code is valid and will compile without modification. The
.map_err(|e| e.into())conversions correctly transformStringerrors intoBox<dyn Error>through the standard library's conversion implementation. No changes are required.Likely an incorrect or invalid review comment.
|
Okay, I'm done. 4AM is clearly not a good time to implement your things. |
Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: I45dc425610d50916430154c0e03f47946a6a6964
Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: Iba944c182f60967216d0d52626a39d926a6a6964
|
as |
|
I'm a little confused to why you would need to do this on startup? The current package should install them as the previous one did, but I guess this requires you to be using NixOS? P.S. I'm back at the hospital today and won't have access to my main system for a few days. Feel free to create an issue about it. |
Initial support for better handling of activation errors in the NixOS module. The implementation is complete, but is not available in Home Manager and Darwin platforms due to lack of maintainer time.
Implements systemctl hints feature akin to
nixos-rebuildthat displays a hint for failing systemctl services if the user opts in.Summary by CodeRabbit
New Features
Removed
Bug Fixes
Documentation
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.