From f314e35b3726e7295eb39e70fd2d67e473c12ef8 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 1 Nov 2024 16:33:51 +0100 Subject: [PATCH] Simplify "final" inputs We now just check that the fetcher doesn't change any attributes in the input, and return all the original attributes (i.e. discarding any new attributes and keeping any attributes that the fetcher didn't keep). --- src/libfetchers/fetchers.cc | 50 ++++++++++++++++++++++--------------- src/libfetchers/fetchers.hh | 27 ++++++++++++-------- src/libfetchers/tarball.cc | 12 ++++----- 3 files changed, 51 insertions(+), 38 deletions(-) diff --git a/src/libfetchers/fetchers.cc b/src/libfetchers/fetchers.cc index cce1971ff..2aedb8a2e 100644 --- a/src/libfetchers/fetchers.cc +++ b/src/libfetchers/fetchers.cc @@ -193,7 +193,7 @@ std::pair Input::fetchToStore(ref store) const assert(final.isFinal()); - scheme->checkLocks(*this, final); + checkLocks(*this, final); return {storePath, final}; } catch (Error & e) { @@ -205,8 +205,35 @@ std::pair Input::fetchToStore(ref store) const return {std::move(storePath), input}; } -void InputScheme::checkLocks(const Input & specified, const Input & final) const +void Input::checkLocks(Input specified, Input & final) { + /* If the original input is final, then we just return the + original attributes, dropping any new fields returned by the + fetcher. However, any fields that are in both the original and + final input must be identical. */ + if (specified.isFinal()) { + + /* Backwards compatibility hack: we had some lock files in the + past that 'narHash' fields with incorrect base-64 + formatting (lacking the trailing '=', e.g. 'sha256-ri...Mw' + instead of ''sha256-ri...Mw='). So fix that. */ + if (auto prevNarHash = specified.getNarHash()) + specified.attrs.insert_or_assign("narHash", prevNarHash->to_string(HashFormat::SRI, true)); + + for (auto & field : specified.attrs) { + auto field2 = final.attrs.find(field.first); + if (field2 != final.attrs.end() && field.second != field2->second) + throw Error("mismatch in field '%s' of final input '%s', got '%s'", + field.first, + attrsToJSON(specified.attrs), + attrsToJSON(final.attrs)); + } + + final.attrs = specified.attrs; + + return; + } + if (auto prevNarHash = specified.getNarHash()) { if (final.getNarHash() != prevNarHash) { if (final.getNarHash()) @@ -235,23 +262,6 @@ void InputScheme::checkLocks(const Input & specified, const Input & final) const throw Error("'revCount' attribute mismatch in input '%s', expected %d", final.to_string(), *prevRevCount); } - - /* If the original input is final, then the result must be the - same (i.e. cannot remove, add or change fields. */ - if (specified.isFinal()) { - - /* Backwards compatibility hack: we had some lock files in the - past that 'narHash' fields with incorrect base-64 - formatting (lacking the trailing '=', e.g. 'sha256-ri...Mw' - instead of ''sha256-ri...Mw='). So fix */ - auto specified2 = specified; - if (auto prevNarHash = specified2.getNarHash()) - specified2.attrs.insert_or_assign("narHash", prevNarHash->to_string(HashFormat::SRI, true)); - - if (specified2.attrs != final.attrs) - throw Error("fetching final input '%s' resulted in different input '%s'", - attrsToJSON(specified2.attrs), attrsToJSON(final.attrs)); - } } std::pair, Input> Input::getAccessor(ref store) const @@ -259,7 +269,7 @@ std::pair, Input> Input::getAccessor(ref store) const try { auto [accessor, final] = getAccessorUnchecked(store); - scheme->checkLocks(*this, final); + checkLocks(*this, final); return {accessor, std::move(final)}; } catch (Error & e) { diff --git a/src/libfetchers/fetchers.hh b/src/libfetchers/fetchers.hh index ce535e6fe..430d6e943 100644 --- a/src/libfetchers/fetchers.hh +++ b/src/libfetchers/fetchers.hh @@ -91,9 +91,9 @@ public: bool isLocked() const; /** - * Return 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` + * Return whether this is a "final" input, meaning that fetching + * it will not add, remove 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. * @@ -113,6 +113,19 @@ public: */ std::pair fetchToStore(ref store) const; + /** + * Check the locking attributes in `final` against + * `specified`. E.g. if `specified` has a `rev` attribute, then + * `final` must have the same `rev` attribute. Throw an exception + * if there is a mismatch. + * + * If `specified` is marked final (i.e. has the `__final` + * attribute), then the intersection of attributes in `specified` + * and `final` must be equal, and `final.attrs` is set to + * `specified.attrs` (i.e. we discard any new attributes). + */ + static void checkLocks(Input specified, Input & final); + /** * Return a `SourceAccessor` that allows access to files in the * input without copying it to the store. Also return a possibly @@ -238,14 +251,6 @@ struct InputScheme virtual bool isLocked(const Input & input) const { return false; } - - /** - * Check the locking attributes in `final` against - * `specified`. E.g. if `specified` has a `rev` attribute, then - * `final` must have the same `rev` attribute. Throw an exception - * if there is a mismatch. - */ - virtual void checkLocks(const Input & specified, const Input & final) const; }; void registerInputScheme(std::shared_ptr && fetcher); diff --git a/src/libfetchers/tarball.cc b/src/libfetchers/tarball.cc index e723d3061..27ad89b6e 100644 --- a/src/libfetchers/tarball.cc +++ b/src/libfetchers/tarball.cc @@ -384,13 +384,11 @@ struct TarballInputScheme : CurlInputScheme input = immutableInput; } - /* 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"))) + /* 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",