From 188d97e1f1a6ce41f1eaed813adf878cfa6acdeb Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 15 Oct 2024 20:55:05 +0200 Subject: [PATCH 1/5] Restore input substitution The ability to substitute inputs was removed in #10612 because it was broken: with user-specified inputs containing a `narHash` attribute, substitution resulted in an input that lacked the attributes returned by the real fetcher (such as `lastModified`). To fix this, we introduce a new input attribute `final`. If `final = true`, fetching the input cannot add or change any attributes. We only attempt to substitute inputs that have `final = true`. This is implied by lock file entries; we only write a lock file if all its entries are "final". The user can specified `final = true` in `fetchTree`, in which case it is their responsibility to ensure that all attributes returned by the fetcher are included in the `fetchTree` call. For example, nix eval --impure --expr 'builtins.fetchTree { type = "github"; owner = "NixOS"; repo = "patchelf"; final = true; narHash = "sha256-FSoxTcRZMGHNJh8dNtKOkcUtjhmhU6yQXcZZfUPLhQM="; }' succeeds in a store path with the specified NAR hash exists or is substitutable, but fails with error: fetching final input '{"final":true,"narHash":"sha256-FSoxTcRZMGHNJh8dNtKOkcUtjhmhU6yQXcZZfUPLhQM=","owner":"NixOS","repo":"patchelf","type":"github"}' resulted in different input '{"final":true,"lastModified":1718457448,"narHash":"sha256-FSoxTcRZMGHNJh8dNtKOkcUtjhmhU6yQXcZZfUPLhQM=","owner":"NixOS","repo":"patchelf","rev":"a0f54334df36770b335c051e540ba40afcbf8378","type":"github"}' --- src/libexpr/call-flake.nix | 3 ++- src/libfetchers/fetchers.cc | 46 +++++++++++++++++++++++++++++++++- src/libfetchers/fetchers.hh | 33 +++++++++++++----------- src/libfetchers/path.cc | 1 + src/libflake/flake/flake.cc | 1 - src/libflake/flake/lockfile.cc | 12 +++++++-- src/libflake/flake/lockfile.hh | 4 +-- 7 files changed, 79 insertions(+), 21 deletions(-) diff --git a/src/libexpr/call-flake.nix b/src/libexpr/call-flake.nix index a411564df..c44d64885 100644 --- a/src/libexpr/call-flake.nix +++ b/src/libexpr/call-flake.nix @@ -44,7 +44,8 @@ let overrides.${key}.sourceInfo else # FIXME: remove obsolete node.info. - fetchTree (node.info or {} // removeAttrs node.locked ["dir"]); + # Note: lock file entries are always final. + fetchTree (node.info or {} // removeAttrs node.locked ["dir"] // { final = true; }); subdir = overrides.${key}.dir or node.locked.dir or ""; diff --git a/src/libfetchers/fetchers.cc b/src/libfetchers/fetchers.cc index b07e8cb6e..ff4c7567f 100644 --- a/src/libfetchers/fetchers.cc +++ b/src/libfetchers/fetchers.cc @@ -3,6 +3,7 @@ #include "source-path.hh" #include "fetch-to-store.hh" #include "json-utils.hh" +#include "store-path-accessor.hh" #include @@ -100,7 +101,7 @@ Input Input::fromAttrs(const Settings & settings, Attrs && attrs) auto allowedAttrs = inputScheme->allowedAttrs(); for (auto & [name, _] : attrs) - if (name != "type" && allowedAttrs.count(name) == 0) + if (name != "type" && name != "final" && allowedAttrs.count(name) == 0) throw Error("input attribute '%s' not supported by scheme '%s'", name, schemeName); auto res = inputScheme->inputFromAttrs(settings, attrs); @@ -145,6 +146,11 @@ bool Input::isLocked() const return scheme && scheme->isLocked(*this); } +bool Input::isFinal() const +{ + return maybeGetBoolAttr(attrs, "final").value_or(false); +} + Attrs Input::toAttrs() const { return attrs; @@ -221,6 +227,12 @@ void InputScheme::checkLocks(const Input & specified, const Input & final) const throw Error("'revCount' attribute mismatch in input '%s', expected %d", final.to_string(), *prevRevCount); } + + assert(final.isFinal()); + + if (specified.isFinal() && specified.attrs != final.attrs) + throw Error("fetching final input '%s' resulted in different input '%s'", + attrsToJSON(specified.attrs), attrsToJSON(final.attrs)); } std::pair, Input> Input::getAccessor(ref store) const @@ -244,11 +256,43 @@ std::pair, Input> Input::getAccessorUnchecked(ref sto 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. We only do this for final + inputs, otherwise there is a risk that we don't return the + same attributes (like `lastModified`) that the "real" fetcher + would return. + + FIXME: add a setting to disable this. + FIXME: substituting may be slower than fetching normally, + e.g. for fetchers like that Git that are incremental! + */ + if (isFinal() && getNarHash()) { + try { + auto storePath = computeStorePath(*store); + + store->ensurePath(storePath); + + debug("using substituted/cached input '%s' in '%s'", + to_string(), store->printStorePath(storePath)); + + auto accessor = makeStorePathAccessor(store, storePath); + + accessor->fingerprint = scheme->getFingerprint(store, *this); + + return {accessor, *this}; + } catch (Error & e) { + debug("substitution of input '%s' failed: %s", to_string(), e.what()); + } + } + auto [accessor, final] = scheme->getAccessor(store, *this); assert(!accessor->fingerprint); accessor->fingerprint = scheme->getFingerprint(store, final); + final.attrs.insert_or_assign("final", Explicit(true)); + return {accessor, std::move(final)}; } diff --git a/src/libfetchers/fetchers.hh b/src/libfetchers/fetchers.hh index a5f9bdcc6..e74625f7f 100644 --- a/src/libfetchers/fetchers.hh +++ b/src/libfetchers/fetchers.hh @@ -84,11 +84,21 @@ public: bool isDirect() const; /** - * Check whether this is a "locked" input, that is, - * one that contains a commit hash or content hash. + * Check whether this is a "locked" input, that is, it has + * attributes like a Git revision or NAR hash that uniquely + * identify its contents. */ bool isLocked() const; + /** + * Check whether this is a "final" input, meaning that fetching it + * will not add or change any attributes. For instance, a Git + * input with a `rev` attribute but without a `lastModified` + * attribute is considered locked but not final. Only "final" + * inputs can be substituted from a binary cache. + */ + bool isFinal() const; + bool operator ==(const Input & other) const noexcept; bool contains(const Input & other) const; @@ -144,6 +154,10 @@ public: /** * For locked inputs, return a string that uniquely specifies the * content of the input (typically a commit hash or content hash). + * + * Only known-equivalent inputs should return the same fingerprint. + * + * This is not a stable identifier between Nix versions, but not guaranteed to change either. */ std::optional getFingerprint(ref store) const; }; @@ -212,24 +226,15 @@ struct InputScheme */ virtual std::optional experimentalFeature() const; + /// See `Input::isDirect()`. virtual bool isDirect(const Input & input) const { return true; } - /** - * A sufficiently unique string that can be used as a cache key to identify the `input`. - * - * Only known-equivalent inputs should return the same fingerprint. - * - * This is not a stable identifier between Nix versions, but not guaranteed to change either. - */ + /// See `Input::getFingerprint()`. virtual std::optional getFingerprint(ref store, const Input & input) const { return std::nullopt; } - /** - * Return `true` if this input is considered "locked", i.e. it has - * attributes like a Git revision or NAR hash that uniquely - * identify its contents. - */ + /// See `Input::isLocked()`. virtual bool isLocked(const Input & input) const { return false; } diff --git a/src/libfetchers/path.cc b/src/libfetchers/path.cc index fca0df84b..564ad6e71 100644 --- a/src/libfetchers/path.cc +++ b/src/libfetchers/path.cc @@ -72,6 +72,7 @@ struct PathInputScheme : InputScheme auto query = attrsToQuery(input.attrs); query.erase("path"); query.erase("type"); + query.erase("final"); return ParsedURL { .scheme = "path", .path = getStrAttr(input.attrs, "path"), diff --git a/src/libflake/flake/flake.cc b/src/libflake/flake/flake.cc index d18e01464..f6f29f241 100644 --- a/src/libflake/flake/flake.cc +++ b/src/libflake/flake/flake.cc @@ -85,7 +85,6 @@ static void forceTrivialValue(EvalState & state, Value & value, const PosIdx pos state.forceValue(value, pos); } - static void expectType(EvalState & state, ValueType type, Value & value, const PosIdx pos) { diff --git a/src/libflake/flake/lockfile.cc b/src/libflake/flake/lockfile.cc index 70b60716f..f80c27acd 100644 --- a/src/libflake/flake/lockfile.cc +++ b/src/libflake/flake/lockfile.cc @@ -46,6 +46,10 @@ LockedNode::LockedNode( if (!lockedRef.input.isLocked()) throw Error("lock file contains unlocked input '%s'", fetchers::attrsToJSON(lockedRef.input.toAttrs())); + + // For backward compatibility, lock file entries are implicitly final. + assert(!lockedRef.input.attrs.contains("final")); + lockedRef.input.attrs.insert_or_assign("final", Explicit(true)); } StorePath LockedNode::computeStorePath(Store & store) const @@ -53,7 +57,6 @@ StorePath LockedNode::computeStorePath(Store & store) const return lockedRef.input.computeStorePath(store); } - static std::shared_ptr doFind(const ref & root, const InputPath & path, std::vector & visited) { auto pos = root; @@ -191,6 +194,11 @@ std::pair LockFile::toJSON() const if (auto lockedNode = node.dynamic_pointer_cast()) { n["original"] = fetchers::attrsToJSON(lockedNode->originalRef.toAttrs()); n["locked"] = fetchers::attrsToJSON(lockedNode->lockedRef.toAttrs()); + /* For backward compatibility, omit the "final" + attribute. We never allow non-final inputs in lock files + anyway. */ + assert(lockedNode->lockedRef.input.isFinal()); + n["locked"].erase("final"); if (!lockedNode->isFlake) n["flake"] = false; } @@ -239,7 +247,7 @@ std::optional LockFile::isUnlocked() const for (auto & i : nodes) { if (i == ref(root)) continue; auto node = i.dynamic_pointer_cast(); - if (node && !node->lockedRef.input.isLocked()) + if (node && (!node->lockedRef.input.isLocked() || !node->lockedRef.input.isFinal())) return node->lockedRef; } diff --git a/src/libflake/flake/lockfile.hh b/src/libflake/flake/lockfile.hh index 841931c11..a2711a516 100644 --- a/src/libflake/flake/lockfile.hh +++ b/src/libflake/flake/lockfile.hh @@ -68,8 +68,8 @@ struct LockFile std::pair to_string() const; /** - * Check whether this lock file has any unlocked inputs. If so, - * return one. + * Check whether this lock file has any unlocked or non-final + * inputs. If so, return one. */ std::optional isUnlocked() const; From fc09815eda00e3ba9211932ab14d2bdf4feab7db Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 16 Oct 2024 15:17:38 +0200 Subject: [PATCH 2/5] Typo Co-authored-by: Cole Helbling --- src/libfetchers/fetchers.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libfetchers/fetchers.cc b/src/libfetchers/fetchers.cc index ff4c7567f..f25781a12 100644 --- a/src/libfetchers/fetchers.cc +++ b/src/libfetchers/fetchers.cc @@ -265,7 +265,7 @@ std::pair, Input> Input::getAccessorUnchecked(ref sto FIXME: add a setting to disable this. FIXME: substituting may be slower than fetching normally, - e.g. for fetchers like that Git that are incremental! + e.g. for fetchers like Git that are incremental! */ if (isFinal() && getNarHash()) { try { From ed1f9dd13f23450aad86f7687dd1b596d06ceed4 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 16 Oct 2024 15:18:23 +0200 Subject: [PATCH 3/5] Don't mark inputs as final in getAccessorUnchecked() We haven't added the narHash attribute yet at that point. And if the caller uses getAccesor() instead of fetchToStore() (e.g. in `nix registry pin`), the narHash attribute will never be added. This could lead to a mismatch. --- src/libfetchers/fetchers.cc | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/libfetchers/fetchers.cc b/src/libfetchers/fetchers.cc index f25781a12..26229134d 100644 --- a/src/libfetchers/fetchers.cc +++ b/src/libfetchers/fetchers.cc @@ -185,6 +185,14 @@ std::pair Input::fetchToStore(ref store) const auto narHash = store->queryPathInfo(storePath)->narHash; final.attrs.insert_or_assign("narHash", narHash.to_string(HashFormat::SRI, true)); + // FIXME: we would like to mark inputs as final in + // getAccessorUnchecked(), but then we can't add + // narHash. Or maybe narHash should be excluded from the + // concept of "final" inputs? + final.attrs.insert_or_assign("final", Explicit(true)); + + assert(final.isFinal()); + scheme->checkLocks(*this, final); return {storePath, final}; @@ -228,8 +236,6 @@ void InputScheme::checkLocks(const Input & specified, const Input & final) const final.to_string(), *prevRevCount); } - assert(final.isFinal()); - if (specified.isFinal() && specified.attrs != final.attrs) throw Error("fetching final input '%s' resulted in different input '%s'", attrsToJSON(specified.attrs), attrsToJSON(final.attrs)); @@ -291,8 +297,6 @@ std::pair, Input> Input::getAccessorUnchecked(ref sto assert(!accessor->fingerprint); accessor->fingerprint = scheme->getFingerprint(store, final); - final.attrs.insert_or_assign("final", Explicit(true)); - return {accessor, std::move(final)}; } From 78b5b4c105f1adb33c416889f4378cede154cf68 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 17 Oct 2024 14:12:39 +0200 Subject: [PATCH 4/5] Tarball fetcher: Fix compat with old lock files that didn't include lastModified Fixes flake-regressions/tests/DeterminateSystems/fh/0.1.10: error: fetching final input '{"final":true,"narHash":"sha256-0dZpggYjjmWEk+rGixiBHOHuQfLzEzNfrtjSig04s6Q=","rev":"9ccae1754eec0341b640d5705302ac0923d22875","revCount":1618,"type":"tarball","url":"https://api.flakehub.com/f/pinned/nix-community/fenix/0.1.1618%2Brev-9ccae1754eec0341b640d5705302ac0923d22875/018aea4c-03c9-7734-95d5-b84cc8881e3d/source.tar.gz"}' resulted in different input '{"final":true,"lastModified":1696141234,"narHash":"sha256-0dZpggYjjmWEk+rGixiBHOHuQfLzEzNfrtjSig04s6Q=","rev":"9ccae1754eec0341b640d5705302ac0923d22875","revCount":1618,"type":"tarball","url":"https://api.flakehub.com/f/pinned/nix-community/fenix/0.1.1618%2Brev-9ccae1754eec0341b640d5705302ac0923d22875/018aea4c-03c9-7734-95d5-b84cc8881e3d/source.tar.gz"}' --- src/libfetchers/tarball.cc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/libfetchers/tarball.cc b/src/libfetchers/tarball.cc index 28574e7b1..27ad89b6e 100644 --- a/src/libfetchers/tarball.cc +++ b/src/libfetchers/tarball.cc @@ -384,7 +384,11 @@ struct TarballInputScheme : CurlInputScheme input = immutableInput; } - if (result.lastModified && !input.attrs.contains("lastModified")) + /* If we got a lastModified and the input is not final and + doesn't have one, then return it. Note that we don't do + this if the input is final for compatibility with old lock + files that didn't include lastModified. */ + if (result.lastModified && !_input.isFinal() && !input.attrs.contains("lastModified")) input.attrs.insert_or_assign("lastModified", uint64_t(result.lastModified)); input.attrs.insert_or_assign("narHash", From 7d1f7f8d59fe1a9bbed3adc09a76de07ba84e8e8 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 17 Oct 2024 16:20:08 +0200 Subject: [PATCH 5/5] Tarball fetcher: Handle lock files that *do* contain lastModified Fixes flake-regressions/tests/DeterminateSystems/eva/0.1.0: error: 'lastModified' attribute mismatch in input 'https://api.flakehub.com/f/pinned/ipetkov/crane/0.14.1/018ac45c-ff5e-7076-b956-d478a0336516/source.tar.gz?narHash=sha256-mnE14re43v3/Jc50Jv0BKPMtEk7FEtDSligP6B5HwlI%3D', expected 1695511445 --- src/libfetchers/fetchers.cc | 4 ++-- src/libfetchers/tarball.cc | 12 +++++++----- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/libfetchers/fetchers.cc b/src/libfetchers/fetchers.cc index 26229134d..9717533d6 100644 --- a/src/libfetchers/fetchers.cc +++ b/src/libfetchers/fetchers.cc @@ -220,8 +220,8 @@ void InputScheme::checkLocks(const Input & specified, const Input & final) const if (auto prevLastModified = specified.getLastModified()) { if (final.getLastModified() != prevLastModified) - throw Error("'lastModified' attribute mismatch in input '%s', expected %d", - final.to_string(), *prevLastModified); + throw Error("'lastModified' attribute mismatch in input '%s', expected %d, got %d", + final.to_string(), *prevLastModified, final.getLastModified().value_or(-1)); } if (auto prevRev = specified.getRev()) { diff --git a/src/libfetchers/tarball.cc b/src/libfetchers/tarball.cc index 27ad89b6e..e723d3061 100644 --- a/src/libfetchers/tarball.cc +++ b/src/libfetchers/tarball.cc @@ -384,11 +384,13 @@ struct TarballInputScheme : CurlInputScheme input = immutableInput; } - /* If we got a lastModified and the input is not final and - doesn't have one, then return it. Note that we don't do - this if the input is final for compatibility with old lock - files that didn't include lastModified. */ - if (result.lastModified && !_input.isFinal() && !input.attrs.contains("lastModified")) + /* If we got a lastModified, then return it. But for + compatibility with old lock files that didn't include + lastModified, don't do this if the original input was final + and didn't contain a lastModified. */ + if (result.lastModified + && !input.attrs.contains("lastModified") + && (!_input.isFinal() || _input.attrs.contains("lastModified"))) input.attrs.insert_or_assign("lastModified", uint64_t(result.lastModified)); input.attrs.insert_or_assign("narHash",