Skip to content

Make nix-collect-garbage -d look into more places#8154

Merged
Ericson2314 merged 2 commits intoNixOS:masterfrom
tweag:delete-old-on-all-profiles-dir
May 16, 2023
Merged

Make nix-collect-garbage -d look into more places#8154
Ericson2314 merged 2 commits intoNixOS:masterfrom
tweag:delete-old-on-all-profiles-dir

Conversation

@thufschmitt
Copy link
Member

@thufschmitt thufschmitt commented Apr 3, 2023 •
edited by Ericson2314
Loading

Motivation

Make it look into the new-style profiles dir, the old-style one, and the
target of ~/.nix-profile to be sure that we don't miss anything when running
the GC.

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
  • agreed on implementation strategy
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests - tests/nixos/*
  • documentation in the manual
  • documentation in the internal API docs
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or incompatible change: updated release notes

Priorities

Add 👍 to pull requests you find important.

Fix #8294

@thufschmitt thufschmitt requested a review from edolstra as a code owner April 3, 2023 13:34
Ericson2314
Ericson2314 previously approved these changes Apr 3, 2023
View reviewed changes
@Ericson2314
Copy link
Member

Ericson2314 commented Apr 3, 2023

Oh great, the dreaded error: without warning: for SQLite.

@thufschmitt thufschmitt mentioned this pull request May 8, 2023
Closed
@thufschmitt thufschmitt force-pushed the delete-old-on-all-profiles-dir branch from 72baa83 to a57810b Compare May 8, 2023 05:56
@thufschmitt
Copy link
Member Author

thufschmitt commented May 8, 2023

Ping @NixOS/nix-team. This fixes #8294 which is a quite annoying bug

fricklerhandwerk
fricklerhandwerk reviewed May 8, 2023
View reviewed changes
Copy link
Contributor

@fricklerhandwerk fricklerhandwerk left a comment

Choose a reason for hiding this comment

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

Discussed in Nix team meeting:

This needs a test to prevent regressions in the future.

@nixos-discourse
Copy link

nixos-discourse commented May 9, 2023

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

https://discourse.nixos.org/t/2023-05-08-nix-team-meeting-minutes-53/27967/1

@Ericson2314
Copy link
Member

Ericson2314 commented May 9, 2023

I checked some of the review boxes.

@Ericson2314 Ericson2314 dismissed their stale review May 12, 2023 11:09

needs test

Théophane Hufschmitt added 2 commits May 15, 2023 11:36
Make nix-collect-garbage -d look into more places
64b0cc5 
Make it look into the new-style profiles dir, the old-style one, and the
target of `~/.nix-profile` to be sure that we don't miss anything
test the garbage collection with the old profile dir
e97e9e9 
Regression test for NixOS#8294
@thufschmitt thufschmitt force-pushed the delete-old-on-all-profiles-dir branch from a57810b to e97e9e9 Compare May 15, 2023 09:59
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label May 15, 2023
@thufschmitt
Copy link
Member Author

thufschmitt commented May 15, 2023

@Ericson2314 I've added a test for it (which fails without the first commit). PTAL

@thufschmitt thufschmitt requested a review from Ericson2314 May 15, 2023 10:07
Ericson2314
Ericson2314 approved these changes May 16, 2023
View reviewed changes
Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

Looks great!

@Ericson2314
Copy link
Member

Ericson2314 commented May 16, 2023 •
edited
Loading

We need docs too, but this can be done in a follow-up (since this is fixing a regression, there is no intended new behavior).

@Ericson2314 Ericson2314 merged commit 0a715ff into NixOS:master May 16, 2023
@github-actions github-actions bot mentioned this pull request May 16, 2023
Merged
@github-actions
Copy link

github-actions bot commented May 16, 2023

Successfully created backport PR for 2.15-maintenance:

@Ericson2314
Copy link
Member

Ericson2314 commented May 16, 2023 •
edited
Loading

Oh, I suspect this test is incomplete too, and there are still bugs. I will take look into this.

False alarm, my bad. The additional test passed.

@Ericson2314 Ericson2314 mentioned this pull request May 16, 2023
Merged
8 tasks
max-privatevoid added a commit to privatevoid-net/nix-super that referenced this pull request May 19, 2023
@max-privatevoid
Merge remote-tracking branch 'tweag/delete-old-on-all-profiles-dir'
d9e7c97 
Pre-merge of NixOS/nix#8154
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Ericson2314 Ericson2314 Ericson2314 approved these changes

@edolstra edolstra Awaiting requested review from edolstra edolstra is a code owner

+3 more reviewers

@fricklerhandwerk fricklerhandwerk fricklerhandwerk left review comments

@trofi trofi trofi approved these changes

@CobaltCause CobaltCause CobaltCause approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Ericson2314 Ericson2314

Labels

bug with-tests Issues related to testing. PRs with tests have some priority

Projects

Nix team
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

sudo nix-collect-garbage -d does not remove all old generations

6 participants

@thufschmitt @Ericson2314 @nixos-discourse @trofi @fricklerhandwerk @CobaltCause