From a4e5de1b9d26584615946057430df9e63d842f53 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 12 Oct 2020 23:51:23 +0000 Subject: [PATCH 01/24] Derivations can output "text-hashed" data In particular, this means that derivations can output derivations. But that ramification isn't (yet!) useful as we would want, since there is no way to have a dependent derivation that is itself a dependent derivation. --- src/libexpr/primops.cc | 40 +++++---- src/libstore/build.cc | 52 +++++++----- src/libstore/content-address.cc | 139 ++++++++++++++++++++++++-------- src/libstore/content-address.hh | 71 +++++++++++----- src/libstore/daemon.cc | 11 ++- src/libstore/derivations.cc | 42 +++++----- src/libstore/derivations.hh | 4 +- src/libstore/local-store.cc | 2 +- src/libstore/misc.cc | 18 ++++- src/libstore/remote-store.cc | 20 +++-- src/libstore/remote-store.hh | 1 + src/nix/show-derivation.cc | 7 +- tests/local.mk | 3 +- tests/text-hashed-output.nix | 29 +++++++ tests/text-hashed-output.sh | 26 ++++++ 15 files changed, 325 insertions(+), 140 deletions(-) create mode 100644 tests/text-hashed-output.nix create mode 100644 tests/text-hashed-output.sh diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index c74b6765830..9d49eeef384 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -851,7 +851,7 @@ static void prim_derivationStrict(EvalState & state, const Pos & pos, Value * * bool contentAddressed = false; std::optional outputHash; std::string outputHashAlgo; - auto ingestionMethod = FileIngestionMethod::Flat; + ContentAddressMethod ingestionMethod = FileIngestionMethod::Flat; StringSet outputs; outputs.insert("out"); @@ -864,6 +864,7 @@ static void prim_derivationStrict(EvalState & state, const Pos & pos, Value * * auto handleHashMode = [&](const std::string & s) { if (s == "recursive") ingestionMethod = FileIngestionMethod::Recursive; else if (s == "flat") ingestionMethod = FileIngestionMethod::Flat; + else if (s == "text") ingestionMethod = TextHashMethod {}; else throw EvalError({ .hint = hintfmt("invalid value '%s' for 'outputHashMode' attribute", s), @@ -995,8 +996,11 @@ static void prim_derivationStrict(EvalState & state, const Pos & pos, Value * * state.store->computeFSClosure(state.store->parseStorePath(std::string_view(path).substr(1)), refs); for (auto & j : refs) { drv.inputSrcs.insert(j); - if (j.isDerivation()) - drv.inputDrvs[j] = state.store->readDerivation(j).outputNames(); + if (j.isDerivation()) { + Derivation jDrv = state.store->readDerivation(j); + if(jDrv.type() != DerivationType::CAFloating) + drv.inputDrvs[j] = jDrv.outputNames(); + } } } @@ -1025,9 +1029,9 @@ static void prim_derivationStrict(EvalState & state, const Pos & pos, Value * * }); /* Check whether the derivation name is valid. */ - if (isDerivation(drvName)) + if (isDerivation(drvName) && ingestionMethod != ContentAddressMethod { TextHashMethod { } }) throw EvalError({ - .hint = hintfmt("derivation names are not allowed to end in '%s'", drvExtension), + .hint = hintfmt("derivation names are allowed to end in '%s' only if they produce a single derivation file", drvExtension), .errPos = posDrvName }); @@ -1045,22 +1049,16 @@ static void prim_derivationStrict(EvalState & state, const Pos & pos, Value * * std::optional ht = parseHashTypeOpt(outputHashAlgo); Hash h = newHashAllowEmpty(*outputHash, ht); - auto outPath = state.store->makeFixedOutputPath(drvName, FixedOutputInfo { - { - .method = ingestionMethod, - .hash = h, - }, - {}, - }); - drv.env["out"] = state.store->printStorePath(outPath); - drv.outputs.insert_or_assign("out", DerivationOutput { - .output = DerivationOutputCAFixed { - .hash = FixedOutputHash { - .method = ingestionMethod, - .hash = std::move(h), - }, - }, - }); + // FIXME non-trivial fixed refs set + auto ca = contentAddressFromMethodHashAndRefs( + ingestionMethod, + std::move(h), + {}); + + DerivationOutputCAFixed dof { .ca = ca }; + + drv.env["out"] = state.store->printStorePath(dof.path(*state.store, drvName, "out")); + drv.outputs.insert_or_assign("out", DerivationOutput { .output = dof }); } else if (contentAddressed) { diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 48e123f364a..a23c7f333ce 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -4070,26 +4070,35 @@ void DerivationGoal::registerOutputs() auto newInfoFromCA = [&](const DerivationOutputCAFloating outputHash) -> ValidPathInfo { auto & st = outputStats.at(outputName); - if (outputHash.method == FileIngestionMethod::Flat) { + if (outputHash.method == ContentAddressMethod { FileIngestionMethod::Flat } || + outputHash.method == ContentAddressMethod { TextHashMethod {} }) + { /* The output path should be a regular file without execute permission. */ if (!S_ISREG(st.st_mode) || (st.st_mode & S_IXUSR) != 0) throw BuildError( "output path '%1%' should be a non-executable regular file " - "since recursive hashing is not enabled (outputHashMode=flat)", + "since recursive hashing is not enabled (one of outputHashMode={flat,text} is true)", actualPath); } rewriteOutput(); /* FIXME optimize and deduplicate with addToStore */ std::string oldHashPart { scratchPath.hashPart() }; HashModuloSink caSink { outputHash.hashType, oldHashPart }; - switch (outputHash.method) { - case FileIngestionMethod::Recursive: - dumpPath(actualPath, caSink); - break; - case FileIngestionMethod::Flat: - readFile(actualPath, caSink); - break; - } + std::visit(overloaded { + [&](TextHashMethod _) { + readFile(actualPath, caSink); + }, + [&](FileIngestionMethod m2) { + switch (m2) { + case FileIngestionMethod::Recursive: + dumpPath(actualPath, caSink); + break; + case FileIngestionMethod::Flat: + readFile(actualPath, caSink); + break; + } + }, + }, outputHash.method); auto got = caSink.finish().first; HashModuloSink narSink { htSHA256, oldHashPart }; dumpPath(actualPath, narSink); @@ -4098,13 +4107,10 @@ void DerivationGoal::registerOutputs() worker.store, { .name = outputPathName(drv->name, outputName), - .info = FixedOutputInfo { - { - .method = outputHash.method, - .hash = got, - }, - rewriteRefs(), - }, + .info = contentAddressFromMethodHashAndRefs( + outputHash.method, + std::move(got), + rewriteRefs()), }, narHashAndSize.first, }; @@ -4132,13 +4138,14 @@ void DerivationGoal::registerOutputs() return newInfo0; }, [&](DerivationOutputCAFixed dof) { + auto wanted = getContentAddressHash(dof.ca); + auto newInfo0 = newInfoFromCA(DerivationOutputCAFloating { - .method = dof.hash.method, - .hashType = dof.hash.hash.type, + .method = getContentAddressMethod(dof.ca), + .hashType = wanted.type, }); /* Check wanted hash */ - Hash & wanted = dof.hash.hash; assert(newInfo0.ca); auto got = getContentAddressHash(*newInfo0.ca); if (wanted != got) { @@ -4151,6 +4158,11 @@ void DerivationGoal::registerOutputs() wanted.to_string(SRI, true), got.to_string(SRI, true))); } + if (static_cast &>(newInfo0) != PathReferences {}) + delayedException = std::make_exception_ptr( + BuildError("illegal path references in fixed-output derivation '%s'", + worker.store.printStorePath(drvPath))); + return newInfo0; }, [&](DerivationOutputCAFloating dof) { diff --git a/src/libstore/content-address.cc b/src/libstore/content-address.cc index d68c60f4fbe..4226213b9df 100644 --- a/src/libstore/content-address.cc +++ b/src/libstore/content-address.cc @@ -10,7 +10,7 @@ std::string FixedOutputHash::printMethodAlgo() const } -std::string makeFileIngestionPrefix(const FileIngestionMethod m) +std::string makeFileIngestionPrefix(FileIngestionMethod m) { switch (m) { case FileIngestionMethod::Flat: @@ -21,6 +21,27 @@ std::string makeFileIngestionPrefix(const FileIngestionMethod m) assert(false); } +std::string makeContentAddressingPrefix(ContentAddressMethod m) { + return std::visit(overloaded { + [](TextHashMethod _) -> std::string { return "text:"; }, + [](FileIngestionMethod m2) { + /* Not prefixed for back compat with things that couldn't produce text before. */ + return makeFileIngestionPrefix(m2); + }, + }, m); +} + +ContentAddressMethod parseContentAddressingPrefix(std::string_view & m) +{ + ContentAddressMethod method = FileIngestionMethod::Flat; + if (splitPrefix(m, "r:")) + method = FileIngestionMethod::Recursive; + else if (splitPrefix(m, "text:")) + method = TextHashMethod {}; + return method; +} + + std::string makeFixedOutputCA(FileIngestionMethod method, const Hash & hash) { return "fixed:" @@ -43,14 +64,14 @@ std::string renderContentAddress(ContentAddress ca) }, ca); } -std::string renderContentAddressMethod(ContentAddressMethod cam) +std::string renderContentAddressMethodAndHash(ContentAddressMethod cam, HashType ht) { return std::visit(overloaded { - [](TextHashMethod &th) { - return std::string{"text:"} + printHashType(htSHA256); + [&](TextHashMethod & th) { + return std::string{"text:"} + printHashType(ht); }, - [](FixedOutputHashMethod &fshm) { - return "fixed:" + makeFileIngestionPrefix(fshm.fileIngestionMethod) + printHashType(fshm.hashType); + [&](FileIngestionMethod & fim) { + return "fixed:" + makeFileIngestionPrefix(fim) + printHashType(ht); } }, cam); } @@ -58,7 +79,7 @@ std::string renderContentAddressMethod(ContentAddressMethod cam) /* Parses content address strings up to the hash. */ -static ContentAddressMethod parseContentAddressMethodPrefix(std::string_view & rest) +static std::pair parseContentAddressMethodPrefix(std::string_view & rest) { std::string_view wholeInput { rest }; @@ -82,19 +103,19 @@ static ContentAddressMethod parseContentAddressMethodPrefix(std::string_view & r if (prefix == "text") { // No parsing of the ingestion method, "text" only support flat. HashType hashType = parseHashType_(); - if (hashType != htSHA256) - throw Error("text content address hash should use %s, but instead uses %s", - printHashType(htSHA256), printHashType(hashType)); - return TextHashMethod {}; + return { + TextHashMethod {}, + std::move(hashType), + }; } else if (prefix == "fixed") { // Parse method auto method = FileIngestionMethod::Flat; if (splitPrefix(rest, "r:")) method = FileIngestionMethod::Recursive; HashType hashType = parseHashType_(); - return FixedOutputHashMethod { - .fileIngestionMethod = method, - .hashType = std::move(hashType), + return { + std::move(method), + std::move(hashType), }; } else throw UsageError("content address prefix '%s' is unrecognized. Recogonized prefixes are 'text' or 'fixed'", prefix); @@ -103,25 +124,24 @@ static ContentAddressMethod parseContentAddressMethodPrefix(std::string_view & r ContentAddress parseContentAddress(std::string_view rawCa) { auto rest = rawCa; - ContentAddressMethod caMethod = parseContentAddressMethodPrefix(rest); - - return std::visit( - overloaded { - [&](TextHashMethod thm) { - return ContentAddress(TextHash { - .hash = Hash::parseNonSRIUnprefixed(rest, htSHA256) - }); - }, - [&](FixedOutputHashMethod fohMethod) { - return ContentAddress(FixedOutputHash { - .method = fohMethod.fileIngestionMethod, - .hash = Hash::parseNonSRIUnprefixed(rest, std::move(fohMethod.hashType)), - }); - }, - }, caMethod); -} - -ContentAddressMethod parseContentAddressMethod(std::string_view caMethod) + auto [caMethod, hashType] = parseContentAddressMethodPrefix(rest); + + return std::visit(overloaded { + [&](TextHashMethod _) { + return ContentAddress(TextHash { + .hash = Hash::parseNonSRIUnprefixed(rest, hashType) + }); + }, + [&](FileIngestionMethod fim) { + return ContentAddress(FixedOutputHash { + .method = fim, + .hash = Hash::parseNonSRIUnprefixed(rest, hashType), + }); + }, + }, caMethod); +} + +std::pair parseContentAddressMethod(std::string_view caMethod) { std::string_view asPrefix {std::string{caMethod} + ":"}; return parseContentAddressMethodPrefix(asPrefix); @@ -137,6 +157,42 @@ std::string renderContentAddress(std::optional ca) return ca ? renderContentAddress(*ca) : ""; } +ContentAddressWithReferences contentAddressFromMethodHashAndRefs( + ContentAddressMethod method, Hash && hash, PathReferences && refs) +{ + return std::visit(overloaded { + [&](TextHashMethod _) -> ContentAddressWithReferences { + if (refs.hasSelfReference) + throw UsageError("Cannot have a self reference with text hashing scheme"); + return TextInfo { + { .hash = std::move(hash) }, + std::move(refs.references), + }; + }, + [&](FileIngestionMethod m2) -> ContentAddressWithReferences { + return FixedOutputInfo { + { + .method = m2, + .hash = std::move(hash), + }, + std::move(refs), + }; + }, + }, method); +} + +ContentAddressMethod getContentAddressMethod(const ContentAddressWithReferences & ca) +{ + return std::visit(overloaded { + [](TextInfo th) -> ContentAddressMethod { + return TextHashMethod {}; + }, + [](FixedOutputInfo fsh) -> ContentAddressMethod { + return fsh.method; + }, + }, ca); +} + Hash getContentAddressHash(const ContentAddress & ca) { return std::visit(overloaded { @@ -160,4 +216,21 @@ ContentAddressWithReferences caWithoutRefs(const ContentAddress & ca) { }, ca); } +Hash getContentAddressHash(const ContentAddressWithReferences & ca) +{ + return std::visit(overloaded { + [](TextInfo th) { + return th.hash; + }, + [](FixedOutputInfo fsh) { + return fsh.hash; + }, + }, ca); +} + +std::string printMethodAlgo(const ContentAddressWithReferences & ca) { + return makeContentAddressingPrefix(getContentAddressMethod(ca)) + + printHashType(getContentAddressHash(ca).type); +} + } diff --git a/src/libstore/content-address.hh b/src/libstore/content-address.hh index e15d76bd761..1fdd16e9244 100644 --- a/src/libstore/content-address.hh +++ b/src/libstore/content-address.hh @@ -10,11 +10,45 @@ namespace nix { * Mini content address */ +/* We only have one way to hash text with references, so this is a single-value + type, mainly useful with std::variant. +*/ +struct TextHashMethod : std::monostate { }; + enum struct FileIngestionMethod : uint8_t { Flat = false, Recursive = true }; +/* Compute the prefix to the hash algorithm which indicates how the files were + ingested. */ +std::string makeFileIngestionPrefix(FileIngestionMethod m); + + +/* Just the type of a content address. Combine with the hash itself, and we + have a `ContentAddress` as defined below. Combine that, in turn, with info + on references, and we have `ContentAddressWithReferences`, as defined + further below. */ +typedef std::variant< + TextHashMethod, + FileIngestionMethod +> ContentAddressMethod; + +/* Parse and pretty print the algorithm which indicates how the files + were ingested, with the the fixed output case not prefixed for back + compat. */ + +std::string makeContentAddressingPrefix(ContentAddressMethod m); + +ContentAddressMethod parseContentAddressingPrefix(std::string_view & m); + +/* Parse and pretty print a content addressing method and hash in a + nicer way, prefixing both cases. */ + +std::string renderContentAddressMethodAndHash(ContentAddressMethod cam, HashType ht); + +std::pair parseContentAddressMethod(std::string_view caMethod); + struct TextHash { Hash hash; @@ -27,6 +61,7 @@ struct FixedOutputHash { std::string printMethodAlgo() const; }; + /* We've accumulated several types of content-addressed paths over the years; fixed-output derivations support multiple hash algorithms and serialisation @@ -43,10 +78,6 @@ typedef std::variant< FixedOutputHash // for path computed by makeFixedOutputPath > ContentAddress; -/* Compute the prefix to the hash algorithm which indicates how the files were - ingested. */ -std::string makeFileIngestionPrefix(const FileIngestionMethod m); - std::string renderContentAddress(ContentAddress ca); std::string renderContentAddress(std::optional ca); @@ -57,24 +88,6 @@ std::optional parseContentAddressOpt(std::string_view rawCaOpt); Hash getContentAddressHash(const ContentAddress & ca); -/* - We only have one way to hash text with references, so this is single-value - type is only useful in std::variant. -*/ -struct TextHashMethod { }; -struct FixedOutputHashMethod { - FileIngestionMethod fileIngestionMethod; - HashType hashType; -}; - -typedef std::variant< - TextHashMethod, - FixedOutputHashMethod - > ContentAddressMethod; - -ContentAddressMethod parseContentAddressMethod(std::string_view rawCaMethod); - -std::string renderContentAddressMethod(ContentAddressMethod caMethod); /* * References set @@ -92,6 +105,12 @@ struct PathReferences && hasSelfReference == other.hasSelfReference; } + bool operator != (const PathReferences & other) const + { + return references != other.references + || hasSelfReference != other.hasSelfReference; + } + /* Functions to view references + hasSelfReference as one set, mainly for compatibility's sake. */ StorePathSet referencesPossiblyToSelf(const Ref & self) const; @@ -151,6 +170,14 @@ typedef std::variant< ContentAddressWithReferences caWithoutRefs(const ContentAddress &); +ContentAddressWithReferences contentAddressFromMethodHashAndRefs( + ContentAddressMethod method, Hash && hash, PathReferences && refs); + +ContentAddressMethod getContentAddressMethod(const ContentAddressWithReferences & ca); +Hash getContentAddressHash(const ContentAddressWithReferences & ca); + +std::string printMethodAlgo(const ContentAddressWithReferences &); + struct StorePathDescriptor { std::string name; ContentAddressWithReferences info; diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc index 7ae88b49a93..0e2ae4134c2 100644 --- a/src/libstore/daemon.cc +++ b/src/libstore/daemon.cc @@ -380,20 +380,23 @@ static void performOp(TunnelLogger * logger, ref store, logger->startWork(); auto pathInfo = [&]() { // NB: FramedSource must be out of scope before logger->stopWork(); - ContentAddressMethod contentAddressMethod = parseContentAddressMethod(camStr); + auto [contentAddressMethod, hashType] = parseContentAddressMethod(camStr); FramedSource source(from); // TODO this is essentially RemoteStore::addCAToStore. Move it up to Store. return std::visit(overloaded { - [&](TextHashMethod &_) { + [&](TextHashMethod _) { + if (hashType != htSHA256) + throw UnimplementedError("Only SHA-256 is supported for adding text-hashed data, but '%1' was given", + printHashType(hashType)); // We could stream this by changing Store std::string contents = source.drain(); auto path = store->addTextToStore(name, contents, refs, repair); return store->queryPathInfo(path); }, - [&](FixedOutputHashMethod &fohm) { + [&](FileIngestionMethod fim) { if (!refs.empty()) throw UnimplementedError("cannot yet have refs with flat or nar-hashed data"); - auto path = store->addToStoreFromDump(source, name, fohm.fileIngestionMethod, fohm.hashType, repair); + auto path = store->addToStoreFromDump(source, name, fim, hashType, repair); return store->queryPathInfo(path); }, }, contentAddressMethod); diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index 925a7808335..e15f9e25a05 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -2,6 +2,7 @@ #include "store-api.hh" #include "globals.hh" #include "util.hh" +#include "split.hh" #include "worker-protocol.hh" #include "fs-accessor.hh" @@ -26,9 +27,10 @@ std::optional DerivationOutput::path(const Store & store, std::string StorePath DerivationOutputCAFixed::path(const Store & store, std::string_view drvName, std::string_view outputName) const { - return store.makeFixedOutputPath( - outputPathName(drvName, outputName), - { hash, {} }); + return store.makeFixedOutputPathFromCA(StorePathDescriptor { + .name = outputPathName(drvName, outputName), + .info = ca, + }); } @@ -150,23 +152,19 @@ static StringSet parseStrings(std::istream & str, bool arePaths) static DerivationOutput parseDerivationOutput(const Store & store, - std::string_view pathS, std::string_view hashAlgo, std::string_view hash) + std::string_view pathS, std::string_view hashAlgo, std::string_view hashS) { if (hashAlgo != "") { - auto method = FileIngestionMethod::Flat; - if (string(hashAlgo, 0, 2) == "r:") { - method = FileIngestionMethod::Recursive; - hashAlgo = hashAlgo.substr(2); - } + ContentAddressMethod method = parseContentAddressingPrefix(hashAlgo); const auto hashType = parseHashType(hashAlgo); - if (hash != "") { + if (hashS != "") { validatePath(pathS); + auto hash = Hash::parseNonSRIUnprefixed(hashS, hashType); return DerivationOutput { .output = DerivationOutputCAFixed { - .hash = FixedOutputHash { - .method = std::move(method), - .hash = Hash::parseNonSRIUnprefixed(hash, hashType), - }, + // FIXME non-trivial fixed refs set + .ca = contentAddressFromMethodHashAndRefs( + method, std::move(hash), {}), }, }; } else { @@ -317,12 +315,12 @@ string Derivation::unparse(const Store & store, bool maskOutputs, }, [&](DerivationOutputCAFixed dof) { s += ','; printUnquotedString(s, maskOutputs ? "" : store.printStorePath(dof.path(store, name, i.first))); - s += ','; printUnquotedString(s, dof.hash.printMethodAlgo()); - s += ','; printUnquotedString(s, dof.hash.hash.to_string(Base16, false)); + s += ','; printUnquotedString(s, printMethodAlgo(dof.ca)); + s += ','; printUnquotedString(s, getContentAddressHash(dof.ca).to_string(Base16, false)); }, [&](DerivationOutputCAFloating dof) { s += ','; printUnquotedString(s, ""); - s += ','; printUnquotedString(s, makeFileIngestionPrefix(dof.method) + printHashType(dof.hashType)); + s += ','; printUnquotedString(s, makeContentAddressingPrefix(dof.method) + printHashType(dof.hashType)); s += ','; printUnquotedString(s, ""); }, }, i.second.output); @@ -482,8 +480,8 @@ DrvHashModulo hashDerivationModulo(Store & store, const Derivation & drv, bool m for (const auto & i : drv.outputs) { auto & dof = std::get(i.second.output); auto hash = hashString(htSHA256, "fixed:out:" - + dof.hash.printMethodAlgo() + ":" - + dof.hash.hash.to_string(Base16, false) + ":" + + printMethodAlgo(dof.ca) + ":" + + getContentAddressHash(dof.ca).to_string(Base16, false) + ":" + store.printStorePath(dof.path(store, drv.name, i.first))); outputHashes.insert_or_assign(i.first, std::move(hash)); } @@ -612,12 +610,12 @@ void writeDerivation(Sink & out, const Store & store, const BasicDerivation & dr }, [&](DerivationOutputCAFixed dof) { out << store.printStorePath(dof.path(store, drv.name, i.first)) - << dof.hash.printMethodAlgo() - << dof.hash.hash.to_string(Base16, false); + << printMethodAlgo(dof.ca) + << getContentAddressHash(dof.ca).to_string(Base16, false); }, [&](DerivationOutputCAFloating dof) { out << "" - << (makeFileIngestionPrefix(dof.method) + printHashType(dof.hashType)) + << (makeContentAddressingPrefix(dof.method) + printHashType(dof.hashType)) << ""; }, }, i.second.output); diff --git a/src/libstore/derivations.hh b/src/libstore/derivations.hh index e2e5578a813..3bfaabad314 100644 --- a/src/libstore/derivations.hh +++ b/src/libstore/derivations.hh @@ -27,7 +27,7 @@ struct DerivationOutputInputAddressed according to that fixed output. */ struct DerivationOutputCAFixed { - FixedOutputHash hash; /* hash used for expected hash computation */ + ContentAddressWithReferences ca; /* hash and refs used for validating output */ StorePath path(const Store & store, std::string_view drvName, std::string_view outputName) const; }; @@ -37,7 +37,7 @@ struct DerivationOutputCAFixed struct DerivationOutputCAFloating { /* information used for expected hash computation */ - FileIngestionMethod method; + ContentAddressMethod method; HashType hashType; }; diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index e6b02cce624..f573845f149 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -567,7 +567,7 @@ void LocalStore::checkDerivationOutputs(const StorePath & drvPath, const Derivat envHasRightPath(doia.path, i.first); }, [&](DerivationOutputCAFixed dof) { - StorePath path = makeFixedOutputPath(drvName, { dof.hash, {} }); + auto path = dof.path(*this, drvName, i.first); envHasRightPath(path, i.first); }, [&](DerivationOutputCAFloating _) { diff --git a/src/libstore/misc.cc b/src/libstore/misc.cc index 052936a8b8f..71fa8154600 100644 --- a/src/libstore/misc.cc +++ b/src/libstore/misc.cc @@ -111,9 +111,21 @@ void Store::computeFSClosure(const StorePath & startPath, std::optional getDerivationCA(const BasicDerivation & drv) { auto out = drv.outputs.find("out"); - if (out != drv.outputs.end()) { - if (auto v = std::get_if(&out->second.output)) - return v->hash; + if (out == drv.outputs.end()) + return std::nullopt; + if (auto dof = std::get_if(&out->second.output)) { + return std::visit(overloaded { + [&](TextInfo ti) -> std::optional { + if (!ti.references.empty()) + return std::nullopt; + return static_cast(ti); + }, + [&](FixedOutputInfo fi) -> std::optional { + if (fi.references != PathReferences {}) + return std::nullopt; + return static_cast(fi); + }, + }, dof->ca); } return std::nullopt; } diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 5ff787ed293..4f71d8120f3 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -457,6 +457,7 @@ ref RemoteStore::addCAToStore( Source & dump, const string & name, ContentAddressMethod caMethod, + HashType hashType, const StorePathSet & references, RepairFlag repair) { @@ -468,7 +469,7 @@ ref RemoteStore::addCAToStore( conn->to << wopAddToStore << name - << renderContentAddressMethod(caMethod); + << renderContentAddressMethodAndHash(caMethod, hashType); worker_proto::write(*this, conn->to, references); conn->to << repair; @@ -484,18 +485,21 @@ ref RemoteStore::addCAToStore( std::visit(overloaded { [&](TextHashMethod thm) -> void { + if (hashType != htSHA256) + throw UnimplementedError("Only SHA-256 is supported for adding text-hashed data, but '%1' was given", + printHashType(hashType)); std::string s = dump.drain(); conn->to << wopAddTextToStore << name << s; worker_proto::write(*this, conn->to, references); conn.processStderr(); }, - [&](FixedOutputHashMethod fohm) -> void { + [&](FileIngestionMethod fim) -> void { conn->to << wopAddToStore << name - << ((fohm.hashType == htSHA256 && fohm.fileIngestionMethod == FileIngestionMethod::Recursive) ? 0 : 1) /* backwards compatibility hack */ - << (fohm.fileIngestionMethod == FileIngestionMethod::Recursive ? 1 : 0) - << printHashType(fohm.hashType); + << ((hashType == htSHA256 && fim == FileIngestionMethod::Recursive) ? 0 : 1) /* backwards compatibility hack */ + << (fim == FileIngestionMethod::Recursive ? 1 : 0) + << printHashType(hashType); try { conn->to.written = 0; @@ -503,7 +507,7 @@ ref RemoteStore::addCAToStore( connections->incCapacity(); { Finally cleanup([&]() { connections->decCapacity(); }); - if (fohm.fileIngestionMethod == FileIngestionMethod::Recursive) { + if (fim == FileIngestionMethod::Recursive) { dump.drainInto(conn->to); } else { std::string contents = dump.drain(); @@ -536,7 +540,7 @@ StorePath RemoteStore::addToStoreFromDump(Source & dump, const string & name, FileIngestionMethod method, HashType hashType, RepairFlag repair) { StorePathSet references; - return addCAToStore(dump, name, FixedOutputHashMethod{ .fileIngestionMethod = method, .hashType = hashType }, references, repair)->path; + return addCAToStore(dump, name, method, hashType, references, repair)->path; } @@ -597,7 +601,7 @@ StorePath RemoteStore::addTextToStore(const string & name, const string & s, const StorePathSet & references, RepairFlag repair) { StringSource source(s); - return addCAToStore(source, name, TextHashMethod{}, references, repair)->path; + return addCAToStore(source, name, TextHashMethod {}, htSHA256, references, repair)->path; } diff --git a/src/libstore/remote-store.hh b/src/libstore/remote-store.hh index 9f78fcb02d2..0260e32213d 100644 --- a/src/libstore/remote-store.hh +++ b/src/libstore/remote-store.hh @@ -68,6 +68,7 @@ public: Source & dump, const string & name, ContentAddressMethod caMethod, + HashType hashType, const StorePathSet & references, RepairFlag repair); diff --git a/src/nix/show-derivation.cc b/src/nix/show-derivation.cc index 2542537d399..0799f21ebd6 100644 --- a/src/nix/show-derivation.cc +++ b/src/nix/show-derivation.cc @@ -76,11 +76,12 @@ struct CmdShowDerivation : InstallablesCommand }, [&](DerivationOutputCAFixed dof) { outputObj.attr("path", store->printStorePath(dof.path(*store, drv.name, outputName))); - outputObj.attr("hashAlgo", dof.hash.printMethodAlgo()); - outputObj.attr("hash", dof.hash.hash.to_string(Base16, false)); + outputObj.attr("hashAlgo", printMethodAlgo(dof.ca)); + outputObj.attr("hash", getContentAddressHash(dof.ca).to_string(Base16, false)); + // FIXME print refs? }, [&](DerivationOutputCAFloating dof) { - outputObj.attr("hashAlgo", makeFileIngestionPrefix(dof.method) + printHashType(dof.hashType)); + outputObj.attr("hashAlgo", makeContentAddressingPrefix(dof.method) + printHashType(dof.hashType)); }, }, output.output); } diff --git a/tests/local.mk b/tests/local.mk index a1929f96d29..ce50a4ca566 100644 --- a/tests/local.mk +++ b/tests/local.mk @@ -35,7 +35,8 @@ nix_tests = \ recursive.sh \ describe-stores.sh \ flakes.sh \ - content-addressed.sh + content-addressed.sh \ + text-hashed-output.sh # parallel.sh # build-remote-content-addressed-fixed.sh \ diff --git a/tests/text-hashed-output.nix b/tests/text-hashed-output.nix new file mode 100644 index 00000000000..23434c0a1aa --- /dev/null +++ b/tests/text-hashed-output.nix @@ -0,0 +1,29 @@ +with import ./config.nix; + +# A simple content-addressed derivation. +# The derivation can be arbitrarily modified by passing a different `seed`, +# but the output will always be the same +rec { + root = mkDerivation { + name = "text-hashed-root"; + buildCommand = '' + set -x + echo "Building a CA derivation" + mkdir -p $out + echo "Hello World" > $out/hello + ''; + __contentAddressed = true; + outputHashMode = "recursive"; + outputHashAlgo = "sha256"; + }; + dependent = mkDerivation { + name = "text-hashed-root.drv"; + buildCommand = '' + echo "Copying the derivation" + cp ${root.drvPath} $out + ''; + __contentAddressed = true; + outputHashMode = "text"; + outputHashAlgo = "sha256"; + }; +} diff --git a/tests/text-hashed-output.sh b/tests/text-hashed-output.sh new file mode 100644 index 00000000000..2ee3d65907b --- /dev/null +++ b/tests/text-hashed-output.sh @@ -0,0 +1,26 @@ +#!/usr/bin/env bash + +source common.sh + +# In the corresponding nix file, we have two derivations: the first, named root, +# is a normal recursive derivation, while the second, named dependent, has the +# new outputHashMode "text". Note that in "dependent", we don't refer to the +# build output of root, but only to the path of the drv file. For this reason, +# we only need to: +# +# - instantiate the root derivation +# - build the dependent derivation +# - check that the path of the output coincides with that of the original derivation + +drv=$(nix-instantiate --experimental-features ca-derivations ./text-hashed-output.nix -A root) +nix --experimental-features 'nix-command ca-derivations' show-derivation --derivation "$drv" + +drvDep=$(nix-instantiate --experimental-features ca-derivations ./text-hashed-output.nix -A dependent) +nix --experimental-features 'nix-command ca-derivations' show-derivation --derivation "$drvDep" + +out1=$(nix-build --experimental-features ca-derivations ./text-hashed-output.nix -A dependent --no-out-link) + +nix --experimental-features 'nix-command ca-derivations' path-info $drv --derivation --json | jq +nix --experimental-features 'nix-command ca-derivations' path-info $out1 --derivation --json | jq + +test $out1 == $drv From 00c607b5637e6e7b54764c0edb53312abfe032c7 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 13 Oct 2020 04:11:25 +0000 Subject: [PATCH 02/24] Work around clang destructing + capturing bug again --- src/libstore/content-address.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libstore/content-address.cc b/src/libstore/content-address.cc index 4226213b9df..6a695fe6894 100644 --- a/src/libstore/content-address.cc +++ b/src/libstore/content-address.cc @@ -124,7 +124,8 @@ static std::pair parseContentAddressMethodPrefix ContentAddress parseContentAddress(std::string_view rawCa) { auto rest = rawCa; - auto [caMethod, hashType] = parseContentAddressMethodPrefix(rest); + auto [caMethod, hashType_] = parseContentAddressMethodPrefix(rest); + auto hashType = hashType_; // work around clang bug return std::visit(overloaded { [&](TextHashMethod _) { From b6b383d5697c7d5a6a4ecc3d5260f8f1f99c2213 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 13 Oct 2020 04:36:20 +0000 Subject: [PATCH 03/24] Work around clang destructing + capturing bug yet again --- src/libstore/daemon.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc index 0e2ae4134c2..76f463af475 100644 --- a/src/libstore/daemon.cc +++ b/src/libstore/daemon.cc @@ -380,7 +380,8 @@ static void performOp(TunnelLogger * logger, ref store, logger->startWork(); auto pathInfo = [&]() { // NB: FramedSource must be out of scope before logger->stopWork(); - auto [contentAddressMethod, hashType] = parseContentAddressMethod(camStr); + auto [contentAddressMethod, hashType_] = parseContentAddressMethod(camStr); + auto hashType = hashType_; // work around clang bug FramedSource source(from); // TODO this is essentially RemoteStore::addCAToStore. Move it up to Store. return std::visit(overloaded { From 47f0d7b79833c3b5934c768fcc4b65223c8a7c4b Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 13 Oct 2020 16:22:30 +0000 Subject: [PATCH 04/24] Cleanup tabs --- src/libstore/misc.cc | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/libstore/misc.cc b/src/libstore/misc.cc index 71fa8154600..778ee4709d2 100644 --- a/src/libstore/misc.cc +++ b/src/libstore/misc.cc @@ -115,16 +115,16 @@ std::optional getDerivationCA(const BasicDerivation & drv) return std::nullopt; if (auto dof = std::get_if(&out->second.output)) { return std::visit(overloaded { - [&](TextInfo ti) -> std::optional { - if (!ti.references.empty()) - return std::nullopt; - return static_cast(ti); - }, - [&](FixedOutputInfo fi) -> std::optional { - if (fi.references != PathReferences {}) - return std::nullopt; - return static_cast(fi); - }, + [&](TextInfo ti) -> std::optional { + if (!ti.references.empty()) + return std::nullopt; + return static_cast(ti); + }, + [&](FixedOutputInfo fi) -> std::optional { + if (fi.references != PathReferences {}) + return std::nullopt; + return static_cast(fi); + }, }, dof->ca); } return std::nullopt; From d6e0c511ec1201d212ce181ba0e3cd2b7774d3c0 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 30 Sep 2021 23:57:07 +0000 Subject: [PATCH 05/24] Fix texted hash output test to work when testing daemon Need to get experiment features to daemon like with the other tests. --- tests/text-hashed-output.sh | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/tests/text-hashed-output.sh b/tests/text-hashed-output.sh index 2ee3d65907b..1c4d3a4384e 100644 --- a/tests/text-hashed-output.sh +++ b/tests/text-hashed-output.sh @@ -2,6 +2,9 @@ source common.sh +# Globally enable the ca derivations experimental flag +sed -i 's/experimental-features = .*/& ca-derivations ca-references/' "$NIX_CONF_DIR/nix.conf" + # In the corresponding nix file, we have two derivations: the first, named root, # is a normal recursive derivation, while the second, named dependent, has the # new outputHashMode "text". Note that in "dependent", we don't refer to the @@ -13,14 +16,14 @@ source common.sh # - check that the path of the output coincides with that of the original derivation drv=$(nix-instantiate --experimental-features ca-derivations ./text-hashed-output.nix -A root) -nix --experimental-features 'nix-command ca-derivations' show-derivation --derivation "$drv" +nix show-derivation --derivation "$drv" drvDep=$(nix-instantiate --experimental-features ca-derivations ./text-hashed-output.nix -A dependent) -nix --experimental-features 'nix-command ca-derivations' show-derivation --derivation "$drvDep" +nix show-derivation --derivation "$drvDep" out1=$(nix-build --experimental-features ca-derivations ./text-hashed-output.nix -A dependent --no-out-link) -nix --experimental-features 'nix-command ca-derivations' path-info $drv --derivation --json | jq -nix --experimental-features 'nix-command ca-derivations' path-info $out1 --derivation --json | jq +nix path-info $drv --derivation --json | jq +nix path-info $out1 --derivation --json | jq test $out1 == $drv From 7e1cfa97c6b410ed25ae0d3c3f10274aae9f6758 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 6 Jan 2023 12:52:16 -0500 Subject: [PATCH 06/24] Make derivation primop code for fixed output more concise --- src/libexpr/primops.cc | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 7e4b944b1ab..0398a5b6613 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -1248,16 +1248,14 @@ static void prim_derivationStrict(EvalState & state, const PosIdx pos, Value * * auto h = newHashAllowEmpty(*outputHash, parseHashTypeOpt(outputHashAlgo)); auto method = ingestionMethod.value_or(FileIngestionMethod::Flat); - // FIXME non-trivial fixed refs set - auto ca = contentAddressFromMethodHashAndRefs( - method, - std::move(h), - {}); - DerivationOutput::CAFixed dof { .ca = ca }; + DerivationOutput::CAFixed dof { + // FIXME non-trivial fixed refs set + .ca = contentAddressFromMethodHashAndRefs(method, std::move(h), {}), + }; drv.env["out"] = state.store->printStorePath(dof.path(*state.store, drvName, "out")); - drv.outputs.insert_or_assign("out", dof); + drv.outputs.insert_or_assign("out", std::move(dof)); } else if (contentAddressed || isImpure) { From 30610f260d61964a4d91e7f7f590f621ea03fef6 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sat, 14 Jan 2023 17:12:42 -0500 Subject: [PATCH 07/24] Use `builtins.unsafeDiscardOutputDependency` in the ca/text-hash-out test We don't want to build that drv file yet, just depend on it itself. --- tests/ca/text-hashed-output.nix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/ca/text-hashed-output.nix b/tests/ca/text-hashed-output.nix index 23434c0a1aa..31a66dfa85a 100644 --- a/tests/ca/text-hashed-output.nix +++ b/tests/ca/text-hashed-output.nix @@ -20,7 +20,7 @@ rec { name = "text-hashed-root.drv"; buildCommand = '' echo "Copying the derivation" - cp ${root.drvPath} $out + cp ${builtins.unsafeDiscardOutputDependency root.drvPath} $out ''; __contentAddressed = true; outputHashMode = "text"; From 2eb493ca51e97228a7dc8e28e414df627cb3a329 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 17 Apr 2023 10:28:54 -0400 Subject: [PATCH 08/24] Fix `DerivationOutput::fromJSON` --- src/libstore/derivations.cc | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index 56f30f2e4fa..fc76ae7ad9c 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -997,15 +997,13 @@ DerivationOutput DerivationOutput::fromJSON( for (const auto & [key, _] : json) keys.insert(key); - auto methodAlgo = [&]() -> std::pair { + auto methodAlgo = [&]() -> std::pair { std::string hashAlgo = json["hashAlgo"]; - auto method = FileIngestionMethod::Flat; - if (hashAlgo.substr(0, 2) == "r:") { - method = FileIngestionMethod::Recursive; - hashAlgo = hashAlgo.substr(2); - } - auto hashType = parseHashType(hashAlgo); - return { method, hashType }; + // remaining to parse, will be mutated by parsers + std::string_view s = hashAlgo; + ContentAddressMethod method = ContentAddressMethod::parsePrefix(s); + auto hashType = parseHashType(s); + return { std::move(method), std::move(hashType) }; }; if (keys == (std::set { "path" })) { @@ -1018,7 +1016,7 @@ DerivationOutput DerivationOutput::fromJSON( auto [method, hashType] = methodAlgo(); auto dof = DerivationOutput::CAFixed { .ca = ContentAddressWithReferences::fromParts( - method, + std::move(method), Hash::parseNonSRIUnprefixed((std::string) json["hash"], hashType), {}), }; @@ -1030,8 +1028,8 @@ DerivationOutput DerivationOutput::fromJSON( else if (keys == (std::set { "hashAlgo" })) { auto [method, hashType] = methodAlgo(); return DerivationOutput::CAFloating { - .method = method, - .hashType = hashType, + .method = std::move(method), + .hashType = std::move(hashType), }; } @@ -1042,7 +1040,7 @@ DerivationOutput DerivationOutput::fromJSON( else if (keys == (std::set { "hashAlgo", "impure" })) { auto [method, hashType] = methodAlgo(); return DerivationOutput::Impure { - .method = method, + .method = std::move(method), .hashType = hashType, }; } From 668377f217c0fa4053d746f7094dfe887e07887c Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 17 Apr 2023 19:02:45 -0400 Subject: [PATCH 09/24] `TextHashMethod` -> `TextIngestionMethod`, gate with XP feature I suppose we can use `dynamic-derivations` for the few things we neeed. --- src/libexpr/primops.cc | 13 ++++++++++--- src/libstore/build/local-derivation-goal.cc | 4 ++-- src/libstore/content-address.cc | 14 +++++++------- src/libstore/content-address.hh | 4 ++-- src/libstore/daemon.cc | 2 +- src/libstore/derivations.cc | 2 ++ src/libstore/derivations.hh | 14 ++++++++------ src/libstore/remote-store.cc | 4 ++-- src/libutil/experimental-features.cc | 12 +++++++++++- src/libutil/experimental-features.hh | 1 + tests/ca/text-hashed-output.sh | 12 +++++++----- 11 files changed, 53 insertions(+), 29 deletions(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 2476b7e735d..fc397db3312 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -1105,8 +1105,10 @@ drvName, Bindings * attrs, Value & v) auto handleHashMode = [&](const std::string_view s) { if (s == "recursive") ingestionMethod = FileIngestionMethod::Recursive; else if (s == "flat") ingestionMethod = FileIngestionMethod::Flat; - else if (s == "text") ingestionMethod = TextHashMethod {}; - else + else if (s == "text") { + experimentalFeatureSettings.require(Xp::DynamicDerivations); + ingestionMethod = TextIngestionMethod {}; + } else state.debugThrowLastTrace(EvalError({ .msg = hintfmt("invalid value '%s' for 'outputHashMode' attribute", s), .errPos = state.positions[noPos] @@ -1274,11 +1276,16 @@ drvName, Bindings * attrs, Value & v) })); /* Check whether the derivation name is valid. */ - if (isDerivation(drvName) && ingestionMethod != ContentAddressMethod { TextHashMethod { } }) + if (isDerivation(drvName) && + !(ingestionMethod == ContentAddressMethod { TextIngestionMethod { } } && + outputs.size() == 1 && + *(outputs.begin()) == "out")) + { state.debugThrowLastTrace(EvalError({ .msg = hintfmt("derivation names are allowed to end in '%s' only if they produce a single derivation file", drvExtension), .errPos = state.positions[noPos] })); + } if (outputHash) { /* Handle fixed-output derivations. diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 0a208750eb4..7a424fbfc03 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -2427,7 +2427,7 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs() "output path %1% without valid stats info", actualPath); if (outputHash.method == ContentAddressMethod { FileIngestionMethod::Flat } || - outputHash.method == ContentAddressMethod { TextHashMethod {} }) + outputHash.method == ContentAddressMethod { TextIngestionMethod {} }) { /* The output path should be a regular file without execute permission. */ if (!S_ISREG(st->st_mode) || (st->st_mode & S_IXUSR) != 0) @@ -2441,7 +2441,7 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs() std::string oldHashPart { scratchPath->hashPart() }; HashModuloSink caSink { outputHash.hashType, oldHashPart }; std::visit(overloaded { - [&](const TextHashMethod &) { + [&](const TextIngestionMethod &) { readFile(actualPath, caSink); }, [&](const FileIngestionMethod & m2) { diff --git a/src/libstore/content-address.cc b/src/libstore/content-address.cc index 8c04dd285d8..2bde23e79de 100644 --- a/src/libstore/content-address.cc +++ b/src/libstore/content-address.cc @@ -23,7 +23,7 @@ std::string makeFileIngestionPrefix(FileIngestionMethod m) std::string ContentAddressMethod::renderPrefix() const { return std::visit(overloaded { - [](TextHashMethod) -> std::string { return "text:"; }, + [](TextIngestionMethod) -> std::string { return "text:"; }, [](FileIngestionMethod m2) { /* Not prefixed for back compat with things that couldn't produce text before. */ return makeFileIngestionPrefix(m2); @@ -37,7 +37,7 @@ ContentAddressMethod ContentAddressMethod::parsePrefix(std::string_view & m) if (splitPrefix(m, "r:")) method = FileIngestionMethod::Recursive; else if (splitPrefix(m, "text:")) - method = TextHashMethod {}; + method = TextIngestionMethod {}; return method; } @@ -59,7 +59,7 @@ std::string ContentAddress::render() const std::string ContentAddressMethod::render(HashType ht) const { return std::visit(overloaded { - [&](const TextHashMethod & th) { + [&](const TextIngestionMethod & th) { return std::string{"text:"} + printHashType(ht); }, [&](const FileIngestionMethod & fim) { @@ -96,7 +96,7 @@ static std::pair parseContentAddressMethodPrefix // No parsing of the ingestion method, "text" only support flat. HashType hashType = parseHashType_(); return { - TextHashMethod {}, + TextIngestionMethod {}, std::move(hashType), }; } else if (prefix == "fixed") { @@ -120,7 +120,7 @@ ContentAddress ContentAddress::parse(std::string_view rawCa) { auto hashType = hashType_; // work around clang bug return std::visit(overloaded { - [&](TextHashMethod &) { + [&](TextIngestionMethod &) { return ContentAddress(TextHash { .hash = Hash::parseNonSRIUnprefixed(rest, hashType) }); @@ -158,7 +158,7 @@ ContentAddressWithReferences ContentAddressWithReferences::fromParts( ContentAddressMethod method, Hash hash, StoreReferences refs) { return std::visit(overloaded { - [&](TextHashMethod _) -> ContentAddressWithReferences { + [&](TextIngestionMethod _) -> ContentAddressWithReferences { if (refs.self) throw UsageError("Cannot have a self reference with text hashing scheme"); return TextInfo { @@ -182,7 +182,7 @@ ContentAddressMethod ContentAddressWithReferences::getMethod() const { return std::visit(overloaded { [](const TextInfo & th) -> ContentAddressMethod { - return TextHashMethod {}; + return TextIngestionMethod {}; }, [](const FixedOutputInfo & fsh) -> ContentAddressMethod { return fsh.hash.method; diff --git a/src/libstore/content-address.hh b/src/libstore/content-address.hh index 962b63e83a5..8668acacf22 100644 --- a/src/libstore/content-address.hh +++ b/src/libstore/content-address.hh @@ -22,7 +22,7 @@ namespace nix { * Somewhat obscure, used by \ref Derivation derivations and * `builtins.toFile` currently. */ -struct TextHashMethod : std::monostate { }; +struct TextIngestionMethod : std::monostate { }; /** * An enumeration of the main ways we can serialize file system @@ -57,7 +57,7 @@ std::string makeFileIngestionPrefix(FileIngestionMethod m); struct ContentAddressMethod { typedef std::variant< - TextHashMethod, + TextIngestionMethod, FileIngestionMethod > Raw; diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc index d3b9988c956..31e2e2af505 100644 --- a/src/libstore/daemon.cc +++ b/src/libstore/daemon.cc @@ -406,7 +406,7 @@ static void performOp(TunnelLogger * logger, ref store, FramedSource source(from); // TODO this is essentially RemoteStore::addCAToStore. Move it up to Store. return std::visit(overloaded { - [&](const TextHashMethod &) { + [&](const TextIngestionMethod &) { if (hashType != htSHA256) throw UnimplementedError("Only SHA-256 is supported for adding text-hashed data, but '%1' was given", printHashType(hashType)); diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index fc76ae7ad9c..1f5f789649c 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -216,6 +216,8 @@ static DerivationOutput parseDerivationOutput(const Store & store, { if (hashAlgo != "") { ContentAddressMethod method = ContentAddressMethod::parsePrefix(hashAlgo); + if (method == TextIngestionMethod {}) + experimentalFeatureSettings.require(Xp::DynamicDerivations); const auto hashType = parseHashType(hashAlgo); if (hashS == "impure") { experimentalFeatureSettings.require(Xp::ImpureDerivations); diff --git a/src/libstore/derivations.hh b/src/libstore/derivations.hh index 65901ec6d08..b36e4ea917f 100644 --- a/src/libstore/derivations.hh +++ b/src/libstore/derivations.hh @@ -339,12 +339,14 @@ struct Derivation : BasicDerivation Store & store, const std::map, StorePath> & inputDrvOutputs) const; - /* Check that the derivation is valid and does not present any - illegal states. - - This is mainly a matter of checking the outputs, where our C++ - representation supports all sorts of combinations we do not yet - allow. */ + /** + * Check that the derivation is valid and does not present any + * illegal states. + * + * This is mainly a matter of checking the outputs, where our C++ + * representation supports all sorts of combinations we do not yet + * allow. + */ void checkInvariants(Store & store, const StorePath & drvPath) const; Derivation() = default; diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index ff534883005..b3f5251f225 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -629,7 +629,7 @@ ref RemoteStore::addCAToStore( if (repair) throw Error("repairing is not supported when building through the Nix daemon protocol < 1.25"); std::visit(overloaded { - [&](const TextHashMethod & thm) -> void { + [&](const TextIngestionMethod & thm) -> void { if (hashType != htSHA256) throw UnimplementedError("Only SHA-256 is supported for adding text-hashed data, but '%1' was given", printHashType(hashType)); @@ -782,7 +782,7 @@ StorePath RemoteStore::addTextToStore( RepairFlag repair) { StringSource source(s); - return addCAToStore(source, name, TextHashMethod {}, htSHA256, references, repair)->path; + return addCAToStore(source, name, TextIngestionMethod {}, htSHA256, references, repair)->path; } void RemoteStore::registerDrvOutput(const Realisation & info) diff --git a/src/libutil/experimental-features.cc b/src/libutil/experimental-features.cc index bd189966253..ad0ec0427e5 100644 --- a/src/libutil/experimental-features.cc +++ b/src/libutil/experimental-features.cc @@ -12,7 +12,7 @@ struct ExperimentalFeatureDetails std::string_view description; }; -constexpr std::array xpFeatureDetails = {{ +constexpr std::array xpFeatureDetails = {{ { .tag = Xp::CaDerivations, .name = "ca-derivations", @@ -199,6 +199,16 @@ constexpr std::array xpFeatureDetails = {{ networking. )", }, + { + .tag = Xp::DynamicDerivations, + .name = "dynamic-derivations", + .description = R"( + Allow the use of a few things related to dynamic derivations: + + - "text hashing" derivation outputs, so we can build .drv + files. + )", + }, }}; static_assert( diff --git a/src/libutil/experimental-features.hh b/src/libutil/experimental-features.hh index 3c00bc4e55b..409100592a8 100644 --- a/src/libutil/experimental-features.hh +++ b/src/libutil/experimental-features.hh @@ -29,6 +29,7 @@ enum struct ExperimentalFeature Cgroups, DiscardReferences, DaemonTrustOverride, + DynamicDerivations, }; /** diff --git a/tests/ca/text-hashed-output.sh b/tests/ca/text-hashed-output.sh index bbe5de76362..dcb7e1e9642 100644 --- a/tests/ca/text-hashed-output.sh +++ b/tests/ca/text-hashed-output.sh @@ -2,8 +2,10 @@ source common.sh -# Globally enable the ca derivations experimental flag -sed -i 's/experimental-features = .*/& ca-derivations ca-references/' "$NIX_CONF_DIR/nix.conf" +# Globally enable dynamic-derivations in addition to CA derivations +enableFeatures "dynamic-derivations" + +restartDaemon # In the corresponding nix file, we have two derivations: the first, named root, # is a normal recursive derivation, while the second, named dependent, has the @@ -15,13 +17,13 @@ sed -i 's/experimental-features = .*/& ca-derivations ca-references/' "$NIX_CONF # - build the dependent derivation # - check that the path of the output coincides with that of the original derivation -drv=$(nix-instantiate --experimental-features ca-derivations ./text-hashed-output.nix -A root) +drv=$(nix-instantiate ./text-hashed-output.nix -A root) nix show-derivation "$drv" -drvDep=$(nix-instantiate --experimental-features ca-derivations ./text-hashed-output.nix -A dependent) +drvDep=$(nix-instantiate ./text-hashed-output.nix -A dependent) nix show-derivation "$drvDep" -out1=$(nix-build --experimental-features ca-derivations ./text-hashed-output.nix -A dependent --no-out-link) +out1=$(nix-build ./text-hashed-output.nix -A dependent --no-out-link) nix path-info $drv --derivation --json | jq nix path-info $out1 --derivation --json | jq From 20decfd30261bd46d2bf78209cb2bdd144fcd0b4 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 19 Apr 2023 11:33:48 -0400 Subject: [PATCH 10/24] Gate `dynamic-derivations` with drv `fromJSON` too Don't want `nix derivation add` to be a way to sneak by experimental feature checks! --- src/libstore/derivations.cc | 2 ++ src/libstore/tests/derivation.cc | 10 +++++++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index 9f529c75370..564c12f9e0f 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -1005,6 +1005,8 @@ DerivationOutput DerivationOutput::fromJSON( // remaining to parse, will be mutated by parsers std::string_view s = hashAlgo; ContentAddressMethod method = ContentAddressMethod::parsePrefix(s); + if (method == TextIngestionMethod {}) + xpSettings.require(Xp::DynamicDerivations); auto hashType = parseHashType(s); return { std::move(method), std::move(hashType) }; }; diff --git a/src/libstore/tests/derivation.cc b/src/libstore/tests/derivation.cc index 48524a71090..85e451b29f7 100644 --- a/src/libstore/tests/derivation.cc +++ b/src/libstore/tests/derivation.cc @@ -26,6 +26,14 @@ class CaDerivationTest : public DerivationTest } }; +class DynDerivationTest : public DerivationTest +{ + void SetUp() override + { + mockXpSettings.set("experimental-features", "dynamic-derivations ca-derivations"); + } +}; + class ImpureDerivationTest : public DerivationTest { void SetUp() override @@ -83,7 +91,7 @@ TEST_JSON(DerivationTest, caFixed, }), "drv-name", "output-name") -TEST_JSON(CaDerivationTest, caFixedText, +TEST_JSON(DynDerivationTest, caFixedText, R"({ "hashAlgo": "text:sha256", "hash": "894517c9163c896ec31a2adbd33c0681fd5f45b2c0ef08a64c92a03fb97f390f", From aba8a8a83a89d577769a39a69e9b90e3ed0d4f82 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 19 Apr 2023 14:13:30 -0400 Subject: [PATCH 11/24] Add a few more content addressing methods Good to round out the library interface. --- src/libstore/content-address.cc | 68 ++++++++++++++++++++++++--------- src/libstore/content-address.hh | 23 ++++++++--- 2 files changed, 67 insertions(+), 24 deletions(-) diff --git a/src/libstore/content-address.cc b/src/libstore/content-address.cc index 2bde23e79de..8a65059e3f0 100644 --- a/src/libstore/content-address.cc +++ b/src/libstore/content-address.cc @@ -154,38 +154,32 @@ std::string renderContentAddress(std::optional ca) return ca ? ca->render() : ""; } -ContentAddressWithReferences ContentAddressWithReferences::fromParts( - ContentAddressMethod method, Hash hash, StoreReferences refs) +ContentAddress ContentAddress::fromParts( + ContentAddressMethod method, Hash hash) { return std::visit(overloaded { - [&](TextIngestionMethod _) -> ContentAddressWithReferences { - if (refs.self) - throw UsageError("Cannot have a self reference with text hashing scheme"); - return TextInfo { - .hash = { .hash = std::move(hash) }, - .references = std::move(refs.others), + [&](TextIngestionMethod _) -> ContentAddress { + return TextHash { + .hash = std::move(hash), }; }, - [&](FileIngestionMethod m2) -> ContentAddressWithReferences { - return FixedOutputInfo { - .hash = { - .method = m2, - .hash = std::move(hash), - }, - .references = std::move(refs), + [&](FileIngestionMethod m2) -> ContentAddress { + return FixedOutputHash { + .method = std::move(m2), + .hash = std::move(hash), }; }, }, method.raw); } -ContentAddressMethod ContentAddressWithReferences::getMethod() const +ContentAddressMethod ContentAddress::getMethod() const { return std::visit(overloaded { - [](const TextInfo & th) -> ContentAddressMethod { + [](const TextHash & th) -> ContentAddressMethod { return TextIngestionMethod {}; }, - [](const FixedOutputInfo & fsh) -> ContentAddressMethod { - return fsh.hash.method; + [](const FixedOutputHash & fsh) -> ContentAddressMethod { + return fsh.method; }, }, raw); } @@ -229,6 +223,42 @@ ContentAddressWithReferences ContentAddressWithReferences::withoutRefs(const Con }, ca.raw); } +ContentAddressWithReferences ContentAddressWithReferences::fromParts( + ContentAddressMethod method, Hash hash, StoreReferences refs) +{ + return std::visit(overloaded { + [&](TextIngestionMethod _) -> ContentAddressWithReferences { + if (refs.self) + throw UsageError("Cannot have a self reference with text hashing scheme"); + return TextInfo { + .hash = { .hash = std::move(hash) }, + .references = std::move(refs.others), + }; + }, + [&](FileIngestionMethod m2) -> ContentAddressWithReferences { + return FixedOutputInfo { + .hash = { + .method = m2, + .hash = std::move(hash), + }, + .references = std::move(refs), + }; + }, + }, method.raw); +} + +ContentAddressMethod ContentAddressWithReferences::getMethod() const +{ + return std::visit(overloaded { + [](const TextInfo & th) -> ContentAddressMethod { + return TextIngestionMethod {}; + }, + [](const FixedOutputInfo & fsh) -> ContentAddressMethod { + return fsh.hash.method; + }, + }, raw); +} + Hash ContentAddressWithReferences::getHash() const { return std::visit(overloaded { diff --git a/src/libstore/content-address.hh b/src/libstore/content-address.hh index 8668acacf22..eb01e9ce405 100644 --- a/src/libstore/content-address.hh +++ b/src/libstore/content-address.hh @@ -154,8 +154,9 @@ struct ContentAddress { } /** - * Compute the content-addressability assertion (ValidPathInfo::ca) for - * paths created by Store::makeFixedOutputPath() / Store::addToStore(). + * Compute the content-addressability assertion + * (`ValidPathInfo::ca`) for paths created by + * `Store::makeFixedOutputPath()` / `Store::addToStore()`. */ std::string render() const; @@ -163,6 +164,18 @@ struct ContentAddress static std::optional parseOpt(std::string_view rawCaOpt); + /** + * Create a `ContentAddress` from 2 parts: + * + * @param method Way ingesting the file system data. + * + * @param hash Hash of ingested file system data. + */ + static ContentAddress fromParts( + ContentAddressMethod method, Hash hash); + + ContentAddressMethod getMethod() const; + const Hash & getHash() const; }; @@ -251,13 +264,13 @@ struct ContentAddressWithReferences { } /** - * Create a ContentAddressWithReferences from a mere ContentAddress, by - * assuming no references in all cases. + * Create a `ContentAddressWithReferences` from a mere + * `ContentAddress`, by assuming no references in all cases. */ static ContentAddressWithReferences withoutRefs(const ContentAddress &); /** - * Create a ContentAddressWithReferences from 3 parts: + * Create a `ContentAddressWithReferences` from 3 parts: * * @param method Way ingesting the file system data. * From 7103c6da705c8a18fef9e8f8d404e8d0ab5082ff Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 19 Apr 2023 14:48:53 -0400 Subject: [PATCH 12/24] Remove references from fixed output derivation ab syntax In other words, use a plain `ContentAddress` not `ContentAddressWithReferences` for `DerivationOutput::CAFixed`. Supporting fixed output derivations with (fixed) references would be a cool feature, but it is out of scope at this moment. --- src/libexpr/primops.cc | 6 ++--- src/libstore/build/derivation-goal.cc | 6 +++-- src/libstore/content-address.cc | 10 ++++----- src/libstore/content-address.hh | 4 ++-- src/libstore/derivations.cc | 13 +++++------ src/libstore/derivations.hh | 6 +++-- src/libstore/misc.cc | 28 ++++++++++------------- src/libstore/store-api.hh | 2 +- src/libstore/tests/derivation.cc | 32 +++++++++++++++++---------- 9 files changed, 55 insertions(+), 52 deletions(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index fc397db3312..4f93f3d9872 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -1303,11 +1303,9 @@ drvName, Bindings * attrs, Value & v) auto method = ingestionMethod.value_or(FileIngestionMethod::Flat); DerivationOutput::CAFixed dof { - .ca = ContentAddressWithReferences::fromParts( + .ca = ContentAddress::fromParts( std::move(method), - std::move(h), - // FIXME non-trivial fixed refs set - {}), + std::move(h)), }; drv.env["out"] = state.store->printStorePath(dof.path(*state.store, drvName, "out")); diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index a4bb94b0ee2..af153ebfb4c 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -274,11 +274,13 @@ void DerivationGoal::haveDerivation() ) ) ); - else + else { + auto * cap = getDerivationCA(*drv); addWaitee(upcast_goal(worker.makePathSubstitutionGoal( status.known->path, buildMode == bmRepair ? Repair : NoRepair, - getDerivationCA(*drv)))); + cap ? std::optional { *cap } : std::nullopt))); + } } if (waitees.empty()) /* to prevent hang (no wake-up event) */ diff --git a/src/libstore/content-address.cc b/src/libstore/content-address.cc index 8a65059e3f0..167f7af6c4f 100644 --- a/src/libstore/content-address.cc +++ b/src/libstore/content-address.cc @@ -196,6 +196,11 @@ const Hash & ContentAddress::getHash() const }, raw); } +std::string ContentAddress::printMethodAlgo() const { + return getMethod().renderPrefix() + + printHashType(getHash().type); +} + bool StoreReferences::empty() const { return !self && others.empty(); @@ -271,9 +276,4 @@ Hash ContentAddressWithReferences::getHash() const }, raw); } -std::string ContentAddressWithReferences::printMethodAlgo() const { - return getMethod().renderPrefix() - + printHashType(getHash().type); -} - } diff --git a/src/libstore/content-address.hh b/src/libstore/content-address.hh index eb01e9ce405..b25e6d49d66 100644 --- a/src/libstore/content-address.hh +++ b/src/libstore/content-address.hh @@ -177,6 +177,8 @@ struct ContentAddress ContentAddressMethod getMethod() const; const Hash & getHash() const; + + std::string printMethodAlgo() const; }; std::string renderContentAddress(std::optional ca); @@ -286,8 +288,6 @@ struct ContentAddressWithReferences ContentAddressMethod getMethod() const; Hash getHash() const; - - std::string printMethodAlgo() const; }; } diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index 564c12f9e0f..d56dc727bcd 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -38,7 +38,7 @@ StorePath DerivationOutput::CAFixed::path(const Store & store, std::string_view { return store.makeFixedOutputPathFromCA( outputPathName(drvName, outputName), - ca); + ContentAddressWithReferences::withoutRefs(ca)); } @@ -230,11 +230,9 @@ static DerivationOutput parseDerivationOutput(const Store & store, validatePath(pathS); auto hash = Hash::parseNonSRIUnprefixed(hashS, hashType); return DerivationOutput::CAFixed { - .ca = ContentAddressWithReferences::fromParts( + .ca = ContentAddress::fromParts( std::move(method), - std::move(hash), - // FIXME non-trivial fixed refs set - {}), + std::move(hash)), }; } else { experimentalFeatureSettings.require(Xp::CaDerivations); @@ -1020,10 +1018,9 @@ DerivationOutput DerivationOutput::fromJSON( else if (keys == (std::set { "path", "hashAlgo", "hash" })) { auto [method, hashType] = methodAlgo(); auto dof = DerivationOutput::CAFixed { - .ca = ContentAddressWithReferences::fromParts( + .ca = ContentAddress::fromParts( std::move(method), - Hash::parseNonSRIUnprefixed((std::string) json["hash"], hashType), - {}), + Hash::parseNonSRIUnprefixed((std::string) json["hash"], hashType)), }; if (dof.path(store, drvName, outputName) != store.parseStorePath((std::string) json["path"])) throw Error("Path doesn't match derivation output"); diff --git a/src/libstore/derivations.hh b/src/libstore/derivations.hh index 666dbff4157..1e2143f3123 100644 --- a/src/libstore/derivations.hh +++ b/src/libstore/derivations.hh @@ -36,9 +36,11 @@ struct DerivationOutputInputAddressed struct DerivationOutputCAFixed { /** - * hash and refs used for expected hash computation + * Method and hash used for expected hash computation. + * + * References are not allowed by fiat. */ - ContentAddressWithReferences ca; /* hash and refs used for validating output */ + ContentAddress ca; /** * Return the \ref StorePath "store path" corresponding to this output diff --git a/src/libstore/misc.cc b/src/libstore/misc.cc index 39bdfec6e98..00f629a85e9 100644 --- a/src/libstore/misc.cc +++ b/src/libstore/misc.cc @@ -83,26 +83,16 @@ void Store::computeFSClosure(const StorePath & startPath, } -std::optional getDerivationCA(const BasicDerivation & drv) +const ContentAddress * getDerivationCA(const BasicDerivation & drv) { auto out = drv.outputs.find("out"); if (out == drv.outputs.end()) - return std::nullopt; + return nullptr; if (auto dof = std::get_if(&out->second)) { - return std::visit(overloaded { - [&](const TextInfo & ti) -> std::optional { - if (!ti.references.empty()) - return std::nullopt; - return ti.hash; - }, - [&](const FixedOutputInfo & fi) -> std::optional { - if (!fi.references.empty()) - return std::nullopt; - return fi.hash; - }, - }, dof->ca.raw); + + return &dof->ca; } - return std::nullopt; + return nullptr; } void Store::queryMissing(const std::vector & targets, @@ -152,7 +142,13 @@ void Store::queryMissing(const std::vector & targets, if (drvState_->lock()->done) return; SubstitutablePathInfos infos; - querySubstitutablePathInfos({{outPath, getDerivationCA(*drv)}}, infos); + auto * cap = getDerivationCA(*drv); + querySubstitutablePathInfos({ + { + outPath, + cap ? std::optional { *cap } : std::nullopt, + }, + }, infos); if (infos.empty()) { drvState_->lock()->done = true; diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index c910d1c96ea..bad61001405 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -1022,7 +1022,7 @@ std::optional decodeValidPathInfo( */ std::pair splitUriAndParams(const std::string & uri); -std::optional getDerivationCA(const BasicDerivation & drv); +const ContentAddress * getDerivationCA(const BasicDerivation & drv); std::map drvOutputReferences( Store & store, diff --git a/src/libstore/tests/derivation.cc b/src/libstore/tests/derivation.cc index 85e451b29f7..6328ad3700e 100644 --- a/src/libstore/tests/derivation.cc +++ b/src/libstore/tests/derivation.cc @@ -74,19 +74,30 @@ TEST_JSON(DerivationTest, inputAddressed, }), "drv-name", "output-name") -TEST_JSON(DerivationTest, caFixed, +TEST_JSON(DerivationTest, caFixedFlat, + R"({ + "hashAlgo": "sha256", + "hash": "894517c9163c896ec31a2adbd33c0681fd5f45b2c0ef08a64c92a03fb97f390f", + "path": "/nix/store/rhcg9h16sqvlbpsa6dqm57sbr2al6nzg-drv-name-output-name" + })", + (DerivationOutput::CAFixed { + .ca = FixedOutputHash { + .method = FileIngestionMethod::Flat, + .hash = Hash::parseAnyPrefixed("sha256-iUUXyRY8iW7DGirb0zwGgf1fRbLA7wimTJKgP7l/OQ8="), + }, + }), + "drv-name", "output-name") + +TEST_JSON(DerivationTest, caFixedNAR, R"({ "hashAlgo": "r:sha256", "hash": "894517c9163c896ec31a2adbd33c0681fd5f45b2c0ef08a64c92a03fb97f390f", "path": "/nix/store/c015dhfh5l0lp6wxyvdn7bmwhbbr6hr9-drv-name-output-name" })", (DerivationOutput::CAFixed { - .ca = FixedOutputInfo { - .hash = { - .method = FileIngestionMethod::Recursive, - .hash = Hash::parseAnyPrefixed("sha256-iUUXyRY8iW7DGirb0zwGgf1fRbLA7wimTJKgP7l/OQ8="), - }, - .references = {}, + .ca = FixedOutputHash { + .method = FileIngestionMethod::Recursive, + .hash = Hash::parseAnyPrefixed("sha256-iUUXyRY8iW7DGirb0zwGgf1fRbLA7wimTJKgP7l/OQ8="), }, }), "drv-name", "output-name") @@ -98,11 +109,8 @@ TEST_JSON(DynDerivationTest, caFixedText, "path": "/nix/store/6s1zwabh956jvhv4w9xcdb5jiyanyxg1-drv-name-output-name" })", (DerivationOutput::CAFixed { - .ca = TextInfo { - .hash = { - .hash = Hash::parseAnyPrefixed("sha256-iUUXyRY8iW7DGirb0zwGgf1fRbLA7wimTJKgP7l/OQ8="), - }, - .references = {}, + .ca = TextHash { + .hash = Hash::parseAnyPrefixed("sha256-iUUXyRY8iW7DGirb0zwGgf1fRbLA7wimTJKgP7l/OQ8="), }, }), "drv-name", "output-name") From 61d3e64fd0277bd6d331ec47d234eb166e4b09ef Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 19 Apr 2023 17:24:55 -0400 Subject: [PATCH 13/24] Require daemon version for text hashing test --- tests/ca/text-hashed-output.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/ca/text-hashed-output.sh b/tests/ca/text-hashed-output.sh index dcb7e1e9642..7d01d5cd798 100644 --- a/tests/ca/text-hashed-output.sh +++ b/tests/ca/text-hashed-output.sh @@ -2,6 +2,9 @@ source common.sh +# Need backend to support text-hashing too +requireDaemonNewerThan "2.16.0pre20230419" + # Globally enable dynamic-derivations in addition to CA derivations enableFeatures "dynamic-derivations" From f3a31b14db6629ec424f31842157e6f614231bcd Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 19 Apr 2023 18:49:50 -0400 Subject: [PATCH 14/24] Make `tests/dyn-drv` test dir --- .gitignore | 1 + tests/dyn-drv/common.sh | 8 ++++++++ tests/dyn-drv/config.nix.in | 1 + tests/{ca => dyn-drv}/text-hashed-output.nix | 0 tests/{ca => dyn-drv}/text-hashed-output.sh | 8 -------- tests/local.mk | 15 +++++++++++---- 6 files changed, 21 insertions(+), 12 deletions(-) create mode 100644 tests/dyn-drv/common.sh create mode 120000 tests/dyn-drv/config.nix.in rename tests/{ca => dyn-drv}/text-hashed-output.nix (100%) rename tests/{ca => dyn-drv}/text-hashed-output.sh (81%) diff --git a/.gitignore b/.gitignore index 8ceff4ef23e..e25fd7d0c19 100644 --- a/.gitignore +++ b/.gitignore @@ -85,6 +85,7 @@ perl/Makefile.config /tests/shell.drv /tests/config.nix /tests/ca/config.nix +/tests/dyn-drv/config.nix /tests/repl-result-out # /tests/lang/ diff --git a/tests/dyn-drv/common.sh b/tests/dyn-drv/common.sh new file mode 100644 index 00000000000..c786f6925fb --- /dev/null +++ b/tests/dyn-drv/common.sh @@ -0,0 +1,8 @@ +source ../common.sh + +# Need backend to support text-hashing too +requireDaemonNewerThan "2.16.0pre20230419" + +enableFeatures "ca-derivations dynamic-derivations" + +restartDaemon diff --git a/tests/dyn-drv/config.nix.in b/tests/dyn-drv/config.nix.in new file mode 120000 index 00000000000..af24ddb30b0 --- /dev/null +++ b/tests/dyn-drv/config.nix.in @@ -0,0 +1 @@ +../config.nix.in \ No newline at end of file diff --git a/tests/ca/text-hashed-output.nix b/tests/dyn-drv/text-hashed-output.nix similarity index 100% rename from tests/ca/text-hashed-output.nix rename to tests/dyn-drv/text-hashed-output.nix diff --git a/tests/ca/text-hashed-output.sh b/tests/dyn-drv/text-hashed-output.sh similarity index 81% rename from tests/ca/text-hashed-output.sh rename to tests/dyn-drv/text-hashed-output.sh index 7d01d5cd798..8c70e03354e 100644 --- a/tests/ca/text-hashed-output.sh +++ b/tests/dyn-drv/text-hashed-output.sh @@ -2,14 +2,6 @@ source common.sh -# Need backend to support text-hashing too -requireDaemonNewerThan "2.16.0pre20230419" - -# Globally enable dynamic-derivations in addition to CA derivations -enableFeatures "dynamic-derivations" - -restartDaemon - # In the corresponding nix file, we have two derivations: the first, named root, # is a normal recursive derivation, while the second, named dependent, has the # new outputHashMode "text". Note that in "dependent", we don't refer to the diff --git a/tests/local.mk b/tests/local.mk index e54726787a2..d3467aac285 100644 --- a/tests/local.mk +++ b/tests/local.mk @@ -110,7 +110,7 @@ nix_tests = \ ca/derivation-json.sh \ import-derivation.sh \ ca/import-derivation.sh \ - ca/text-hashed-output.sh \ + dyn-drv/text-hashed-output.sh \ nix_path.sh \ case-hack.sh \ placeholders.sh \ @@ -138,11 +138,18 @@ ifeq ($(HAVE_LIBCPUID), 1) nix_tests += compute-levels.sh endif -install-tests += $(foreach x, $(nix_tests), tests/$(x)) +install-tests += $(foreach x, $(nix_tests), $(d)/$(x)) -clean-files += $(d)/common/vars-and-functions.sh $(d)/config.nix $(d)/ca/config.nix +clean-files += \ + $(d)/common/vars-and-functions.sh \ + $(d)/config.nix \ + $(d)/ca/config.nix \ + $(d)/dyn-drv/config.nix -test-deps += tests/common/vars-and-functions.sh tests/config.nix tests/ca/config.nix +test-deps += \ + tests/common/vars-and-functions.sh \ + tests/config.nix \ + tests/dyn-drv/config.nix \ ifeq ($(BUILD_SHARED_LIBS), 1) test-deps += tests/plugins/libplugintest.$(SO_EXT) From e26662709e4f458e0916d43a613d44b174ad4679 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 19 Apr 2023 20:36:33 -0400 Subject: [PATCH 15/24] Add a more interesting test In this one, we don't just output an existing derivation as is, but modify it first. --- tests/dyn-drv/recursive-mod-json.nix | 33 ++++++++++++++++++++++++++++ tests/dyn-drv/recursive-mod-json.sh | 25 +++++++++++++++++++++ tests/local.mk | 1 + tests/recursive.sh | 6 ++--- 4 files changed, 62 insertions(+), 3 deletions(-) create mode 100644 tests/dyn-drv/recursive-mod-json.nix create mode 100644 tests/dyn-drv/recursive-mod-json.sh diff --git a/tests/dyn-drv/recursive-mod-json.nix b/tests/dyn-drv/recursive-mod-json.nix new file mode 100644 index 00000000000..9b32c55e9a1 --- /dev/null +++ b/tests/dyn-drv/recursive-mod-json.nix @@ -0,0 +1,33 @@ +with import ./config.nix; + +let innerName = "foo"; in + +mkDerivation rec { + name = "${innerName}.drv"; + SHELL = shell; + + requiredSystemFeatures = [ "recursive-nix" ]; + + drv = builtins.unsafeDiscardOutputDependency (import ./text-hashed-output.nix).root.drvPath; + + buildCommand = '' + export NIX_CONFIG='experimental-features = nix-command ca-derivations' + + PATH=${builtins.getEnv "EXTRA_PATH"}:$PATH + + # JSON of pre-existing drv + nix derivation show $drv | jq .[] > drv0.json + + # Fix name + jq < drv0.json '.name = "${innerName}"' > drv1.json + + # Extend `buildCommand` + jq < drv1.json '.env.buildCommand += "echo \"I am alive!\" >> $out/hello\n"' > drv0.json + + # Used as our output + cp $(nix derivation add < drv0.json) $out + ''; + __contentAddressed = true; + outputHashMode = "text"; + outputHashAlgo = "sha256"; +} diff --git a/tests/dyn-drv/recursive-mod-json.sh b/tests/dyn-drv/recursive-mod-json.sh new file mode 100644 index 00000000000..070c5c2cb80 --- /dev/null +++ b/tests/dyn-drv/recursive-mod-json.sh @@ -0,0 +1,25 @@ +source common.sh + +# FIXME +if [[ $(uname) != Linux ]]; then skipTest "Not running Linux"; fi + +enableFeatures 'recursive-nix' +restartDaemon + +clearStore + +rm -f $TEST_ROOT/result + +EXTRA_PATH=$(dirname $(type -p nix)):$(dirname $(type -p jq)) +export EXTRA_PATH + +# Will produce a drv +metaDrv=$(nix-instantiate ./recursive-mod-json.nix) + +# computed "dynamic" derivation +drv=$(nix-store -r $metaDrv) + +# build that dyn drv +res=$(nix-store -r $drv) + +grep 'I am alive!' $res/hello diff --git a/tests/local.mk b/tests/local.mk index d3467aac285..a3c2ef3d587 100644 --- a/tests/local.mk +++ b/tests/local.mk @@ -111,6 +111,7 @@ nix_tests = \ import-derivation.sh \ ca/import-derivation.sh \ dyn-drv/text-hashed-output.sh \ + dyn-drv/recursive-mod-json.sh \ nix_path.sh \ case-hack.sh \ placeholders.sh \ diff --git a/tests/recursive.sh b/tests/recursive.sh index 6335d44a526..27e1674ca9e 100644 --- a/tests/recursive.sh +++ b/tests/recursive.sh @@ -1,11 +1,11 @@ source common.sh -sed -i 's/experimental-features .*/& recursive-nix/' "$NIX_CONF_DIR"/nix.conf -restartDaemon - # FIXME if [[ $(uname) != Linux ]]; then skipTest "Not running Linux"; fi +enableFeatures 'recursive-nix' +restartDaemon + clearStore rm -f $TEST_ROOT/result From 969def696ae188113643925364799e419202cf4f Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 19 Apr 2023 20:47:23 -0400 Subject: [PATCH 16/24] Fix typo in tests --- tests/local.mk | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/local.mk b/tests/local.mk index a3c2ef3d587..d37c21abf5d 100644 --- a/tests/local.mk +++ b/tests/local.mk @@ -150,7 +150,8 @@ clean-files += \ test-deps += \ tests/common/vars-and-functions.sh \ tests/config.nix \ - tests/dyn-drv/config.nix \ + tests/ca/config.nix \ + tests/dyn-drv/config.nix ifeq ($(BUILD_SHARED_LIBS), 1) test-deps += tests/plugins/libplugintest.$(SO_EXT) From 8eeaf591db2814d13921fffc290e278817dbae0c Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 21 Apr 2023 01:30:55 -0400 Subject: [PATCH 17/24] Add more docs to `TextIngestionMethod` Thanks so much! Co-authored-by: Adam Joseph <54836058+amjoseph-nixpkgs@users.noreply.github.com> --- src/libstore/content-address.hh | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/libstore/content-address.hh b/src/libstore/content-address.hh index b25e6d49d66..9986d387f7c 100644 --- a/src/libstore/content-address.hh +++ b/src/libstore/content-address.hh @@ -21,6 +21,12 @@ namespace nix { * * Somewhat obscure, used by \ref Derivation derivations and * `builtins.toFile` currently. + * + * TextIngestionMethod is identical to FileIngestionMethod::Fixed except that + * the former may not have self-references and is tagged `text:${algo}:${hash}` + * rather than `fixed:${algo}:${hash}`. The contents of the store path are + * ingested and hashed identically, aside from the slightly different tag and + * restriction on self-references. */ struct TextIngestionMethod : std::monostate { }; From 278c94d607dae69226c782de44dcfa6381e3f90b Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 5 May 2023 11:49:41 -0400 Subject: [PATCH 18/24] Rename a few things in new tests Co-authored-by: Robert Hensing --- src/libstore/misc.cc | 1 - tests/dyn-drv/recursive-mod-json.nix | 2 +- tests/dyn-drv/text-hashed-output.nix | 10 +++++----- tests/dyn-drv/text-hashed-output.sh | 8 ++++---- 4 files changed, 10 insertions(+), 11 deletions(-) diff --git a/src/libstore/misc.cc b/src/libstore/misc.cc index 00f629a85e9..50336c77971 100644 --- a/src/libstore/misc.cc +++ b/src/libstore/misc.cc @@ -89,7 +89,6 @@ const ContentAddress * getDerivationCA(const BasicDerivation & drv) if (out == drv.outputs.end()) return nullptr; if (auto dof = std::get_if(&out->second)) { - return &dof->ca; } return nullptr; diff --git a/tests/dyn-drv/recursive-mod-json.nix b/tests/dyn-drv/recursive-mod-json.nix index 9b32c55e9a1..c6a24ca4f3b 100644 --- a/tests/dyn-drv/recursive-mod-json.nix +++ b/tests/dyn-drv/recursive-mod-json.nix @@ -8,7 +8,7 @@ mkDerivation rec { requiredSystemFeatures = [ "recursive-nix" ]; - drv = builtins.unsafeDiscardOutputDependency (import ./text-hashed-output.nix).root.drvPath; + drv = builtins.unsafeDiscardOutputDependency (import ./text-hashed-output.nix).hello.drvPath; buildCommand = '' export NIX_CONFIG='experimental-features = nix-command ca-derivations' diff --git a/tests/dyn-drv/text-hashed-output.nix b/tests/dyn-drv/text-hashed-output.nix index 31a66dfa85a..a700fd102de 100644 --- a/tests/dyn-drv/text-hashed-output.nix +++ b/tests/dyn-drv/text-hashed-output.nix @@ -4,8 +4,8 @@ with import ./config.nix; # The derivation can be arbitrarily modified by passing a different `seed`, # but the output will always be the same rec { - root = mkDerivation { - name = "text-hashed-root"; + hello = mkDerivation { + name = "hello"; buildCommand = '' set -x echo "Building a CA derivation" @@ -16,11 +16,11 @@ rec { outputHashMode = "recursive"; outputHashAlgo = "sha256"; }; - dependent = mkDerivation { - name = "text-hashed-root.drv"; + producingDrv = mkDerivation { + name = "hello.drv"; buildCommand = '' echo "Copying the derivation" - cp ${builtins.unsafeDiscardOutputDependency root.drvPath} $out + cp ${builtins.unsafeDiscardOutputDependency hello.drvPath} $out ''; __contentAddressed = true; outputHashMode = "text"; diff --git a/tests/dyn-drv/text-hashed-output.sh b/tests/dyn-drv/text-hashed-output.sh index 8c70e03354e..f3e5aa93bb7 100644 --- a/tests/dyn-drv/text-hashed-output.sh +++ b/tests/dyn-drv/text-hashed-output.sh @@ -12,13 +12,13 @@ source common.sh # - build the dependent derivation # - check that the path of the output coincides with that of the original derivation -drv=$(nix-instantiate ./text-hashed-output.nix -A root) +drv=$(nix-instantiate ./text-hashed-output.nix -A hello) nix show-derivation "$drv" -drvDep=$(nix-instantiate ./text-hashed-output.nix -A dependent) -nix show-derivation "$drvDep" +drvProducingDrv=$(nix-instantiate ./text-hashed-output.nix -A producingDrv) +nix show-derivation "$drvProducingDrv" -out1=$(nix-build ./text-hashed-output.nix -A dependent --no-out-link) +out1=$(nix-build ./text-hashed-output.nix -A producingDrv --no-out-link) nix path-info $drv --derivation --json | jq nix path-info $out1 --derivation --json | jq From 35dcbe1c21bbc898d34e6f0a3f9879a8c53cdaff Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 9 May 2023 12:19:03 -0400 Subject: [PATCH 19/24] Fix spurious change Didn't mean to use the private name that shouldn't be exposed. --- src/libstore/build/local-derivation-goal.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 7a424fbfc03..c8944ec8d11 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -2511,7 +2511,7 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs() [&](const DerivationOutput::CAFixed & dof) { auto wanted = dof.ca.getHash(); - auto newInfo0 = newInfoFromCA(DerivationOutputCAFloating { + auto newInfo0 = newInfoFromCA(DerivationOutput::CAFloating { .method = dof.ca.getMethod(), .hashType = wanted.type, }); From 6513f4fe92726baf2300448762c99733f6fe132a Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 9 May 2023 12:31:36 -0400 Subject: [PATCH 20/24] Fix bug, `newInfo` -> `newInfo0` It appears we were checking a variable in the process of definining it. --- src/libstore/build/local-derivation-goal.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index c8944ec8d11..eb6c00e77d3 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -2529,7 +2529,7 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs() wanted.to_string(SRI, true), got.to_string(SRI, true))); } - if (!newInfo.references.empty()) + if (!newInfo0.references.empty()) delayedException = std::make_exception_ptr( BuildError("illegal path references in fixed-output derivation '%s'", worker.store.printStorePath(drvPath))); From d3c125e5a8ab8ffa1a1ed614b1e174f5670a881c Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 9 May 2023 12:45:51 -0400 Subject: [PATCH 21/24] Apply suggestions from code review Thanks! Co-authored-by: Robert Hensing --- src/libstore/content-address.hh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libstore/content-address.hh b/src/libstore/content-address.hh index 9986d387f7c..4183ac1e78b 100644 --- a/src/libstore/content-address.hh +++ b/src/libstore/content-address.hh @@ -78,8 +78,8 @@ struct ContentAddressMethod /** - * Parse and pretty print the algorithm which indicates how the files - * were ingested, with the the fixed output case not prefixed for back + * Parse the prefix tag which indicates how the files + * were ingested, with the fixed output case not prefixed for back * compat. */ static ContentAddressMethod parsePrefix(std::string_view & m); @@ -273,9 +273,9 @@ struct ContentAddressWithReferences /** * Create a `ContentAddressWithReferences` from a mere - * `ContentAddress`, by assuming no references in all cases. + * `ContentAddress`, by claiming no references. */ - static ContentAddressWithReferences withoutRefs(const ContentAddress &); + static ContentAddressWithReferences withoutRefs(const ContentAddress &) noexcept; /** * Create a `ContentAddressWithReferences` from 3 parts: From 753fc1661d9b5275ee7048353872f10bcd9c2953 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 9 May 2023 13:05:38 -0400 Subject: [PATCH 22/24] Cleanups to content address types --- src/libstore/content-address.cc | 14 +++++++++----- src/libstore/content-address.hh | 23 ++++++++++++++++++++--- 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/src/libstore/content-address.cc b/src/libstore/content-address.cc index 167f7af6c4f..b62818e1189 100644 --- a/src/libstore/content-address.cc +++ b/src/libstore/content-address.cc @@ -21,7 +21,8 @@ std::string makeFileIngestionPrefix(FileIngestionMethod m) } } -std::string ContentAddressMethod::renderPrefix() const { +std::string ContentAddressMethod::renderPrefix() const +{ return std::visit(overloaded { [](TextIngestionMethod) -> std::string { return "text:"; }, [](FileIngestionMethod m2) { @@ -113,7 +114,8 @@ static std::pair parseContentAddressMethodPrefix throw UsageError("content address prefix '%s' is unrecognized. Recogonized prefixes are 'text' or 'fixed'", prefix); } -ContentAddress ContentAddress::parse(std::string_view rawCa) { +ContentAddress ContentAddress::parse(std::string_view rawCa) +{ auto rest = rawCa; auto [caMethod, hashType_] = parseContentAddressMethodPrefix(rest); @@ -155,7 +157,7 @@ std::string renderContentAddress(std::optional ca) } ContentAddress ContentAddress::fromParts( - ContentAddressMethod method, Hash hash) + ContentAddressMethod method, Hash hash) noexcept { return std::visit(overloaded { [&](TextIngestionMethod _) -> ContentAddress { @@ -196,7 +198,8 @@ const Hash & ContentAddress::getHash() const }, raw); } -std::string ContentAddress::printMethodAlgo() const { +std::string ContentAddress::printMethodAlgo() const +{ return getMethod().renderPrefix() + printHashType(getHash().type); } @@ -211,7 +214,8 @@ size_t StoreReferences::size() const return (self ? 1 : 0) + others.size(); } -ContentAddressWithReferences ContentAddressWithReferences::withoutRefs(const ContentAddress & ca) { +ContentAddressWithReferences ContentAddressWithReferences::withoutRefs(const ContentAddress & ca) noexcept +{ return std::visit(overloaded { [&](const TextHash & h) -> ContentAddressWithReferences { return TextInfo { diff --git a/src/libstore/content-address.hh b/src/libstore/content-address.hh index 4183ac1e78b..03a2e2a2325 100644 --- a/src/libstore/content-address.hh +++ b/src/libstore/content-address.hh @@ -81,17 +81,30 @@ struct ContentAddressMethod * Parse the prefix tag which indicates how the files * were ingested, with the fixed output case not prefixed for back * compat. + * + * @param [in] m A string that should begin with the prefix. + * @param [out] m The remainder of the string after the prefix. */ static ContentAddressMethod parsePrefix(std::string_view & m); + /** + * Render the prefix tag which indicates how the files wre ingested. + * + * The rough inverse of `parsePrefix()`. + */ std::string renderPrefix() const; /** - * Parse and pretty print a content addressing method and hash type in a - * nicer way, prefixing both cases. + * Parse a content addressing method and hash type. */ static std::pair parse(std::string_view rawCaMethod); + /** + * Render a content addressing method and hash type in a + * nicer way, prefixing both cases. + * + * The rough inverse of `parse()`. + */ std::string render(HashType ht) const; }; @@ -178,7 +191,7 @@ struct ContentAddress * @param hash Hash of ingested file system data. */ static ContentAddress fromParts( - ContentAddressMethod method, Hash hash); + ContentAddressMethod method, Hash hash) noexcept; ContentAddressMethod getMethod() const; @@ -187,6 +200,10 @@ struct ContentAddress std::string printMethodAlgo() const; }; +/** + * Render the `ContentAddress` if it exists to a string, return empty + * string otherwise. + */ std::string renderContentAddress(std::optional ca); From e514b3939adb087338a9ef7445afcecb0efb42b1 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 9 May 2023 13:24:53 -0400 Subject: [PATCH 23/24] Add name to some error messages --- src/libstore/daemon.cc | 4 ++-- src/libstore/remote-store.cc | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc index 31e2e2af505..5083497a97e 100644 --- a/src/libstore/daemon.cc +++ b/src/libstore/daemon.cc @@ -408,8 +408,8 @@ static void performOp(TunnelLogger * logger, ref store, return std::visit(overloaded { [&](const TextIngestionMethod &) { if (hashType != htSHA256) - throw UnimplementedError("Only SHA-256 is supported for adding text-hashed data, but '%1' was given", - printHashType(hashType)); + throw UnimplementedError("When adding text-hashed data called '%s', only SHA-256 is supported but '%s' was given", + name, printHashType(hashType)); // We could stream this by changing Store std::string contents = source.drain(); auto path = store->addTextToStore(name, contents, refs, repair); diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index b3f5251f225..0ed17a6ce50 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -631,8 +631,8 @@ ref RemoteStore::addCAToStore( std::visit(overloaded { [&](const TextIngestionMethod & thm) -> void { if (hashType != htSHA256) - throw UnimplementedError("Only SHA-256 is supported for adding text-hashed data, but '%1' was given", - printHashType(hashType)); + throw UnimplementedError("When adding text-hashed data called '%s', only SHA-256 is supported but '%s' was given", + name, printHashType(hashType)); std::string s = dump.drain(); conn->to << wopAddTextToStore << name << s; worker_proto::write(*this, conn->to, references); From 6a3a87a714e1f3be1464f8fd4c82714b7d032879 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 9 May 2023 14:44:08 -0400 Subject: [PATCH 24/24] Improve error message for self reference with text hashing The `ContentAddressWithReferences` method is made total, with error handling now squarely the caller's job. This is better. --- src/libstore/build/local-derivation-goal.cc | 16 ++++++++--- src/libstore/content-address.cc | 32 ++++++++++++--------- src/libstore/content-address.hh | 7 +++-- 3 files changed, 34 insertions(+), 21 deletions(-) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index eb6c00e77d3..2e4020f338e 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -2456,13 +2456,21 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs() }, }, outputHash.method.raw); auto got = caSink.finish().first; + + auto optCA = ContentAddressWithReferences::fromPartsOpt( + outputHash.method, + std::move(got), + rewriteRefs()); + if (!optCA) { + // TODO track distinct failure modes separately (at the time of + // writing there is just one but `nullopt` is unclear) so this + // message can't get out of sync. + throw BuildError("output path '%s' has illegal content address, probably a spurious self-reference with text hashing"); + } ValidPathInfo newInfo0 { worker.store, outputPathName(drv->name, outputName), - ContentAddressWithReferences::fromParts( - outputHash.method, - std::move(got), - rewriteRefs()), + *std::move(optCA), Hash::dummy, }; if (*scratchPath != newInfo0.path) { diff --git a/src/libstore/content-address.cc b/src/libstore/content-address.cc index b62818e1189..04f7ac214f4 100644 --- a/src/libstore/content-address.cc +++ b/src/libstore/content-address.cc @@ -232,25 +232,29 @@ ContentAddressWithReferences ContentAddressWithReferences::withoutRefs(const Con }, ca.raw); } -ContentAddressWithReferences ContentAddressWithReferences::fromParts( - ContentAddressMethod method, Hash hash, StoreReferences refs) +std::optional ContentAddressWithReferences::fromPartsOpt( + ContentAddressMethod method, Hash hash, StoreReferences refs) noexcept { return std::visit(overloaded { - [&](TextIngestionMethod _) -> ContentAddressWithReferences { + [&](TextIngestionMethod _) -> std::optional { if (refs.self) - throw UsageError("Cannot have a self reference with text hashing scheme"); - return TextInfo { - .hash = { .hash = std::move(hash) }, - .references = std::move(refs.others), + return std::nullopt; + return ContentAddressWithReferences { + TextInfo { + .hash = { .hash = std::move(hash) }, + .references = std::move(refs.others), + } }; }, - [&](FileIngestionMethod m2) -> ContentAddressWithReferences { - return FixedOutputInfo { - .hash = { - .method = m2, - .hash = std::move(hash), - }, - .references = std::move(refs), + [&](FileIngestionMethod m2) -> std::optional { + return ContentAddressWithReferences { + FixedOutputInfo { + .hash = { + .method = m2, + .hash = std::move(hash), + }, + .references = std::move(refs), + } }; }, }, method.raw); diff --git a/src/libstore/content-address.hh b/src/libstore/content-address.hh index 03a2e2a2325..e1e80448ba0 100644 --- a/src/libstore/content-address.hh +++ b/src/libstore/content-address.hh @@ -303,10 +303,11 @@ struct ContentAddressWithReferences * * @param refs References to other store objects or oneself. * - * Do note that not all combinations are supported. + * Do note that not all combinations are supported; `nullopt` is + * returns for invalid combinations. */ - static ContentAddressWithReferences fromParts( - ContentAddressMethod method, Hash hash, StoreReferences refs); + static std::optional fromPartsOpt( + ContentAddressMethod method, Hash hash, StoreReferences refs) noexcept; ContentAddressMethod getMethod() const;