From ff107d9d03246ea8c15bbea7647e60bba65e9643 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 26 Apr 2024 16:05:03 +0200 Subject: [PATCH 1/5] Input::fetchToStore(): Don't try to substitute Having a narHash doesn't mean that we have the other attributes returned by the fetcher (such as lastModified or rev). For instance, $ nix flake metadata github:NixOS/patchelf/7c2f768bf9601268a4e71c2ebe91e2011918a70f Last modified: 2024-01-15 10:51:22 but $ nix flake metadata github:NixOS/patchelf/7c2f768bf9601268a4e71c2ebe91e2011918a70f?narHash=sha256-PPXqKY2hJng4DBVE0I4xshv/vGLUskL7jl53roB8UdU%3D (does not print a "Last modified") The latter only happens if the store path already exists or is substitutable, which made this impure behaviour unpredictable. Fixes #10601. --- src/libfetchers/fetchers.cc | 18 ------------------ tests/functional/tarball.sh | 3 --- 2 files changed, 21 deletions(-) diff --git a/src/libfetchers/fetchers.cc b/src/libfetchers/fetchers.cc index 0577b8d9d..a92b1e5ed 100644 --- a/src/libfetchers/fetchers.cc +++ b/src/libfetchers/fetchers.cc @@ -167,24 +167,6 @@ std::pair Input::fetchToStore(ref store) const if (!scheme) throw Error("cannot fetch unsupported input '%s'", attrsToJSON(toAttrs())); - /* The tree may already be in the Nix store, or it could be - substituted (which is often faster than fetching from the - original source). So check that. */ - if (getNarHash()) { - try { - auto storePath = computeStorePath(*store); - - store->ensurePath(storePath); - - debug("using substituted/cached input '%s' in '%s'", - to_string(), store->printStorePath(storePath)); - - return {std::move(storePath), *this}; - } catch (Error & e) { - debug("substitution of input '%s' failed: %s", to_string(), e.what()); - } - } - auto [storePath, input] = [&]() -> std::pair { try { auto [accessor, final] = getAccessorUnchecked(store); diff --git a/tests/functional/tarball.sh b/tests/functional/tarball.sh index 062f27ad6..158a73f55 100644 --- a/tests/functional/tarball.sh +++ b/tests/functional/tarball.sh @@ -33,9 +33,6 @@ test_tarball() { nix-build -o $TEST_ROOT/result -E "import (fetchTree file://$tarball)" nix-build -o $TEST_ROOT/result -E "import (fetchTree { type = \"tarball\"; url = file://$tarball; })" nix-build -o $TEST_ROOT/result -E "import (fetchTree { type = \"tarball\"; url = file://$tarball; narHash = \"$hash\"; })" - # Do not re-fetch paths already present - nix-build -o $TEST_ROOT/result -E "import (fetchTree { type = \"tarball\"; url = file:///does-not-exist/must-remain-unused/$tarball; narHash = \"$hash\"; })" - expectStderr 102 nix-build -o $TEST_ROOT/result -E "import (fetchTree { type = \"tarball\"; url = file://$tarball; narHash = \"sha256-xdKv2pq/IiwLSnBBJXW8hNowI4MrdZfW+SYqDQs7Tzc=\"; })" | grep 'NAR hash mismatch in input' [[ $(nix eval --impure --expr "(fetchTree file://$tarball).lastModified") = 1000000000 ]] From 3fbd71701a39dea79dea576ba90fe42925d85459 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 11 Sep 2024 17:27:39 +0200 Subject: [PATCH 2/5] Add test --- tests/nixos/github-flakes.nix | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/nixos/github-flakes.nix b/tests/nixos/github-flakes.nix index 221045009..8e646f6dd 100644 --- a/tests/nixos/github-flakes.nix +++ b/tests/nixos/github-flakes.nix @@ -181,8 +181,14 @@ in print(out) info = json.loads(out) assert info["revision"] == "${private-flake-rev}", f"revision mismatch: {info['revision']} != ${private-flake-rev}" + assert info["fingerprint"] cat_log() + # Fetching with the resolved URL should produce the same result. + info2 = json.loads(client.succeed(f"nix flake metadata {info['url']} --json --access-tokens github.com=ghp_000000000000000000000000000000000000 --tarball-ttl 0")) + print(info["fingerprint"], info2["fingerprint"]) + assert info["fingerprint"] == info2["fingerprint"], "fingerprint mismatch" + client.succeed("nix registry pin nixpkgs") client.succeed("nix flake metadata nixpkgs --tarball-ttl 0 >&2") From e557096cefad10ccef86dc674fc5e053c13716e6 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 11 Sep 2024 17:31:53 +0200 Subject: [PATCH 3/5] Add release note --- doc/manual/rl-next/no-flake-substitution.md | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 doc/manual/rl-next/no-flake-substitution.md diff --git a/doc/manual/rl-next/no-flake-substitution.md b/doc/manual/rl-next/no-flake-substitution.md new file mode 100644 index 000000000..67ec58750 --- /dev/null +++ b/doc/manual/rl-next/no-flake-substitution.md @@ -0,0 +1,8 @@ +--- +synopsis: Flakes are no longer substituted +prs: [10612] +--- + +Nix will no longer attempt to substitute the source code of flakes from a binary cache. This functionality was broken because it could lead to different evaluation results depending on whether the flake was available in the binary cache, or even depending on whether the flake was already in the local store. + +Author: [**@edolstra**](https://github.com/edolstra) From 30aa45a37313f54006ad88b9f42f11d8c175b1db Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 11 Sep 2024 20:35:04 +0200 Subject: [PATCH 4/5] Formatting --- src/libflake/flake/lockfile.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/libflake/flake/lockfile.cc b/src/libflake/flake/lockfile.cc index 80f14ff6f..70b60716f 100644 --- a/src/libflake/flake/lockfile.cc +++ b/src/libflake/flake/lockfile.cc @@ -54,12 +54,13 @@ StorePath LockedNode::computeStorePath(Store & store) const } -static std::shared_ptr doFind(const ref& root, const InputPath & path, std::vector& visited) { +static std::shared_ptr doFind(const ref & root, const InputPath & path, std::vector & visited) +{ auto pos = root; auto found = std::find(visited.cbegin(), visited.cend(), path); - if(found != visited.end()) { + if (found != visited.end()) { std::vector cycle; std::transform(found, visited.cend(), std::back_inserter(cycle), printInputPath); cycle.push_back(printInputPath(path)); From 12fd65d1799f6b1fe0c7e07d5a8022afc6b6dc40 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 11 Sep 2024 21:58:32 +0200 Subject: [PATCH 5/5] Disable subflakes test Relative path flakes ("subflakes") are basically fundamentally broken, since they produce lock file entries like "locked": { "lastModified": 1, "narHash": "sha256-/2tW9SKjQbRLzfcJs5SHijli6l3+iPr1235zylGynK8=", "path": "./flakeC", "type": "path" }, that don't specify what "./flakeC" is relative to. They *sometimes* worked by accident because the `narHash` field allowed `fetchToStore()` to get the store path of the subflake *if* it happened to exist in the local store or in a substituter. Subflakes are properly fixed in #10089 (which adds a "parent" field to the lock file). Rather than come up with some crazy hack to make them work in the interim, let's just disable the only test that depends on the broken behaviour for now. --- tests/functional/flakes/follow-paths.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/functional/flakes/follow-paths.sh b/tests/functional/flakes/follow-paths.sh index ea56b9503..d908a03c4 100755 --- a/tests/functional/flakes/follow-paths.sh +++ b/tests/functional/flakes/follow-paths.sh @@ -2,6 +2,9 @@ source ./common.sh +# FIXME: this test is disabled because relative path flakes are broken. Re-enable this in #10089. +exit 0 + requireGit flakeFollowsA=$TEST_ROOT/follows/flakeA