From 8ea2f5bc83b8e0371ba4fc6b28783dc45620a4ad Mon Sep 17 00:00:00 2001 From: Hamish Mackenzie Date: Sat, 21 Mar 2020 18:10:01 +1300 Subject: [PATCH 1/6] Use `subDir` not `src` to construct default names When no name is provided to `cleanSourceWith` or `cleanGit` we currently use `baseNameOf src` as a default. This was cute, but it lead to cache misses. For instance if `x = cleanSourceWith { src = ./.; }` then `baseName src` will be different when `src` resolves to "/nix/store/X" than when it is in a local directory. If people use git worktrees they also may wind up with different values for `name`. Anything that depends on `x.name` will propagate the issue. This change uses `subDir` if present or if it is not it encourages adding a suitable `name` with: * A warning message. * A default name that gives a hint as to why there is no name. --- lib/clean-git.nix | 3 ++- lib/clean-source-with.nix | 31 +++++++++++++++++++++++++++---- 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/lib/clean-git.nix b/lib/clean-git.nix index 7673da5e40..82236bdfc3 100644 --- a/lib/clean-git.nix +++ b/lib/clean-git.nix @@ -59,6 +59,7 @@ then # Identify the .git directory and filter just the files that we need. gitDir = cleanSourceWith ({ + inherit name; filter = path: type: type == "directory" || lib.any (i: (lib.hasSuffix i path)) [ @@ -102,7 +103,7 @@ then gitModules = builtins.path { name = "gitmodules"; path = gitModulesStr; }; gitSubmoduleFiles = cleanSourceWith { - inherit src; + inherit name src; filter = path: type: type == "directory" # TODO get sudmodule directories from `.gitmodules` # and use that to filter directory tree here diff --git a/lib/clean-source-with.nix b/lib/clean-source-with.nix index 2fefe82121..82c6f5e006 100644 --- a/lib/clean-source-with.nix +++ b/lib/clean-source-with.nix @@ -44,6 +44,7 @@ cleanSourceWith = { filter ? _path: _type: true, src, subDir ? "", name ? null }: let subDir' = if subDir == "" then "" else "/" + subDir; + subDirName = __replaceStrings ["/"] ["-"] subDir; # In case this is mixed with older versions of cleanSourceWith isFiltered = src ? _isLibCleanSourceWith; isFilteredEx = src ? _isLibCleanSourceWithEx; @@ -62,10 +63,32 @@ && (filter path type && parentFilter path type)); name' = if name != null then name - else (if isFiltered && src ? name - then src.name - else baseNameOf src) - + (lib.optionalString (origSubDir != "") ("--" + baseNameOf origSubDir)); + else + if subDirName != "" + then if src ? name + then src.name + "-" + subDirName + else "cleaned-" + subDirName + else if src ? name + then src.name + else + # No name was provided and one could not be constructed from + # the `subDirName`. + + # We used to use `baseNameOf src` as a default here. + # This was cute, but it lead to cache misses. For instance if + # `x = cleanSourceWith { src = ./.; }` then `baseName src` + # will be different when `src` resolves to "/nix/store/X" + # than when it is in a local directory. If people use + # git worktrees they also may wind up with different + # values for `name`. Anything that depends on `x.name` will + # propagate the issue. + + # Encourage adding a suitable `name` with: + # * A warning message. + # * A default name that gives a hint as to why there is no name. + __trace ( + "WARNING: `cleanSourceWith` called on ${toString src} without a `name`. " + + "Consider adding `name = \"${baseNameOf src};\"`") "unnamed-cleanSourceWith"; in { inherit origSrc origSubDir origSrcSubDir; filter = filter'; From 0620593eca66ba67fb2d0cd91a79c4139178f6de Mon Sep 17 00:00:00 2001 From: Hamish Mackenzie Date: Mon, 23 Mar 2020 15:53:47 +1300 Subject: [PATCH 2/6] Improve drv names unnamed warning for cleanGit --- lib/clean-git.nix | 9 +++++++-- lib/clean-source-with.nix | 23 +++++++++++++++-------- 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/lib/clean-git.nix b/lib/clean-git.nix index 82236bdfc3..f460dcea14 100644 --- a/lib/clean-git.nix +++ b/lib/clean-git.nix @@ -59,7 +59,8 @@ then # Identify the .git directory and filter just the files that we need. gitDir = cleanSourceWith ({ - inherit name; + caller = "cleanGit"; + name = (if name == null then "" else name + "-") + "gitFiles"; filter = path: type: type == "directory" || lib.any (i: (lib.hasSuffix i path)) [ @@ -103,7 +104,9 @@ then gitModules = builtins.path { name = "gitmodules"; path = gitModulesStr; }; gitSubmoduleFiles = cleanSourceWith { - inherit name src; + caller = "cleanGit"; + name = (if name == null then "" else name + "-") + "gitSubmoduleFiles"; + inherit src; filter = path: type: type == "directory" # TODO get sudmodule directories from `.gitmodules` # and use that to filter directory tree here @@ -144,12 +147,14 @@ then filter = filter_from_list src whitelist; in cleanSourceWith { + caller = "cleanGit"; inherit name src subDir filter; } else trace "gitSource.nix: ${toString src} does not seem to be a git repository,\nassuming it is a clean checkout." ( cleanSourceWith { + caller = "cleanGit"; inherit name src subDir; } ) diff --git a/lib/clean-source-with.nix b/lib/clean-source-with.nix index 82c6f5e006..8ced0a3098 100644 --- a/lib/clean-source-with.nix +++ b/lib/clean-source-with.nix @@ -32,16 +32,23 @@ # https://nixos.org/nix/manual/#builtin-filterSource # # subDir: Descend into a subdirectory in a way that will compose. - # It will be ase if `src = src + "/${subDir}` and filters + # It will be as if `src = src + "/${subDir}` and filters # already applied to `src` will be respected. # # 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. + # If you do not provide a `name` it wil be derived + # from the `subDir`. You should provide `name` or + # `subDir`. If you do not a warning will be displayed + # and the name used will be `unnamed-${caller}`. # - cleanSourceWith = { filter ? _path: _type: true, src, subDir ? "", name ? null }: + # caller: Name of the function used in warning message and + # in the default `unnamed-${caller}` name. Functions + # that are implemented using `cleanSourceWith`, and + # forward a `name` argument, can use this to make + # the message to the use more meaningful. + # + cleanSourceWith = { filter ? _path: _type: true, src, subDir ? "", name ? null + , caller ? "cleanSourceWith" }: let subDir' = if subDir == "" then "" else "/" + subDir; subDirName = __replaceStrings ["/"] ["-"] subDir; @@ -87,8 +94,8 @@ # * A warning message. # * A default name that gives a hint as to why there is no name. __trace ( - "WARNING: `cleanSourceWith` called on ${toString src} without a `name`. " - + "Consider adding `name = \"${baseNameOf src};\"`") "unnamed-cleanSourceWith"; + "WARNING: `${caller}` called on ${toString src} without a `name`. " + + "Consider adding `name = \"${baseNameOf src};\"`") "unnamed-${caller}"; in { inherit origSrc origSubDir origSrcSubDir; filter = filter'; From 2dcf27e618d14cee43a6f63e9e70890d7f42edcc Mon Sep 17 00:00:00 2001 From: Hamish Mackenzie Date: Mon, 23 Mar 2020 22:27:24 +1300 Subject: [PATCH 3/6] Use `unnamed` when `src` has no name. --- lib/clean-source-with.nix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/clean-source-with.nix b/lib/clean-source-with.nix index 8ced0a3098..ead19c3c6e 100644 --- a/lib/clean-source-with.nix +++ b/lib/clean-source-with.nix @@ -74,7 +74,7 @@ if subDirName != "" then if src ? name then src.name + "-" + subDirName - else "cleaned-" + subDirName + else "unnamed-" + subDirName else if src ? name then src.name else From 34a00cf5872e52e93dda9b8116bc17d160a2ac75 Mon Sep 17 00:00:00 2001 From: Hamish Mackenzie Date: Mon, 23 Mar 2020 22:36:22 +1300 Subject: [PATCH 4/6] Use `unnamed-${caller}` to be consistent --- lib/clean-source-with.nix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/clean-source-with.nix b/lib/clean-source-with.nix index ead19c3c6e..bc19989a4d 100644 --- a/lib/clean-source-with.nix +++ b/lib/clean-source-with.nix @@ -74,7 +74,7 @@ if subDirName != "" then if src ? name then src.name + "-" + subDirName - else "unnamed-" + subDirName + else "unnamed-${caller}-" + subDirName else if src ? name then src.name else From 9d98d1c147939f1cbaca84ab52761258e5383d10 Mon Sep 17 00:00:00 2001 From: Hamish Mackenzie Date: Mon, 23 Mar 2020 23:07:03 +1300 Subject: [PATCH 5/6] s/unnamed/source --- lib/clean-source-with.nix | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/clean-source-with.nix b/lib/clean-source-with.nix index bc19989a4d..a0e0faafc5 100644 --- a/lib/clean-source-with.nix +++ b/lib/clean-source-with.nix @@ -39,12 +39,12 @@ # If you do not provide a `name` it wil be derived # from the `subDir`. You should provide `name` or # `subDir`. If you do not a warning will be displayed - # and the name used will be `unnamed-${caller}`. + # and the name used will be `source-${caller}`. # # caller: Name of the function used in warning message and - # in the default `unnamed-${caller}` name. Functions - # that are implemented using `cleanSourceWith`, and - # forward a `name` argument, can use this to make + # in the default `source-${caller}` name. Functions + # that are implemented using `cleanSourceWith` (and + # forward a `name` argument) can use this to make # the message to the use more meaningful. # cleanSourceWith = { filter ? _path: _type: true, src, subDir ? "", name ? null @@ -74,7 +74,7 @@ if subDirName != "" then if src ? name then src.name + "-" + subDirName - else "unnamed-${caller}-" + subDirName + else "source-${caller}-" + subDirName else if src ? name then src.name else @@ -95,7 +95,7 @@ # * A default name that gives a hint as to why there is no name. __trace ( "WARNING: `${caller}` called on ${toString src} without a `name`. " - + "Consider adding `name = \"${baseNameOf src};\"`") "unnamed-${caller}"; + + "Consider adding `name = \"${baseNameOf src};\"`") "source-${caller}"; in { inherit origSrc origSubDir origSrcSubDir; filter = filter'; From 66cfe7bf72ce7b9fb676e430666af557b87a2cde Mon Sep 17 00:00:00 2001 From: Hamish Mackenzie Date: Tue, 24 Mar 2020 00:15:27 +1300 Subject: [PATCH 6/6] Use just `source` as default name https://github.com/NixOS/nix/commit/65b5f177b5fbb1b0778ede047a13a3cee9c59cfe --- lib/clean-source-with.nix | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/lib/clean-source-with.nix b/lib/clean-source-with.nix index a0e0faafc5..dbbc5d62c2 100644 --- a/lib/clean-source-with.nix +++ b/lib/clean-source-with.nix @@ -39,12 +39,11 @@ # If you do not provide a `name` it wil be derived # from the `subDir`. You should provide `name` or # `subDir`. If you do not a warning will be displayed - # and the name used will be `source-${caller}`. + # and the name used will be `source`. # - # caller: Name of the function used in warning message and - # in the default `source-${caller}` name. Functions - # that are implemented using `cleanSourceWith` (and - # forward a `name` argument) can use this to make + # caller: Name of the function used in warning message. + # Functions that are implemented using `cleanSourceWith` + # (and forward a `name` argument) can use this to make # the message to the use more meaningful. # cleanSourceWith = { filter ? _path: _type: true, src, subDir ? "", name ? null @@ -74,7 +73,7 @@ if subDirName != "" then if src ? name then src.name + "-" + subDirName - else "source-${caller}-" + subDirName + else "source-" + subDirName else if src ? name then src.name else @@ -95,7 +94,7 @@ # * A default name that gives a hint as to why there is no name. __trace ( "WARNING: `${caller}` called on ${toString src} without a `name`. " - + "Consider adding `name = \"${baseNameOf src};\"`") "source-${caller}"; + + "Consider adding `name = \"${baseNameOf src};\"`") "source"; in { inherit origSrc origSubDir origSrcSubDir; filter = filter';