-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Tell user why a given path is live when it can't be deleted #2046
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, thank you! Just some small changes.
src/libstore/gc.cc
Outdated
queryReferrers(i, incomingLinks); | ||
for (auto & j : incomingLinks) | ||
if(canReachRoot(state, alreadyVisited, i)) | ||
gcRootLink = j; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we break
after this?
src/libstore/gc.cc
Outdated
for (auto & j : incomingLinks) | ||
if(canReachRoot(state, alreadyVisited, i)) | ||
gcRootLink = j; | ||
throw Error(format("cannot delete path '%1%': it is still needed by '%2%'") % i % gcRootLink); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be possible for gcRootLink
to be unassigned here? If yes, we should account for that in the error message. If no, can you add an assert before the throw
?
Thanks for the review!
I also clarified some assumptions in the comments.
Should I squash this commit to the previous one?
…On Fri, Apr 06, 2018 at 07:57:13AM +0000, Shea Levy wrote:
shlevy requested changes on this pull request.
Overall looks good, thank you! Just some small changes.
> @@ -786,8 +786,18 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results)
for (auto & i : options.pathsToDelete) {
assertStorePath(i);
tryToDelete(state, i);
- if (state.dead.find(i) == state.dead.end())
- throw Error(format("cannot delete path '%1%' since it is still alive") % i);
+ if (state.dead.find(i) == state.dead.end()) {
+ /* We cannot delete the current path. It is needed by a
+ * derivation that cannot be garbage collected. */
+ PathSet incomingLinks;
+ PathSet alreadyVisited;
+ Path gcRootLink;
+ queryReferrers(i, incomingLinks);
+ for (auto & j : incomingLinks)
+ if(canReachRoot(state, alreadyVisited, i))
+ gcRootLink = j;
Should we `break` after this?
> @@ -786,8 +786,18 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results)
for (auto & i : options.pathsToDelete) {
assertStorePath(i);
tryToDelete(state, i);
- if (state.dead.find(i) == state.dead.end())
- throw Error(format("cannot delete path '%1%' since it is still alive") % i);
+ if (state.dead.find(i) == state.dead.end()) {
+ /* We cannot delete the current path. It is needed by a
+ * derivation that cannot be garbage collected. */
+ PathSet incomingLinks;
+ PathSet alreadyVisited;
+ Path gcRootLink;
+ queryReferrers(i, incomingLinks);
+ for (auto & j : incomingLinks)
+ if(canReachRoot(state, alreadyVisited, i))
+ gcRootLink = j;
+ throw Error(format("cannot delete path '%1%': it is still needed by '%2%'") % i % gcRootLink);
Should it be possible for `gcRootLink` to be unassigned here? If yes, we should account for that in the error message. If no, can you add an assert before the `throw`?
--
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#2046 (review)
|
Thanks! Yeah, please squash, otherwise looks good! |
Alright, done :).
…On Fri, Apr 06, 2018 at 10:17:13AM +0000, Shea Levy wrote:
Thanks! Yeah, please squash, otherwise looks good!
--
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#2046 (comment)
|
src/libstore/gc.cc
Outdated
throw Error(format("cannot delete path '%1%' since it is still alive") % i); | ||
if (state.dead.find(i) == state.dead.end()) { | ||
/* We cannot delete the current path: it is referenced by | ||
at least an intermediate path linked to a GC root. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assumption is not correct. The path might be a GC root itself, or be a temporary / in-memory root. In either case it does not necessarily have a referrer.
src/libstore/gc.cc
Outdated
/* We are sure this path is linked to a GC root. We should | ||
have found something.*/ | ||
assertStorePath(gcRootLink); | ||
throw Error(format("cannot delete path '%1%': it is still referenced by '%2%'") % i % gcRootLink); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should just tell the user about nix-store -q --roots
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My general preference is rather than having a computer telling a user what to do, if at all possible it should just do that for the user. But perhaps that means just showing the output of --roots
on a failure here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing is that the message is incomplete: it shows a single referrer, even though there might be many more. Also, the referrer might be irrelevant: you want to know what roots cause the path to be reachable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say that I can imagine (i.e. remember) all three situations: when a list of roots is the best explanation, when a list of referrers explains everything and the relevance of roots is unclear, and when the best explanation is a randomly selected route from a store path to a root.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[edit] I was talking about implementing the nix-store -q --tree feature without knowing it already exists.
Reassinged to @edolstra due to his feedback |
Print a tree showing the links to the garbage roots when a path cannot be removed from nix-store.
Alright, take 2. This is a suggestion, please do not merge this PR yet. UI ProposalAs noted by @edolstra , showing a random link to a GC root is clearly not a correct way to show what's happening in case of failure. I think a good way to show the references would be to use a tree pretty printer. I made a quick prototype to see if it was possible to implement that without writing a lot of code. Looks like it is. Here's how it looks like when I try to delete my browser for instance:
It may get messy if the path is referenced in a lot of places. @edolstra gave a more succinct alternative in the code review comments: printing the command to use (nix-store -q --roots) to start with and let the user investigate the problem. I don't have a strong opinion on which one to choose (I just wanted to dive in the nix-store GC code, I'm already satisfied). If we decide to go the tree way, I'll need to make some small adjustments before any review. Pretty Printer Code DesignI extracted the already existing tree pretty printer from nix-store and moved it in libstore. The pretty printer now also takes a function as parameter which returns the children for a given path. I still don't have a big picture of the codebase design: I'm not sure this is the best way to accomplish that. TODO before merging
If we go the tree way:
|
@nh2 @copumpkin @edolstra @7c6f434c What do you think about this UI? |
I cannot vote on the validity of the output, but a feature like this is very much appreciated. The tree is somewhat verbose, but it would've helped me already in my quest for why a store path was still alive. I didn't know about |
I ran into this problem again today and came across this issue again. If it's less friction to merge just a mention of |
I'm still waiting for feedback. I'm open to make some modifications on this if needed. This PR seems to be stalled though, I think opening another one suggesting --roots might be a good idea. |
Hello,
This PR changes the error message displayed while trying to remove a path from the store still referenced by a GC root.
To:
This pull request fixes #1810.
Note: this is my first contribution to Nix, I'm not familiar with the code-base yet, I guess you should be extra-cautious while reviewing this.