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

cleanSourceWith: don't use baseNameOf #83201

Merged
merged 1 commit into from
Mar 24, 2020

Conversation

michaelpj
Copy link
Contributor

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 "unnamed", 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.

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";
Copy link
Member

Choose a reason for hiding this comment

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

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`.
@danbst danbst requested a review from roberth March 24, 2020 06:24
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

💯

@roberth roberth merged commit 1484e1a into NixOS:master Mar 24, 2020
@roberth roberth mentioned this pull request Dec 14, 2020
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants