Skip to content

lib: cleanup after bump of minver #433101

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.

Sign up for GitHub

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

Merged
wolfgangwalther merged 2 commits into from
Aug 22, 2025
Merged

lib: cleanup after bump of minver #433101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dd43569
lib.customisation: remove unsafeGetAttrPos workaround
wolfgangwalther Aug 11, 2025
9b9bef0
lib.fileset: remove conditionals for Nix 2.4
wolfgangwalther Aug 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 1 addition & 10 deletions lib/customisation.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,18 +285,9 @@ rec {
arg:
let
loc = builtins.unsafeGetAttrPos arg fargs;
# loc' can be removed once lib/minver.nix is >2.3.4, since that includes
# https://github.com/NixOS/nix/pull/3468 which makes loc be non-null
loc' =
if loc != null then
loc.file + ":" + toString loc.line
else if !isFunction fn then
toString (lib.filesystem.resolveDefaultNix fn)
else
"<unknown location>";
in
"Function called without required argument \"${arg}\" at "
+ "${loc'}${prettySuggestions (getSuggestions arg)}";
+ "${loc.file}:${loc.line}${prettySuggestions (getSuggestions arg)}";
Copy link
Contributor Author

@wolfgangwalther wolfgangwalther Aug 22, 2025

Choose a reason for hiding this comment

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

Looks like this change doesn't quite work, yet. The error message in https://github.com/NixOS/nixpkgs/actions/runs/17161276034/job/48690946895 is:

       … while evaluating a path segment
         at /nix/store/jabhxzs8rhw9gv4y7vlz4qrd85rsi9qv-source/lib/customisation.nix:290:24:
          289|         "Function called without required argument \"${arg}\" at "
          290|         + "${loc.file}:${loc.line}${prettySuggestions (getSuggestions arg)}";
             |                        ^
          291|

       error: cannot coerce an integer to a string: 9

Copy link
Contributor

@MattSturgeon MattSturgeon Aug 22, 2025

Choose a reason for hiding this comment

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

Looks like we just missed that it still needs toString. I'll open a PR

Copy link
Contributor

@MattSturgeon MattSturgeon Aug 22, 2025

Choose a reason for hiding this comment

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

Looking closer, I'm also skeptical of directly interpolating loc.file. Is it guaranteed to be a string not a path value?

If it's a path value then direct interpolation would result in creating a new store object.

Copy link
Contributor Author

@wolfgangwalther wolfgangwalther Aug 22, 2025

Choose a reason for hiding this comment

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

That looks like a string to me:

nix-repl> builtins.unsafeGetAttrPos "meta" hello
{
  column = 17;
  file = "[...]/pkgs/stdenv/generic/make-derivation.nix";
  line = 881;
}

Copy link
Contributor

@MattSturgeon MattSturgeon Aug 22, 2025

Choose a reason for hiding this comment

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

That looks like a string to me:

Ah for some reason I had the module system in mind, where I'm not sure if _file is allowed to be a path value.

If unsafeGetAttrPos always uses a string for file, consistently accross versions/implementations, then I guess we don't need to toString or + it.

Copy link
Contributor

@MattSturgeon MattSturgeon Aug 22, 2025

Choose a reason for hiding this comment

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

#435941


# Only show the error for the first missing argument
error = errorForArg (head (attrNames missingArgs));
Expand Down
13 changes: 3 additions & 10 deletions lib/fileset/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,6 @@ let
_intersection
_difference
_fromFetchGit
_fetchGitSubmodulesMinver
_emptyWithoutBase
;

Expand Down Expand Up @@ -1000,16 +999,10 @@ in
path:
if !isBool recurseSubmodules then
throw "lib.fileset.gitTrackedWith: Expected the attribute `recurseSubmodules` of the first argument to be a boolean, but it's a ${typeOf recurseSubmodules} instead."
else if recurseSubmodules && versionOlder nixVersion _fetchGitSubmodulesMinver then
throw "lib.fileset.gitTrackedWith: Setting the attribute `recurseSubmodules` to `true` is only supported for Nix version ${_fetchGitSubmodulesMinver} and after, but Nix version ${nixVersion} is used."
else
_fromFetchGit "gitTrackedWith" "second argument" path
# This is the only `fetchGit` parameter that makes sense in this context.
# We can't just pass `submodules = recurseSubmodules` here because
# this would fail for Nix versions that don't support `submodules`.
(
lib.optionalAttrs recurseSubmodules {
submodules = true;
}
);
{
submodules = recurseSubmodules;
};
}
18 changes: 1 addition & 17 deletions lib/fileset/internal.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -899,14 +899,6 @@ rec {
${baseNameOf root} = fromFile (baseNameOf root) rootType;
};

# Support for `builtins.fetchGit` with `submodules = true` was introduced in 2.4
# https://github.com/NixOS/nix/commit/55cefd41d63368d4286568e2956afd535cb44018
_fetchGitSubmodulesMinver = "2.4";

# Support for `builtins.fetchGit` with `shallow = true` was introduced in 2.4
# https://github.com/NixOS/nix/commit/d1165d8791f559352ff6aa7348e1293b2873db1c
_fetchGitShallowMinver = "2.4";

# Mirrors the contents of a Nix store path relative to a local path as a file set.
# Some notes:
# - The store path is read at evaluation time.
Expand Down Expand Up @@ -961,16 +953,8 @@ rec {
fetchResult = fetchGit (
{
url = path;
shallow = true;
}
# In older Nix versions, repositories were always assumed to be deep clones, which made `fetchGit` fail for shallow clones
# For newer versions this was fixed, but the `shallow` flag is required.
# The only behavioral difference is that for shallow clones, `fetchGit` doesn't return a `revCount`,
# which we don't need here, so it's fine to always pass it.

# Unfortunately this means older Nix versions get a poor error message for shallow repositories, and there's no good way to improve that.
# Checking for `.git/shallow` doesn't seem worth it, especially since that's more of an implementation detail,
# and would also require more code to handle worktrees where `.git` is a file.
// optionalAttrs (versionAtLeast nixVersion _fetchGitShallowMinver) { shallow = true; }
// extraFetchGitAttrs
);
in
Expand Down
92 changes: 38 additions & 54 deletions lib/fileset/tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1336,14 +1336,6 @@ expectFailure 'gitTrackedWith {} ./.' 'lib.fileset.gitTrackedWith: Expected the
# recurseSubmodules has to be a boolean
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
fetchGitSupportsSubmodules=1
else
fetchGitSupportsSubmodules=
expectFailure 'gitTrackedWith { recurseSubmodules = true; } ./.' 'lib.fileset.gitTrackedWith: Setting the attribute `recurseSubmodules` to `true` is only supported for Nix version 2.4 and after, but Nix version [0-9.]+ is used.'
fi

# Checks that `gitTrackedWith` contains the same files as `git ls-files`
# for the current working directory.
# If --recurse-submodules is passed, the flag is passed through to `git ls-files`
Expand Down Expand Up @@ -1393,9 +1385,7 @@ checkGitTrackedWith() {
# Allows testing both variants together
checkGitTracked() {
checkGitTrackedWith
if [[ -n "$fetchGitSupportsSubmodules" ]]; then
checkGitTrackedWith --recurse-submodules
fi
checkGitTrackedWith --recurse-submodules
}

createGitRepo() {
Expand Down Expand Up @@ -1430,51 +1420,45 @@ expectFailure 'import "${./.}" { fs = lib.fileset; }' 'lib.fileset.gitTracked: T
[[:blank:]]*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.
[[:blank:]]*This indicates that a source directory was imported into the store using a method such as `import "\$\{./.\}"` or `path:.`.
[[:blank:]]*This function currently does not support such a use case, since it currently relies on `builtins.fetchGit`.
[[:blank:]]*You could make this work by using a fetcher such as `fetchGit` instead of copying the whole repository.
[[:blank:]]*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.
[[:blank:]]*This indicates that a source directory was imported into the store using a method such as `import "\$\{./.\}"` or `path:.`.
[[:blank:]]*This function currently does not support such a use case, since it currently relies on `builtins.fetchGit`.
[[:blank:]]*You could make this work by using a fetcher such as `fetchGit` instead of copying the whole repository.
[[:blank:]]*If you can'\''t avoid copying the repo to the store, see https://github.com/NixOS/nix/issues/9292.'
fi
## 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.
[[:blank:]]*This indicates that a source directory was imported into the store using a method such as `import "\$\{./.\}"` or `path:.`.
[[:blank:]]*This function currently does not support such a use case, since it currently relies on `builtins.fetchGit`.
[[:blank:]]*You could make this work by using a fetcher such as `fetchGit` instead of copying the whole repository.
[[:blank:]]*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.
[[:blank:]]*This indicates that a source directory was imported into the store using a method such as `import "\$\{./.\}"` or `path:.`.
[[:blank:]]*This function currently does not support such a use case, since it currently relies on `builtins.fetchGit`.
[[:blank:]]*You could make this work by using a fetcher such as `fetchGit` instead of copying the whole repository.
[[:blank:]]*If you can'\''t avoid copying the repo to the store, see https://github.com/NixOS/nix/issues/9292.'
rm -rf -- *

# shallow = true is not supported on all Nix versions
# and older versions don't support shallow clones at all
if [[ "$(nix-instantiate --eval --expr "$prefixExpression (versionAtLeast builtins.nixVersion _fetchGitShallowMinver)")" == true ]]; then
createGitRepo full
# Extra commit such that there's a commit that won't be in the shallow clone
git -C full commit --allow-empty -q -m extra
git clone -q --depth 1 "file://${PWD}/full" shallow
cd shallow
checkGitTracked
cd ..
rm -rf -- *
fi
createGitRepo full
# Extra commit such that there's a commit that won't be in the shallow clone
git -C full commit --allow-empty -q -m extra
git clone -q --depth 1 "file://${PWD}/full" shallow
cd shallow
checkGitTracked
cd ..
rm -rf -- *

# Go through all stages of Git files
# See https://www.git-scm.com/book/en/v2/Git-Basics-Recording-Changes-to-the-Repository
Expand Down
Loading