Skip to content

Comments

lib.cleanSourceWith: Allow name to be set, optional filter, doc#67996

Merged
infinisil merged 1 commit intoNixOS:masterfrom
roberth:cleanSourceWith-name
Sep 6, 2019
Merged

lib.cleanSourceWith: Allow name to be set, optional filter, doc#67996
infinisil merged 1 commit intoNixOS:masterfrom
roberth:cleanSourceWith-name

Conversation

@roberth
Copy link
Member

@roberth roberth commented Sep 3, 2019

Motivation for this change

Worktree locations were causing hash changes. With the name parameter you can avoid that.

To quote the new inline doc: 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.

From the commit message

This change is API-compatible and hash-compatible with the previous
version.

At first I considered to write a rename function too, but adding
it name to cleanSourceWith was a no-brainer for ease of use. It
turns out that a rename function isn't any more useful than
cleanSourceWith.
To avoid having to write the identity predicate when renaming,
the filter is now optional.

builtins.path is supported since Nix 2.0 which is required by nixpkgs

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

This change is API-compatible and hash-compatible with the previous
version.

At first I considered to write a rename function too, but adding
it name to cleanSourceWith was a no-brainer for ease of use. It
turns out that a rename function isn't any more useful than
cleanSourceWith.
To avoid having to write the identity predicate when renaming,
the filter is now optional.

builtins.path is supported since Nix 2.0 which is required by nixpkgs
@roberth roberth requested review from edolstra and nbp as code owners September 3, 2019 08:56
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Sep 3, 2019
@roberth roberth requested a review from infinisil September 6, 2019 10:58
@roberth roberth mentioned this pull request Sep 6, 2019
14 tasks
@roberth roberth requested a review from nh2 September 6, 2019 18:53
@FRidh FRidh added this to the 19.09 milestone Sep 6, 2019
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Nice, LGTM!

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: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants