Skip to content
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.

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

Refactor generation deletion for better code reuse between CLI tools #9894

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

MichaelOultram
Copy link

@MichaelOultram MichaelOultram commented Jan 31, 2024

Motivation

The nix-env command allows the user to specify the number of generations to keep using the --delete-generations flag by prefixing the argument with a +. This functionality was missing from the nix-collect-garbage --delete-older-than flag.

As pointed out by @Ericson2314, simply adding this functionality to nix-collect-garbage would not be the right approach, and instead the logic should be shared between CLI tools.

Context

From the nix-collect-garbage docs for the --delete-older-than flag mention looking at the nix-env docs to learn more about the <period> argument.

- <span id="opt-delete-older-than">[`--delete-older-than`](#opt-delete-older-than)</span> *period*\
Delete all generations of profiles older than the specified amount (except for the generations that were active at that point in time).
*period* is a value such as `30d`, which would mean 30 days.
This is the equivalent of invoking [`nix-env --delete-generations <period>`](@docroot@/command-ref/nix-env/delete-generations.md#generations-time) on each found profile.
See the documentation of that command for additional information about the *period* argument.

In the nix-env docs, it mentions that + can be used to specify the number of generations to keep.

- <span id="generations-count">`+<number>`</span>:\
The last *number* generations up to the present
*Example*: `+5`
Keep the last *number* generations, along with any newer than current.

However, when I tried to use the nix-collect-garbage in this way, I got an error as the functionality was not implemented.

Jan 31 20:17:24 nixos-zephyrus nix-gc-start[18575]: removing old generations of profile /nix/var/nix/profiles/system
Jan 31 20:17:24 nixos-zephyrus nix-gc-start[18575]: error: invalid number of days specifier '+15', expected something like '14d'
Jan 31 20:17:24 nixos-zephyrus nix-gc-start[18575]: Try '/nix/store/j7nl2pj606d8ld5818hw3z3fbz00sdc5-nix-2.18.1/bin/nix-collect-garbage --help' for more information.

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@Ericson2314
Copy link
Member

Ericson2314 commented Jan 31, 2024

Making the interface actually be in sync is a fine idea, but adding more nix-collect-garbage-specific code is not the right way to do this. We should have a deduplicated implementation for both commands that forces them to stay in sink.

Can you figure out why the commands supported different things in the first place? Answering that question will in turn tell use what code needs to change.

@MichaelOultram
Copy link
Author

@Ericson2314 yes I agree. That would be the better way to do this. I'm not much of a C++ programmer, and it's my first PR to this repo, so I went for the smaller diff.

I'll mark this PR as a draft, and look into a better way to refactor this.

@MichaelOultram MichaelOultram marked this pull request as draft January 31, 2024 21:00
@MichaelOultram
Copy link
Author

MichaelOultram commented Feb 1, 2024

in dcc433d, the --delete-generations flag was added to nix-env. This would accept old to delete all generations except the current one, or a list of specific generations to delete. At this point, nix-collect-garbage could not delete old generations.

In 034b6f6, the --delete-older-than option to nix-collect-garbage. This would make a system call to nix-env to actually remove the generations.

In 4ca5a9d, nix-collect-garbage was changed to avoid calling nix-env. At this point, both tools would accept either the number of days format, or old which would delete all except the current generation.

In 429154b, support for specifying the number of generations to keep was added to nix-env, but not for nix-collect-garbage.

In 4b738fc, nix profile got new functionality: wipe-history. This version only accepts the number of days format, or no argument which would delete all except the current generation.

So my proposal is to add a new function into https://github.com/NixOS/nix/blob/master/src/libstore/profiles.cc which would accept the string parameter, and call the relevant deleteXXX function. Then all three different tools would call this new function instead.

@github-actions github-actions bot added documentation new-cli Relating to the "nix" command labels Feb 5, 2024
@MichaelOultram MichaelOultram changed the title nix-collect-garbage: keep last N number of generations Refactor generation deletion for better code reuse between CLI tools Feb 5, 2024
@Ericson2314
Copy link
Member

Sorry I missed this @MichaelOultram, great job with the deep dive there!

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-02-28-nix-team-meeting-129/40499/1

@MichaelOultram
Copy link
Author

Current status: I believe I've fixed the bug that the first CI run picked up on. I'm leaving this as a draft PR as I'd like to spend a little more time looking if any tests can be added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation new-cli Relating to the "nix" command
Projects
Status: 🏁 Review
Development

Successfully merging this pull request may close these issues.

None yet

3 participants