From b99062b023e56865ba20371b29fa3be6e6149d46 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 4 Sep 2020 10:29:28 -0400 Subject: [PATCH 1/5] Update tests/content-addressed.nix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Théophane Hufschmitt --- tests/content-addressed.nix | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/content-addressed.nix b/tests/content-addressed.nix index 79e8a8bf9..b0eb29c3f 100644 --- a/tests/content-addressed.nix +++ b/tests/content-addressed.nix @@ -13,9 +13,6 @@ rec { mkdir -p $out echo "Hello World" > $out/hello ''; - __contentAddressed = true; - outputHashMode = "recursive"; - outputHashAlgo = "sha256"; }; rootCA = mkDerivation { name = "dependent"; From c9f1ed912c2ed3d53fdfe84cbb0012b5c7c2332f Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 4 Sep 2020 14:38:45 +0000 Subject: [PATCH 2/5] Don't chmod symlink before moving outputs around MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Théophane Hufschmitt --- src/libstore/build.cc | 11 ++++++++--- tests/content-addressed.nix | 3 +++ 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index ba28e78c8..f406c89d2 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -2065,7 +2065,7 @@ void linkOrCopy(const Path & from, const Path & to) file (e.g. 32000 of ext3), which is quite possible after a 'nix-store --optimise'. FIXME: actually, why don't we just bind-mount in this case? - + It can also fail with EPERM in BeegFS v7 and earlier versions which don't allow hard-links to other directories */ if (errno != EMLINK && errno != EPERM) @@ -4101,8 +4101,13 @@ void DerivationGoal::registerOutputs() if (lstat(actualPath.c_str(), &st)) throw SysError("getting attributes of path '%1%'", actualPath); mode |= 0200; - if (chmod(actualPath.c_str(), mode) == -1) - throw SysError("changing mode of '%1%' to %2$o", actualPath, mode); + /* Try to change the perms, but only if the file isn't a + symlink as symlinks permissions are mostly ignored and + calling `chmod` on it will just forward the call to the + target of the link. */ + if (!S_ISLNK(st.st_mode)) + if (chmod(actualPath.c_str(), mode) == -1) + throw SysError("changing mode of '%1%' to %2$o", actualPath, mode); } if (rename( actualPath.c_str(), diff --git a/tests/content-addressed.nix b/tests/content-addressed.nix index b0eb29c3f..5e9bad0ac 100644 --- a/tests/content-addressed.nix +++ b/tests/content-addressed.nix @@ -16,11 +16,14 @@ rec { }; rootCA = mkDerivation { name = "dependent"; + outputs = [ "out" "dev" ]; buildCommand = '' echo "building a CA derivation" echo "The seed is ${toString seed}" mkdir -p $out echo ${rootLegacy}/hello > $out/dep + # test symlink at root + ln -s $out $dev ''; __contentAddressed = true; outputHashMode = "recursive"; From e86dd59dccb550c7f1a4d9333eee3c3a5c4043e9 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 4 Sep 2020 10:48:50 -0400 Subject: [PATCH 3/5] Apply suggestions from code review Thanks! Co-authored-by: Eelco Dolstra --- src/libstore/build.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index f406c89d2..5584249d2 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -1266,7 +1266,7 @@ void DerivationGoal::haveDerivation() for (auto & [_, status] : initialOutputs) { if (!status.wanted) continue; if (!status.known) { - warn("Do not know how to query for unknown floating CA drv output yet"); + warn("do not know how to query for unknown floating content-addressed derivation output yet"); /* Nothing to wait for; tail call */ return DerivationGoal::gaveUpOnSubstitution(); } @@ -1463,7 +1463,7 @@ void DerivationGoal::inputsRealised() auto optRealizedInput = outputs.at(j); if (!optRealizedInput) throw Error( - "derivation '%s' requires output '%s' from input derivation '%s', which is supposedly realized already, yet we still don't know what path corresponds to that output.", + "derivation '%s' requires output '%s' from input derivation '%s', which is supposedly realized already, yet we still don't know what path corresponds to that output", worker.store.printStorePath(drvPath), j, worker.store.printStorePath(drvPath)); worker.store.computeFSClosure(*optRealizedInput, inputPaths); } else @@ -2032,7 +2032,7 @@ StorePathSet DerivationGoal::exportReferences(const StorePathSet & storePaths) `computeFSClosure` on the output path, rather than derivation itself. That doesn't seem right to me, so I won't try to implemented this for CA derivations. */ - throw UnimplementedError("export references including CA derivations (themselves) is not yet implemented"); + throw UnimplementedError("exportReferences on CA derivations is not yet implemented"); worker.store.computeFSClosure(*k.second.second, paths); } } @@ -2175,7 +2175,7 @@ void DerivationGoal::startBuilder() differ. */ if (fixedFinalPath == scratchPath) continue; - /* Ensure scratch scratch path is ours to use */ + /* Ensure scratch path is ours to use. */ deletePath(worker.store.printStorePath(scratchPath)); /* Rewrite and unrewrite paths */ From e9fad3006b0b6f3a6430fdbfc61392cac6834502 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 4 Sep 2020 15:15:51 +0000 Subject: [PATCH 4/5] Fix some of the issues raised by @edolstra - More and better comments - The easier renames --- src/libstore/build.cc | 25 +++++++++++++------------ src/libstore/derivations.hh | 18 ++++++++++++++++++ src/libstore/local-store.hh | 5 ++--- src/nix/command.cc | 5 +++-- 4 files changed, 36 insertions(+), 17 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 5584249d2..9eff03f7b 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -718,7 +718,7 @@ typedef enum {rpAccept, rpDecline, rpPostpone} HookReply; class SubstitutionGoal; -struct KnownInitialOutputStatus { +struct InitialOutputStatus { StorePath path; /* The output optional indicates whether it's already valid; i.e. exists and is registered. If we're repairing, inner bool indicates whether the @@ -731,9 +731,9 @@ struct KnownInitialOutputStatus { } }; -struct InitialOutputStatus { +struct InitialOutput { bool wanted; - std::optional known; + std::optional known; }; class DerivationGoal : public Goal @@ -770,7 +770,7 @@ private: immediate input paths). */ StorePathSet inputPaths; - std::map initialOutputs; + std::map initialOutputs; /* User selected for running the builder. */ std::unique_ptr buildUser; @@ -1050,11 +1050,14 @@ private: /* Forcibly kill the child process, if any. */ void killChild(); - /* Map a path to another (reproducably) so we can avoid overwriting outputs + /* Create alternative path calculated from but distinct from the + input, so we can avoid overwriting outputs (or other store paths) that already exist. */ StorePath makeFallbackPath(const StorePath & path); - /* Make a path to another based on the output name alone, if one doesn't - want to use a random path for CA builds. */ + /* Make a path to another based on the output name along with the + derivation hash. */ + /* FIXME add option to randomize, so we can audit whether our + rewrites caught everything */ StorePath makeFallbackPath(std::string_view outputName); void repairClosure(); @@ -2141,8 +2144,6 @@ void DerivationGoal::startBuilder() with the actual hashes. */ auto scratchPath = !status.known - /* FIXME add option to randomize, so we can audit whether our - * rewrites caught everything */ ? makeFallbackPath(outputName) : !needsHashRewrite() /* Can always use original path in sandbox */ @@ -4582,19 +4583,19 @@ void DerivationGoal::checkPathValidity() { bool checkHash = buildMode == bmRepair; for (auto & i : queryPartialDerivationOutputMap()) { - InitialOutputStatus status { + InitialOutput info { .wanted = wantOutput(i.first, wantedOutputs), }; if (i.second) { auto outputPath = *i.second; - status.known = { + info.known = { .path = outputPath, .valid = !worker.store.isValidPath(outputPath) ? std::optional {} : !checkHash || worker.pathContentsGood(outputPath), }; } - initialOutputs.insert_or_assign(i.first, status); + initialOutputs.insert_or_assign(i.first, info); } } diff --git a/src/libstore/derivations.hh b/src/libstore/derivations.hh index c5c6e90ca..2c0b1feab 100644 --- a/src/libstore/derivations.hh +++ b/src/libstore/derivations.hh @@ -145,6 +145,11 @@ Derivation parseDerivation(const Store & store, std::string && s, std::string_vi // FIXME: remove bool isDerivation(const string & fileName); +/* Calculate the name that will be used for the store path for this + output. + + This is usually -, but is just when + the output name is "out". */ std::string outputPathName(std::string_view drvName, std::string_view outputName); // known CA drv's output hashes, current just for fixed-output derivations @@ -194,8 +199,21 @@ struct Sink; Source & readDerivation(Source & in, const Store & store, BasicDerivation & drv, std::string_view name); void writeDerivation(Sink & out, const Store & store, const BasicDerivation & drv); +/* This creates an opaque and almost certainly unique string + deterministically from the output name. + + It is used as a placeholder to allow derivations to refer to their + own outputs without needing to use the hash of a derivation in + itself, making the hash near-impossible to calculate. */ std::string hashPlaceholder(const std::string & outputName); +/* This creates an opaque and almost certainly unique string + deterministically from a derivation path and output name. + + It is used as a placeholder to allow derivations to refer to + content-addressed paths whose content --- and thus the path + themselves --- isn't yet known. This occurs when a derivation has a + dependency which is a CA derivation. */ std::string downstreamPlaceholder(const Store & store, const StorePath & drvPath, std::string_view outputName); } diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index ea430fa14..d5e6d68ef 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -279,9 +279,8 @@ private: specified by the ‘secret-key-files’ option. */ void signPathInfo(ValidPathInfo & info); - /* Add a mapping from the deriver of the path info (if specified) to its - * out path - */ + /* Register the store path 'output' as the output named 'outputName' of + derivation 'deriver'. */ void linkDeriverToPath(const StorePath & deriver, const string & outputName, const StorePath & output); void linkDeriverToPath(State & state, uint64_t deriver, const string & outputName, const StorePath & output); diff --git a/src/nix/command.cc b/src/nix/command.cc index f697eb84c..37a4bc785 100644 --- a/src/nix/command.cc +++ b/src/nix/command.cc @@ -150,8 +150,9 @@ void MixProfile::updateProfile(const Buildables & buildables) }, [&](BuildableFromDrv bfd) { for (auto & output : bfd.outputs) { - if (!output.second) - throw Error("output path should be known because we just tried to build it"); + /* Output path should be known because we just tried to + build it. */ + assert(!output.second); result.push_back(*output.second); } }, From 5aed6f9b25b8547feb67bbbfaa263d013b82fb52 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 4 Sep 2020 15:58:42 +0000 Subject: [PATCH 5/5] Document `mkOutputString` --- src/libexpr/primops.cc | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 4e248f979..78dc314fa 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -75,6 +75,18 @@ void EvalState::realiseContext(const PathSet & context) } } +/* Add and attribute to the given attribute map from the output name to + the output path, or a placeholder. + + Where possible the path is used, but for floating CA derivations we + may not know it. For sake of determinism we always assume we don't + and instead put in a place holder. In either case, however, the + string context will contain the drv path and output name, so + downstream derivations will have the proper dependency, and in + addition, before building, the placeholder will be rewritten to be + the actual path. + + The 'drv' and 'drvPath' outputs must correspond. */ static void mkOutputString(EvalState & state, Value & v, const StorePath & drvPath, const BasicDerivation & drv, std::pair o)