Skip to content
Permalink

Comparing changes

Choose two branches to see what’s changed or to start a new pull request. If you need to, you can also or learn more about diff comparisons.

Open a pull request

Create a new pull request by comparing changes across two branches. If you need to, you can also . Learn more about diff comparisons here.
base repository: NixOS/nixpkgs
Failed to load repositories. Confirm that selected base ref is valid, then try again.
Loading
base: e93767d5a080
Choose a base ref
...
head repository: NixOS/nixpkgs
Failed to load repositories. Confirm that selected head ref is valid, then try again.
Loading
compare: 1484e1acfeee
Choose a head ref
  • 2 commits
  • 1 file changed
  • 2 contributors

Commits on Mar 23, 2020

  1. cleanSourceWith: don't use baseNameOf 

    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
    michaelpj committed Mar 23, 2020
    Copy the full SHA
    07f363f View commit details

Commits on Mar 24, 2020

  1. Merge pull request #83201 from michaelpj/imp/sources-unnamed 

    cleanSourceWith: don't use baseNameOf
    @roberth
    roberth authored Mar 24, 2020
    Copy the full SHA
    1484e1a View commit details
Showing with 2 additions and 5 deletions.
  1. +2 −5 lib/sources.nix
7 changes: 2 additions & 5 deletions lib/sources.nix
View file
Original file line number Diff line number Diff line change
@@ -63,17 +63,14 @@ rec {
# https://nixos.org/nix/manual/#builtin-filterSource
#
# name: Optional name to use as part of the store path.
# This defaults `src.name` or otherwise `baseNameOf src`.
# We recommend setting `name` whenever `src` is syntactically `./.`.
# Otherwise, you depend on `./.`'s name in the parent directory,
# which can cause inconsistent names, defeating caching.
# This defaults to `src.name` or otherwise `"source"`.
#
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 "source";
in {
inherit origSrc;
filter = filter';