Print the store paths to be fetched sorted by StorePath name() #7889
Conversation
|
Note: There is another trivial way to implement the functionality in this PR without creating a In this alternative approach, we always store path in the In this alternative approach, we need to simply make a small change to diff --git a/src/libstore/path.hh b/src/libstore/path.hh
index 1e5579b90..5d7cd8b85 100644
--- a/src/libstore/path.hh
+++ b/src/libstore/path.hh
@@ -32,7 +32,10 @@ public:
bool operator < (const StorePath & other) const
{
- return baseName < other.baseName;
+ if (name() == other.name())
+ return baseName < other.baseName;
+ else
+ return name() < other.name();
}
bool operator == (const StorePath & other) constThat's it! Nothing else is needed. I finally avoided this approach because:
|
|
@thufschmitt Your feedback would be invaluable. If this PR is not useful as it currently stands, do let me know. |
|
Please look at the motivation section where there is a comparison of the output before the PR and after the PR. |
There was a problem hiding this comment.
I think the idea makes a lot of sense, but the implementation seems a bit overkill for that.
It feels to me like “just” copying willSubstitute into an ordered data-structure like an std::vector and calling std::sort on it with a custom comparison function (which could be a new static method of nix::StorePath) would make this more local and simpler. Or am I missing anything?
(Obviously that might add a bit more copies and remove some “correct-by-construction” properties of this implementation, but unless there's something else I didn't see, I think the removal of a layer of indirection and boilerplate makes it worth it)
Your suggested approach would definitely work. One of the reasons this might look like a bit boilderplate-y is the creation of a new class And because we are using |
|
That's a fair point :) But it seems to me that we (Nix contributors) are often fairly good at introducing new abstractions, and then hardly ever using it. That's starting to make me very wary of things that “can prove useful” 😛 At a lower level, I'm not entirely convinced by the broad usefulness of For the specifics of
That's indeed true, but I don't think it really matters, does it? We're dumping these paths in the terminal any way, just that feels like it's way more copies than what our |
sidkshatriya
force-pushed
the
sorted-fetch-paths
branch
from
ff9e8a0
to
e38af79
Compare
…t baseName
Presently when nix says something like:
```
these 486 paths will be fetched (511.54 MiB download, 6458.64 MiB unpacked):
...path1
...path2
...path3
...
...
...path486
```
It sorts path1, path2, path3, ..., path486 in lexicographic order of the
store path.
After this commit, nix will show path1, path2, path3, ..., path486 sorted by
StorePath name() (basically everything after the hash) rather than the store path.
This makes it easier to review what exactly is being downloaded at a glance,
especially when many paths need to be fetched.
sidkshatriya
force-pushed
the
sorted-fetch-paths
branch
from
e38af79
to
4275558
Compare
|
This is ready to review again. It is more along the lines of what we discussed and much simpler. |
|
@thufschmitt Gentle ping, do check the PR please -- this is ready to review again |
|
I think all the binary protocols do not assueme sets are serialized in sorted order, so I would consider just changing the comparison functions on |
Thanks -- that's reassuring. And as discussed at #7889 (comment) that would just be a trivial change. This current PR is a conservative change designed to not break anything else, even inadvertently. |
|
Yeah definitely good to start small :) |
Print the store paths to be fetched sorted by StorePath name(), rather than by store path baseName.
Presently when nix says something like:
It sorts path1, path2, path3, ..., path486 in lexicographic order of the store path.
After this commit, nix will show path1, path2, path3, ..., path486 sorted by StorePath name() (basically everything after the hash) rather than the store path.
This makes it easier to review what exactly is being downloaded at a glance, especially when many paths need to be fetched.
Motivation
This makes it easier to review what exactly is being downloaded at a glance, especially when many paths need to be fetched. See an example of
nix-env -iA nixpkgs.firefox --dry-runwith the PR and without the PR below. (Click to uncollapse)Without PR: nix-env -iA nixpkgs.firefox --dry-run
With PR: nix-env -iA nixpkgs.firefox --dry-run
Context
Checklist for maintainers
Maintainers: tick if completed or explain if not relevant
tests/**.shsrc/*/teststests/nixos/*