nh-nixos: add delete subcommand to remove and gc specific generations#656
nh-nixos: add delete subcommand to remove and gc specific generations#656d4rk wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthroughThis PR adds a new ChangesOS Delete Subcommand
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 27-29: Update the CHANGELOG entry for `nh os delete` to use the
same gc command form as the implementation by replacing "nix-store gc" with "nix
store gc" so the release note text exactly matches the command invoked by `nh os
delete`.
In `@crates/nh-nixos/src/args.rs`:
- Around line 332-333: Update the help/doc comment text for the no-gc flag to
match the actual command executed: change the current comment "Don't run nix
store --gc" to "Don't run nix store gc" next to the #[arg(long = "no-gc", alias
= "nogc")] attribute so the CLI help correctly reflects the implemented
behavior.
In `@crates/nh-nixos/src/nixos.rs`:
- Around line 1547-1555: When !self.no_boot and the switch_to_configuration path
does not exist, emit a clear warning instead of silently skipping; locate the
block around switch_to_configuration and add a branch that checks
switch_to_configuration.exists() and, if false, calls the project's logging
facility (e.g. tracing::warn! or log::warn!) with a message like "Bootloader
refresh requested but /run/current-system/bin/switch-to-configuration not found;
skipping boot menu update" so users know the post-delete boot refresh was not
performed; keep the existing successful-path
Command::new(switch_to_configuration)... unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9ed35ed0-ee99-45d1-9ea6-4ac594ebbb7d
📒 Files selected for processing (3)
CHANGELOG.mdcrates/nh-nixos/src/args.rscrates/nh-nixos/src/nixos.rs
| - `nh os delete` deletes specific generations by generation ID. Runs | ||
| `nix-store gc` and `switch-to-configuration boot` by default after the | ||
| generations are removed. |
There was a problem hiding this comment.
Use the same GC command form as the implementation.
This entry says nix-store gc, while the command invoked is nix store gc. Aligning phrasing avoids ambiguity in release notes.
Suggested patch
-- `nh os delete` deletes specific generations by generation ID. Runs
- `nix-store gc` and `switch-to-configuration boot` by default after the
+`nh os delete` deletes specific generations by generation ID. Runs
+ `nix store gc` and `switch-to-configuration boot` by default after the
generations are removed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` around lines 27 - 29, Update the CHANGELOG entry for `nh os
delete` to use the same gc command form as the implementation by replacing
"nix-store gc" with "nix store gc" so the release note text exactly matches the
command invoked by `nh os delete`.
| /// Don't run nix store --gc | ||
| #[arg(long = "no-gc", alias = "nogc")] |
There was a problem hiding this comment.
Align --no-gc help text with the executed command.
Line 332 currently says nix store --gc, but the implementation runs nix store gc. Updating this avoids CLI-help confusion.
Suggested patch
- /// Don't run nix store --gc
+ /// Don't run nix store gc📝 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.
| /// Don't run nix store --gc | |
| #[arg(long = "no-gc", alias = "nogc")] | |
| /// Don't run nix store gc | |
| #[arg(long = "no-gc", alias = "nogc")] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/nh-nixos/src/args.rs` around lines 332 - 333, Update the help/doc
comment text for the no-gc flag to match the actual command executed: change the
current comment "Don't run nix store --gc" to "Don't run nix store gc" next to
the #[arg(long = "no-gc", alias = "nogc")] attribute so the CLI help correctly
reflects the implemented behavior.
| if !self.no_boot && switch_to_configuration.exists() { | ||
| Command::new(switch_to_configuration) | ||
| .arg("boot") | ||
| .elevate(elevate.then_some(elevation)) | ||
| .message("Updating bootloader entries") | ||
| .with_required_env() | ||
| .run() | ||
| .wrap_err("Failed to update bootloader after deleting generations")?; | ||
| } |
There was a problem hiding this comment.
Warn when bootloader refresh is requested but hook is unavailable.
If --no-boot is not set and /run/current-system/bin/switch-to-configuration is missing, this currently skips boot menu refresh silently. A warning here would make post-delete state clearer.
Suggested patch
let switch_to_configuration =
Path::new("/run/current-system/bin/switch-to-configuration");
if !self.no_boot && switch_to_configuration.exists() {
Command::new(switch_to_configuration)
.arg("boot")
.elevate(elevate.then_some(elevation))
.message("Updating bootloader entries")
.with_required_env()
.run()
.wrap_err("Failed to update bootloader after deleting generations")?;
+ } else if !self.no_boot {
+ warn!(
+ "Skipping bootloader update: {} not found",
+ switch_to_configuration.display()
+ );
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/nh-nixos/src/nixos.rs` around lines 1547 - 1555, When !self.no_boot
and the switch_to_configuration path does not exist, emit a clear warning
instead of silently skipping; locate the block around switch_to_configuration
and add a branch that checks switch_to_configuration.exists() and, if false,
calls the project's logging facility (e.g. tracing::warn! or log::warn!) with a
message like "Bootloader refresh requested but
/run/current-system/bin/switch-to-configuration not found; skipping boot menu
update" so users know the post-delete boot refresh was not performed; keep the
existing successful-path Command::new(switch_to_configuration)... unchanged.
There was a problem hiding this comment.
Hey there! First of all, thank you for your pull request and thank you for the AI disclosure. <3 I do agree that this is something that nh is currently missing and that this is quite useful.
Two preliminary notes:
- I'm not too happy about this being yet another thing in
nh os. In the long term, I feel like we should add some sort of "tidy" sub-command, which bundles several things like this exact feature. This is something for me and @NotAShelf to consider in the future though; Just adding this tonh oscompletely fine for this PR. - It may make sense to add a check if all of the generations actually exist before we start deleting them. I prefer "all-or-nothing" behavior more than it just failing at some point during running if the user specifies an incorrect generation number.
I left a few review comments on things I spotted a first glance. I'll do a proper review later; I'm still not caffeinated for enough for that yet.
| } | ||
| } | ||
|
|
||
| info!("Deleting generation(s)..."); |
There was a problem hiding this comment.
It'd be nice if it told you here which generations it's about to remove.
|
|
||
| info!("Deleting generation(s)..."); | ||
| for (generation_num, path) in &paths_to_delete { | ||
| Command::new("rm") |
There was a problem hiding this comment.
Let's not shell out to rm when we have a great standard library.
There was a problem hiding this comment.
Is there a way to use remove_file with privilege escalation? This removal, at least on my system, requires sudo.
I tested it by swapping the Command with a simple fs::remove_file(path).context("Failed to remove generation")?;, and if failed. Apologies if I missed the obvious approach.
> Deleting generation(s)...
Error:
0: Failed to remove generation
1: Permission denied (os error 13)
Location:
crates/nh-nixos/src/nixos.rs:1529
Thinking about it some more, another option is to invoke nix-env --delete-generations, also wrapped in an elevated Command. Something like:
nix-env -p /nix/var/nix/profiles/system --delete-generations 90 91
But your FAQ notes that you're trying to move away from shelling out to nix CLI when possible, so I haven't made the change yet. Will await feedback.
| Command::new("nix") | ||
| .args(["store", "gc"]) | ||
| .dry(self.dry) | ||
| .message("Performing garbage collection on the nix store") | ||
| .show_output(true) | ||
| .with_required_env() | ||
| .run()?; | ||
| } |
There was a problem hiding this comment.
instead of calling nix store gc, why not just use nh-clean?
There was a problem hiding this comment.
This is definitely my ignorance, but how would that work? My understanding is that nh clean will perform deletions based on a retention policy (--keep, etc) and then run gc. So a few clarifying questions:
- Is there a way to invoke it to just have it do gc?
- If there is, I assume we don't want to shell it out since that would be
nhspawning itself?
It seems like potentially refactoring this snippet into nh-core and invoking it is one approach, but wondering if that's overkill :)
|
Thanks for the quick initial review! I had a few follow-up questions. I'll make changes based on responses. Thanks! |
2 participants
This pull request introduces a
deletesubcommand tonh osallowing users to delete and garbage collect specific system generations.Examples Usages
nh os delete 80nh os delete 91 92The generation IDs can be obtained from
nh os info.By default, it will also run
nix store gcandswitch-to-configuration bootto remove the boot menu of the pruned generations. These can be controlled withno-gcandno-boot.I'm a new user of
nhand in my workflow I sometimes prefer to prune specific generations rather than everything older than a timestamp, so I decided to try adding support for it tonh.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-darwinAI Disclosure
Per the AI Policy, I'm disclosing that this feature was implemented with the assistance of Antigravity CLI. With that said, I've:
Add a 👍 reaction to pull requests you find important.