From 74f2e495436983b9b3d8c9a94a638bb411cfb245 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Tue, 12 Dec 2023 01:19:00 +0100 Subject: [PATCH 1/3] lib.fileset.fetchGit: Refactoring --- lib/fileset/internal.nix | 42 +++++++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 18 deletions(-) diff --git a/lib/fileset/internal.nix b/lib/fileset/internal.nix index 9de5590d3eff..c8a659cd09fd 100644 --- a/lib/fileset/internal.nix +++ b/lib/fileset/internal.nix @@ -861,28 +861,34 @@ rec { # Type: String -> String -> Path -> Attrs -> FileSet _fromFetchGit = function: argument: path: extraFetchGitAttrs: let - # This imports the files unnecessarily, which currently can't be avoided - # because `builtins.fetchGit` is the only function exposing which files are tracked by Git. - # With the [lazy trees PR](https://github.com/NixOS/nix/pull/6530), - # the unnecessarily import could be avoided. - # However a simpler alternative still would be [a builtins.gitLsFiles](https://github.com/NixOS/nix/issues/2944). - fetchResult = fetchGit ({ - url = path; - } // extraFetchGitAttrs); + tryFetchGit = + let + # This imports the files unnecessarily, which currently can't be avoided + # because `builtins.fetchGit` is the only function exposing which files are tracked by Git. + # With the [lazy trees PR](https://github.com/NixOS/nix/pull/6530), + # the unnecessarily import could be avoided. + # However a simpler alternative still would be [a builtins.gitLsFiles](https://github.com/NixOS/nix/issues/2944). + fetchResult = fetchGit ({ + url = path; + } // extraFetchGitAttrs); + in + if inPureEvalMode then + throw "lib.fileset.${function}: This function is currently not supported in pure evaluation mode, since it currently relies on `builtins.fetchGit`. See https://github.com/NixOS/nix/issues/9292." + # We can identify local working directories by checking for .git, + # see https://git-scm.com/docs/gitrepository-layout#_description. + # Note that `builtins.fetchGit` _does_ work for bare repositories (where there's no `.git`), + # even though `git ls-files` wouldn't return any files in that case. + else if ! pathExists (path + "/.git") then + throw "lib.fileset.${function}: Expected the ${argument} (${toString path}) to point to a local working tree of a Git repository, but it's not." + else + _mirrorStorePath path fetchResult.outPath; + in - if inPureEvalMode then - throw "lib.fileset.${function}: This function is currently not supported in pure evaluation mode, since it currently relies on `builtins.fetchGit`. See https://github.com/NixOS/nix/issues/9292." - else if ! isPath path then + if ! isPath path then throw "lib.fileset.${function}: Expected the ${argument} to be a path, but it's a ${typeOf path} instead." else if pathType path != "directory" then throw "lib.fileset.${function}: Expected the ${argument} (${toString path}) to be a directory, but it's a file instead." - # We can identify local working directories by checking for .git, - # see https://git-scm.com/docs/gitrepository-layout#_description. - # Note that `builtins.fetchGit` _does_ work for bare repositories (where there's no `.git`), - # even though `git ls-files` wouldn't return any files in that case. - else if ! pathExists (path + "/.git") then - throw "lib.fileset.${function}: Expected the ${argument} (${toString path}) to point to a local working tree of a Git repository, but it's not." else - _mirrorStorePath path fetchResult.outPath; + tryFetchGit; } From 4a70c1e4da8bd66fe27885d1fba54ffee775e3da Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Wed, 13 Dec 2023 05:42:19 +0100 Subject: [PATCH 2/3] lib.fileset.gitTracked: Support out-of-tree builds --- lib/fileset/default.nix | 6 +++++ lib/fileset/internal.nix | 30 ++++++++++++++++++--- lib/fileset/tests.sh | 56 +++++++++++++++++++++++++++++++++++++--- 3 files changed, 86 insertions(+), 6 deletions(-) diff --git a/lib/fileset/default.nix b/lib/fileset/default.nix index 18acb9a980ca..c007b60def0a 100644 --- a/lib/fileset/default.nix +++ b/lib/fileset/default.nix @@ -763,6 +763,12 @@ in { Create a file set containing all [Git-tracked files](https://git-scm.com/book/en/v2/Git-Basics-Recording-Changes-to-the-Repository) in a repository. The first argument allows configuration with an attribute set, while the second argument is the path to the Git working tree. + + `gitTrackedWith` does not perform any filtering when the path is a [Nix store path](https://nixos.org/manual/nix/stable/store/store-path.html#store-path) and not a repository. + In this way, it accommodates the use case where the expression that makes the `gitTracked` call does not reside in an actual git repository anymore, + and has presumably already been fetched in a way that excludes untracked files. + Fetchers with such equivalent behavior include `builtins.fetchGit`, `builtins.fetchTree` (experimental), and `pkgs.fetchgit` when used without `leaveDotGit`. + If you don't need the configuration, you can use [`gitTracked`](#function-library-lib.fileset.gitTracked) instead. diff --git a/lib/fileset/internal.nix b/lib/fileset/internal.nix index c8a659cd09fd..4059d2e24426 100644 --- a/lib/fileset/internal.nix +++ b/lib/fileset/internal.nix @@ -41,6 +41,8 @@ let inherit (lib.path) append splitRoot + hasStorePathPrefix + splitStorePath ; inherit (lib.path.subpath) @@ -861,6 +863,28 @@ rec { # Type: String -> String -> Path -> Attrs -> FileSet _fromFetchGit = function: argument: path: extraFetchGitAttrs: let + # The code path for when isStorePath is true + tryStorePath = + if pathExists (path + "/.git") then + # If there is a `.git` directory in the path, + # it means that the path was imported unfiltered into the Nix store. + # This function should throw in such a case, because + # - `fetchGit` doesn't generally work with `.git` directories in store paths + # - Importing the entire path could include Git-tracked files + throw '' + lib.fileset.${function}: The ${argument} (${toString path}) is a store path within a working tree of a Git repository. + This indicates that a source directory was imported into the store using a method such as `import "''${./.}"` or `path:.`. + This function currently does not support such a use case, since it currently relies on `builtins.fetchGit`. + You could make this work by using a fetcher such as `fetchGit` instead of copying the whole repository. + If you can't avoid copying the repo to the store, see https://github.com/NixOS/nix/issues/9292.'' + else + # Otherwise we're going to assume that the path was a Git directory originally, + # but it was fetched using a method that already removed files not tracked by Git, + # such as `builtins.fetchGit`, `pkgs.fetchgit` or others. + # So we can just import the path in its entirety. + _singleton path; + + # The code path for when isStorePath is false tryFetchGit = let # This imports the files unnecessarily, which currently can't be avoided @@ -872,13 +896,11 @@ rec { url = path; } // extraFetchGitAttrs); in - if inPureEvalMode then - throw "lib.fileset.${function}: This function is currently not supported in pure evaluation mode, since it currently relies on `builtins.fetchGit`. See https://github.com/NixOS/nix/issues/9292." # We can identify local working directories by checking for .git, # see https://git-scm.com/docs/gitrepository-layout#_description. # Note that `builtins.fetchGit` _does_ work for bare repositories (where there's no `.git`), # even though `git ls-files` wouldn't return any files in that case. - else if ! pathExists (path + "/.git") then + if ! pathExists (path + "/.git") then throw "lib.fileset.${function}: Expected the ${argument} (${toString path}) to point to a local working tree of a Git repository, but it's not." else _mirrorStorePath path fetchResult.outPath; @@ -888,6 +910,8 @@ rec { throw "lib.fileset.${function}: Expected the ${argument} to be a path, but it's a ${typeOf path} instead." else if pathType path != "directory" then throw "lib.fileset.${function}: Expected the ${argument} (${toString path}) to be a directory, but it's a file instead." + else if hasStorePathPrefix path then + tryStorePath else tryFetchGit; diff --git a/lib/fileset/tests.sh b/lib/fileset/tests.sh index d55c4fbfdbeb..a5dced038a21 100755 --- a/lib/fileset/tests.sh +++ b/lib/fileset/tests.sh @@ -1401,10 +1401,60 @@ createGitRepo() { git -C "$1" commit -q --allow-empty -m "Empty commit" } -# Check the error message for pure eval mode +# Check that gitTracked[With] works as expected when evaluated out-of-tree + +## First we create a git repositories (and a subrepository) with `default.nix` files referring to their local paths +## Simulating how it would be used in the wild createGitRepo . -expectFailure --simulate-pure-eval 'toSource { root = ./.; fileset = gitTracked ./.; }' 'lib.fileset.gitTracked: This function is currently not supported in pure evaluation mode, since it currently relies on `builtins.fetchGit`. See https://github.com/NixOS/nix/issues/9292.' -expectFailure --simulate-pure-eval 'toSource { root = ./.; fileset = gitTrackedWith {} ./.; }' 'lib.fileset.gitTrackedWith: This function is currently not supported in pure evaluation mode, since it currently relies on `builtins.fetchGit`. See https://github.com/NixOS/nix/issues/9292.' +echo '{ fs }: fs.toSource { root = ./.; fileset = fs.gitTracked ./.; }' > default.nix +git add . + +## We can evaluate it locally just fine, `fetchGit` is used underneath to filter git-tracked files +expectEqual '(import ./. { fs = lib.fileset; }).outPath' '(builtins.fetchGit ./.).outPath' + +## We can also evaluate when importing from fetched store paths +storePath=$(expectStorePath 'builtins.fetchGit ./.') +expectEqual '(import '"$storePath"' { fs = lib.fileset; }).outPath' \""$storePath"\" + +## But it fails if the path is imported with a fetcher that doesn't remove .git (like just using "${./.}") +expectFailure 'import "${./.}" { fs = lib.fileset; }' 'lib.fileset.gitTracked: The argument \(.*\) is a store path within a working tree of a Git repository. +\s*This indicates that a source directory was imported into the store using a method such as `import "\$\{./.\}"` or `path:.`. +\s*This function currently does not support such a use case, since it currently relies on `builtins.fetchGit`. +\s*You could make this work by using a fetcher such as `fetchGit` instead of copying the whole repository. +\s*If you can'\''t avoid copying the repo to the store, see https://github.com/NixOS/nix/issues/9292.' + +## Even with submodules +if [[ -n "$fetchGitSupportsSubmodules" ]]; then + ## Both the main repo with the submodule + echo '{ fs }: fs.toSource { root = ./.; fileset = fs.gitTrackedWith { recurseSubmodules = true; } ./.; }' > default.nix + createGitRepo sub + git submodule add ./sub sub >/dev/null + ## But also the submodule itself + echo '{ fs }: fs.toSource { root = ./.; fileset = fs.gitTracked ./.; }' > sub/default.nix + git -C sub add . + + ## We can evaluate it locally just fine, `fetchGit` is used underneath to filter git-tracked files + expectEqual '(import ./. { fs = lib.fileset; }).outPath' '(builtins.fetchGit { url = ./.; submodules = true; }).outPath' + expectEqual '(import ./sub { fs = lib.fileset; }).outPath' '(builtins.fetchGit ./sub).outPath' + + ## We can also evaluate when importing from fetched store paths + storePathWithSub=$(expectStorePath 'builtins.fetchGit { url = ./.; submodules = true; }') + expectEqual '(import '"$storePathWithSub"' { fs = lib.fileset; }).outPath' \""$storePathWithSub"\" + storePathSub=$(expectStorePath 'builtins.fetchGit ./sub') + expectEqual '(import '"$storePathSub"' { fs = lib.fileset; }).outPath' \""$storePathSub"\" + + ## But it fails if the path is imported with a fetcher that doesn't remove .git (like just using "${./.}") + expectFailure 'import "${./.}" { fs = lib.fileset; }' 'lib.fileset.gitTrackedWith: The second argument \(.*\) is a store path within a working tree of a Git repository. + \s*This indicates that a source directory was imported into the store using a method such as `import "\$\{./.\}"` or `path:.`. + \s*This function currently does not support such a use case, since it currently relies on `builtins.fetchGit`. + \s*You could make this work by using a fetcher such as `fetchGit` instead of copying the whole repository. + \s*If you can'\''t avoid copying the repo to the store, see https://github.com/NixOS/nix/issues/9292.' + expectFailure 'import "${./.}/sub" { fs = lib.fileset; }' 'lib.fileset.gitTracked: The argument \(.*/sub\) is a store path within a working tree of a Git repository. + \s*This indicates that a source directory was imported into the store using a method such as `import "\$\{./.\}"` or `path:.`. + \s*This function currently does not support such a use case, since it currently relies on `builtins.fetchGit`. + \s*You could make this work by using a fetcher such as `fetchGit` instead of copying the whole repository. + \s*If you can'\''t avoid copying the repo to the store, see https://github.com/NixOS/nix/issues/9292.' +fi rm -rf -- * # Go through all stages of Git files From 38cf6ff099856e19f83649fcdbc98b5bde58eaca Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Tue, 19 Dec 2023 01:26:42 +0100 Subject: [PATCH 3/3] Remove --simulate-pure-eval --- lib/fileset/tests.sh | 54 +++++++++++++++----------------------------- 1 file changed, 18 insertions(+), 36 deletions(-) diff --git a/lib/fileset/tests.sh b/lib/fileset/tests.sh index a5dced038a21..e809aef6935a 100755 --- a/lib/fileset/tests.sh +++ b/lib/fileset/tests.sh @@ -43,29 +43,17 @@ crudeUnquoteJSON() { cut -d \" -f2 } -prefixExpression() { - echo 'let - lib = - (import ) - ' - if [[ "${1:-}" == "--simulate-pure-eval" ]]; then - echo ' - .extend (final: prev: { - trivial = prev.trivial // { - inPureEvalMode = true; - }; - })' - fi - echo ' - ; - internal = import { - inherit lib; - }; - in - with lib; - with internal; - with lib.fileset;' -} +prefixExpression=' + let + lib = import ; + internal = import { + inherit lib; + }; + in + with lib; + with internal; + with lib.fileset; +' # Check that two nix expression successfully evaluate to the same value. # The expressions have `lib.fileset` in scope. @@ -74,7 +62,7 @@ expectEqual() { local actualExpr=$1 local expectedExpr=$2 if actualResult=$(nix-instantiate --eval --strict --show-trace 2>"$tmp"/actualStderr \ - --expr "$(prefixExpression) ($actualExpr)"); then + --expr "$prefixExpression ($actualExpr)"); then actualExitCode=$? else actualExitCode=$? @@ -82,7 +70,7 @@ expectEqual() { actualStderr=$(< "$tmp"/actualStderr) if expectedResult=$(nix-instantiate --eval --strict --show-trace 2>"$tmp"/expectedStderr \ - --expr "$(prefixExpression) ($expectedExpr)"); then + --expr "$prefixExpression ($expectedExpr)"); then expectedExitCode=$? else expectedExitCode=$? @@ -110,7 +98,7 @@ expectEqual() { expectStorePath() { local expr=$1 if ! result=$(nix-instantiate --eval --strict --json --read-write-mode --show-trace 2>"$tmp"/stderr \ - --expr "$(prefixExpression) ($expr)"); then + --expr "$prefixExpression ($expr)"); then cat "$tmp/stderr" >&2 die "$expr failed to evaluate, but it was expected to succeed" fi @@ -123,16 +111,10 @@ expectStorePath() { # The expression has `lib.fileset` in scope. # Usage: expectFailure NIX REGEX expectFailure() { - if [[ "$1" == "--simulate-pure-eval" ]]; then - maybePure="--simulate-pure-eval" - shift - else - maybePure="" - fi local expr=$1 local expectedErrorRegex=$2 if result=$(nix-instantiate --eval --strict --read-write-mode --show-trace 2>"$tmp/stderr" \ - --expr "$(prefixExpression $maybePure) $expr"); then + --expr "$prefixExpression $expr"); then die "$expr evaluated successfully to $result, but it was expected to fail" fi stderr=$(<"$tmp/stderr") @@ -149,12 +131,12 @@ expectTrace() { local expectedTrace=$2 nix-instantiate --eval --show-trace >/dev/null 2>"$tmp"/stderrTrace \ - --expr "$(prefixExpression) trace ($expr)" || true + --expr "$prefixExpression trace ($expr)" || true actualTrace=$(sed -n 's/^trace: //p' "$tmp/stderrTrace") nix-instantiate --eval --show-trace >/dev/null 2>"$tmp"/stderrTraceVal \ - --expr "$(prefixExpression) traceVal ($expr)" || true + --expr "$prefixExpression traceVal ($expr)" || true actualTraceVal=$(sed -n 's/^trace: //p' "$tmp/stderrTraceVal") @@ -1331,7 +1313,7 @@ expectFailure 'gitTrackedWith {} ./.' 'lib.fileset.gitTrackedWith: Expected the expectFailure 'gitTrackedWith { recurseSubmodules = null; } ./.' 'lib.fileset.gitTrackedWith: Expected the attribute `recurseSubmodules` of the first argument to be a boolean, but it'\''s a null instead.' # recurseSubmodules = true is not supported on all Nix versions -if [[ "$(nix-instantiate --eval --expr "$(prefixExpression) (versionAtLeast builtins.nixVersion _fetchGitSubmodulesMinver)")" == true ]]; then +if [[ "$(nix-instantiate --eval --expr "$prefixExpression (versionAtLeast builtins.nixVersion _fetchGitSubmodulesMinver)")" == true ]]; then fetchGitSupportsSubmodules=1 else fetchGitSupportsSubmodules=