cleanSourceWith: don't use baseNameOf #83201
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
edolstra
reviewed
Mar 23, 2020
lib/sources.nix
Outdated
| # | ||
| cleanSourceWith = { filter ? _path: _type: true, src, name ? null }: | ||
| let | ||
| isFiltered = src ? _isLibCleanSourceWith; | ||
| origSrc = if isFiltered then src.origSrc else src; | ||
| filter' = if isFiltered then name: type: filter name type && src.filter name type else filter; | ||
| name' = if name != null then name else if isFiltered then src.name else baseNameOf src; | ||
| name' = if name != null then name else if isFiltered then src.name else "unnamed"; |
There was a problem hiding this comment.
This should probably be "source" (unconditionally!) to be in line with other tree fetchers. (See NixOS/nix@65b5f17.)
Currently, not providing `name` to `cleanSourceWith` will use the name of the imported directory. However, a common case is for this to be the top level of some repository. In that case, the name will be the name of the checkout on the current machine, which is not necessarily reproducible across different settings, and can lead to e.g. cache misses in CI. This is documented in the comment on `cleanSourceWith`, but this does not stop it being a subtle trap for users. There are different tradeoffs in each case: 1. If `cleanSourceWith` defaults to `"source"`, then we may end up with a user not knowing what directory a source store path corresponds to. However, it being called "unnamed" may give them a clue that there is a way for them to name it, and lead them to the definition of the function, which has a clear `name` parameter. 2. If `cleanSoureWith` defaults to the directory name, then a user may face occasional loss of caching, which is hard to notice, and hard to track down. Tracking it down likely requires use of more advanced tools like `nix-diff`, and reading the source of a lot of nix code. I think the downside of the status quo is worse. This is really another iteration of NixOS/nix#1305: that led to adding the `name` argument in the first place, this just makes us use a better default `name`.
michaelpj
force-pushed
the
imp/sources-unnamed
branch
from
33d2ac6
to
07f363f
Compare
danbst
approved these changes
Mar 24, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, not providing
nametocleanSourceWithwill use the nameof the imported directory. However, a common case is for this to be the
top level of some repository. In that case, the name will be the name of
the checkout on the current machine, which is not necessarily
reproducible across different settings, and can lead to e.g. cache
misses in CI.
This is documented in the comment on
cleanSourceWith, but this doesnot stop it being a subtle trap for users.
There are different tradeoffs in each case:
If
cleanSourceWithdefaults to"unnamed", then we may end up with auser not knowing what directory a source store path corresponds to.
However, it being called "unnamed" may give them a clue that there is a
way for them to name it, and lead them to the definition of the
function, which has a clear
nameparameter.If
cleanSoureWithdefaults to the directory name, then a user may faceoccasional loss of caching, which is hard to notice, and hard to track
down. Tracking it down likely requires use of more advanced tools like
nix-diff, and reading the source of a lot of nix code.I think the downside of the status quo is worse.
This is really another iteration of
NixOS/nix#1305: that led to adding the
nameargument in the first place, this just makes us use a better default
name.