From e913a2989fd7dfabfd93c89fd4295386eda4277f Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 7 Aug 2020 19:09:26 +0000 Subject: [PATCH 01/52] Squashed get CA derivations building --- src/libexpr/get-drvs.cc | 13 +- src/libexpr/get-drvs.hh | 2 +- src/libexpr/primops.cc | 59 ++- src/libstore/build.cc | 964 ++++++++++++++++++++++++----------- src/libstore/derivations.cc | 158 +++--- src/libstore/derivations.hh | 14 +- src/libstore/local-store.cc | 31 +- src/libstore/local-store.hh | 6 + src/libstore/misc.cc | 23 +- src/libstore/references.cc | 16 +- src/libstore/references.hh | 2 + src/libstore/remote-store.cc | 2 +- src/libstore/store-api.cc | 19 +- src/libstore/store-api.hh | 6 +- src/libutil/serialise.hh | 6 + src/nix-build/nix-build.cc | 56 +- src/nix-store/nix-store.cc | 26 +- src/nix/build.cc | 3 +- src/nix/command.cc | 4 +- src/nix/installables.cc | 13 +- src/nix/installables.hh | 4 +- src/nix/profile.cc | 10 +- src/nix/repl.cc | 6 +- src/nix/show-derivation.cc | 4 +- tests/content-addressed.nix | 19 + tests/content-addressed.sh | 16 + tests/local.mk | 3 +- tests/multiple-outputs.nix | 15 + tests/multiple-outputs.sh | 6 + 29 files changed, 1021 insertions(+), 485 deletions(-) create mode 100644 tests/content-addressed.nix create mode 100644 tests/content-addressed.sh diff --git a/src/libexpr/get-drvs.cc b/src/libexpr/get-drvs.cc index 5d6e39aa0..d41cac132 100644 --- a/src/libexpr/get-drvs.cc +++ b/src/libexpr/get-drvs.cc @@ -39,7 +39,9 @@ DrvInfo::DrvInfo(EvalState & state, ref store, const std::string & drvPat if (i == drv.outputs.end()) throw Error("derivation '%s' does not have output '%s'", store->printStorePath(drvPath), outputName); - outPath = store->printStorePath(i->second.path(*store, drv.name)); + auto optStorePath = i->second.pathOpt(*store, drv.name); + if (optStorePath) + outPath = store->printStorePath(*optStorePath); } @@ -77,12 +79,15 @@ string DrvInfo::queryDrvPath() const string DrvInfo::queryOutPath() const { - if (outPath == "" && attrs) { + if (!outPath && attrs) { Bindings::iterator i = attrs->find(state->sOutPath); PathSet context; - outPath = i != attrs->end() ? state->coerceToPath(*i->pos, *i->value, context) : ""; + if (i != attrs->end()) + outPath = state->coerceToPath(*i->pos, *i->value, context); } - return outPath; + if (!outPath) + throw UnimplementedError("CA derivations are not yet supported"); + return *outPath; } diff --git a/src/libexpr/get-drvs.hh b/src/libexpr/get-drvs.hh index d7860fc6a..29bb6a660 100644 --- a/src/libexpr/get-drvs.hh +++ b/src/libexpr/get-drvs.hh @@ -20,7 +20,7 @@ private: mutable string name; mutable string system; mutable string drvPath; - mutable string outPath; + mutable std::optional outPath; mutable string outputName; Outputs outputs; diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 65d36ca0e..11f44fb97 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -44,16 +44,6 @@ void EvalState::realiseContext(const PathSet & context) throw InvalidPathError(store->printStorePath(ctx)); if (!outputName.empty() && ctx.isDerivation()) { drvs.push_back(StorePathWithOutputs{ctx, {outputName}}); - - /* Add the output of this derivation to the allowed - paths. */ - if (allowedPaths) { - auto drv = store->derivationFromPath(ctx); - DerivationOutputs::iterator i = drv.outputs.find(outputName); - if (i == drv.outputs.end()) - throw Error("derivation '%s' does not have an output named '%s'", ctxS, outputName); - allowedPaths->insert(store->printStorePath(i->second.path(*store, drv.name))); - } } } @@ -69,8 +59,38 @@ void EvalState::realiseContext(const PathSet & context) store->queryMissing(drvs, willBuild, willSubstitute, unknown, downloadSize, narSize); store->buildPaths(drvs); + + /* Add the output of this derivations to the allowed + paths. */ + if (allowedPaths) { + for (auto & [drvPath, outputs] : drvs) { + auto outputPaths = store->queryDerivationOutputMapAssumeTotal(drvPath); + for (auto & outputName : outputs) { + if (outputPaths.count(outputName) == 0) + throw Error("derivation '%s' does not have an output named '%s'", + store->printStorePath(drvPath), outputName); + allowedPaths->insert(store->printStorePath(outputPaths.at(outputName))); + } + } + } } +static void mkOutputString(EvalState & state, Value & v, + const StorePath & drvPath, const BasicDerivation & drv, + std::pair o) +{ + auto optOutputPath = o.second.pathOpt(*state.store, drv.name); + mkString( + *state.allocAttr(v, state.symbols.create(o.first)), + state.store->printStorePath(optOutputPath + ? *optOutputPath + /* Downstream we would substitute this for an actual path once + we build the floating CA derivation */ + /* FIXME: we need to depend on the basic derivation, not + derivation */ + : downstreamPlaceholder(*state.store, drvPath, o.first)), + {"!" + o.first + "!" + state.store->printStorePath(drvPath)}); +} /* Load and evaluate an expression from path specified by the argument. */ @@ -114,8 +134,7 @@ static void prim_scopedImport(EvalState & state, const Pos & pos, Value * * args unsigned int outputs_index = 0; for (const auto & o : drv.outputs) { - v2 = state.allocAttr(w, state.symbols.create(o.first)); - mkString(*v2, state.store->printStorePath(o.second.path(*state.store, drv.name)), {"!" + o.first + "!" + path}); + mkOutputString(state, w, storePath, drv, o); outputsVal->listElems()[outputs_index] = state.allocValue(); mkString(*(outputsVal->listElems()[outputs_index++]), o.first); } @@ -846,16 +865,18 @@ static void prim_derivationStrict(EvalState & state, const Pos & pos, Value * * /* Optimisation, but required in read-only mode! because in that case we don't actually write store derivations, so we can't - read them later. */ - drvHashes.insert_or_assign(drvPath, - hashDerivationModulo(*state.store, Derivation(drv), false)); + read them later. + + However, we don't bother doing this for floating CA derivations because + their "hash modulo" is indeterminate until built. */ + if (drv.type() != DerivationType::CAFloating) + drvHashes.insert_or_assign(drvPath, + hashDerivationModulo(*state.store, Derivation(drv), false)); state.mkAttrs(v, 1 + drv.outputs.size()); mkString(*state.allocAttr(v, state.sDrvPath), drvPathS, {"=" + drvPathS}); - for (auto & i : drv.outputs) { - mkString(*state.allocAttr(v, state.symbols.create(i.first)), - state.store->printStorePath(i.second.path(*state.store, drv.name)), {"!" + i.first + "!" + drvPathS}); - } + for (auto & i : drv.outputs) + mkOutputString(state, v, drvPath, drv, i); v.attrs->sort(); } diff --git a/src/libstore/build.cc b/src/libstore/build.cc index b47aeff3b..7c2225805 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -16,6 +16,7 @@ #include "machines.hh" #include "daemon.hh" #include "worker-protocol.hh" +#include "topo-sort.hh" #include #include @@ -717,6 +718,24 @@ typedef enum {rpAccept, rpDecline, rpPostpone} HookReply; class SubstitutionGoal; +struct KnownInitialOutputStatus { + 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 + valid path is in fact not corrupt. Otherwise, the inner bool is always + true (assumed no corruption). */ + std::optional valid; + /* Valid in the store, and additionally non-corrupt if we are repairing */ + bool isValid() const { + return valid && *valid; + } +}; + +struct InitialOutputStatus { + bool wanted; + std::optional known; +}; + class DerivationGoal : public Goal { private: @@ -744,19 +763,14 @@ private: /* The remainder is state held during the build. */ - /* Locks on the output paths. */ + /* Locks on (fixed) output paths. */ PathLocks outputLocks; /* All input paths (that is, the union of FS closures of the immediate input paths). */ StorePathSet inputPaths; - /* Outputs that are already valid. If we're repairing, these are - the outputs that are valid *and* not corrupt. */ - StorePathSet validPaths; - - /* Outputs that are corrupt or not valid. */ - StorePathSet missingPaths; + std::map initialOutputs; /* User selected for running the builder. */ std::unique_ptr buildUser; @@ -839,6 +853,31 @@ private: typedef map RedirectedOutputs; RedirectedOutputs redirectedOutputs; + /* The outputs paths used during the build. + + - Input-addressed derivations or fixed content-addressed outputs are + sometimes built when some of their outputs already exist, and can not + be hidden via sandboxing. We use temporary locations instead and + rewrite after the build. Otherwise the regular predetermined paths are + put here. + + - Floating content-addressed derivations do not know their final build + output paths until the outputs are hashed, so random locations are + used, and then renamed. The randomness helps guard against hidden + self-references. + */ + OutputPathMap scratchOutputs; + + /* The final output paths of the build. + + - For input-addressed derivations, always the precomputed paths + + - For content-addressed derivations, calcuated from whatever the hash + ends up being. (Note that fixed outputs derivations that produce the + "wrong" output still install that data under its true content-address.) + */ + std::map finalOutputs; + BuildMode buildMode; /* If we're repairing without a chroot, there may be outputs that @@ -937,7 +976,8 @@ private: void getDerivation(); void loadDerivation(); void haveDerivation(); - void outputsSubstituted(); + void outputsSubstitutionTried(); + void gaveUpOnSubstitution(); void closureRepaired(); void inputsRealised(); void tryToBuild(); @@ -998,13 +1038,24 @@ private: void handleEOF(int fd) override; void flushLine(); + /* Wrappers around the corresponding Store methods that first consult the + derivation. This is currently needed because when there is no drv file + there also is no DB entry. */ + std::map> queryDerivationOutputMap(); + OutputPathMap queryDerivationOutputMapAssumeTotal(); + /* Return the set of (in)valid paths. */ - StorePathSet checkPathValidity(bool returnValid, bool checkHash); + void checkPathValidity(); /* Forcibly kill the child process, if any. */ void killChild(); - void addHashRewrite(const StorePath & path); + /* Map a path to another (reproducably) so we can avoid overwriting outputs + 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. */ + StorePath makeFallbackPath(std::string_view path); void repairClosure(); @@ -1047,7 +1098,7 @@ DerivationGoal::DerivationGoal(const StorePath & drvPath, const BasicDerivation { this->drv = std::make_unique(BasicDerivation(drv)); state = &DerivationGoal::haveDerivation; - name = fmt("building of %s", worker.store.showPaths(drv.outputPaths(worker.store))); + name = fmt("building of %s", StorePathWithOutputs { drvPath, drv.outputNames() }.to_string(worker.store)); trace("created"); mcExpectedBuilds = std::make_unique>(worker.expectedBuilds); @@ -1179,43 +1230,63 @@ void DerivationGoal::haveDerivation() { trace("have derivation"); + if (drv->type() == DerivationType::CAFloating) + settings.requireExperimentalFeature("ca-derivations"); + retrySubstitution = false; - for (auto & i : drv->outputs) - worker.store.addTempRoot(i.second.path(worker.store, drv->name)); + /* Temporarily root output paths that are known a priori building */ + for (auto & i : drv->outputs) { + auto optOutputPath = i.second.pathOpt(worker.store, drv->name); + if (optOutputPath) + worker.store.addTempRoot(*optOutputPath); + } /* Check what outputs paths are not already valid. */ - auto invalidOutputs = checkPathValidity(false, buildMode == bmRepair); + checkPathValidity(); + bool allValid = true; + for (auto & [_, status] : initialOutputs) { + if (!status.wanted) continue; + if (!status.known || !status.known->isValid()) { + allValid = false; + break; + } + } /* If they are all valid, then we're done. */ - if (invalidOutputs.size() == 0 && buildMode == bmNormal) { + if (allValid && buildMode == bmNormal) { done(BuildResult::AlreadyValid); return; } parsedDrv = std::make_unique(drvPath, *drv); - if (drv->type() == DerivationType::CAFloating) { - settings.requireExperimentalFeature("ca-derivations"); - throw UnimplementedError("ca-derivations isn't implemented yet"); - } - /* We are first going to try to create the invalid output paths through substitutes. If that doesn't work, we'll build them. */ if (settings.useSubstitutes && parsedDrv->substitutesAllowed()) - for (auto & i : invalidOutputs) - addWaitee(worker.makeSubstitutionGoal(i, buildMode == bmRepair ? Repair : NoRepair, getDerivationCA(*drv))); + 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"); + /* Nothing to wait for; tail call */ + return DerivationGoal::gaveUpOnSubstitution(); + } + addWaitee(worker.makeSubstitutionGoal( + status.known->path, + buildMode == bmRepair ? Repair : NoRepair, + getDerivationCA(*drv))); + } if (waitees.empty()) /* to prevent hang (no wake-up event) */ - outputsSubstituted(); + outputsSubstitutionTried(); else - state = &DerivationGoal::outputsSubstituted; + state = &DerivationGoal::outputsSubstitutionTried; } -void DerivationGoal::outputsSubstituted() +void DerivationGoal::outputsSubstitutionTried() { trace("all outputs substituted (maybe)"); @@ -1239,7 +1310,14 @@ void DerivationGoal::outputsSubstituted() return; } - auto nrInvalid = checkPathValidity(false, buildMode == bmRepair).size(); + checkPathValidity(); + size_t nrInvalid = 0; + for (auto & [_, status] : initialOutputs) { + if (!status.wanted) continue; + if (!status.known || !status.known->isValid()) + nrInvalid++; + } + if (buildMode == bmNormal && nrInvalid == 0) { done(BuildResult::Substituted); return; @@ -1252,6 +1330,12 @@ void DerivationGoal::outputsSubstituted() throw Error("some outputs of '%s' are not valid, so checking is not possible", worker.store.printStorePath(drvPath)); + /* Nothing to wait for; tail call */ + gaveUpOnSubstitution(); +} + +void DerivationGoal::gaveUpOnSubstitution() +{ /* Otherwise, at least one of the output paths could not be produced using a substitute. So we have to build instead. */ @@ -1287,15 +1371,16 @@ void DerivationGoal::repairClosure() that produced those outputs. */ /* Get the output closure. */ + auto outputs = queryDerivationOutputMapAssumeTotal(); StorePathSet outputClosure; - for (auto & i : drv->outputs) { + for (auto & i : outputs) { if (!wantOutput(i.first, wantedOutputs)) continue; - worker.store.computeFSClosure(i.second.path(worker.store, drv->name), outputClosure); + worker.store.computeFSClosure(i.second, outputClosure); } /* Filter out our own outputs (which we have already checked). */ - for (auto & i : drv->outputs) - outputClosure.erase(i.second.path(worker.store, drv->name)); + for (auto & i : outputs) + outputClosure.erase(i.second); /* Get all dependencies of this derivation so that we know which derivation is responsible for which path in the output @@ -1305,9 +1390,9 @@ void DerivationGoal::repairClosure() std::map outputsToDrv; for (auto & i : inputClosure) if (i.isDerivation()) { - Derivation drv = worker.store.derivationFromPath(i); - for (auto & j : drv.outputs) - outputsToDrv.insert_or_assign(j.second.path(worker.store, drv.name), i); + auto depOutputs = worker.store.queryDerivationOutputMapAssumeTotal(i); + for (auto & j : depOutputs) + outputsToDrv.insert_or_assign(j.second, i); } /* Check each path (slow!). */ @@ -1370,20 +1455,19 @@ void DerivationGoal::inputsRealised() /* First, the input derivations. */ if (useDerivation) - for (auto & i : dynamic_cast(drv.get())->inputDrvs) { + for (auto & [depDrvPath, wantedDepOutputs] : dynamic_cast(drv.get())->inputDrvs) { /* Add the relevant output closures of the input derivation `i' as input paths. Only add the closures of output paths that are specified as inputs. */ - assert(worker.store.isValidPath(i.first)); - Derivation inDrv = worker.store.derivationFromPath(i.first); - for (auto & j : i.second) { - auto k = inDrv.outputs.find(j); - if (k != inDrv.outputs.end()) - worker.store.computeFSClosure(k->second.path(worker.store, inDrv.name), inputPaths); + assert(worker.store.isValidPath(drvPath)); + auto outputs = worker.store.queryDerivationOutputMapAssumeTotal(depDrvPath); + for (auto & j : wantedDepOutputs) { + if (outputs.count(j) > 0) + worker.store.computeFSClosure(outputs.at(j), inputPaths); else throw Error( "derivation '%s' requires non-existent output '%s' from input derivation '%s'", - worker.store.printStorePath(drvPath), j, worker.store.printStorePath(i.first)); + worker.store.printStorePath(drvPath), j, worker.store.printStorePath(drvPath)); } } @@ -1426,14 +1510,20 @@ void DerivationGoal::tryToBuild() { trace("trying to build"); - /* Obtain locks on all output paths. The locks are automatically - released when we exit this function or Nix crashes. If we - can't acquire the lock, then continue; hopefully some other - goal can start a build, and if not, the main loop will sleep a - few seconds and then retry this goal. */ + /* Obtain locks on all output paths, if the paths are known a priori. + + The locks are automatically released when we exit this function or Nix + crashes. If we can't acquire the lock, then continue; hopefully some + other goal can start a build, and if not, the main loop will sleep a few + seconds and then retry this goal. */ PathSet lockFiles; - for (auto & outPath : drv->outputPaths(worker.store)) - lockFiles.insert(worker.store.Store::toRealPath(outPath)); + /* FIXME: Should lock something like the drv itself so we don't build same + CA drv concurrently */ + for (auto & i : drv->outputs) { + auto optPath = i.second.pathOpt(worker.store, drv->name); + if (optPath) + lockFiles.insert(worker.store.Store::toRealPath(*optPath)); + } if (!outputLocks.lockPaths(lockFiles, "", false)) { if (!actLock) @@ -1452,24 +1542,28 @@ void DerivationGoal::tryToBuild() omitted, but that would be less efficient.) Note that since we now hold the locks on the output paths, no other process can build this derivation, so no further checks are necessary. */ - validPaths = checkPathValidity(true, buildMode == bmRepair); - if (buildMode != bmCheck && validPaths.size() == drv->outputs.size()) { + bool allValid = true; + for (auto & [_, status] : initialOutputs) { + if (!status.wanted) continue; + if (!status.known || !status.known->isValid()) { + allValid = false; + break; + } + } + if (buildMode != bmCheck && allValid) { debug("skipping build of derivation '%s', someone beat us to it", worker.store.printStorePath(drvPath)); outputLocks.setDeletion(true); done(BuildResult::AlreadyValid); return; } - missingPaths = drv->outputPaths(worker.store); - if (buildMode != bmCheck) - for (auto & i : validPaths) missingPaths.erase(i); - /* If any of the outputs already exist but are not valid, delete them. */ - for (auto & i : drv->outputs) { - if (worker.store.isValidPath(i.second.path(worker.store, drv->name))) continue; - debug("removing invalid path '%s'", worker.store.printStorePath(i.second.path(worker.store, drv->name))); - deletePath(worker.store.Store::toRealPath(i.second.path(worker.store, drv->name))); + for (auto & [_, status] : initialOutputs) { + if (!status.known || status.known->isValid()) continue; + auto storePath = status.known->path; + debug("removing invalid path '%s'", worker.store.printStorePath(status.known->path)); + deletePath(worker.store.Store::toRealPath(storePath)); } /* Don't do a remote build if the derivation has the attribute @@ -1477,7 +1571,6 @@ void DerivationGoal::tryToBuild() supported for local builds. */ bool buildLocally = buildMode != bmNormal || parsedDrv->willBuildLocally(); - /* Is the build hook willing to accept this job? */ if (!buildLocally) { switch (tryBuildHook()) { case rpAccept: @@ -1661,8 +1754,10 @@ void DerivationGoal::buildDone() /* Move paths out of the chroot for easier debugging of build failures. */ if (useChroot && buildMode == bmNormal) - for (auto & i : missingPaths) { - auto p = worker.store.printStorePath(i); + for (auto & [_, status] : initialOutputs) { + if (!status.known) continue; + if (buildMode != bmCheck && status.known->isValid()) continue; + auto p = worker.store.printStorePath(status.known->path); if (pathExists(chrootRootDir + p)) rename((chrootRootDir + p).c_str(), p.c_str()); } @@ -1692,7 +1787,10 @@ void DerivationGoal::buildDone() fmt("running post-build-hook '%s'", settings.postBuildHook), Logger::Fields{worker.store.printStorePath(drvPath)}); PushActivity pact(act.id); - auto outputPaths = drv->outputPaths(worker.store); + StorePathSet outputPaths; + for (auto i : drv->outputs) { + outputPaths.insert(finalOutputs.at(i.first)); + } std::map hookEnvironment = getEnv(); hookEnvironment.emplace("DRV_PATH", worker.store.printStorePath(drvPath)); @@ -1868,7 +1966,15 @@ HookReply DerivationGoal::tryBuildHook() /* Tell the hooks the missing outputs that have to be copied back from the remote system. */ - writeStorePaths(worker.store, hook->sink, missingPaths); + { + StorePathSet missingPaths; + for (auto & [_, status] : initialOutputs) { + if (!status.known) continue; + if (buildMode != bmCheck && status.known->isValid()) continue; + missingPaths.insert(status.known->path); + } + writeStorePaths(worker.store, hook->sink, missingPaths); + } hook->sink = FdSink(); hook->toHook.writeSide = -1; @@ -1919,8 +2025,16 @@ StorePathSet DerivationGoal::exportReferences(const StorePathSet & storePaths) for (auto & j : paths2) { if (j.isDerivation()) { Derivation drv = worker.store.derivationFromPath(j); - for (auto & k : drv.outputs) - worker.store.computeFSClosure(k.second.path(worker.store, drv.name), paths); + for (auto & k : drv.outputs) { + auto optPath = k.second.pathOpt(worker.store, drv.name); + if (!optPath) + /* FIXME: I am confused why we are calling + `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"); + worker.store.computeFSClosure(*optPath, paths); + } } } @@ -2013,9 +2127,66 @@ void DerivationGoal::startBuilder() chownToBuilder(tmpDir); - /* Substitute output placeholders with the actual output paths. */ - for (auto & output : drv->outputs) - inputRewrites[hashPlaceholder(output.first)] = worker.store.printStorePath(output.second.path(worker.store, drv->name)); + for (auto & [outputName, status] : initialOutputs) { + /* Set scratch path we'll actually use during the build. + + If we're not doing a chroot build, but we have some valid + output paths. Since we can't just overwrite or delete + them, we have to do hash rewriting: i.e. in the + environment/arguments passed to the build, we replace the + hashes of the valid outputs with unique dummy strings; + after the build, we discard the redirected outputs + corresponding to the valid outputs, and rewrite the + contents of the new outputs to replace the dummy strings + 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 */ + ? status.known->path + : !status.known->valid + /* If path doesn't yet exist can just use it */ + ? status.known->path + : buildMode != bmRepair && !*status.known->valid + /* If we aren't repairing we'll delete a corrupted path, so we + can use original path */ + ? status.known->path + : /* If we are repairing or the path is totally valid, we'll need + to use a temporary path */ + makeFallbackPath(status.known->path); + scratchOutputs.insert_or_assign(outputName, scratchPath); + + /* A non-removed corrupted path needs to be stored here, too */ + if (buildMode == bmRepair && !*status.known->valid) + redirectedBadOutputs.insert(status.known->path); + + /* Substitute output placeholders with the scratch output paths. + We'll use during the build. */ + inputRewrites[hashPlaceholder(outputName)] = worker.store.printStorePath(scratchPath); + + /* Additional tasks if we know the final path a priori. */ + if (!status.known) continue; + auto fixedFinalPath = status.known->path; + + /* Additional tasks if the final and scratch are both known and + differ. */ + if (fixedFinalPath == scratchPath) continue; + + /* Ensure scratch scratch path is ours to use */ + deletePath(worker.store.printStorePath(scratchPath)); + + /* Rewrite and unrewrite paths */ + { + std::string h1 { fixedFinalPath.hashPart() }; + std::string h2 { scratchPath.hashPart() }; + inputRewrites[h1] = h2; + } + + redirectedOutputs.insert_or_assign(std::move(fixedFinalPath), std::move(scratchPath)); + } /* Construct the environment passed to the builder. */ initEnv(); @@ -2199,8 +2370,14 @@ void DerivationGoal::startBuilder() rebuilding a path that is in settings.dirsInChroot (typically the dependencies of /bin/sh). Throw them out. */ - for (auto & i : drv->outputs) - dirsInChroot.erase(worker.store.printStorePath(i.second.path(worker.store, drv->name))); + for (auto & i : drv->outputs) { + /* If the name isn't known a prior (i.e. floating content-addressed + derivation), the temporary location we use should be fresh and + never in the sandbox in the first place. */ + auto optPath = i.second.pathOpt(worker.store, drv->name); + if (optPath) + dirsInChroot.erase(worker.store.printStorePath(*optPath)); + } #elif __APPLE__ /* We don't really have any parent prep work to do (yet?) @@ -2210,33 +2387,8 @@ void DerivationGoal::startBuilder() #endif } - if (needsHashRewrite()) { - - if (pathExists(homeDir)) - throw Error("home directory '%1%' exists; please remove it to assure purity of builds without sandboxing", homeDir); - - /* We're not doing a chroot build, but we have some valid - output paths. Since we can't just overwrite or delete - them, we have to do hash rewriting: i.e. in the - environment/arguments passed to the build, we replace the - hashes of the valid outputs with unique dummy strings; - after the build, we discard the redirected outputs - corresponding to the valid outputs, and rewrite the - contents of the new outputs to replace the dummy strings - with the actual hashes. */ - if (validPaths.size() > 0) - for (auto & i : validPaths) - addHashRewrite(i); - - /* If we're repairing, then we don't want to delete the - corrupt outputs in advance. So rewrite them as well. */ - if (buildMode == bmRepair) - for (auto & i : missingPaths) - if (worker.store.isValidPath(i) && pathExists(worker.store.printStorePath(i))) { - addHashRewrite(i); - redirectedBadOutputs.insert(i); - } - } + if (needsHashRewrite() && pathExists(homeDir)) + throw Error("home directory '%1%' exists; please remove it to assure purity of builds without sandboxing", homeDir); if (useChroot && settings.preBuildHook != "" && dynamic_cast(drv.get())) { printMsg(lvlChatty, format("executing pre-build hook '%1%'") @@ -2612,8 +2764,11 @@ void DerivationGoal::writeStructuredAttrs() /* Add an "outputs" object containing the output paths. */ nlohmann::json outputs; - for (auto & i : drv->outputs) - outputs[i.first] = rewriteStrings(worker.store.printStorePath(i.second.path(worker.store, drv->name)), inputRewrites); + for (auto & i : drv->outputs) { + /* The placeholder must have a rewrite, so we use it to cover both the + cases where we know or don't know the output path ahead of time. */ + outputs[i.first] = rewriteStrings(hashPlaceholder(i.first), inputRewrites); + } json["outputs"] = outputs; /* Handle exportReferencesGraph. */ @@ -2815,19 +2970,20 @@ struct RestrictedStore : public LocalFSStore StorePathSet newPaths; for (auto & path : paths) { - if (path.path.isDerivation()) { - if (!goal.isAllowed(path.path)) - throw InvalidPath("cannot build unknown path '%s' in recursive Nix", printStorePath(path.path)); - auto drv = derivationFromPath(path.path); - for (auto & output : drv.outputs) - if (wantOutput(output.first, path.outputs)) - newPaths.insert(output.second.path(*this, drv.name)); - } else if (!goal.isAllowed(path.path)) + if (!goal.isAllowed(path.path)) throw InvalidPath("cannot build unknown path '%s' in recursive Nix", printStorePath(path.path)); } next->buildPaths(paths, buildMode); + for (auto & path : paths) { + if (!path.path.isDerivation()) continue; + auto outputs = next->queryDerivationOutputMapAssumeTotal(path.path); + for (auto & output : outputs) + if (wantOutput(output.first, path.outputs)) + newPaths.insert(output.second); + } + StorePathSet closure; next->computeFSClosure(newPaths, closure); for (auto & path : closure) @@ -3443,14 +3599,10 @@ void DerivationGoal::runChild() if (derivationIsImpure(derivationType)) sandboxProfile += "(import \"sandbox-network.sb\")\n"; - /* Our rwx outputs */ + /* Add the output paths we'll use at build-time to the chroot */ sandboxProfile += "(allow file-read* file-write* process-exec\n"; - for (auto & i : missingPaths) - sandboxProfile += fmt("\t(subpath \"%s\")\n", worker.store.printStorePath(i)); - - /* Also add redirected outputs to the chroot */ - for (auto & i : redirectedOutputs) - sandboxProfile += fmt("\t(subpath \"%s\")\n", worker.store.printStorePath(i.second)); + for (auto & [_, path] : scratchOutputs) + sandboxProfile += fmt("\t(subpath \"%s\")\n", worker.store.printStorePath(path)); sandboxProfile += ")\n"; @@ -3573,23 +3725,6 @@ void DerivationGoal::runChild() } -/* Parse a list of reference specifiers. Each element must either be - a store path, or the symbolic name of the output of the derivation - (such as `out'). */ -StorePathSet parseReferenceSpecifiers(Store & store, const BasicDerivation & drv, const Strings & paths) -{ - StorePathSet result; - for (auto & i : paths) { - if (store.isStorePath(i)) - result.insert(store.parseStorePath(i)); - else if (drv.outputs.count(i)) - result.insert(drv.outputs.find(i)->second.path(store, drv.name)); - else throw BuildError("derivation contains an illegal reference specifier '%s'", i); - } - return result; -} - - static void moveCheckToStore(const Path & src, const Path & dst) { /* For the rename of directory to succeed, we must be running as root or @@ -3617,11 +3752,18 @@ void DerivationGoal::registerOutputs() { /* When using a build hook, the build hook can register the output as valid (by doing `nix-store --import'). If so we don't have - to do anything here. */ + to do anything here. + + We can only early return when the outputs are known a priori. For + floating content-addressed derivations this isn't the case. + */ if (hook) { bool allValid = true; - for (auto & i : drv->outputs) - if (!worker.store.isValidPath(i.second.path(worker.store, drv->name))) allValid = false; + for (auto & i : drv->outputs) { + auto optStorePath = i.second.pathOpt(worker.store, drv->name); + if (!optStorePath || !worker.store.isValidPath(*optStorePath)) + allValid = false; + } if (allValid) return; } @@ -3642,47 +3784,47 @@ void DerivationGoal::registerOutputs() Nix calls. */ StorePathSet referenceablePaths; for (auto & p : inputPaths) referenceablePaths.insert(p); - for (auto & i : drv->outputs) referenceablePaths.insert(i.second.path(worker.store, drv->name)); + for (auto & i : scratchOutputs) referenceablePaths.insert(i.second); for (auto & p : addedPaths) referenceablePaths.insert(p); - /* Check whether the output paths were created, and grep each - output path to determine what other paths it references. Also make all - output paths read-only. */ - for (auto & i : drv->outputs) { - auto path = worker.store.printStorePath(i.second.path(worker.store, drv->name)); - if (!missingPaths.count(i.second.path(worker.store, drv->name))) continue; + /* FIXME `needsHashRewrite` should probably be removed and we get to the + real reason why we aren't using the chroot dir */ + auto toRealPathChroot = [&](const Path & p) -> Path { + return useChroot && !needsHashRewrite() + ? chrootRootDir + p + : worker.store.toRealPath(p); + }; - Path actualPath = path; - if (needsHashRewrite()) { - auto r = redirectedOutputs.find(i.second.path(worker.store, drv->name)); - if (r != redirectedOutputs.end()) { - auto redirected = worker.store.Store::toRealPath(r->second); - if (buildMode == bmRepair - && redirectedBadOutputs.count(i.second.path(worker.store, drv->name)) - && pathExists(redirected)) - replaceValidPath(path, redirected); - if (buildMode == bmCheck) - actualPath = redirected; - } - } else if (useChroot) { - actualPath = chrootRootDir + path; - if (pathExists(actualPath)) { - /* Move output paths from the chroot to the Nix store. */ - if (buildMode == bmRepair) - replaceValidPath(path, actualPath); - else - if (buildMode != bmCheck && rename(actualPath.c_str(), worker.store.toRealPath(path).c_str()) == -1) - throw SysError("moving build output '%1%' from the sandbox to the Nix store", path); - } - if (buildMode != bmCheck) actualPath = worker.store.toRealPath(path); + /* Check whether the output paths were created, and make all + output paths read-only. Then get the references of each output (that we + might need to register), so we can topologically sort them. For the ones + that are most definitely already installed, we just store their final + name so we can also use it in rewrites. */ + StringSet outputsToSort; + std::map> outputReferences; + std::map outputStats; + for (auto & [outputName, _] : drv->outputs) { + auto actualPath = toRealPathChroot(worker.store.printStorePath(scratchOutputs.at(outputName))); + + outputsToSort.insert(outputName); + + /* Updated wanted info to remove the outputs we definitely don't need to register */ + auto & initialInfo = initialOutputs.at(outputName); + + /* Don't register if already valid, and not checking */ + initialInfo.wanted = buildMode == bmCheck + || !(initialInfo.known && initialInfo.known->isValid()); + if (!initialInfo.wanted) { + outputReferences.insert_or_assign(outputName, initialInfo.known->path); + continue; } struct stat st; if (lstat(actualPath.c_str(), &st) == -1) { if (errno == ENOENT) throw BuildError( - "builder for '%s' failed to produce output path '%s'", - worker.store.printStorePath(drvPath), path); + "builder for '%s' failed to produce output path for output '%s' at '%s'", + worker.store.printStorePath(drvPath), outputName, actualPath); throw SysError("getting attributes of path '%s'", actualPath); } @@ -3693,116 +3835,280 @@ void DerivationGoal::registerOutputs() user. */ if ((!S_ISLNK(st.st_mode) && (st.st_mode & (S_IWGRP | S_IWOTH))) || (buildUser && st.st_uid != buildUser->getUID())) - throw BuildError("suspicious ownership or permission on '%1%'; rejecting this build output", path); + throw BuildError( + "suspicious ownership or permission on '%s' for output '%s'; rejecting this build output", + actualPath, outputName); #endif - /* Apply hash rewriting if necessary. */ + /* Canonicalise first. This ensures that the path we're + rewriting doesn't contain a hard link to /etc/shadow or + something like that. */ + canonicalisePathMetaData(actualPath, buildUser ? buildUser->getUID() : -1, inodesSeen); + + debug("scanning for references for output %1 in temp location '%1%'", outputName, actualPath); + + /* Pass blank Sink as we are not ready to hash data at this stage. */ + NullSink blank; + auto references = worker.store.parseStorePathSet( + scanForReferences(blank, actualPath, worker.store.printStorePathSet(referenceablePaths))); + + outputReferences.insert_or_assign(outputName, references); + outputStats.insert_or_assign(outputName, std::move(st)); + } + + auto sortedOutputNames = topoSort(outputsToSort, + {[&](const std::string & name) { + auto x = outputReferences.at(name); + return std::visit(overloaded { + /* Since we'll use the already installed versions of these, we + can treat them as leaves and ignore any references they + have. */ + [&](StorePath _) { return StringSet {}; }, + [&](StorePathSet refs) { + StringSet referencedOutputs; + /* FIXME build inverted map up front so no quadratic waste here */ + for (auto & r : refs) + for (auto & [o, p] : scratchOutputs) + if (r == p) + referencedOutputs.insert(o); + return referencedOutputs; + }, + }, x); + }}, + {[&](const std::string & path, const std::string & parent) { + // TODO with more -vvvv also show the temporary paths for manual inspection. + return BuildError( + "cycle detected in build of '%s' in the references of output '%s' from output '%s'", + worker.store.printStorePath(drvPath), path, parent); + }}); + + std::reverse(sortedOutputNames.begin(), sortedOutputNames.end()); + + for (auto & outputName : sortedOutputNames) { + auto output = drv->outputs.at(outputName); + auto & scratchPath = scratchOutputs.at(outputName); + auto actualPath = toRealPathChroot(worker.store.printStorePath(scratchOutputs.at(outputName))); + + auto finish = [&](StorePath finalStorePath) { + /* Store the final path */ + finalOutputs.insert_or_assign(outputName, finalStorePath); + /* The rewrite rule will be used in downstream outputs that refer to + use. This is why the topological sort is essential to do first + before this for loop. */ + if (scratchPath != finalStorePath) + outputRewrites[std::string { scratchPath.hashPart() }] = std::string { finalStorePath.hashPart() }; + }; + bool rewritten = false; - if (!outputRewrites.empty()) { - logWarning({ - .name = "Rewriting hashes", - .hint = hintfmt("rewriting hashes in '%1%'; cross fingers", path) - }); + std::optional referencesOpt = std::visit(overloaded { + [&](StorePath skippedFinalPath) -> std::optional { + finish(skippedFinalPath); + return std::nullopt; + }, + [&](StorePathSet references) -> std::optional { + return references; + }, + }, outputReferences.at(outputName)); - /* Canonicalise first. This ensures that the path we're - rewriting doesn't contain a hard link to /etc/shadow or - something like that. */ - canonicalisePathMetaData(actualPath, buildUser ? buildUser->getUID() : -1, inodesSeen); + if (!referencesOpt) + continue; + auto references = *referencesOpt; - /* FIXME: this is in-memory. */ - StringSink sink; - dumpPath(actualPath, sink); - deletePath(actualPath); - sink.s = make_ref(rewriteStrings(*sink.s, outputRewrites)); - StringSource source(*sink.s); - restorePath(actualPath, source); + auto rewriteOutput = [&]() { + /* Apply hash rewriting if necessary. */ + if (!outputRewrites.empty()) { + logWarning({ + .name = "Rewriting hashes", + .hint = hintfmt("rewriting hashes in '%1%'; cross fingers", actualPath), + }); - rewritten = true; - } + /* FIXME: this is in-memory. */ + StringSink sink; + dumpPath(actualPath, sink); + deletePath(actualPath); + sink.s = make_ref(rewriteStrings(*sink.s, outputRewrites)); + StringSource source(*sink.s); + restorePath(actualPath, source); - /* Check that fixed-output derivations produced the right - outputs (i.e., the content hash should match the specified - hash). */ - std::optional ca; + rewritten = true; + } + }; - if (! std::holds_alternative(i.second.output)) { - DerivationOutputCAFloating outputHash; - std::visit(overloaded { - [&](DerivationOutputInputAddressed doi) { - assert(false); // Enclosing `if` handles this case in other branch + auto rewriteRefs = [&]() -> std::pair { + /* In the CA case, we need the rewritten refs to calculate the + final path, therefore we look for a *non-rewritten + self-reference, and use a bool rather try to solve the + computationally intractable fixed point. */ + std::pair res { + false, + {}, + }; + for (auto & r : references) { + auto name = r.name(); + auto origHash = std::string { r.hashPart() }; + if (r == scratchPath) + res.first = true; + else if (outputRewrites.count(origHash) == 0) + res.second.insert(r); + else { + std::string newRef = outputRewrites.at(origHash); + newRef += '-'; + newRef += name; + res.second.insert(StorePath { newRef }); + } + } + return res; + }; + + ValidPathInfo newInfo = [&]() {if (auto outputP = std::get_if(&output.output)) { + /* input-addressed case */ + auto requiredFinalPath = outputP->path; + /* Preemtively add rewrite rule for final hash, as that is + what the NAR hash will use rather than normalized-self references */ + if (scratchPath != requiredFinalPath) + outputRewrites.insert_or_assign( + std::string { scratchPath.hashPart() }, + std::string { requiredFinalPath.hashPart() }); + rewriteOutput(); + auto narHashAndSize = hashPath(htSHA256, actualPath); + ValidPathInfo newInfo0 { requiredFinalPath }; + newInfo0.narHash = narHashAndSize.first; + newInfo0.narSize = narHashAndSize.second; + auto refs = rewriteRefs(); + newInfo0.references = refs.second; + if (refs.first) + newInfo0.references.insert(newInfo0.path); + return newInfo0; + } else { + /* content-addressed case */ + DerivationOutputCAFloating outputHash = std::visit(overloaded { + [&](DerivationOutputInputAddressed doi) -> DerivationOutputCAFloating { + // Enclosing `if` handles this case in other branch + throw Error("ought to unreachable, handled in other branch"); }, [&](DerivationOutputCAFixed dof) { - outputHash = DerivationOutputCAFloating { + return DerivationOutputCAFloating { .method = dof.hash.method, .hashType = dof.hash.hash.type, }; }, [&](DerivationOutputCAFloating dof) { - outputHash = dof; + return dof; }, - }, i.second.output); - + }, output.output); + auto & st = outputStats.at(outputName); if (outputHash.method == FileIngestionMethod::Flat) { /* 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)", - path); + actualPath); } - - /* Check the hash. In hash mode, move the path produced by - the derivation to its content-addressed location. */ - Hash h2 = outputHash.method == FileIngestionMethod::Recursive - ? hashPath(outputHash.hashType, actualPath).first - : hashFile(outputHash.hashType, actualPath); - - auto dest = worker.store.makeFixedOutputPath(outputHash.method, h2, i.second.path(worker.store, drv->name).name()); - - // true if either floating CA, or incorrect fixed hash. - bool needsMove = true; - - if (auto p = std::get_if(& i.second.output)) { - Hash & h = p->hash.hash; - if (h != h2) { - - /* Throw an error after registering the path as - valid. */ - worker.hashMismatch = true; - delayedException = std::make_exception_ptr( - BuildError("hash mismatch in fixed-output derivation '%s':\n wanted: %s\n got: %s", - worker.store.printStorePath(dest), - h.to_string(SRI, true), - h2.to_string(SRI, true))); - } else { - // matched the fixed hash, so no move needed. - needsMove = false; - } + 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; } - - if (needsMove) { - Path actualDest = worker.store.Store::toRealPath(dest); - - if (worker.store.isValidPath(dest)) - std::rethrow_exception(delayedException); - - if (actualPath != actualDest) { - PathLocks outputLocks({actualDest}); - deletePath(actualDest); - if (rename(actualPath.c_str(), actualDest.c_str()) == -1) - throw SysError("moving '%s' to '%s'", actualPath, worker.store.printStorePath(dest)); - } - - path = worker.store.printStorePath(dest); - actualPath = actualDest; - } - else - assert(worker.store.parseStorePath(path) == dest); - - ca = FixedOutputHash { - .method = outputHash.method, - .hash = h2, + auto got = caSink.finish().first; + auto refs = rewriteRefs(); + ValidPathInfo newInfo0 { + worker.store.makeFixedOutputPath( + outputHash.method, + got, + outputPathName(drv->name, outputName), + refs.second, + refs.first), }; + newInfo0.ca = FixedOutputHash { + .method = outputHash.method, + .hash = got, + }; + newInfo0.references = refs.second; + if (refs.first) + newInfo0.references.insert(newInfo0.path); + { + HashModuloSink narSink { htSHA256, oldHashPart }; + dumpPath(actualPath, narSink); + auto narHashAndSize = narSink.finish(); + newInfo0.narHash = narHashAndSize.first; + newInfo0.narSize = narHashAndSize.second; + } + + /* Check wanted hash if output is fixed */ + if (auto p = std::get_if(&output.output)) { + Hash & wanted = p->hash.hash; + if (wanted != got) { + /* Throw an error after registering the path as + valid. */ + worker.hashMismatch = true; + delayedException = std::make_exception_ptr( + BuildError("hash mismatch in fixed-output derivation '%s':\n wanted: %s\n got: %s", + worker.store.printStorePath(drvPath), + wanted.to_string(SRI, true), + got.to_string(SRI, true))); + } + } + + assert(newInfo0.ca); + return newInfo0; + }}(); + + /* Calculate where we'll move the output files. In the checking case we + will leave leave them where they are, for now, rather than move to + their usual "final destination" */ + auto finalDestPath = worker.store.printStorePath(newInfo.path); + + /* Lock final output path, if not already locked. This happens with + floating CA derivations and hash-mismatching fixed-output + derivations. */ + PathLocks dynamicOutputLock; + auto optFixedPath = output.pathOpt(worker.store, drv->name); + if (!optFixedPath || + worker.store.printStorePath(*optFixedPath) != finalDestPath) + { + assert(newInfo.ca); + dynamicOutputLock.lockPaths({worker.store.toRealPath(finalDestPath)}); + } + + /* Move files, if needed */ + if (worker.store.toRealPath(finalDestPath) != actualPath) { + if (buildMode == bmRepair) { + /* Path already exists, need to replace it */ + replaceValidPath(worker.store.toRealPath(finalDestPath), actualPath); + actualPath = worker.store.toRealPath(finalDestPath); + } else if (buildMode == bmCheck) { + /* Path already exists, and we want to compare, so we leave out + new path in place. */ + } else if (finalDestPath == finalDestPath && worker.store.isValidPath(newInfo.path)) { + /* Path already exists because CA path produced by something + else. No moving needed. */ + assert(newInfo.ca); + } else { + /* Temporarily add write perm so we can move, will be fixed + later. */ + { + struct stat st; + auto & mode = st.st_mode; + 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); + } + if (rename( + actualPath.c_str(), + worker.store.toRealPath(finalDestPath).c_str()) == -1) + throw SysError("moving build output '%1%' from it's temporary location to the Nix store", finalDestPath); + actualPath = worker.store.toRealPath(finalDestPath); + } } /* Get rid of all weird permissions. This also checks that @@ -3810,45 +4116,33 @@ void DerivationGoal::registerOutputs() canonicalisePathMetaData(actualPath, buildUser && !rewritten ? buildUser->getUID() : -1, inodesSeen); - /* For this output path, find the references to other paths - contained in it. Compute the SHA-256 NAR hash at the same - time. The hash is stored in the database so that we can - verify later on whether nobody has messed with the store. */ - debug("scanning for references inside '%1%'", path); - // HashResult hash; - auto pathSetAndHash = scanForReferences(actualPath, worker.store.printStorePathSet(referenceablePaths)); - auto references = worker.store.parseStorePathSet(pathSetAndHash.first); - HashResult hash = pathSetAndHash.second; - if (buildMode == bmCheck) { - if (!worker.store.isValidPath(worker.store.parseStorePath(path))) continue; - ValidPathInfo info(*worker.store.queryPathInfo(worker.store.parseStorePath(path))); - if (hash.first != info.narHash) { + if (!worker.store.isValidPath(newInfo.path)) continue; + ValidPathInfo oldInfo(*worker.store.queryPathInfo(newInfo.path)); + if (newInfo.narHash != oldInfo.narHash) { worker.checkMismatch = true; if (settings.runDiffHook || settings.keepFailed) { - Path dst = worker.store.toRealPath(path + checkSuffix); + Path dst = worker.store.toRealPath(finalDestPath + checkSuffix); deletePath(dst); moveCheckToStore(actualPath, dst); handleDiffHook( buildUser ? buildUser->getUID() : getuid(), buildUser ? buildUser->getGID() : getgid(), - path, dst, worker.store.printStorePath(drvPath), tmpDir); + finalDestPath, dst, worker.store.printStorePath(drvPath), tmpDir); throw NotDeterministic("derivation '%s' may not be deterministic: output '%s' differs from '%s'", - worker.store.printStorePath(drvPath), worker.store.toRealPath(path), dst); + worker.store.printStorePath(drvPath), worker.store.toRealPath(finalDestPath), dst); } else throw NotDeterministic("derivation '%s' may not be deterministic: output '%s' differs", - worker.store.printStorePath(drvPath), worker.store.toRealPath(path)); + worker.store.printStorePath(drvPath), worker.store.toRealPath(finalDestPath)); } /* Since we verified the build, it's now ultimately trusted. */ - if (!info.ultimate) { - info.ultimate = true; - worker.store.signPathInfo(info); - ValidPathInfos infos; - infos.push_back(std::move(info)); - worker.store.registerValidPaths(infos); + if (!oldInfo.ultimate) { + oldInfo.ultimate = true; + worker.store.signPathInfo(oldInfo); + worker.store.registerValidPaths({ std::move(oldInfo) }); } continue; @@ -3865,24 +4159,22 @@ void DerivationGoal::registerOutputs() if (curRound == nrRounds) { worker.store.optimisePath(actualPath); // FIXME: combine with scanForReferences() - worker.markContentsGood(worker.store.parseStorePath(path)); + worker.markContentsGood(newInfo.path); } - ValidPathInfo info(worker.store.parseStorePath(path)); - info.narHash = hash.first; - info.narSize = hash.second; - info.references = std::move(references); - info.deriver = drvPath; - info.ultimate = true; - info.ca = ca; - worker.store.signPathInfo(info); + newInfo.deriver = drvPath; + newInfo.ultimate = true; + worker.store.signPathInfo(newInfo); - if (!info.references.empty()) { - // FIXME don't we have an experimental feature for fixed output with references? - info.ca = {}; - } + finish(newInfo.path); - infos.emplace(i.first, std::move(info)); + /* If it's a CA path, register it right away. This is necessary if it + isn't statically known so that we can safely unlock the path before + the next iteration */ + if (newInfo.ca) + worker.store.registerValidPaths({newInfo}); + + infos.emplace(outputName, std::move(newInfo)); } if (buildMode == bmCheck) return; @@ -3925,8 +4217,8 @@ void DerivationGoal::registerOutputs() /* If this is the first round of several, then move the output out of the way. */ if (nrRounds > 1 && curRound == 1 && curRound < nrRounds && keepPreviousRound) { - for (auto & i : drv->outputs) { - auto path = worker.store.printStorePath(i.second.path(worker.store, drv->name)); + for (auto & [_, outputStorePath] : finalOutputs) { + auto path = worker.store.printStorePath(outputStorePath); Path prev = path + checkSuffix; deletePath(prev); Path dst = path + checkSuffix; @@ -3943,8 +4235,8 @@ void DerivationGoal::registerOutputs() /* Remove the .check directories if we're done. FIXME: keep them if the result was not determistic? */ if (curRound == nrRounds) { - for (auto & i : drv->outputs) { - Path prev = worker.store.printStorePath(i.second.path(worker.store, drv->name)) + checkSuffix; + for (auto & [_, outputStorePath] : finalOutputs) { + Path prev = worker.store.printStorePath(outputStorePath) + checkSuffix; deletePath(prev); } } @@ -3954,7 +4246,13 @@ void DerivationGoal::registerOutputs() outputs, this will fail. */ { ValidPathInfos infos2; - for (auto & i : infos) infos2.push_back(i.second); + for (auto & [outputName, newInfo] : infos) { + /* FIXME: we will want to track this mapping in the DB whether or + not we have a drv file. */ + if (useDerivation) + worker.store.linkDeriverToPath(drvPath, outputName, newInfo.path); + infos2.push_back(newInfo); + } worker.store.registerValidPaths(infos2); } @@ -4030,7 +4328,17 @@ void DerivationGoal::checkOutputs(const std::map & outputs) { if (!value) return; - auto spec = parseReferenceSpecifiers(worker.store, *drv, *value); + /* Parse a list of reference specifiers. Each element must + either be a store path, or the symbolic name of the output + of the derivation (such as `out'). */ + StorePathSet spec; + for (auto & i : *value) { + if (worker.store.isStorePath(i)) + spec.insert(worker.store.parseStorePath(i)); + else if (finalOutputs.count(i)) + spec.insert(finalOutputs.at(i)); + else throw BuildError("derivation contains an illegal reference specifier '%s'", i); + } auto used = recursive ? getClosure(info.path).first @@ -4239,31 +4547,65 @@ void DerivationGoal::flushLine() } -StorePathSet DerivationGoal::checkPathValidity(bool returnValid, bool checkHash) +std::map> DerivationGoal::queryDerivationOutputMap() { - StorePathSet result; - for (auto & i : drv->outputs) { - if (!wantOutput(i.first, wantedOutputs)) continue; - bool good = - worker.store.isValidPath(i.second.path(worker.store, drv->name)) && - (!checkHash || worker.pathContentsGood(i.second.path(worker.store, drv->name))); - if (good == returnValid) result.insert(i.second.path(worker.store, drv->name)); + if (drv->type() != DerivationType::CAFloating) { + std::map> res; + for (auto & [name, output] : drv->outputs) + res.insert_or_assign(name, output.pathOpt(worker.store, drv->name)); + return res; + } else { + return worker.store.queryDerivationOutputMap(drvPath); + } +} + +OutputPathMap DerivationGoal::queryDerivationOutputMapAssumeTotal() +{ + if (drv->type() != DerivationType::CAFloating) { + OutputPathMap res; + for (auto & [name, output] : drv->outputs) + res.insert_or_assign(name, *output.pathOpt(worker.store, drv->name)); + return res; + } else { + return worker.store.queryDerivationOutputMapAssumeTotal(drvPath); } - return result; } -void DerivationGoal::addHashRewrite(const StorePath & path) +void DerivationGoal::checkPathValidity() { - auto h1 = std::string(((std::string_view) path.to_string()).substr(0, 32)); - auto p = worker.store.makeStorePath( + bool checkHash = buildMode == bmRepair; + for (auto & i : queryDerivationOutputMap()) { + InitialOutputStatus status { + .wanted = wantOutput(i.first, wantedOutputs), + }; + if (i.second) { + auto outputPath = *i.second; + status.known = { + .path = outputPath, + .valid = !worker.store.isValidPath(outputPath) + ? std::optional {} + : !checkHash || worker.pathContentsGood(outputPath), + }; + } + initialOutputs.insert_or_assign(i.first, status); + } +} + + +StorePath DerivationGoal::makeFallbackPath(std::string_view outputName) +{ + return worker.store.makeStorePath( + "rewrite:" + std::string(drvPath.to_string()) + ":name:" + std::string(outputName), + Hash(htSHA256), outputPathName(drv->name, outputName)); +} + + +StorePath DerivationGoal::makeFallbackPath(const StorePath & path) +{ + return worker.store.makeStorePath( "rewrite:" + std::string(drvPath.to_string()) + ":" + std::string(path.to_string()), Hash(htSHA256), path.name()); - auto h2 = std::string(((std::string_view) p.to_string()).substr(0, 32)); - deletePath(worker.store.printStorePath(p)); - inputRewrites[h1] = h2; - outputRewrites[h2] = h1; - redirectedOutputs.insert_or_assign(path, std::move(p)); } diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index 68b081058..58c67e40c 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -15,7 +15,8 @@ std::optional DerivationOutput::pathOpt(const Store & store, std::str }, [&](DerivationOutputCAFixed dof) -> std::optional { return { - store.makeFixedOutputPath(dof.hash.method, dof.hash.hash, drvName) + // FIXME if we intend to support multiple CA outputs. + dof.path(store, drvName, "out") }; }, [](DerivationOutputCAFloating dof) -> std::optional { @@ -25,6 +26,13 @@ std::optional DerivationOutput::pathOpt(const Store & store, std::str } +StorePath DerivationOutputCAFixed::path(const Store & store, std::string_view drvName, std::string_view outputName) const { + return store.makeFixedOutputPath( + hash.method, hash.hash, + outputPathName(drvName, outputName)); +} + + bool derivationIsCA(DerivationType dt) { switch (dt) { case DerivationType::InputAddressed: return false; @@ -106,12 +114,15 @@ static string parseString(std::istream & str) return res; } +static void validatePath(std::string_view s) { + if (s.size() == 0 || s[0] != '/') + throw FormatError("bad path '%1%' in derivation", s); +} static Path parsePath(std::istream & str) { - string s = parseString(str); - if (s.size() == 0 || s[0] != '/') - throw FormatError("bad path '%1%' in derivation", s); + auto s = parseString(str); + validatePath(s); return s; } @@ -141,7 +152,7 @@ static StringSet parseStrings(std::istream & str, bool arePaths) static DerivationOutput parseDerivationOutput(const Store & store, std::istringstream & str) { - expect(str, ","); auto path = store.parseStorePath(parsePath(str)); + expect(str, ","); auto pathS = parseString(str); expect(str, ","); auto hashAlgo = parseString(str); expect(str, ","); const auto hash = parseString(str); expect(str, ")"); @@ -152,30 +163,35 @@ static DerivationOutput parseDerivationOutput(const Store & store, std::istrings method = FileIngestionMethod::Recursive; hashAlgo = string(hashAlgo, 2); } - const HashType hashType = parseHashType(hashAlgo); - - return hash != "" - ? DerivationOutput { - .output = DerivationOutputCAFixed { - .hash = FixedOutputHash { - .method = std::move(method), - .hash = Hash::parseNonSRIUnprefixed(hash, hashType), - }, - } - } - : (settings.requireExperimentalFeature("ca-derivations"), - DerivationOutput { - .output = DerivationOutputCAFloating { - .method = std::move(method), - .hashType = std::move(hashType), - }, - }); - } else + const auto hashType = parseHashType(hashAlgo); + if (hash != "") { + validatePath(pathS); + return DerivationOutput { + .output = DerivationOutputCAFixed { + .hash = FixedOutputHash { + .method = std::move(method), + .hash = Hash::parseNonSRIUnprefixed(hash, hashType), + }, + }, + }; + } else { + settings.requireExperimentalFeature("ca-derivations"); + assert(pathS == ""); + return DerivationOutput { + .output = DerivationOutputCAFloating { + .method = std::move(method), + .hashType = std::move(hashType), + }, + }; + } + } else { + validatePath(pathS); return DerivationOutput { .output = DerivationOutputInputAddressed { - .path = std::move(path), + .path = store.parseStorePath(pathS), } }; + } } @@ -316,17 +332,19 @@ string Derivation::unparse(const Store & store, bool maskOutputs, for (auto & i : outputs) { if (first) first = false; else s += ','; s += '('; printUnquotedString(s, i.first); - s += ','; printUnquotedString(s, maskOutputs ? "" : store.printStorePath(i.second.path(store, name))); std::visit(overloaded { [&](DerivationOutputInputAddressed doi) { + s += ','; printUnquotedString(s, maskOutputs ? "" : store.printStorePath(doi.path)); s += ','; printUnquotedString(s, ""); s += ','; printUnquotedString(s, ""); }, [&](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)); }, [&](DerivationOutputCAFloating dof) { + s += ','; printUnquotedString(s, ""); s += ','; printUnquotedString(s, makeFileIngestionPrefix(dof.method) + printHashType(dof.hashType)); s += ','; printUnquotedString(s, ""); }, @@ -382,6 +400,16 @@ bool isDerivation(const string & fileName) } +std::string outputPathName(std::string_view drvName, std::string_view outputName) { + std::string res { drvName }; + if (outputName != "out") { + res += "-"; + res += outputName; + } + return res; +} + + DerivationType BasicDerivation::type() const { std::set inputAddressedOutputs, fixedCAOutputs, floatingCAOutputs; @@ -479,7 +507,7 @@ DrvHashModulo hashDerivationModulo(Store & store, const Derivation & drv, bool m auto hash = hashString(htSHA256, "fixed:out:" + dof.hash.printMethodAlgo() + ":" + dof.hash.hash.to_string(Base16, false) + ":" - + store.printStorePath(i.second.path(store, drv.name))); + + store.printStorePath(dof.path(store, drv.name, i.first))); outputHashes.insert_or_assign(i.first, std::move(hash)); } return outputHashes; @@ -530,17 +558,9 @@ bool wantOutput(const string & output, const std::set & wanted) } -StorePathSet BasicDerivation::outputPaths(const Store & store) const -{ - StorePathSet paths; - for (auto & i : outputs) - paths.insert(i.second.path(store, name)); - return paths; -} - static DerivationOutput readDerivationOutput(Source & in, const Store & store) { - auto path = store.parseStorePath(readString(in)); + auto pathS = readString(in); auto hashAlgo = readString(in); auto hash = readString(in); @@ -550,29 +570,35 @@ static DerivationOutput readDerivationOutput(Source & in, const Store & store) method = FileIngestionMethod::Recursive; hashAlgo = string(hashAlgo, 2); } - auto hashType = parseHashType(hashAlgo); - return hash != "" - ? DerivationOutput { - .output = DerivationOutputCAFixed { - .hash = FixedOutputHash { - .method = std::move(method), - .hash = Hash::parseNonSRIUnprefixed(hash, hashType), - }, - } - } - : (settings.requireExperimentalFeature("ca-derivations"), - DerivationOutput { - .output = DerivationOutputCAFloating { - .method = std::move(method), - .hashType = std::move(hashType), - }, - }); - } else + const auto hashType = parseHashType(hashAlgo); + if (hash != "") { + validatePath(pathS); + return DerivationOutput { + .output = DerivationOutputCAFixed { + .hash = FixedOutputHash { + .method = std::move(method), + .hash = Hash::parseNonSRIUnprefixed(hash, hashType), + }, + }, + }; + } else { + settings.requireExperimentalFeature("ca-derivations"); + assert(pathS == ""); + return DerivationOutput { + .output = DerivationOutputCAFloating { + .method = std::move(method), + .hashType = std::move(hashType), + }, + }; + } + } else { + validatePath(pathS); return DerivationOutput { .output = DerivationOutputInputAddressed { - .path = std::move(path), + .path = store.parseStorePath(pathS), } }; + } } StringSet BasicDerivation::outputNames() const @@ -624,18 +650,21 @@ void writeDerivation(Sink & out, const Store & store, const BasicDerivation & dr { out << drv.outputs.size(); for (auto & i : drv.outputs) { - out << i.first - << store.printStorePath(i.second.path(store, drv.name)); + out << i.first; std::visit(overloaded { [&](DerivationOutputInputAddressed doi) { - out << "" << ""; + out << store.printStorePath(doi.path) + << "" + << ""; }, [&](DerivationOutputCAFixed dof) { - out << dof.hash.printMethodAlgo() + out << store.printStorePath(dof.path(store, drv.name, i.first)) + << dof.hash.printMethodAlgo() << dof.hash.hash.to_string(Base16, false); }, [&](DerivationOutputCAFloating dof) { - out << (makeFileIngestionPrefix(dof.method) + printHashType(dof.hashType)) + out << "" + << (makeFileIngestionPrefix(dof.method) + printHashType(dof.hashType)) << ""; }, }, i.second.output); @@ -654,5 +683,14 @@ std::string hashPlaceholder(const std::string & outputName) return "/" + hashString(htSHA256, "nix-output:" + outputName).to_string(Base32, false); } +StorePath downstreamPlaceholder(const Store & store, const StorePath & drvPath, std::string_view outputName) +{ + auto drvNameWithExtension = drvPath.name(); + auto drvName = drvNameWithExtension.substr(0, drvNameWithExtension.size() - 4); + return store.makeStorePath( + "downstream-placeholder:" + std::string { drvPath.name() } + ":" + std::string { outputName }, + "compressed:" + std::string { drvPath.hashPart() }, + outputPathName(drvName, outputName)); +} } diff --git a/src/libstore/derivations.hh b/src/libstore/derivations.hh index 14e0e947a..963b44b8c 100644 --- a/src/libstore/derivations.hh +++ b/src/libstore/derivations.hh @@ -27,6 +27,7 @@ struct DerivationOutputInputAddressed struct DerivationOutputCAFixed { FixedOutputHash hash; /* hash used for expected hash computation */ + StorePath path(const Store & store, std::string_view drvName, std::string_view outputName) const; }; /* Floating-output derivations, whose output paths are content addressed, but @@ -48,12 +49,6 @@ struct DerivationOutput > output; std::optional hashAlgoOpt(const Store & store) const; std::optional pathOpt(const Store & store, std::string_view drvName) const; - /* DEPRECATED: Remove after CA drvs are fully implemented */ - StorePath path(const Store & store, std::string_view drvName) const { - auto p = pathOpt(store, drvName); - if (!p) throw UnimplementedError("floating content-addressed derivations are not yet implemented"); - return *p; - } }; typedef std::map DerivationOutputs; @@ -101,9 +96,6 @@ struct BasicDerivation /* Return true iff this is a fixed-output derivation. */ DerivationType type() const; - /* Return the output paths of a derivation. */ - StorePathSet outputPaths(const Store & store) const; - /* Return the output names of a derivation. */ StringSet outputNames() const; @@ -136,6 +128,8 @@ Derivation readDerivation(const Store & store, const Path & drvPath, std::string // FIXME: remove bool isDerivation(const string & fileName); +std::string outputPathName(std::string_view drvName, std::string_view outputName); + // known CA drv's output hashes, current just for fixed-output derivations // whose output hashes are always known since they are fixed up-front. typedef std::map CaOutputHashes; @@ -185,4 +179,6 @@ void writeDerivation(Sink & out, const Store & store, const BasicDerivation & dr std::string hashPlaceholder(const std::string & outputName); +StorePath downstreamPlaceholder(const Store & store, const StorePath & drvPath, std::string_view outputName); + } diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index e96091aae..027efc7c9 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -578,13 +578,32 @@ void LocalStore::checkDerivationOutputs(const StorePath & drvPath, const Derivat envHasRightPath(path, i.first); }, [&](DerivationOutputCAFloating _) { - throw UnimplementedError("floating CA output derivations are not yet implemented"); + /* Nothing to check */ }, }, i.second.output); } } +void LocalStore::linkDeriverToPath(const StorePath & deriver, const string & outputName, const StorePath & output) +{ + auto state(_state.lock()); + return linkDeriverToPath(*state, queryValidPathId(*state, deriver), outputName, output); +} + +void LocalStore::linkDeriverToPath(State & state, uint64_t deriver, const string & outputName, const StorePath & output) +{ + retrySQLite([&]() { + state.stmtAddDerivationOutput.use() + (deriver) + (outputName) + (printStorePath(output)) + .exec(); + }); + +} + + uint64_t LocalStore::addValidPath(State & state, const ValidPathInfo & info, bool checkOutputs) { @@ -619,11 +638,11 @@ uint64_t LocalStore::addValidPath(State & state, if (checkOutputs) checkDerivationOutputs(info.path, drv); for (auto & i : drv.outputs) { - state.stmtAddDerivationOutput.use() - (id) - (i.first) - (printStorePath(i.second.path(*this, drv.name))) - .exec(); + /* Floating CA derivations have indeterminate output paths until + they are built, so don't register anything in that case */ + auto optPath = i.second.pathOpt(*this, drv.name); + if (optPath) + linkDeriverToPath(state, id, i.first, *optPath); } } diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 627b1f557..bf4016b13 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -282,6 +282,12 @@ 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 + */ + void linkDeriverToPath(const StorePath & deriver, const string & outputName, const StorePath & output); + void linkDeriverToPath(State & state, uint64_t deriver, const string & outputName, const StorePath & output); + Path getRealStoreDir() override { return realStoreDir; } void createUser(const std::string & userName, uid_t userId) override; diff --git a/src/libstore/misc.cc b/src/libstore/misc.cc index 0ae1ceaad..9024fb2bd 100644 --- a/src/libstore/misc.cc +++ b/src/libstore/misc.cc @@ -203,17 +203,24 @@ void Store::queryMissing(const std::vector & targets, return; } + PathSet invalid; + /* true for regular derivations, and CA derivations for which we + have a trust mapping for all wanted outputs. */ + auto knownOutputPaths = true; + for (auto & [outputName, pathOpt] : queryDerivationOutputMap(path.path)) { + if (!pathOpt) { + knownOutputPaths = false; + break; + } + if (wantOutput(outputName, path.outputs) && !isValidPath(*pathOpt)) + invalid.insert(printStorePath(*pathOpt)); + } + if (knownOutputPaths && invalid.empty()) return; + auto drv = make_ref(derivationFromPath(path.path)); ParsedDerivation parsedDrv(StorePath(path.path), *drv); - PathSet invalid; - for (auto & j : drv->outputs) - if (wantOutput(j.first, path.outputs) - && !isValidPath(j.second.path(*this, drv->name))) - invalid.insert(printStorePath(j.second.path(*this, drv->name))); - if (invalid.empty()) return; - - if (settings.useSubstitutes && parsedDrv.substitutesAllowed()) { + if (knownOutputPaths && settings.useSubstitutes && parsedDrv.substitutesAllowed()) { auto drvState = make_ref>(DrvState(invalid.size())); for (auto & output : invalid) pool.enqueue(std::bind(checkOutput, printStorePath(path.path), drv, output, drvState)); diff --git a/src/libstore/references.cc b/src/libstore/references.cc index 62a3cda61..d2096cb49 100644 --- a/src/libstore/references.cc +++ b/src/libstore/references.cc @@ -79,9 +79,17 @@ void RefScanSink::operator () (const unsigned char * data, size_t len) std::pair scanForReferences(const string & path, const PathSet & refs) { - RefScanSink refsSink; HashSink hashSink { htSHA256 }; - TeeSink sink { refsSink, hashSink }; + auto found = scanForReferences(hashSink, path, refs); + auto hash = hashSink.finish(); + return std::pair(found, hash); +} + +PathSet scanForReferences(Sink & toTee, + const string & path, const PathSet & refs) +{ + RefScanSink refsSink; + TeeSink sink { refsSink, toTee }; std::map backMap; /* For efficiency (and a higher hit rate), just search for the @@ -111,9 +119,7 @@ std::pair scanForReferences(const string & path, found.insert(j->second); } - auto hash = hashSink.finish(); - - return std::pair(found, hash); + return found; } diff --git a/src/libstore/references.hh b/src/libstore/references.hh index 598a3203a..c2efd095c 100644 --- a/src/libstore/references.hh +++ b/src/libstore/references.hh @@ -7,6 +7,8 @@ namespace nix { std::pair scanForReferences(const Path & path, const PathSet & refs); +PathSet scanForReferences(Sink & toTee, const Path & path, const PathSet & refs); + struct RewritingSink : Sink { std::string from, to, prev; diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 005415666..e69d15c53 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -74,7 +74,7 @@ void write(const Store & store, Sink & out, const StorePath & storePath) template<> std::optional read(const Store & store, Source & from, Phantom> _) { - auto s = readString(from); + auto s = readString(from); return s == "" ? std::optional {} : store.parseStorePath(s); } diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index e66c04df4..63accd514 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -140,21 +140,28 @@ StorePathWithOutputs Store::followLinksToStorePathWithOutputs(std::string_view p */ -StorePath Store::makeStorePath(const string & type, - const Hash & hash, std::string_view name) const +StorePath Store::makeStorePath(std::string_view type, + std::string_view hash, std::string_view name) const { /* e.g., "source:sha256:1abc...:/nix/store:foo.tar.gz" */ - string s = type + ":" + hash.to_string(Base16, true) + ":" + storeDir + ":" + std::string(name); + string s = std::string { type } + ":" + std::string { hash } + + ":" + storeDir + ":" + std::string { name }; auto h = compressHash(hashString(htSHA256, s), 20); return StorePath(h, name); } -StorePath Store::makeOutputPath(const string & id, +StorePath Store::makeStorePath(std::string_view type, const Hash & hash, std::string_view name) const { - return makeStorePath("output:" + id, hash, - std::string(name) + (id == "out" ? "" : "-" + id)); + return makeStorePath(type, hash.to_string(Base16, true), name); +} + + +StorePath Store::makeOutputPath(std::string_view id, + const Hash & hash, std::string_view name) const +{ + return makeStorePath("output:" + std::string { id }, hash, outputPathName(name, id)); } diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 9003ab541..943f08211 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -244,10 +244,12 @@ public: StorePathWithOutputs followLinksToStorePathWithOutputs(std::string_view path) const; /* Constructs a unique store path name. */ - StorePath makeStorePath(const string & type, + StorePath makeStorePath(std::string_view type, + std::string_view hash, std::string_view name) const; + StorePath makeStorePath(std::string_view type, const Hash & hash, std::string_view name) const; - StorePath makeOutputPath(const string & id, + StorePath makeOutputPath(std::string_view id, const Hash & hash, std::string_view name) const; StorePath makeFixedOutputPath(FileIngestionMethod method, diff --git a/src/libutil/serialise.hh b/src/libutil/serialise.hh index c29c6b29b..3650857aa 100644 --- a/src/libutil/serialise.hh +++ b/src/libutil/serialise.hh @@ -22,6 +22,12 @@ struct Sink } }; +/* Just throws away data. */ +struct NullSink : Sink +{ + void operator () (const unsigned char * data, size_t len) override + { } +}; /* A buffered abstract sink. */ struct BufferedSink : virtual Sink diff --git a/src/nix-build/nix-build.cc b/src/nix-build/nix-build.cc index 94412042f..be73ea724 100755 --- a/src/nix-build/nix-build.cc +++ b/src/nix-build/nix-build.cc @@ -487,50 +487,56 @@ static void _main(int argc, char * * argv) std::vector pathsToBuild; - std::map drvPrefixes; - std::map resultSymlinks; - std::vector outPaths; + std::map> drvMap; for (auto & drvInfo : drvs) { - auto drvPath = drvInfo.queryDrvPath(); - auto outPath = drvInfo.queryOutPath(); + auto drvPath = store->parseStorePath(drvInfo.queryDrvPath()); auto outputName = drvInfo.queryOutputName(); if (outputName == "") - throw Error("derivation '%s' lacks an 'outputName' attribute", drvPath); + throw Error("derivation '%s' lacks an 'outputName' attribute", store->printStorePath(drvPath)); - pathsToBuild.push_back({store->parseStorePath(drvPath), {outputName}}); + pathsToBuild.push_back({drvPath, {outputName}}); - std::string drvPrefix; - auto i = drvPrefixes.find(drvPath); - if (i != drvPrefixes.end()) - drvPrefix = i->second; + auto i = drvMap.find(drvPath); + if (i != drvMap.end()) + i->second.second.insert(outputName); else { - drvPrefix = outLink; - if (drvPrefixes.size()) - drvPrefix += fmt("-%d", drvPrefixes.size() + 1); - drvPrefixes[drvPath] = drvPrefix; + drvMap[drvPath] = {drvMap.size(), {outputName}}; } - - std::string symlink = drvPrefix; - if (outputName != "out") symlink += "-" + outputName; - - resultSymlinks[symlink] = outPath; - outPaths.push_back(outPath); } buildPaths(pathsToBuild); if (dryRun) return; - for (auto & symlink : resultSymlinks) - if (auto store2 = store.dynamic_pointer_cast()) - store2->addPermRoot(store->parseStorePath(symlink.second), absPath(symlink.first), true); + std::vector outPaths; + + for (auto & [drvPath, info] : drvMap) { + auto & [counter, wantedOutputs] = info; + std::string drvPrefix = outLink; + if (counter) + drvPrefix += fmt("-%d", counter + 1); + + auto builtOutputs = store->queryDerivationOutputMapAssumeTotal(drvPath); + + for (auto & outputName : wantedOutputs) { + auto outputPath = builtOutputs.at(outputName); + + if (auto store2 = store.dynamic_pointer_cast()) { + std::string symlink = drvPrefix; + if (outputName != "out") symlink += "-" + outputName; + store2->addPermRoot(outputPath, absPath(symlink), true); + } + + outPaths.push_back(outputPath); + } + } logger->stop(); for (auto & path : outPaths) - std::cout << path << '\n'; + std::cout << store->printStorePath(path) << '\n'; } } diff --git a/src/nix-store/nix-store.cc b/src/nix-store/nix-store.cc index 2996e36c4..9eadf62e4 100644 --- a/src/nix-store/nix-store.cc +++ b/src/nix-store/nix-store.cc @@ -65,6 +65,7 @@ static PathSet realisePath(StorePathWithOutputs path, bool build = true) if (path.path.isDerivation()) { if (build) store->buildPaths({path}); + auto outputPaths = store->queryDerivationOutputMapAssumeTotal(path.path); Derivation drv = store->derivationFromPath(path.path); rootNr++; @@ -77,7 +78,8 @@ static PathSet realisePath(StorePathWithOutputs path, bool build = true) if (i == drv.outputs.end()) throw Error("derivation '%s' does not have an output named '%s'", store2->printStorePath(path.path), j); - auto outPath = store2->printStorePath(i->second.path(*store, drv.name)); + auto outPath = outputPaths.at(i->first); + auto retPath = store->printStorePath(outPath); if (store2) { if (gcRoot == "") printGCWarning(); @@ -85,10 +87,10 @@ static PathSet realisePath(StorePathWithOutputs path, bool build = true) Path rootName = gcRoot; if (rootNr > 1) rootName += "-" + std::to_string(rootNr); if (i->first != "out") rootName += "-" + i->first; - outPath = store2->addPermRoot(store->parseStorePath(outPath), rootName, indirectRoot); + retPath = store2->addPermRoot(outPath, rootName, indirectRoot); } } - outputs.insert(outPath); + outputs.insert(retPath); } return outputs; } @@ -218,8 +220,14 @@ static StorePathSet maybeUseOutputs(const StorePath & storePath, bool useOutput, if (useOutput && storePath.isDerivation()) { auto drv = store->derivationFromPath(storePath); StorePathSet outputs; - for (auto & i : drv.outputs) - outputs.insert(i.second.path(*store, drv.name)); + if (forceRealise) + return store->queryDerivationOutputs(storePath); + for (auto & i : drv.outputs) { + auto optPath = i.second.pathOpt(*store, drv.name); + if (!optPath) + throw UsageError("Cannot use output path of floating content-addressed derivation until we know what it is (e.g. by building it)"); + outputs.insert(*optPath); + } return outputs; } else return {storePath}; @@ -309,11 +317,9 @@ static void opQuery(Strings opFlags, Strings opArgs) case qOutputs: { for (auto & i : opArgs) { - auto i2 = store->followLinksToStorePath(i); - if (forceRealise) realisePath({i2}); - Derivation drv = store->derivationFromPath(i2); - for (auto & j : drv.outputs) - cout << fmt("%1%\n", store->printStorePath(j.second.path(*store, drv.name))); + auto outputs = maybeUseOutputs(store->followLinksToStorePath(i), true, forceRealise); + for (auto & outputPath : outputs) + cout << fmt("%1%\n", store->printStorePath(outputPath)); } break; } diff --git a/src/nix/build.cc b/src/nix/build.cc index 13d14a7fb..a66e8dac4 100644 --- a/src/nix/build.cc +++ b/src/nix/build.cc @@ -74,7 +74,8 @@ struct CmdBuild : InstallablesCommand, MixDryRun, MixProfile store2->addPermRoot(bo.path, absPath(symlink), true); }, [&](BuildableFromDrv bfd) { - for (auto & output : bfd.outputs) { + auto builtOutputs = store->queryDerivationOutputMapAssumeTotal(bfd.drvPath); + for (auto & output : builtOutputs) { std::string symlink = outLink; if (i) symlink += fmt("-%d", i); if (output.first != "out") symlink += fmt("-%s", output.first); diff --git a/src/nix/command.cc b/src/nix/command.cc index da32819da..4a93d8e73 100644 --- a/src/nix/command.cc +++ b/src/nix/command.cc @@ -137,7 +137,9 @@ void MixProfile::updateProfile(const Buildables & buildables) }, [&](BuildableFromDrv bfd) { for (auto & output : bfd.outputs) { - result.push_back(output.second); + if (!output.second) + throw Error("output path should be known because we just tried to build it"); + result.push_back(*output.second); } }, }, buildable); diff --git a/src/nix/installables.cc b/src/nix/installables.cc index ea152709f..2ff94cd38 100644 --- a/src/nix/installables.cc +++ b/src/nix/installables.cc @@ -302,10 +302,10 @@ struct InstallableStorePath : Installable Buildables toBuildables() override { if (storePath.isDerivation()) { - std::map outputs; + std::map> outputs; auto drv = store->readDerivation(storePath); for (auto & [name, output] : drv.outputs) - outputs.emplace(name, output.path(*store, drv.name)); + outputs.emplace(name, output.pathOpt(*store, drv.name)); return { BuildableFromDrv { .drvPath = storePath, @@ -331,7 +331,7 @@ Buildables InstallableValue::toBuildables() { Buildables res; - std::map drvsToOutputs; + std::map>> drvsToOutputs; // Group by derivation, helps with .all in particular for (auto & drv : toDerivations()) { @@ -674,8 +674,11 @@ StorePathSet toStorePaths(ref store, outPaths.insert(bo.path); }, [&](BuildableFromDrv bfd) { - for (auto & output : bfd.outputs) - outPaths.insert(output.second); + for (auto & output : bfd.outputs) { + if (!output.second) + throw Error("Cannot operate on output of unbuilt CA drv"); + outPaths.insert(*output.second); + } }, }, b); } else { diff --git a/src/nix/installables.hh b/src/nix/installables.hh index 26e87ee3a..41c75a4ed 100644 --- a/src/nix/installables.hh +++ b/src/nix/installables.hh @@ -20,7 +20,7 @@ struct BuildableOpaque { struct BuildableFromDrv { StorePath drvPath; - std::map outputs; + std::map> outputs; }; typedef std::variant< @@ -82,7 +82,7 @@ struct InstallableValue : Installable struct DerivationInfo { StorePath drvPath; - StorePath outPath; + std::optional outPath; std::string outputName; }; diff --git a/src/nix/profile.cc b/src/nix/profile.cc index 7dcc0b6d4..be002519a 100644 --- a/src/nix/profile.cc +++ b/src/nix/profile.cc @@ -178,7 +178,9 @@ struct CmdProfileInstall : InstallablesCommand, MixDefaultProfile auto [attrPath, resolvedRef, drv] = installable2->toDerivation(); ProfileElement element; - element.storePaths = {drv.outPath}; // FIXME + if (!drv.outPath) + throw UnimplementedError("CA derivations are not yet supported by 'nix profile'"); + element.storePaths = {*drv.outPath}; // FIXME element.source = ProfileElementSource{ installable2->flakeRef, resolvedRef, @@ -189,7 +191,7 @@ struct CmdProfileInstall : InstallablesCommand, MixDefaultProfile manifest.elements.emplace_back(std::move(element)); } else - throw Error("'nix profile install' does not support argument '%s'", installable->what()); + throw UnimplementedError("'nix profile install' does not support argument '%s'", installable->what()); } store->buildPaths(pathsToBuild); @@ -347,7 +349,9 @@ struct CmdProfileUpgrade : virtual SourceExprCommand, MixDefaultProfile, MixProf printInfo("upgrading '%s' from flake '%s' to '%s'", element.source->attrPath, element.source->resolvedRef, resolvedRef); - element.storePaths = {drv.outPath}; // FIXME + if (!drv.outPath) + throw UnimplementedError("CA derivations are not yet supported by 'nix profile'"); + element.storePaths = {*drv.outPath}; // FIXME element.source = ProfileElementSource{ installable.flakeRef, resolvedRef, diff --git a/src/nix/repl.cc b/src/nix/repl.cc index c3c9e54a8..0224f891d 100644 --- a/src/nix/repl.cc +++ b/src/nix/repl.cc @@ -488,10 +488,10 @@ bool NixRepl::processLine(string line) but doing it in a child makes it easier to recover from problems / SIGINT. */ if (runProgram(settings.nixBinDir + "/nix", Strings{"build", "--no-link", drvPath}) == 0) { - auto drv = readDerivation(*state->store, drvPath, Derivation::nameFromPath(state->store->parseStorePath(drvPath))); + auto outputs = state->store->queryDerivationOutputMapAssumeTotal(state->store->parseStorePath(drvPath)); std::cout << std::endl << "this derivation produced the following outputs:" << std::endl; - for (auto & i : drv.outputs) - std::cout << fmt(" %s -> %s\n", i.first, state->store->printStorePath(i.second.path(*state->store, drv.name))); + for (auto & i : outputs) + std::cout << fmt(" %s -> %s\n", i.first, state->store->printStorePath(i.second)); } } else if (command == ":i") { runProgram(settings.nixBinDir + "/nix-env", Strings{"-i", drvPath}); diff --git a/src/nix/show-derivation.cc b/src/nix/show-derivation.cc index 1b51d114f..b204b2d4c 100644 --- a/src/nix/show-derivation.cc +++ b/src/nix/show-derivation.cc @@ -69,12 +69,12 @@ struct CmdShowDerivation : InstallablesCommand auto outputsObj(drvObj.object("outputs")); for (auto & output : drv.outputs) { auto outputObj(outputsObj.object(output.first)); - outputObj.attr("path", store->printStorePath(output.second.path(*store, drv.name))); - std::visit(overloaded { [&](DerivationOutputInputAddressed doi) { + outputObj.attr("path", store->printStorePath(doi.path)); }, [&](DerivationOutputCAFixed dof) { + outputObj.attr("path", store->printStorePath(dof.path(*store, drv.name, output.first))); outputObj.attr("hashAlgo", dof.hash.printMethodAlgo()); outputObj.attr("hash", dof.hash.hash.to_string(Base16, false)); }, diff --git a/tests/content-addressed.nix b/tests/content-addressed.nix new file mode 100644 index 000000000..586e4cba6 --- /dev/null +++ b/tests/content-addressed.nix @@ -0,0 +1,19 @@ +with import ./config.nix; + +{ seed ? 0 }: +# A simple content-addressed derivation. +# The derivation can be arbitrarily modified by passing a different `seed`, +# but the output will always be the same +mkDerivation { + name = "simple-content-addressed"; + buildCommand = '' + set -x + echo "Building a CA derivation" + echo "The seed is ${toString seed}" + mkdir -p $out + echo "Hello World" > $out/hello + ''; + __contentAddressed = true; + outputHashMode = "recursive"; + outputHashAlgo = "sha256"; +} diff --git a/tests/content-addressed.sh b/tests/content-addressed.sh new file mode 100644 index 000000000..2968f3a8c --- /dev/null +++ b/tests/content-addressed.sh @@ -0,0 +1,16 @@ +#!/usr/bin/env bash + +source common.sh + +clearStore +clearCache + +export REMOTE_STORE=file://$cacheDir + +drv=$(nix-instantiate --experimental-features ca-derivations ./content-addressed.nix --arg seed 1) +nix --experimental-features 'nix-command ca-derivations' show-derivation --derivation "$drv" --arg seed 1 + +out1=$(nix-build --experimental-features ca-derivations ./content-addressed.nix --arg seed 1 --no-out-link) +out2=$(nix-build --experimental-features ca-derivations ./content-addressed.nix --arg seed 2 --no-out-link) + +test $out1 == $out2 diff --git a/tests/local.mk b/tests/local.mk index 0f3bfe606..4f4e63a5a 100644 --- a/tests/local.mk +++ b/tests/local.mk @@ -32,7 +32,8 @@ nix_tests = \ post-hook.sh \ function-trace.sh \ recursive.sh \ - flakes.sh + flakes.sh \ + content-addressed.sh # parallel.sh install-tests += $(foreach x, $(nix_tests), tests/$(x)) diff --git a/tests/multiple-outputs.nix b/tests/multiple-outputs.nix index 4a9010d18..b915493f7 100644 --- a/tests/multiple-outputs.nix +++ b/tests/multiple-outputs.nix @@ -2,6 +2,21 @@ with import ./config.nix; rec { + # Want to ensure that "out" doesn't get a suffix on it's path. + nameCheck = mkDerivation { + name = "multiple-outputs-a"; + outputs = [ "out" "dev" ]; + builder = builtins.toFile "builder.sh" + '' + mkdir $first $second + test -z $all + echo "first" > $first/file + echo "second" > $second/file + ln -s $first $second/link + ''; + helloString = "Hello, world!"; + }; + a = mkDerivation { name = "multiple-outputs-a"; outputs = [ "first" "second" ]; diff --git a/tests/multiple-outputs.sh b/tests/multiple-outputs.sh index bedbc39a4..7a6ec181d 100644 --- a/tests/multiple-outputs.sh +++ b/tests/multiple-outputs.sh @@ -4,6 +4,12 @@ clearStore rm -f $TEST_ROOT/result* +# Test whether the output names match our expectations +outPath=$(nix-instantiate multiple-outputs.nix --eval -A nameCheck.out.outPath) +[ "$(echo "$outPath" | sed -E 's_^".*/[^-/]*-([^/]*)"$_\1_')" = "multiple-outputs-a" ] +outPath=$(nix-instantiate multiple-outputs.nix --eval -A nameCheck.dev.outPath) +[ "$(echo "$outPath" | sed -E 's_^".*/[^-/]*-([^/]*)"$_\1_')" = "multiple-outputs-a-dev" ] + # Test whether read-only evaluation works when referring to the # ‘drvPath’ attribute. echo "evaluating c..." From f7696c66e85008e2b60a579bcdf5ff13c141d0af Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sat, 8 Aug 2020 15:48:51 +0000 Subject: [PATCH 02/52] Fix perl FFI for floating ca derivations Path is null when not known statically. --- perl/lib/Nix/Store.xs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/perl/lib/Nix/Store.xs b/perl/lib/Nix/Store.xs index de60fb939..6fe01fff0 100644 --- a/perl/lib/Nix/Store.xs +++ b/perl/lib/Nix/Store.xs @@ -303,11 +303,15 @@ SV * derivationFromPath(char * drvPath) hash = newHV(); HV * outputs = newHV(); - for (auto & i : drv.outputs) + for (auto & i : drv.outputs) { + auto pathOpt = i.second.pathOpt(*store(), drv.name); hv_store( outputs, i.first.c_str(), i.first.size(), - newSVpv(store()->printStorePath(i.second.path(*store(), drv.name)).c_str(), 0), + !pathOpt + ? newSV(0) /* null value */ + : newSVpv(store()->printStorePath(*pathOpt).c_str(), 0), 0); + } hv_stores(hash, "outputs", newRV((SV *) outputs)); AV * inputDrvs = newAV(); From 2a0902634eddded44a1775b275a257c98ca1dec8 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 11 Aug 2020 00:12:54 +0000 Subject: [PATCH 03/52] Fix error in merge breaking floating CA drvs Forgot to add this hunk! --- src/libstore/derivations.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index 732401dc5..e9f35a6f1 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -192,7 +192,7 @@ static DerivationOutput parseDerivationOutput(const Store & store, static DerivationOutput parseDerivationOutput(const Store & store, std::istringstream & str) { - expect(str, ","); const auto pathS = parsePath(str); + expect(str, ","); const auto pathS = parseString(str); expect(str, ","); const auto hashAlgo = parseString(str); expect(str, ","); const auto hash = parseString(str); expect(str, ")"); From d0f6e338ddbd4b7e37c2d944c368ca3094619e9f Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 11 Aug 2020 16:49:10 -0400 Subject: [PATCH 04/52] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Thanks!! Co-authored-by: Théophane Hufschmitt --- src/libstore/build.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 7c2225805..b9f79e5b7 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -876,7 +876,7 @@ private: ends up being. (Note that fixed outputs derivations that produce the "wrong" output still install that data under its true content-address.) */ - std::map finalOutputs; + OutputPathMap finalOutputs; BuildMode buildMode; @@ -1055,7 +1055,7 @@ private: 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. */ - StorePath makeFallbackPath(std::string_view path); + StorePath makeFallbackPath(std::string_view outputName); void repairClosure(); @@ -1336,7 +1336,7 @@ void DerivationGoal::outputsSubstitutionTried() void DerivationGoal::gaveUpOnSubstitution() { - /* Otherwise, at least one of the output paths could not be + /* At least one of the output paths could not be produced using a substitute. So we have to build instead. */ /* Make sure checkPathValidity() from now on checks all @@ -2371,7 +2371,7 @@ void DerivationGoal::startBuilder() (typically the dependencies of /bin/sh). Throw them out. */ for (auto & i : drv->outputs) { - /* If the name isn't known a prior (i.e. floating content-addressed + /* If the name isn't known a priori (i.e. floating content-addressed derivation), the temporary location we use should be fresh and never in the sandbox in the first place. */ auto optPath = i.second.pathOpt(worker.store, drv->name); @@ -3887,7 +3887,7 @@ void DerivationGoal::registerOutputs() for (auto & outputName : sortedOutputNames) { auto output = drv->outputs.at(outputName); auto & scratchPath = scratchOutputs.at(outputName); - auto actualPath = toRealPathChroot(worker.store.printStorePath(scratchOutputs.at(outputName))); + auto actualPath = toRealPathChroot(worker.store.printStorePath(scratchPath)); auto finish = [&](StorePath finalStorePath) { /* Store the final path */ From 07e3466eb4e6d259dc328a5aec834d42f8aacb60 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 11 Aug 2020 21:16:14 +0000 Subject: [PATCH 05/52] Float comment to out describe `gaveUpOnSubstitution` in general --- src/libstore/build.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index b9f79e5b7..1b55b9826 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -1334,11 +1334,10 @@ void DerivationGoal::outputsSubstitutionTried() gaveUpOnSubstitution(); } +/* At least one of the output paths could not be + produced using a substitute. So we have to build instead. */ void DerivationGoal::gaveUpOnSubstitution() { - /* At least one of the output paths could not be - produced using a substitute. So we have to build instead. */ - /* Make sure checkPathValidity() from now on checks all outputs. */ wantedOutputs.clear(); From 8a068bd0252573750be6ea754f0e3ff502fb6019 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 11 Aug 2020 21:25:40 +0000 Subject: [PATCH 06/52] Remove redundant equality check --- src/libstore/build.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 1b55b9826..cc596b063 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -4086,7 +4086,7 @@ void DerivationGoal::registerOutputs() } else if (buildMode == bmCheck) { /* Path already exists, and we want to compare, so we leave out new path in place. */ - } else if (finalDestPath == finalDestPath && worker.store.isValidPath(newInfo.path)) { + } else if (worker.store.isValidPath(newInfo.path)) { /* Path already exists because CA path produced by something else. No moving needed. */ assert(newInfo.ca); From 6d571390500431904d1ca1621fb6791fc25522f1 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 11 Aug 2020 22:34:09 +0000 Subject: [PATCH 07/52] Clarify `outputReferences` variable with self-describing type Thanks for the idea, @Regnat! --- src/libstore/build.cc | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index cc596b063..142149ae3 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -3800,7 +3800,9 @@ void DerivationGoal::registerOutputs() that are most definitely already installed, we just store their final name so we can also use it in rewrites. */ StringSet outputsToSort; - std::map> outputReferences; + struct AlreadyRegistered { StorePath path; }; + struct PerhapsNeedToRegister { StorePathSet refs; }; + std::map> outputReferencesIfUnregistered; std::map outputStats; for (auto & [outputName, _] : drv->outputs) { auto actualPath = toRealPathChroot(worker.store.printStorePath(scratchOutputs.at(outputName))); @@ -3814,7 +3816,9 @@ void DerivationGoal::registerOutputs() initialInfo.wanted = buildMode == bmCheck || !(initialInfo.known && initialInfo.known->isValid()); if (!initialInfo.wanted) { - outputReferences.insert_or_assign(outputName, initialInfo.known->path); + outputReferencesIfUnregistered.insert_or_assign( + outputName, + AlreadyRegistered { .path = initialInfo.known->path }); continue; } @@ -3851,28 +3855,29 @@ void DerivationGoal::registerOutputs() auto references = worker.store.parseStorePathSet( scanForReferences(blank, actualPath, worker.store.printStorePathSet(referenceablePaths))); - outputReferences.insert_or_assign(outputName, references); + outputReferencesIfUnregistered.insert_or_assign( + outputName, + PerhapsNeedToRegister { .refs = references }); outputStats.insert_or_assign(outputName, std::move(st)); } auto sortedOutputNames = topoSort(outputsToSort, {[&](const std::string & name) { - auto x = outputReferences.at(name); return std::visit(overloaded { /* Since we'll use the already installed versions of these, we can treat them as leaves and ignore any references they have. */ - [&](StorePath _) { return StringSet {}; }, - [&](StorePathSet refs) { + [&](AlreadyRegistered _) { return StringSet {}; }, + [&](PerhapsNeedToRegister refs) { StringSet referencedOutputs; /* FIXME build inverted map up front so no quadratic waste here */ - for (auto & r : refs) + for (auto & r : refs.refs) for (auto & [o, p] : scratchOutputs) if (r == p) referencedOutputs.insert(o); return referencedOutputs; }, - }, x); + }, outputReferencesIfUnregistered.at(name)); }}, {[&](const std::string & path, const std::string & parent) { // TODO with more -vvvv also show the temporary paths for manual inspection. @@ -3900,14 +3905,14 @@ void DerivationGoal::registerOutputs() bool rewritten = false; std::optional referencesOpt = std::visit(overloaded { - [&](StorePath skippedFinalPath) -> std::optional { - finish(skippedFinalPath); + [&](AlreadyRegistered skippedFinalPath) -> std::optional { + finish(skippedFinalPath.path); return std::nullopt; }, - [&](StorePathSet references) -> std::optional { - return references; + [&](PerhapsNeedToRegister r) -> std::optional { + return r.refs; }, - }, outputReferences.at(outputName)); + }, outputReferencesIfUnregistered.at(outputName)); if (!referencesOpt) continue; From 4c6aac8fdf2cbc85280115da53acebb910d2e60c Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 11 Aug 2020 22:46:05 +0000 Subject: [PATCH 08/52] Clarify comment on sandbox and temp fresh paths --- src/libstore/build.cc | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 142149ae3..d2bfc73c9 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -2370,9 +2370,11 @@ void DerivationGoal::startBuilder() (typically the dependencies of /bin/sh). Throw them out. */ for (auto & i : drv->outputs) { - /* If the name isn't known a priori (i.e. floating content-addressed - derivation), the temporary location we use should be fresh and - never in the sandbox in the first place. */ + /* If the name isn't known a priori (i.e. floating + content-addressed derivation), the temporary location we use + should be fresh. Freshness means it is impossible that the path + is already in the sandbox, so we don't need to worry about + removing it. */ auto optPath = i.second.pathOpt(worker.store, drv->name); if (optPath) dirsInChroot.erase(worker.store.printStorePath(*optPath)); From 2de201254e8669a6768bb739ff27fd94a87a71b9 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 11 Aug 2020 23:07:50 +0000 Subject: [PATCH 09/52] Don't assume a total output map in two places in build.cc Thanks @regnat for catching one of them. The other follows for many of the same reasons. I'm find fixing others on a need-to-fix basis, provided their are no regressions. --- src/libstore/build.cc | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index d2bfc73c9..c8107c7e8 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -1389,9 +1389,10 @@ void DerivationGoal::repairClosure() std::map outputsToDrv; for (auto & i : inputClosure) if (i.isDerivation()) { - auto depOutputs = worker.store.queryDerivationOutputMapAssumeTotal(i); + auto depOutputs = worker.store.queryDerivationOutputMap(i); for (auto & j : depOutputs) - outputsToDrv.insert_or_assign(j.second, i); + if (j.second) + outputsToDrv.insert_or_assign(*j.second, i); } /* Check each path (slow!). */ @@ -1459,11 +1460,16 @@ void DerivationGoal::inputsRealised() `i' as input paths. Only add the closures of output paths that are specified as inputs. */ assert(worker.store.isValidPath(drvPath)); - auto outputs = worker.store.queryDerivationOutputMapAssumeTotal(depDrvPath); + auto outputs = worker.store.queryDerivationOutputMap(depDrvPath); for (auto & j : wantedDepOutputs) { - if (outputs.count(j) > 0) - worker.store.computeFSClosure(outputs.at(j), inputPaths); - else + if (outputs.count(j) > 0) { + 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.", + worker.store.printStorePath(drvPath), j, worker.store.printStorePath(drvPath)); + worker.store.computeFSClosure(*optRealizedInput, inputPaths); + } else throw Error( "derivation '%s' requires non-existent output '%s' from input derivation '%s'", worker.store.printStorePath(drvPath), j, worker.store.printStorePath(drvPath)); From 18834f77643f5bd1071e1735f23f925d87cce6e5 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 11 Aug 2020 23:44:02 +0000 Subject: [PATCH 10/52] Recheck path validity after acquiring lock It might have changed, and in any event this is how the cod used to work so let's just keep it. --- src/libstore/build.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index c8107c7e8..49042649e 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -1547,6 +1547,7 @@ void DerivationGoal::tryToBuild() omitted, but that would be less efficient.) Note that since we now hold the locks on the output paths, no other process can build this derivation, so no further checks are necessary. */ + checkPathValidity(); bool allValid = true; for (auto & [_, status] : initialOutputs) { if (!status.wanted) continue; From 5f80aea795d3f6f78d682c7b99453b6fb7c9b6ba Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 12 Aug 2020 02:23:31 +0000 Subject: [PATCH 11/52] Break out lambda so output can be matched just once This is much better. --- src/libstore/build.cc | 86 +++++++++++++++++++++---------------------- 1 file changed, 42 insertions(+), 44 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 49042649e..0ea8f9c97 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -3973,42 +3973,7 @@ void DerivationGoal::registerOutputs() return res; }; - ValidPathInfo newInfo = [&]() {if (auto outputP = std::get_if(&output.output)) { - /* input-addressed case */ - auto requiredFinalPath = outputP->path; - /* Preemtively add rewrite rule for final hash, as that is - what the NAR hash will use rather than normalized-self references */ - if (scratchPath != requiredFinalPath) - outputRewrites.insert_or_assign( - std::string { scratchPath.hashPart() }, - std::string { requiredFinalPath.hashPart() }); - rewriteOutput(); - auto narHashAndSize = hashPath(htSHA256, actualPath); - ValidPathInfo newInfo0 { requiredFinalPath }; - newInfo0.narHash = narHashAndSize.first; - newInfo0.narSize = narHashAndSize.second; - auto refs = rewriteRefs(); - newInfo0.references = refs.second; - if (refs.first) - newInfo0.references.insert(newInfo0.path); - return newInfo0; - } else { - /* content-addressed case */ - DerivationOutputCAFloating outputHash = std::visit(overloaded { - [&](DerivationOutputInputAddressed doi) -> DerivationOutputCAFloating { - // Enclosing `if` handles this case in other branch - throw Error("ought to unreachable, handled in other branch"); - }, - [&](DerivationOutputCAFixed dof) { - return DerivationOutputCAFloating { - .method = dof.hash.method, - .hashType = dof.hash.hash.type, - }; - }, - [&](DerivationOutputCAFloating dof) { - return dof; - }, - }, output.output); + auto newInfoFromCA = [&](const DerivationOutputCAFloating outputHash) -> ValidPathInfo { auto & st = outputStats.at(outputName); if (outputHash.method == FileIngestionMethod::Flat) { /* The output path should be a regular file without execute permission. */ @@ -4055,9 +4020,41 @@ void DerivationGoal::registerOutputs() newInfo0.narSize = narHashAndSize.second; } - /* Check wanted hash if output is fixed */ - if (auto p = std::get_if(&output.output)) { - Hash & wanted = p->hash.hash; + assert(newInfo0.ca); + return newInfo0; + }; + + ValidPathInfo newInfo = std::visit(overloaded { + [&](DerivationOutputInputAddressed output) { + /* input-addressed case */ + auto requiredFinalPath = output.path; + /* Preemtively add rewrite rule for final hash, as that is + what the NAR hash will use rather than normalized-self references */ + if (scratchPath != requiredFinalPath) + outputRewrites.insert_or_assign( + std::string { scratchPath.hashPart() }, + std::string { requiredFinalPath.hashPart() }); + rewriteOutput(); + auto narHashAndSize = hashPath(htSHA256, actualPath); + ValidPathInfo newInfo0 { requiredFinalPath }; + newInfo0.narHash = narHashAndSize.first; + newInfo0.narSize = narHashAndSize.second; + auto refs = rewriteRefs(); + newInfo0.references = refs.second; + if (refs.first) + newInfo0.references.insert(newInfo0.path); + return newInfo0; + }, + [&](DerivationOutputCAFixed dof) { + auto newInfo0 = newInfoFromCA(DerivationOutputCAFloating { + .method = dof.hash.method, + .hashType = dof.hash.hash.type, + }); + + /* Check wanted hash */ + Hash & wanted = dof.hash.hash; + assert(newInfo0.ca); + auto got = getContentAddressHash(*newInfo0.ca); if (wanted != got) { /* Throw an error after registering the path as valid. */ @@ -4068,11 +4065,12 @@ void DerivationGoal::registerOutputs() wanted.to_string(SRI, true), got.to_string(SRI, true))); } - } - - assert(newInfo0.ca); - return newInfo0; - }}(); + return newInfo0; + }, + [&](DerivationOutputCAFloating dof) { + return newInfoFromCA(dof); + }, + }, output.output); /* Calculate where we'll move the output files. In the checking case we will leave leave them where they are, for now, rather than move to From f899a7c6d726dbf75e78fce5b5a9aa478387fcbe Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 14 Aug 2020 18:51:31 +0000 Subject: [PATCH 12/52] Work around clang bug --- src/nix/show-derivation.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/nix/show-derivation.cc b/src/nix/show-derivation.cc index cc52e53fb..b9f33499b 100644 --- a/src/nix/show-derivation.cc +++ b/src/nix/show-derivation.cc @@ -67,7 +67,8 @@ struct CmdShowDerivation : InstallablesCommand { auto outputsObj(drvObj.object("outputs")); - for (auto & [outputName, output] : drv.outputs) { + for (auto & [_outputName, output] : drv.outputs) { + auto & outputName = _outputName; // work around clang bug auto outputObj { outputsObj.object(outputName) }; std::visit(overloaded { [&](DerivationOutputInputAddressed doi) { From 45a2f1baaba8dbd4a4fb27b6ab9baee3c2820220 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 20 Aug 2020 18:14:12 +0000 Subject: [PATCH 13/52] Rename drv output querying functions, like master - `queryDerivationOutputMapAssumeTotal` -> `queryPartialDerivationOutputMap` - `queryDerivationOutputMapAssumeTotal` -> `queryDerivationOutputMap --- src/libexpr/primops.cc | 2 +- src/libstore/build.cc | 26 +++++++++++++------------- src/libstore/daemon.cc | 2 +- src/libstore/local-store.cc | 2 +- src/libstore/local-store.hh | 2 +- src/libstore/misc.cc | 2 +- src/libstore/remote-store.cc | 2 +- src/libstore/remote-store.hh | 2 +- src/libstore/store-api.cc | 6 +++--- src/libstore/store-api.hh | 6 +++--- src/nix-build/nix-build.cc | 2 +- src/nix-env/nix-env.cc | 2 +- src/nix-store/nix-store.cc | 2 +- src/nix/build.cc | 2 +- src/nix/repl.cc | 2 +- 15 files changed, 31 insertions(+), 31 deletions(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index e6391f509..c3d01eea3 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -64,7 +64,7 @@ void EvalState::realiseContext(const PathSet & context) paths. */ if (allowedPaths) { for (auto & [drvPath, outputs] : drvs) { - auto outputPaths = store->queryDerivationOutputMapAssumeTotal(drvPath); + auto outputPaths = store->queryDerivationOutputMap(drvPath); for (auto & outputName : outputs) { if (outputPaths.count(outputName) == 0) throw Error("derivation '%s' does not have an output named '%s'", diff --git a/src/libstore/build.cc b/src/libstore/build.cc index db93d6092..ba28e78c8 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -1041,8 +1041,8 @@ private: /* Wrappers around the corresponding Store methods that first consult the derivation. This is currently needed because when there is no drv file there also is no DB entry. */ - std::map> queryDerivationOutputMap(); - OutputPathMap queryDerivationOutputMapAssumeTotal(); + std::map> queryPartialDerivationOutputMap(); + OutputPathMap queryDerivationOutputMap(); /* Return the set of (in)valid paths. */ void checkPathValidity(); @@ -1367,7 +1367,7 @@ void DerivationGoal::repairClosure() that produced those outputs. */ /* Get the output closure. */ - auto outputs = queryDerivationOutputMapAssumeTotal(); + auto outputs = queryDerivationOutputMap(); StorePathSet outputClosure; for (auto & i : outputs) { if (!wantOutput(i.first, wantedOutputs)) continue; @@ -1386,7 +1386,7 @@ void DerivationGoal::repairClosure() std::map outputsToDrv; for (auto & i : inputClosure) if (i.isDerivation()) { - auto depOutputs = worker.store.queryDerivationOutputMap(i); + auto depOutputs = worker.store.queryPartialDerivationOutputMap(i); for (auto & j : depOutputs) if (j.second) outputsToDrv.insert_or_assign(*j.second, i); @@ -1457,7 +1457,7 @@ void DerivationGoal::inputsRealised() `i' as input paths. Only add the closures of output paths that are specified as inputs. */ assert(worker.store.isValidPath(drvPath)); - auto outputs = worker.store.queryDerivationOutputMap(depDrvPath); + auto outputs = worker.store.queryPartialDerivationOutputMap(depDrvPath); for (auto & j : wantedDepOutputs) { if (outputs.count(j) > 0) { auto optRealizedInput = outputs.at(j); @@ -2912,11 +2912,11 @@ struct RestrictedStore : public LocalFSStore void queryReferrers(const StorePath & path, StorePathSet & referrers) override { } - std::map> queryDerivationOutputMap(const StorePath & path) override + std::map> queryPartialDerivationOutputMap(const StorePath & path) override { if (!goal.isAllowed(path)) throw InvalidPath("cannot query output map for unknown path '%s' in recursive Nix", printStorePath(path)); - return next->queryDerivationOutputMap(path); + return next->queryPartialDerivationOutputMap(path); } std::optional queryPathFromHashPart(const std::string & hashPart) override @@ -2979,7 +2979,7 @@ struct RestrictedStore : public LocalFSStore for (auto & path : paths) { if (!path.path.isDerivation()) continue; - auto outputs = next->queryDerivationOutputMapAssumeTotal(path.path); + auto outputs = next->queryDerivationOutputMap(path.path); for (auto & output : outputs) if (wantOutput(output.first, path.outputs)) newPaths.insert(output.second); @@ -4548,7 +4548,7 @@ void DerivationGoal::flushLine() } -std::map> DerivationGoal::queryDerivationOutputMap() +std::map> DerivationGoal::queryPartialDerivationOutputMap() { if (drv->type() != DerivationType::CAFloating) { std::map> res; @@ -4556,11 +4556,11 @@ std::map> DerivationGoal::queryDerivationO res.insert_or_assign(name, output.pathOpt(worker.store, drv->name, name)); return res; } else { - return worker.store.queryDerivationOutputMap(drvPath); + return worker.store.queryPartialDerivationOutputMap(drvPath); } } -OutputPathMap DerivationGoal::queryDerivationOutputMapAssumeTotal() +OutputPathMap DerivationGoal::queryDerivationOutputMap() { if (drv->type() != DerivationType::CAFloating) { OutputPathMap res; @@ -4568,7 +4568,7 @@ OutputPathMap DerivationGoal::queryDerivationOutputMapAssumeTotal() res.insert_or_assign(name, *output.second); return res; } else { - return worker.store.queryDerivationOutputMapAssumeTotal(drvPath); + return worker.store.queryDerivationOutputMap(drvPath); } } @@ -4576,7 +4576,7 @@ OutputPathMap DerivationGoal::queryDerivationOutputMapAssumeTotal() void DerivationGoal::checkPathValidity() { bool checkHash = buildMode == bmRepair; - for (auto & i : queryDerivationOutputMap()) { + for (auto & i : queryPartialDerivationOutputMap()) { InitialOutputStatus status { .wanted = wantOutput(i.first, wantedOutputs), }; diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc index 046dfbe6e..a4eeebaab 100644 --- a/src/libstore/daemon.cc +++ b/src/libstore/daemon.cc @@ -325,7 +325,7 @@ static void performOp(TunnelLogger * logger, ref store, case wopQueryDerivationOutputMap: { auto path = store->parseStorePath(readString(from)); logger->startWork(); - auto outputs = store->queryDerivationOutputMap(path); + auto outputs = store->queryPartialDerivationOutputMap(path); logger->stopWork(); worker_proto::write(*store, to, outputs); break; diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 634a8046b..0086bb13e 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -803,7 +803,7 @@ StorePathSet LocalStore::queryValidDerivers(const StorePath & path) } -std::map> LocalStore::queryDerivationOutputMap(const StorePath & path) +std::map> LocalStore::queryPartialDerivationOutputMap(const StorePath & path) { std::map> outputs; BasicDerivation drv = readDerivation(path); diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index bf4016b13..4d99e5cf6 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -133,7 +133,7 @@ public: StorePathSet queryValidDerivers(const StorePath & path) override; - std::map> queryDerivationOutputMap(const StorePath & path) override; + std::map> queryPartialDerivationOutputMap(const StorePath & path) override; std::optional queryPathFromHashPart(const std::string & hashPart) override; diff --git a/src/libstore/misc.cc b/src/libstore/misc.cc index 9024fb2bd..da3981696 100644 --- a/src/libstore/misc.cc +++ b/src/libstore/misc.cc @@ -207,7 +207,7 @@ void Store::queryMissing(const std::vector & targets, /* true for regular derivations, and CA derivations for which we have a trust mapping for all wanted outputs. */ auto knownOutputPaths = true; - for (auto & [outputName, pathOpt] : queryDerivationOutputMap(path.path)) { + for (auto & [outputName, pathOpt] : queryPartialDerivationOutputMap(path.path)) { if (!pathOpt) { knownOutputPaths = false; break; diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 1b1260900..adba17c5e 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -474,7 +474,7 @@ StorePathSet RemoteStore::queryDerivationOutputs(const StorePath & path) } -std::map> RemoteStore::queryDerivationOutputMap(const StorePath & path) +std::map> RemoteStore::queryPartialDerivationOutputMap(const StorePath & path) { auto conn(getConnection()); conn->to << wopQueryDerivationOutputMap << printStorePath(path); diff --git a/src/libstore/remote-store.hh b/src/libstore/remote-store.hh index 4b093ad3b..b319e774b 100644 --- a/src/libstore/remote-store.hh +++ b/src/libstore/remote-store.hh @@ -51,7 +51,7 @@ public: StorePathSet queryDerivationOutputs(const StorePath & path) override; - std::map> queryDerivationOutputMap(const StorePath & path) override; + std::map> queryPartialDerivationOutputMap(const StorePath & path) override; std::optional queryPathFromHashPart(const std::string & hashPart) override; StorePathSet querySubstitutablePaths(const StorePathSet & paths) override; diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 1f10f0663..09608646e 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -366,8 +366,8 @@ bool Store::PathInfoCacheValue::isKnownNow() return std::chrono::steady_clock::now() < time_point + ttl; } -OutputPathMap Store::queryDerivationOutputMapAssumeTotal(const StorePath & path) { - auto resp = queryDerivationOutputMap(path); +OutputPathMap Store::queryDerivationOutputMap(const StorePath & path) { + auto resp = queryPartialDerivationOutputMap(path); OutputPathMap result; for (auto & [outName, optOutPath] : resp) { if (!optOutPath) @@ -379,7 +379,7 @@ OutputPathMap Store::queryDerivationOutputMapAssumeTotal(const StorePath & path) StorePathSet Store::queryDerivationOutputs(const StorePath & path) { - auto outputMap = this->queryDerivationOutputMapAssumeTotal(path); + auto outputMap = this->queryDerivationOutputMap(path); StorePathSet outputPaths; for (auto & i: outputMap) { outputPaths.emplace(std::move(i.second)); diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 58e9b16c6..6583413a6 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -348,12 +348,12 @@ public: /* Query the mapping outputName => outputPath for the given derivation. All outputs are mentioned so ones mising the mapping are mapped to `std::nullopt`. */ - virtual std::map> queryDerivationOutputMap(const StorePath & path) - { unsupported("queryDerivationOutputMap"); } + virtual std::map> queryPartialDerivationOutputMap(const StorePath & path) + { unsupported("queryPartialDerivationOutputMap"); } /* Query the mapping outputName=>outputPath for the given derivation. Assume every output has a mapping and throw an exception otherwise. */ - OutputPathMap queryDerivationOutputMapAssumeTotal(const StorePath & path); + OutputPathMap queryDerivationOutputMap(const StorePath & path); /* Query the full store path given the hash part of a valid store path, or empty if the path doesn't exist. */ diff --git a/src/nix-build/nix-build.cc b/src/nix-build/nix-build.cc index be73ea724..ea4b407e7 100755 --- a/src/nix-build/nix-build.cc +++ b/src/nix-build/nix-build.cc @@ -518,7 +518,7 @@ static void _main(int argc, char * * argv) if (counter) drvPrefix += fmt("-%d", counter + 1); - auto builtOutputs = store->queryDerivationOutputMapAssumeTotal(drvPath); + auto builtOutputs = store->queryDerivationOutputMap(drvPath); for (auto & outputName : wantedOutputs) { auto outputPath = builtOutputs.at(outputName); diff --git a/src/nix-env/nix-env.cc b/src/nix-env/nix-env.cc index d36804658..ddd036070 100644 --- a/src/nix-env/nix-env.cc +++ b/src/nix-env/nix-env.cc @@ -381,7 +381,7 @@ static void queryInstSources(EvalState & state, if (path.isDerivation()) { elem.setDrvPath(state.store->printStorePath(path)); - auto outputs = state.store->queryDerivationOutputMapAssumeTotal(path); + auto outputs = state.store->queryDerivationOutputMap(path); elem.setOutPath(state.store->printStorePath(outputs.at("out"))); if (name.size() >= drvExtension.size() && string(name, name.size() - drvExtension.size()) == drvExtension) diff --git a/src/nix-store/nix-store.cc b/src/nix-store/nix-store.cc index cfe97f061..ce5de2ae5 100644 --- a/src/nix-store/nix-store.cc +++ b/src/nix-store/nix-store.cc @@ -65,7 +65,7 @@ static PathSet realisePath(StorePathWithOutputs path, bool build = true) if (path.path.isDerivation()) { if (build) store->buildPaths({path}); - auto outputPaths = store->queryDerivationOutputMapAssumeTotal(path.path); + auto outputPaths = store->queryDerivationOutputMap(path.path); Derivation drv = store->derivationFromPath(path.path); rootNr++; diff --git a/src/nix/build.cc b/src/nix/build.cc index a66e8dac4..cb12ee933 100644 --- a/src/nix/build.cc +++ b/src/nix/build.cc @@ -74,7 +74,7 @@ struct CmdBuild : InstallablesCommand, MixDryRun, MixProfile store2->addPermRoot(bo.path, absPath(symlink), true); }, [&](BuildableFromDrv bfd) { - auto builtOutputs = store->queryDerivationOutputMapAssumeTotal(bfd.drvPath); + auto builtOutputs = store->queryDerivationOutputMap(bfd.drvPath); for (auto & output : builtOutputs) { std::string symlink = outLink; if (i) symlink += fmt("-%d", i); diff --git a/src/nix/repl.cc b/src/nix/repl.cc index 0224f891d..2bde85cb7 100644 --- a/src/nix/repl.cc +++ b/src/nix/repl.cc @@ -488,7 +488,7 @@ bool NixRepl::processLine(string line) but doing it in a child makes it easier to recover from problems / SIGINT. */ if (runProgram(settings.nixBinDir + "/nix", Strings{"build", "--no-link", drvPath}) == 0) { - auto outputs = state->store->queryDerivationOutputMapAssumeTotal(state->store->parseStorePath(drvPath)); + auto outputs = state->store->queryDerivationOutputMap(state->store->parseStorePath(drvPath)); std::cout << std::endl << "this derivation produced the following outputs:" << std::endl; for (auto & i : outputs) std::cout << fmt(" %s -> %s\n", i.first, state->store->printStorePath(i.second)); From 3a7b330b64c6ea77e18a0a96aad7fb14947382d9 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 21 Aug 2020 19:35:35 +0000 Subject: [PATCH 14/52] "Downstream placeholders" should not be store paths Insead they should be opaque `/` like the placeholders we already have. --- src/libexpr/primops.cc | 6 +++--- src/libstore/derivations.cc | 8 +++----- src/libstore/derivations.hh | 2 +- 3 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 0ddba6384..d28024639 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -82,13 +82,13 @@ static void mkOutputString(EvalState & state, Value & v, auto optOutputPath = o.second.pathOpt(*state.store, drv.name, o.first); mkString( *state.allocAttr(v, state.symbols.create(o.first)), - state.store->printStorePath(optOutputPath - ? *optOutputPath + optOutputPath + ? state.store->printStorePath(*optOutputPath) /* Downstream we would substitute this for an actual path once we build the floating CA derivation */ /* FIXME: we need to depend on the basic derivation, not derivation */ - : downstreamPlaceholder(*state.store, drvPath, o.first)), + : downstreamPlaceholder(*state.store, drvPath, o.first), {"!" + o.first + "!" + state.store->printStorePath(drvPath)}); } diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index fb1d06466..5319c1a38 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -664,14 +664,12 @@ std::string hashPlaceholder(const std::string & outputName) return "/" + hashString(htSHA256, "nix-output:" + outputName).to_string(Base32, false); } -StorePath downstreamPlaceholder(const Store & store, const StorePath & drvPath, std::string_view outputName) +std::string downstreamPlaceholder(const Store & store, const StorePath & drvPath, std::string_view outputName) { auto drvNameWithExtension = drvPath.name(); auto drvName = drvNameWithExtension.substr(0, drvNameWithExtension.size() - 4); - return store.makeStorePath( - "downstream-placeholder:" + std::string { drvPath.name() } + ":" + std::string { outputName }, - "compressed:" + std::string { drvPath.hashPart() }, - outputPathName(drvName, outputName)); + auto clearText = "nix-upstream-output:" + std::string { drvPath.hashPart() } + ":" + outputPathName(drvName, outputName); + return "/" + hashString(htSHA256, clearText).to_string(Base32, false); } } diff --git a/src/libstore/derivations.hh b/src/libstore/derivations.hh index 49374d046..76f983561 100644 --- a/src/libstore/derivations.hh +++ b/src/libstore/derivations.hh @@ -196,6 +196,6 @@ void writeDerivation(Sink & out, const Store & store, const BasicDerivation & dr std::string hashPlaceholder(const std::string & outputName); -StorePath downstreamPlaceholder(const Store & store, const StorePath & drvPath, std::string_view outputName); +std::string downstreamPlaceholder(const Store & store, const StorePath & drvPath, std::string_view outputName); } From 145915eb3901ad78e61f27df22e755fdf9a2c28a Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 3 Sep 2020 22:14:21 +0000 Subject: [PATCH 15/52] Beef up floating CA derivations test a bit --- tests/content-addressed.nix | 37 +++++++++++++++++++++++++------------ tests/content-addressed.sh | 7 ++++--- 2 files changed, 29 insertions(+), 15 deletions(-) diff --git a/tests/content-addressed.nix b/tests/content-addressed.nix index 586e4cba6..1fa6aabff 100644 --- a/tests/content-addressed.nix +++ b/tests/content-addressed.nix @@ -4,16 +4,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 -mkDerivation { - name = "simple-content-addressed"; - buildCommand = '' - set -x - echo "Building a CA derivation" - echo "The seed is ${toString seed}" - mkdir -p $out - echo "Hello World" > $out/hello - ''; - __contentAddressed = true; - outputHashMode = "recursive"; - outputHashAlgo = "sha256"; +rec { + rootLegacy = mkDerivation { + name = "simple-content-addressed"; + buildCommand = '' + set -x + echo "Building a legacy derivation" + mkdir -p $out + echo "Hello World" > $out/hello + ''; + __contentAddressed = true; + outputHashMode = "recursive"; + outputHashAlgo = "sha256"; + }; + rootCA = mkDerivation { + name = "dependent"; + buildCommand = '' + echo "building a CA derivation" + echo "The seed is ${toString seed}" + mkdir -p $out + echo ${rootLegacy}/hello > $out/dep + ''; + __contentAddressed = true; + outputHashMode = "recursive"; + outputHashAlgo = "sha256"; + }; } diff --git a/tests/content-addressed.sh b/tests/content-addressed.sh index 2968f3a8c..ae9e3c59e 100644 --- a/tests/content-addressed.sh +++ b/tests/content-addressed.sh @@ -7,10 +7,11 @@ clearCache export REMOTE_STORE=file://$cacheDir -drv=$(nix-instantiate --experimental-features ca-derivations ./content-addressed.nix --arg seed 1) +drv=$(nix-instantiate --experimental-features ca-derivations ./content-addressed.nix -A rootCA --arg seed 1) nix --experimental-features 'nix-command ca-derivations' show-derivation --derivation "$drv" --arg seed 1 -out1=$(nix-build --experimental-features ca-derivations ./content-addressed.nix --arg seed 1 --no-out-link) -out2=$(nix-build --experimental-features ca-derivations ./content-addressed.nix --arg seed 2 --no-out-link) +commonArgs=("--experimental-features" "ca-derivations" "./content-addressed.nix" "-A" "rootCA" "--no-out-link") +out1=$(nix-build "${commonArgs[@]}" ./content-addressed.nix --arg seed 1) +out2=$(nix-build "${commonArgs[@]}" ./content-addressed.nix --arg seed 2) test $out1 == $out2 From c224a5e5c1e0a55c8e8e7c3c1c0e50ac831f9964 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 3 Sep 2020 22:35:13 +0000 Subject: [PATCH 16/52] Rename derivation in floating CA test --- tests/content-addressed.nix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/content-addressed.nix b/tests/content-addressed.nix index 1fa6aabff..79e8a8bf9 100644 --- a/tests/content-addressed.nix +++ b/tests/content-addressed.nix @@ -6,7 +6,7 @@ with import ./config.nix; # but the output will always be the same rec { rootLegacy = mkDerivation { - name = "simple-content-addressed"; + name = "simple-input-addressed"; buildCommand = '' set -x echo "Building a legacy derivation" From b99062b023e56865ba20371b29fa3be6e6149d46 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 4 Sep 2020 10:29:28 -0400 Subject: [PATCH 17/52] 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 18/52] 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 19/52] 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 20/52] 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 21/52] 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) From c4bf219b55c76329988671d6b383d073373919f0 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 15 Sep 2020 14:26:56 +0000 Subject: [PATCH 22/52] Don't link deriver until after any delayed exception is thrown Otherwise, we will associate fixed-output derivations with outputs that they did indeed produce, but which had the wrong hash. That's no good. --- src/libstore/build.cc | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 9eff03f7b..d6a775ae9 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -4251,22 +4251,28 @@ void DerivationGoal::registerOutputs() /* Register each output path as valid, and register the sets of paths referenced by each of them. If there are cycles in the outputs, this will fail. */ - { - ValidPathInfos infos2; - for (auto & [outputName, newInfo] : infos) { - /* FIXME: we will want to track this mapping in the DB whether or - not we have a drv file. */ - if (useDerivation) - worker.store.linkDeriverToPath(drvPath, outputName, newInfo.path); - infos2.push_back(newInfo); - } - worker.store.registerValidPaths(infos2); + ValidPathInfos infos2; + for (auto & [outputName, newInfo] : infos) { + infos2.push_back(newInfo); } + worker.store.registerValidPaths(infos2); /* In case of a fixed-output derivation hash mismatch, throw an exception now that we have registered the output as valid. */ if (delayedException) std::rethrow_exception(delayedException); + + /* If we made it this far, we are sure the output matches the derivation + (since the delayedException would be a fixed output CA mismatch). That + means it's safe to link the derivation to the output hash. We must do + that for floating CA derivations, which otherwise couldn't be cached, + but it's fine to do in all cases. */ + for (auto & [outputName, newInfo] : infos) { + /* FIXME: we will want to track this mapping in the DB whether or + not we have a drv file. */ + if (useDerivation) + worker.store.linkDeriverToPath(drvPath, outputName, newInfo.path); + } } From 6387550d58884ac09204c339ed3a3cd700780a75 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 15 Sep 2020 15:19:45 +0000 Subject: [PATCH 23/52] Get rid of confusing `std::optional` for validity --- src/libstore/build.cc | 35 +++++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index d6a775ae9..b800a432f 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -718,16 +718,25 @@ typedef enum {rpAccept, rpDecline, rpPostpone} HookReply; class SubstitutionGoal; +/* Unless we are repairing, we don't both to test validity and just assume it, + so the choices are `Absent` or `Valid`. */ +enum struct PathStatus { + Corrupt, + Absent, + Valid, +}; + 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 - valid path is in fact not corrupt. Otherwise, the inner bool is always - true (assumed no corruption). */ - std::optional valid; + PathStatus status; /* Valid in the store, and additionally non-corrupt if we are repairing */ bool isValid() const { - return valid && *valid; + return status == PathStatus::Valid; + } + /* Merely present, allowed to be corrupt */ + bool isPresent() const { + return status == PathStatus::Corrupt + || status == PathStatus::Valid; } }; @@ -2148,10 +2157,10 @@ void DerivationGoal::startBuilder() : !needsHashRewrite() /* Can always use original path in sandbox */ ? status.known->path - : !status.known->valid + : !status.known->isPresent() /* If path doesn't yet exist can just use it */ ? status.known->path - : buildMode != bmRepair && !*status.known->valid + : buildMode != bmRepair && !status.known->isValid() /* If we aren't repairing we'll delete a corrupted path, so we can use original path */ ? status.known->path @@ -2161,7 +2170,7 @@ void DerivationGoal::startBuilder() scratchOutputs.insert_or_assign(outputName, scratchPath); /* A non-removed corrupted path needs to be stored here, too */ - if (buildMode == bmRepair && !*status.known->valid) + if (buildMode == bmRepair && !status.known->isValid()) redirectedBadOutputs.insert(status.known->path); /* Substitute output placeholders with the scratch output paths. @@ -4596,9 +4605,11 @@ void DerivationGoal::checkPathValidity() auto outputPath = *i.second; info.known = { .path = outputPath, - .valid = !worker.store.isValidPath(outputPath) - ? std::optional {} - : !checkHash || worker.pathContentsGood(outputPath), + .status = !worker.store.isValidPath(outputPath) + ? PathStatus::Absent + : !checkHash || worker.pathContentsGood(outputPath) + ? PathStatus::Valid + : PathStatus::Corrupt, }; } initialOutputs.insert_or_assign(i.first, info); From 3a5cdd737cf529a7f641c4347f002b821a456a5d Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 15 Sep 2020 15:21:39 +0000 Subject: [PATCH 24/52] Rename `Derivation::pathOpt` to `Derivation::path` We no longer need the `*Opt` to disambiguate. --- src/libexpr/get-drvs.cc | 2 +- src/libexpr/primops.cc | 2 +- src/libstore/build.cc | 4 ++-- src/libstore/derivations.cc | 4 ++-- src/libstore/derivations.hh | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/libexpr/get-drvs.cc b/src/libexpr/get-drvs.cc index e66832182..91916e8bf 100644 --- a/src/libexpr/get-drvs.cc +++ b/src/libexpr/get-drvs.cc @@ -40,7 +40,7 @@ DrvInfo::DrvInfo(EvalState & state, ref store, const std::string & drvPat throw Error("derivation '%s' does not have output '%s'", store->printStorePath(drvPath), outputName); auto & [outputName, output] = *i; - auto optStorePath = output.pathOpt(*store, drv.name, outputName); + auto optStorePath = output.path(*store, drv.name, outputName); if (optStorePath) outPath = store->printStorePath(*optStorePath); } diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 78dc314fa..4a0dd5544 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -91,7 +91,7 @@ static void mkOutputString(EvalState & state, Value & v, const StorePath & drvPath, const BasicDerivation & drv, std::pair o) { - auto optOutputPath = o.second.pathOpt(*state.store, drv.name, o.first); + auto optOutputPath = o.second.path(*state.store, drv.name, o.first); mkString( *state.allocAttr(v, state.symbols.create(o.first)), optOutputPath diff --git a/src/libstore/build.cc b/src/libstore/build.cc index b800a432f..ce973862d 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -4081,7 +4081,7 @@ void DerivationGoal::registerOutputs() floating CA derivations and hash-mismatching fixed-output derivations. */ PathLocks dynamicOutputLock; - auto optFixedPath = output.pathOpt(worker.store, drv->name, outputName); + auto optFixedPath = output.path(worker.store, drv->name, outputName); if (!optFixedPath || worker.store.printStorePath(*optFixedPath) != finalDestPath) { @@ -4574,7 +4574,7 @@ std::map> DerivationGoal::queryPartialDeri if (drv->type() != DerivationType::CAFloating) { std::map> res; for (auto & [name, output] : drv->outputs) - res.insert_or_assign(name, output.pathOpt(worker.store, drv->name, name)); + res.insert_or_assign(name, output.path(worker.store, drv->name, name)); return res; } else { return worker.store.queryPartialDerivationOutputMap(drvPath); diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index e5ec60811..9d8ce5e36 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -7,7 +7,7 @@ namespace nix { -std::optional DerivationOutput::pathOpt(const Store & store, std::string_view drvName, std::string_view outputName) const +std::optional DerivationOutput::path(const Store & store, std::string_view drvName, std::string_view outputName) const { return std::visit(overloaded { [](DerivationOutputInputAddressed doi) -> std::optional { @@ -557,7 +557,7 @@ DerivationOutputsAndOptPaths BasicDerivation::outputsAndOptPaths(const Store & s for (auto output : outputs) outsAndOptPaths.insert(std::make_pair( output.first, - std::make_pair(output.second, output.second.pathOpt(store, name, output.first)) + std::make_pair(output.second, output.second.path(store, name, output.first)) ) ); return outsAndOptPaths; diff --git a/src/libstore/derivations.hh b/src/libstore/derivations.hh index 2c0b1feab..0b5652685 100644 --- a/src/libstore/derivations.hh +++ b/src/libstore/derivations.hh @@ -51,7 +51,7 @@ struct DerivationOutput /* Note, when you use this function you should make sure that you're passing the right derivation name. When in doubt, you should use the safer interface provided by BasicDerivation::outputsAndOptPaths */ - std::optional pathOpt(const Store & store, std::string_view drvName, std::string_view outputName) const; + std::optional path(const Store & store, std::string_view drvName, std::string_view outputName) const; }; typedef std::map DerivationOutputs; From 7d5bdf8b5679cc7b2b9b4d9caf5af9ca52211336 Mon Sep 17 00:00:00 2001 From: regnat Date: Tue, 8 Sep 2020 14:50:23 +0200 Subject: [PATCH 25/52] Make the store plugins more introspectable Directly register the store classes rather than a function to build an instance of them. This gives the possibility to introspect static members of the class or choose different ways of instantiating them. --- result | 1 + src/libexpr/flake/lockfile.hh | 2 +- src/libfetchers/fetchers.hh | 2 +- src/libstore/dummy-store.cc | 18 +++----- src/libstore/http-binary-cache-store.cc | 24 +++++------ src/libstore/legacy-ssh-store.cc | 15 +++---- src/libstore/local-binary-cache-store.cc | 23 +++++----- src/libstore/remote-store.cc | 10 +---- src/libstore/remote-store.hh | 3 ++ src/libstore/s3-binary-cache-store.cc | 15 +++---- src/libstore/ssh-store.cc | 14 ++---- src/libstore/store-api.cc | 55 +++++++++++++----------- src/libstore/store-api.hh | 31 ++++++++----- src/libstore/store.hh | 9 ++++ src/nix/describe-stores.cc | 30 +++++++++++++ src/nix/develop.cc | 2 +- src/nix/diff-closures.cc | 2 +- src/nix/installables.hh | 2 +- 18 files changed, 141 insertions(+), 117 deletions(-) create mode 120000 result create mode 100644 src/libstore/store.hh create mode 100644 src/nix/describe-stores.cc diff --git a/result b/result new file mode 120000 index 000000000..af7bbb6da --- /dev/null +++ b/result @@ -0,0 +1 @@ +/nix/store/9pqfirjppd91mzhkgh8xnn66iwh53zk2-hello-2.10 \ No newline at end of file diff --git a/src/libexpr/flake/lockfile.hh b/src/libexpr/flake/lockfile.hh index 5e7cfda3e..9ec8b39c3 100644 --- a/src/libexpr/flake/lockfile.hh +++ b/src/libexpr/flake/lockfile.hh @@ -6,7 +6,7 @@ namespace nix { class Store; -struct StorePath; +class StorePath; } namespace nix::flake { diff --git a/src/libfetchers/fetchers.hh b/src/libfetchers/fetchers.hh index be71b786b..89b1e6e7d 100644 --- a/src/libfetchers/fetchers.hh +++ b/src/libfetchers/fetchers.hh @@ -23,7 +23,7 @@ struct InputScheme; struct Input { - friend class InputScheme; + friend struct InputScheme; std::shared_ptr scheme; // note: can be null Attrs attrs; diff --git a/src/libstore/dummy-store.cc b/src/libstore/dummy-store.cc index 7a5744bc1..a677e201e 100644 --- a/src/libstore/dummy-store.cc +++ b/src/libstore/dummy-store.cc @@ -2,17 +2,15 @@ namespace nix { -static std::string uriScheme = "dummy://"; - struct DummyStore : public Store { - DummyStore(const Params & params) + DummyStore(const std::string uri, const Params & params) : Store(params) { } string getUri() override { - return uriScheme; + return uriPrefixes()[0] + "://"; } void queryPathInfoUncached(const StorePath & path, @@ -21,6 +19,10 @@ struct DummyStore : public Store callback(nullptr); } + static std::vector uriPrefixes() { + return {"dummy"}; + } + std::optional queryPathFromHashPart(const std::string & hashPart) override { unsupported("queryPathFromHashPart"); } @@ -48,12 +50,6 @@ struct DummyStore : public Store { unsupported("buildDerivation"); } }; -static RegisterStoreImplementation regStore([]( - const std::string & uri, const Store::Params & params) - -> std::shared_ptr -{ - if (uri != uriScheme) return nullptr; - return std::make_shared(params); -}); +[[maybe_unused]] static RegisterStoreImplementation regStore(); } diff --git a/src/libstore/http-binary-cache-store.cc b/src/libstore/http-binary-cache-store.cc index 1733239fb..6c3a16faf 100644 --- a/src/libstore/http-binary-cache-store.cc +++ b/src/libstore/http-binary-cache-store.cc @@ -24,7 +24,8 @@ private: public: HttpBinaryCacheStore( - const Params & params, const Path & _cacheUri) + const Path & _cacheUri, + const Params & params) : BinaryCacheStore(params) , cacheUri(_cacheUri) { @@ -55,6 +56,13 @@ public: } } + static std::vector uriPrefixes() + { + static bool forceHttp = getEnv("_NIX_FORCE_HTTP") == "1"; + auto ret = std::vector({"http", "https"}); + if (forceHttp) ret.push_back("file"); + return ret; + } protected: void maybeDisable() @@ -162,18 +170,6 @@ protected: }; -static RegisterStoreImplementation regStore([]( - const std::string & uri, const Store::Params & params) - -> std::shared_ptr -{ - static bool forceHttp = getEnv("_NIX_FORCE_HTTP") == "1"; - if (std::string(uri, 0, 7) != "http://" && - std::string(uri, 0, 8) != "https://" && - (!forceHttp || std::string(uri, 0, 7) != "file://")) - return 0; - auto store = std::make_shared(params, uri); - store->init(); - return store; -}); +[[maybe_unused]] static RegisterStoreImplementation regStore(); } diff --git a/src/libstore/legacy-ssh-store.cc b/src/libstore/legacy-ssh-store.cc index dc03313f0..72f28a82d 100644 --- a/src/libstore/legacy-ssh-store.cc +++ b/src/libstore/legacy-ssh-store.cc @@ -9,8 +9,6 @@ namespace nix { -static std::string uriScheme = "ssh://"; - struct LegacySSHStore : public Store { const Setting maxConnections{this, 1, "max-connections", "maximum number of concurrent SSH connections"}; @@ -37,6 +35,9 @@ struct LegacySSHStore : public Store SSHMaster master; + static std::vector uriPrefixes() { return {"ssh"}; } + + LegacySSHStore(const string & host, const Params & params) : Store(params) , host(host) @@ -84,7 +85,7 @@ struct LegacySSHStore : public Store string getUri() override { - return uriScheme + host; + return uriPrefixes()[0] + "://" + host; } void queryPathInfoUncached(const StorePath & path, @@ -325,12 +326,6 @@ public: } }; -static RegisterStoreImplementation regStore([]( - const std::string & uri, const Store::Params & params) - -> std::shared_ptr -{ - if (std::string(uri, 0, uriScheme.size()) != uriScheme) return 0; - return std::make_shared(std::string(uri, uriScheme.size()), params); -}); +[[maybe_unused]] static RegisterStoreImplementation regStore(); } diff --git a/src/libstore/local-binary-cache-store.cc b/src/libstore/local-binary-cache-store.cc index 87d8334d7..752838d3f 100644 --- a/src/libstore/local-binary-cache-store.cc +++ b/src/libstore/local-binary-cache-store.cc @@ -13,7 +13,8 @@ private: public: LocalBinaryCacheStore( - const Params & params, const Path & binaryCacheDir) + const Path & binaryCacheDir, + const Params & params) : BinaryCacheStore(params) , binaryCacheDir(binaryCacheDir) { @@ -26,6 +27,8 @@ public: return "file://" + binaryCacheDir; } + static std::vector uriPrefixes(); + protected: bool fileExists(const std::string & path) override; @@ -85,16 +88,14 @@ bool LocalBinaryCacheStore::fileExists(const std::string & path) return pathExists(binaryCacheDir + "/" + path); } -static RegisterStoreImplementation regStore([]( - const std::string & uri, const Store::Params & params) - -> std::shared_ptr +std::vector LocalBinaryCacheStore::uriPrefixes() { - if (getEnv("_NIX_FORCE_HTTP_BINARY_CACHE_STORE") == "1" || - std::string(uri, 0, 7) != "file://") - return 0; - auto store = std::make_shared(params, std::string(uri, 7)); - store->init(); - return store; -}); + if (getEnv("_NIX_FORCE_HTTP_BINARY_CACHE_STORE") == "1") + return {}; + else + return {"file"}; +} + +[[maybe_unused]] static RegisterStoreImplementation regStore(); } diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index dc61951d3..824b6b140 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -982,14 +982,6 @@ std::exception_ptr RemoteStore::Connection::processStderr(Sink * sink, Source * return nullptr; } -static std::string uriScheme = "unix://"; - -static RegisterStoreImplementation regStore([]( - const std::string & uri, const Store::Params & params) - -> std::shared_ptr -{ - if (std::string(uri, 0, uriScheme.size()) != uriScheme) return 0; - return std::make_shared(std::string(uri, uriScheme.size()), params); -}); +[[maybe_unused]] static RegisterStoreImplementation regStore(); } diff --git a/src/libstore/remote-store.hh b/src/libstore/remote-store.hh index 6f05f2197..07cd5daf8 100644 --- a/src/libstore/remote-store.hh +++ b/src/libstore/remote-store.hh @@ -150,6 +150,9 @@ public: std::string getUri() override; + static std::vector uriPrefixes() + { return {"unix"}; } + bool sameMachine() override { return true; } diff --git a/src/libstore/s3-binary-cache-store.cc b/src/libstore/s3-binary-cache-store.cc index a0a446bd3..1bf4d5955 100644 --- a/src/libstore/s3-binary-cache-store.cc +++ b/src/libstore/s3-binary-cache-store.cc @@ -193,7 +193,8 @@ struct S3BinaryCacheStoreImpl : public S3BinaryCacheStore S3Helper s3Helper; S3BinaryCacheStoreImpl( - const Params & params, const std::string & bucketName) + const std::string & bucketName, + const Params & params) : S3BinaryCacheStore(params) , bucketName(bucketName) , s3Helper(profile, region, scheme, endpoint) @@ -426,17 +427,11 @@ struct S3BinaryCacheStoreImpl : public S3BinaryCacheStore return paths; } + static std::vector uriPrefixes() { return {"s3"}; } + }; -static RegisterStoreImplementation regStore([]( - const std::string & uri, const Store::Params & params) - -> std::shared_ptr -{ - if (std::string(uri, 0, 5) != "s3://") return 0; - auto store = std::make_shared(params, std::string(uri, 5)); - store->init(); - return store; -}); +[[maybe_unused]] static RegisterStoreImplementation regStore(); } diff --git a/src/libstore/ssh-store.cc b/src/libstore/ssh-store.cc index 6cb97c1f1..02208ee82 100644 --- a/src/libstore/ssh-store.cc +++ b/src/libstore/ssh-store.cc @@ -8,8 +8,6 @@ namespace nix { -static std::string uriScheme = "ssh-ng://"; - class SSHStore : public RemoteStore { public: @@ -32,9 +30,11 @@ public: { } + static std::vector uriPrefixes() { return {"ssh-ng"}; } + std::string getUri() override { - return uriScheme + host; + return uriPrefixes()[0] + "://" + host; } bool sameMachine() override @@ -76,12 +76,6 @@ ref SSHStore::openConnection() return conn; } -static RegisterStoreImplementation regStore([]( - const std::string & uri, const Store::Params & params) - -> std::shared_ptr -{ - if (std::string(uri, 0, uriScheme.size()) != uriScheme) return 0; - return std::make_shared(std::string(uri, uriScheme.size()), params); -}); +[[maybe_unused]] static RegisterStoreImplementation regStore(); } diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index b3877487c..4a46b5160 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -1009,6 +1009,11 @@ Derivation Store::readDerivation(const StorePath & drvPath) } } +std::shared_ptr Store::getConfig() +{ + return shared_from_this(); +} + } @@ -1019,9 +1024,6 @@ Derivation Store::readDerivation(const StorePath & drvPath) namespace nix { - -RegisterStoreImplementation::Implementations * RegisterStoreImplementation::implementations = 0; - /* Split URI into protocol+hierarchy part and its parameter set. */ std::pair splitUriAndParams(const std::string & uri_) { @@ -1035,24 +1037,6 @@ std::pair splitUriAndParams(const std::string & uri_ return {uri, params}; } -ref openStore(const std::string & uri_, - const Store::Params & extraParams) -{ - auto [uri, uriParams] = splitUriAndParams(uri_); - auto params = extraParams; - params.insert(uriParams.begin(), uriParams.end()); - - for (auto fun : *RegisterStoreImplementation::implementations) { - auto store = fun(uri, params); - if (store) { - store->warnUnknownSettings(); - return ref(store); - } - } - - throw Error("don't know how to open Nix store '%s'", uri); -} - static bool isNonUriPath(const std::string & spec) { return // is not a URL @@ -1080,10 +1064,7 @@ StoreType getStoreType(const std::string & uri, const std::string & stateDir) } } - -static RegisterStoreImplementation regStore([]( - const std::string & uri, const Store::Params & params) - -> std::shared_ptr +std::shared_ptr openFromNonUri(const std::string & uri, const Store::Params & params) { switch (getStoreType(uri, get(params, "state").value_or(settings.nixStateDir))) { case tDaemon: @@ -1098,8 +1079,30 @@ static RegisterStoreImplementation regStore([]( default: return nullptr; } -}); +} +ref openStore(const std::string & uri_, + const Store::Params & extraParams) +{ + auto [uri, uriParams] = splitUriAndParams(uri_); + auto params = extraParams; + params.insert(uriParams.begin(), uriParams.end()); + + if (auto store = openFromNonUri(uri, params)) { + store->warnUnknownSettings(); + return ref(store); + } + + for (auto implem : *implementations) { + auto store = implem.open(uri, params); + if (store) { + store->warnUnknownSettings(); + return ref(store); + } + } + + throw Error("don't know how to open Nix store '%s'", uri); +} std::list> getDefaultSubstituters() { diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 8b42aa6d2..28365542d 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -33,6 +33,7 @@ MakeError(SubstituteGone, Error); MakeError(SubstituterDisabled, Error); MakeError(BadStorePath, Error); +MakeError(InvalidStoreURI, Error); class FSAccessor; class NarInfoDiskCache; @@ -199,6 +200,8 @@ protected: Store(const Params & params); + std::shared_ptr getConfig(); + public: virtual ~Store() { } @@ -744,25 +747,31 @@ StoreType getStoreType(const std::string & uri = settings.storeUri.get(), ‘substituters’ option and various legacy options. */ std::list> getDefaultSubstituters(); +struct StoreFactory +{ + std::vector uriPrefixes; + std::function (const std::string & uri, const Store::Params & params)> open; +}; +typedef std::vector Implementations; +static Implementations * implementations = new Implementations; -/* Store implementation registration. */ -typedef std::function( - const std::string & uri, const Store::Params & params)> OpenStore; - +template struct RegisterStoreImplementation { - typedef std::vector Implementations; - static Implementations * implementations; - - RegisterStoreImplementation(OpenStore fun) + RegisterStoreImplementation() { - if (!implementations) implementations = new Implementations; - implementations->push_back(fun); + StoreFactory factory{ + .uriPrefixes = T::uriPrefixes(), + .open = + ([](const std::string & uri, const Store::Params & params) + -> std::shared_ptr + { return std::make_shared(uri, params); }) + }; + implementations->push_back(factory); } }; - /* Display a set of paths in human-readable form (i.e., between quotes and separated by commas). */ string showPaths(const PathSet & paths); diff --git a/src/libstore/store.hh b/src/libstore/store.hh new file mode 100644 index 000000000..bc73963f3 --- /dev/null +++ b/src/libstore/store.hh @@ -0,0 +1,9 @@ +#pragma once + +namespace nix { + +template class BasicStore; +class StoreConfig; +typedef BasicStore Store; + +} diff --git a/src/nix/describe-stores.cc b/src/nix/describe-stores.cc new file mode 100644 index 000000000..d84378316 --- /dev/null +++ b/src/nix/describe-stores.cc @@ -0,0 +1,30 @@ +#include "command.hh" +#include "common-args.hh" +#include "shared.hh" +#include "store-api.hh" + +#include + +using namespace nix; + +struct CmdDescribeStores : Command, MixJSON +{ + std::string description() override + { + return "show registered store types and their available options"; + } + + Category category() override { return catUtility; } + + void run() override + { + + if (json) { + auto availableStores = *implementations; + } else { + throw Error("Only json is available for describe-stores"); + } + } +}; + +static auto r1 = registerCommand("describe-stores"); diff --git a/src/nix/develop.cc b/src/nix/develop.cc index a2ce9c8c1..f29fa71d2 100644 --- a/src/nix/develop.cc +++ b/src/nix/develop.cc @@ -392,7 +392,7 @@ struct CmdDevelop : Common, MixEnvironment auto bashInstallable = std::make_shared( state, - std::move(installable->nixpkgsFlakeRef()), + installable->nixpkgsFlakeRef(), Strings{"bashInteractive"}, Strings{"legacyPackages." + settings.thisSystem.get() + "."}, lockFlags); diff --git a/src/nix/diff-closures.cc b/src/nix/diff-closures.cc index 4199dae0f..0dc99d05e 100644 --- a/src/nix/diff-closures.cc +++ b/src/nix/diff-closures.cc @@ -81,7 +81,7 @@ void printClosureDiff( auto beforeSize = totalSize(beforeVersions); auto afterSize = totalSize(afterVersions); auto sizeDelta = (int64_t) afterSize - (int64_t) beforeSize; - auto showDelta = abs(sizeDelta) >= 8 * 1024; + auto showDelta = std::abs(sizeDelta) >= 8 * 1024; std::set removed, unchanged; for (auto & [version, _] : beforeVersions) diff --git a/src/nix/installables.hh b/src/nix/installables.hh index 41c75a4ed..c7c2f8981 100644 --- a/src/nix/installables.hh +++ b/src/nix/installables.hh @@ -69,7 +69,7 @@ struct Installable virtual FlakeRef nixpkgsFlakeRef() const { - return std::move(FlakeRef::fromAttrs({{"type","indirect"}, {"id", "nixpkgs"}})); + return FlakeRef::fromAttrs({{"type","indirect"}, {"id", "nixpkgs"}}); } }; From fa32560169ffa55b9e0b6d5f04c3792acf0c544a Mon Sep 17 00:00:00 2001 From: regnat Date: Wed, 9 Sep 2020 11:18:12 +0200 Subject: [PATCH 26/52] Fix the registration of stores --- src/libstore/dummy-store.cc | 2 +- src/libstore/http-binary-cache-store.cc | 2 +- src/libstore/legacy-ssh-store.cc | 2 +- src/libstore/local-binary-cache-store.cc | 2 +- src/libstore/remote-store.cc | 2 +- src/libstore/s3-binary-cache-store.cc | 2 +- src/libstore/ssh-store.cc | 2 +- src/libstore/store-api.cc | 3 ++- src/libstore/store-api.hh | 29 +++++++++++++++--------- 9 files changed, 27 insertions(+), 19 deletions(-) diff --git a/src/libstore/dummy-store.cc b/src/libstore/dummy-store.cc index a677e201e..dd1880877 100644 --- a/src/libstore/dummy-store.cc +++ b/src/libstore/dummy-store.cc @@ -50,6 +50,6 @@ struct DummyStore : public Store { unsupported("buildDerivation"); } }; -[[maybe_unused]] static RegisterStoreImplementation regStore(); +static RegisterStoreImplementation regStore; } diff --git a/src/libstore/http-binary-cache-store.cc b/src/libstore/http-binary-cache-store.cc index 6c3a16faf..c1abe35cb 100644 --- a/src/libstore/http-binary-cache-store.cc +++ b/src/libstore/http-binary-cache-store.cc @@ -170,6 +170,6 @@ protected: }; -[[maybe_unused]] static RegisterStoreImplementation regStore(); +static RegisterStoreImplementation regStore; } diff --git a/src/libstore/legacy-ssh-store.cc b/src/libstore/legacy-ssh-store.cc index 72f28a82d..552b4b176 100644 --- a/src/libstore/legacy-ssh-store.cc +++ b/src/libstore/legacy-ssh-store.cc @@ -326,6 +326,6 @@ public: } }; -[[maybe_unused]] static RegisterStoreImplementation regStore(); +static RegisterStoreImplementation regStore; } diff --git a/src/libstore/local-binary-cache-store.cc b/src/libstore/local-binary-cache-store.cc index 752838d3f..808a1338f 100644 --- a/src/libstore/local-binary-cache-store.cc +++ b/src/libstore/local-binary-cache-store.cc @@ -96,6 +96,6 @@ std::vector LocalBinaryCacheStore::uriPrefixes() return {"file"}; } -[[maybe_unused]] static RegisterStoreImplementation regStore(); +static RegisterStoreImplementation regStore; } diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 824b6b140..5f455a62a 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -982,6 +982,6 @@ std::exception_ptr RemoteStore::Connection::processStderr(Sink * sink, Source * return nullptr; } -[[maybe_unused]] static RegisterStoreImplementation regStore(); +static RegisterStoreImplementation regStore; } diff --git a/src/libstore/s3-binary-cache-store.cc b/src/libstore/s3-binary-cache-store.cc index 1bf4d5955..f426b43a9 100644 --- a/src/libstore/s3-binary-cache-store.cc +++ b/src/libstore/s3-binary-cache-store.cc @@ -431,7 +431,7 @@ struct S3BinaryCacheStoreImpl : public S3BinaryCacheStore }; -[[maybe_unused]] static RegisterStoreImplementation regStore(); +static RegisterStoreImplementation regStore; } diff --git a/src/libstore/ssh-store.cc b/src/libstore/ssh-store.cc index 02208ee82..8177a95b0 100644 --- a/src/libstore/ssh-store.cc +++ b/src/libstore/ssh-store.cc @@ -76,6 +76,6 @@ ref SSHStore::openConnection() return conn; } -[[maybe_unused]] static RegisterStoreImplementation regStore(); +static RegisterStoreImplementation regStore; } diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 4a46b5160..0f321b434 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -1093,7 +1093,7 @@ ref openStore(const std::string & uri_, return ref(store); } - for (auto implem : *implementations) { + for (auto implem : *Implementations::registered) { auto store = implem.open(uri, params); if (store) { store->warnUnknownSettings(); @@ -1136,5 +1136,6 @@ std::list> getDefaultSubstituters() return stores; } +std::vector * Implementations::registered = 0; } diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 28365542d..61080978e 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -749,25 +749,32 @@ std::list> getDefaultSubstituters(); struct StoreFactory { - std::vector uriPrefixes; std::function (const std::string & uri, const Store::Params & params)> open; }; -typedef std::vector Implementations; -static Implementations * implementations = new Implementations; +struct Implementations +{ + static std::vector * registered; + + template + static void add() + { + if (!registered) registered = new std::vector(); + StoreFactory factory{ + .open = + ([](const std::string & uri, const Store::Params & params) + -> std::shared_ptr + { return std::make_shared(uri, params); }), + }; + registered->push_back(factory); + } +}; template struct RegisterStoreImplementation { RegisterStoreImplementation() { - StoreFactory factory{ - .uriPrefixes = T::uriPrefixes(), - .open = - ([](const std::string & uri, const Store::Params & params) - -> std::shared_ptr - { return std::make_shared(uri, params); }) - }; - implementations->push_back(factory); + Implementations::add(); } }; From 3b57181f8ed94cfa149ad4319ba96d41c5fbc30e Mon Sep 17 00:00:00 2001 From: regnat Date: Wed, 9 Sep 2020 11:29:17 +0200 Subject: [PATCH 27/52] Separate the instantiation and initialisation of the stores MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a new `init()` method to the `Store` class that is supposed to handle all the effectful initialisation needed to set-up the store. The constructor should remain side-effect free and just initialize the c++ data structure. The goal behind that is that we can create “dummy” instances of each store to query static properties about it (the parameters it accepts for example) --- src/libstore/binary-cache-store.hh | 2 +- src/libstore/dummy-store.cc | 4 ++++ src/libstore/http-binary-cache-store.cc | 6 ++++++ src/libstore/legacy-ssh-store.cc | 3 +++ src/libstore/local-binary-cache-store.cc | 6 ++++++ src/libstore/s3-binary-cache-store.cc | 3 +++ src/libstore/ssh-store.cc | 6 ++++++ src/libstore/store-api.cc | 3 ++- src/libstore/store-api.hh | 16 ++++++++++++---- 9 files changed, 43 insertions(+), 6 deletions(-) diff --git a/src/libstore/binary-cache-store.hh b/src/libstore/binary-cache-store.hh index 9bcdf5901..881398ea2 100644 --- a/src/libstore/binary-cache-store.hh +++ b/src/libstore/binary-cache-store.hh @@ -58,7 +58,7 @@ public: public: - virtual void init(); + virtual void init() override; private: diff --git a/src/libstore/dummy-store.cc b/src/libstore/dummy-store.cc index dd1880877..52086445e 100644 --- a/src/libstore/dummy-store.cc +++ b/src/libstore/dummy-store.cc @@ -5,6 +5,10 @@ namespace nix { struct DummyStore : public Store { DummyStore(const std::string uri, const Params & params) + : DummyStore(params) + { } + + DummyStore(const Params & params) : Store(params) { } diff --git a/src/libstore/http-binary-cache-store.cc b/src/libstore/http-binary-cache-store.cc index c1abe35cb..e76bac6e2 100644 --- a/src/libstore/http-binary-cache-store.cc +++ b/src/libstore/http-binary-cache-store.cc @@ -23,6 +23,12 @@ private: public: + HttpBinaryCacheStore( + const Params & params) + : HttpBinaryCacheStore("dummy", params) + { + } + HttpBinaryCacheStore( const Path & _cacheUri, const Params & params) diff --git a/src/libstore/legacy-ssh-store.cc b/src/libstore/legacy-ssh-store.cc index 552b4b176..777ba7520 100644 --- a/src/libstore/legacy-ssh-store.cc +++ b/src/libstore/legacy-ssh-store.cc @@ -37,6 +37,9 @@ struct LegacySSHStore : public Store static std::vector uriPrefixes() { return {"ssh"}; } + LegacySSHStore(const Params & params) + : LegacySSHStore("dummy", params) + {} LegacySSHStore(const string & host, const Params & params) : Store(params) diff --git a/src/libstore/local-binary-cache-store.cc b/src/libstore/local-binary-cache-store.cc index 808a1338f..bcd23eb82 100644 --- a/src/libstore/local-binary-cache-store.cc +++ b/src/libstore/local-binary-cache-store.cc @@ -12,6 +12,12 @@ private: public: + LocalBinaryCacheStore( + const Params & params) + : LocalBinaryCacheStore("dummy", params) + { + } + LocalBinaryCacheStore( const Path & binaryCacheDir, const Params & params) diff --git a/src/libstore/s3-binary-cache-store.cc b/src/libstore/s3-binary-cache-store.cc index f426b43a9..ab27f203f 100644 --- a/src/libstore/s3-binary-cache-store.cc +++ b/src/libstore/s3-binary-cache-store.cc @@ -192,6 +192,9 @@ struct S3BinaryCacheStoreImpl : public S3BinaryCacheStore S3Helper s3Helper; + S3BinaryCacheStoreImpl(const Params & params) + : S3BinaryCacheStoreImpl("dummy-bucket", params) {} + S3BinaryCacheStoreImpl( const std::string & bucketName, const Params & params) diff --git a/src/libstore/ssh-store.cc b/src/libstore/ssh-store.cc index 8177a95b0..85ddce8be 100644 --- a/src/libstore/ssh-store.cc +++ b/src/libstore/ssh-store.cc @@ -17,6 +17,12 @@ public: const Setting remoteProgram{(Store*) this, "nix-daemon", "remote-program", "path to the nix-daemon executable on the remote system"}; const Setting remoteStore{(Store*) this, "", "remote-store", "URI of the store on the remote system"}; + SSHStore( + const Params & params) + : SSHStore("dummy", params) + { + } + SSHStore(const std::string & host, const Params & params) : Store(params) , RemoteStore(params) diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 0f321b434..e14f361bd 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -1094,8 +1094,9 @@ ref openStore(const std::string & uri_, } for (auto implem : *Implementations::registered) { - auto store = implem.open(uri, params); + auto store = implem.create(uri, params); if (store) { + store->init(); store->warnUnknownSettings(); return ref(store); } diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 61080978e..6e081da41 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -200,9 +200,12 @@ protected: Store(const Params & params); - std::shared_ptr getConfig(); - public: + /** + * Perform any necessary effectful operation to make the store up and + * running + */ + virtual void init() {}; virtual ~Store() { } @@ -749,7 +752,8 @@ std::list> getDefaultSubstituters(); struct StoreFactory { - std::function (const std::string & uri, const Store::Params & params)> open; + std::function (const std::string & uri, const Store::Params & params)> create; + std::function (const Store::Params & params)> createDummy; }; struct Implementations { @@ -760,10 +764,14 @@ struct Implementations { if (!registered) registered = new std::vector(); StoreFactory factory{ - .open = + .create = ([](const std::string & uri, const Store::Params & params) -> std::shared_ptr { return std::make_shared(uri, params); }), + .createDummy = + ([](const Store::Params & params) + -> std::shared_ptr + { return std::make_shared(params); }) }; registered->push_back(factory); } From 3c525d15903ea98e5401ff57061295b8c625cc45 Mon Sep 17 00:00:00 2001 From: regnat Date: Wed, 9 Sep 2020 11:35:33 +0200 Subject: [PATCH 28/52] Complete the `toJSON` instance for `Setting` Don't let it just contain the value, but also the other fields of the setting (description, aliases, etc..) --- src/libstore/globals.cc | 5 ----- src/libutil/config.cc | 32 ++++++++++---------------------- src/libutil/config.hh | 11 +++++++++-- 3 files changed, 19 insertions(+), 29 deletions(-) diff --git a/src/libstore/globals.cc b/src/libstore/globals.cc index 4a5971c3f..491c664db 100644 --- a/src/libstore/globals.cc +++ b/src/libstore/globals.cc @@ -162,11 +162,6 @@ template<> std::string BaseSetting::to_string() const else abort(); } -template<> nlohmann::json BaseSetting::toJSON() -{ - return AbstractSetting::toJSON(); -} - template<> void BaseSetting::convertToArg(Args & args, const std::string & category) { args.addFlag({ diff --git a/src/libutil/config.cc b/src/libutil/config.cc index 3cf720bce..faa5cdbeb 100644 --- a/src/libutil/config.cc +++ b/src/libutil/config.cc @@ -137,11 +137,7 @@ nlohmann::json Config::toJSON() auto res = nlohmann::json::object(); for (auto & s : _settings) if (!s.second.isAlias) { - auto obj = nlohmann::json::object(); - obj.emplace("description", s.second.setting->description); - obj.emplace("aliases", s.second.setting->aliases); - obj.emplace("value", s.second.setting->toJSON()); - res.emplace(s.first, obj); + res.emplace(s.first, s.second.setting->toJSON()); } return res; } @@ -168,19 +164,21 @@ void AbstractSetting::setDefault(const std::string & str) nlohmann::json AbstractSetting::toJSON() { - return to_string(); + return nlohmann::json(toJSONObject()); +} + +std::map AbstractSetting::toJSONObject() +{ + std::map obj; + obj.emplace("description", description); + obj.emplace("aliases", aliases); + return obj; } void AbstractSetting::convertToArg(Args & args, const std::string & category) { } -template -nlohmann::json BaseSetting::toJSON() -{ - return value; -} - template void BaseSetting::convertToArg(Args & args, const std::string & category) { @@ -259,11 +257,6 @@ template<> std::string BaseSetting::to_string() const return concatStringsSep(" ", value); } -template<> nlohmann::json BaseSetting::toJSON() -{ - return value; -} - template<> void BaseSetting::set(const std::string & str) { value = tokenizeString(str); @@ -274,11 +267,6 @@ template<> std::string BaseSetting::to_string() const return concatStringsSep(" ", value); } -template<> nlohmann::json BaseSetting::toJSON() -{ - return value; -} - template class BaseSetting; template class BaseSetting; template class BaseSetting; diff --git a/src/libutil/config.hh b/src/libutil/config.hh index 2b4265806..be23076b0 100644 --- a/src/libutil/config.hh +++ b/src/libutil/config.hh @@ -206,7 +206,9 @@ protected: virtual std::string to_string() const = 0; - virtual nlohmann::json toJSON(); + nlohmann::json toJSON(); + + virtual std::map toJSONObject(); virtual void convertToArg(Args & args, const std::string & category); @@ -251,7 +253,12 @@ public: void convertToArg(Args & args, const std::string & category) override; - nlohmann::json toJSON() override; + std::map toJSONObject() override + { + auto obj = AbstractSetting::toJSONObject(); + obj.emplace("value", value); + return obj; + } }; template From 35042c96230e922b88c8a5cad6a7f5f06792860f Mon Sep 17 00:00:00 2001 From: regnat Date: Wed, 9 Sep 2020 11:36:48 +0200 Subject: [PATCH 29/52] Add a default value for the settings The default value is initialized when creating the setting and unchanged after that --- src/libutil/config.hh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/libutil/config.hh b/src/libutil/config.hh index be23076b0..42fc856e0 100644 --- a/src/libutil/config.hh +++ b/src/libutil/config.hh @@ -222,6 +222,7 @@ class BaseSetting : public AbstractSetting protected: T value; + const T defaultValue; public: @@ -231,6 +232,7 @@ public: const std::set & aliases = {}) : AbstractSetting(name, description, aliases) , value(def) + , defaultValue(def) { } operator const T &() const { return value; } @@ -257,6 +259,7 @@ public: { auto obj = AbstractSetting::toJSONObject(); obj.emplace("value", value); + obj.emplace("defaultValue", defaultValue); return obj; } }; From aa4eac37881258797c241113a5602913e1a5371a Mon Sep 17 00:00:00 2001 From: regnat Date: Wed, 9 Sep 2020 13:25:32 +0200 Subject: [PATCH 30/52] fixup! Separate the instantiation and initialisation of the stores --- src/libstore/store-api.cc | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index e14f361bd..831d4f30f 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -1009,12 +1009,6 @@ Derivation Store::readDerivation(const StorePath & drvPath) } } -std::shared_ptr Store::getConfig() -{ - return shared_from_this(); -} - - } From 22afa8fb4dd7e459faf531dd8a9c3580d02c7e2a Mon Sep 17 00:00:00 2001 From: regnat Date: Thu, 10 Sep 2020 10:55:51 +0200 Subject: [PATCH 31/52] Separate store configs from the implems Rework the `Store` hierarchy so that there's now one hierarchy for the store configs and one for the implementations (where each implementation extends the corresponding config). So a class hierarchy like ``` StoreConfig-------->Store | | v v SubStoreConfig----->SubStore | | v v SubSubStoreConfig-->SubSubStore ``` (with virtual inheritance to prevent DDD). The advantage of this architecture is that we can now introspect the configuration of a store without having to instantiate the store itself --- src/libstore/binary-cache-store.cc | 3 +- src/libstore/binary-cache-store.hh | 8 +++- src/libstore/dummy-store.cc | 4 +- src/libstore/http-binary-cache-store.cc | 16 ++++---- src/libstore/legacy-ssh-store.cc | 17 ++++---- src/libstore/local-binary-cache-store.cc | 18 ++++----- src/libstore/local-store.hh | 2 +- src/libstore/remote-store.cc | 10 ++--- src/libstore/remote-store.hh | 34 ++++++++++++---- src/libstore/s3-binary-cache-store.cc | 14 ++++--- src/libstore/ssh-store.cc | 25 ++++++------ src/libstore/store-api.cc | 2 +- src/libstore/store-api.hh | 50 ++++++++++++++---------- 13 files changed, 120 insertions(+), 83 deletions(-) diff --git a/src/libstore/binary-cache-store.cc b/src/libstore/binary-cache-store.cc index 5433fe50d..34f844a18 100644 --- a/src/libstore/binary-cache-store.cc +++ b/src/libstore/binary-cache-store.cc @@ -22,7 +22,8 @@ namespace nix { BinaryCacheStore::BinaryCacheStore(const Params & params) - : Store(params) + : BinaryCacheStoreConfig(params) + , Store(params) { if (secretKeyFile != "") secretKey = std::unique_ptr(new SecretKey(readFile(secretKeyFile))); diff --git a/src/libstore/binary-cache-store.hh b/src/libstore/binary-cache-store.hh index 881398ea2..f1fc82013 100644 --- a/src/libstore/binary-cache-store.hh +++ b/src/libstore/binary-cache-store.hh @@ -11,9 +11,9 @@ namespace nix { struct NarInfo; -class BinaryCacheStore : public Store +struct BinaryCacheStoreConfig : virtual StoreConfig { -public: + using StoreConfig::StoreConfig; const Setting compression{this, "xz", "compression", "NAR compression method ('xz', 'bzip2', or 'none')"}; const Setting writeNARListing{this, false, "write-nar-listing", "whether to write a JSON file listing the files in each NAR"}; @@ -22,6 +22,10 @@ public: const Setting localNarCache{this, "", "local-nar-cache", "path to a local cache of NARs"}; const Setting parallelCompression{this, false, "parallel-compression", "enable multi-threading compression, available for xz only currently"}; +}; + +class BinaryCacheStore : public Store, public virtual BinaryCacheStoreConfig +{ private: diff --git a/src/libstore/dummy-store.cc b/src/libstore/dummy-store.cc index 52086445e..d4560c13d 100644 --- a/src/libstore/dummy-store.cc +++ b/src/libstore/dummy-store.cc @@ -2,6 +2,8 @@ namespace nix { +struct DummyStoreConfig : StoreConfig {}; + struct DummyStore : public Store { DummyStore(const std::string uri, const Params & params) @@ -54,6 +56,6 @@ struct DummyStore : public Store { unsupported("buildDerivation"); } }; -static RegisterStoreImplementation regStore; +static RegisterStoreImplementation regStore; } diff --git a/src/libstore/http-binary-cache-store.cc b/src/libstore/http-binary-cache-store.cc index e76bac6e2..c3fb455fa 100644 --- a/src/libstore/http-binary-cache-store.cc +++ b/src/libstore/http-binary-cache-store.cc @@ -7,7 +7,12 @@ namespace nix { MakeError(UploadToHTTP, Error); -class HttpBinaryCacheStore : public BinaryCacheStore +struct HttpBinaryCacheStoreConfig : virtual BinaryCacheStoreConfig +{ + using BinaryCacheStoreConfig::BinaryCacheStoreConfig; +}; + +class HttpBinaryCacheStore : public BinaryCacheStore, public HttpBinaryCacheStoreConfig { private: @@ -23,16 +28,11 @@ private: public: - HttpBinaryCacheStore( - const Params & params) - : HttpBinaryCacheStore("dummy", params) - { - } - HttpBinaryCacheStore( const Path & _cacheUri, const Params & params) : BinaryCacheStore(params) + , HttpBinaryCacheStoreConfig(params) , cacheUri(_cacheUri) { if (cacheUri.back() == '/') @@ -176,6 +176,6 @@ protected: }; -static RegisterStoreImplementation regStore; +static RegisterStoreImplementation regStore; } diff --git a/src/libstore/legacy-ssh-store.cc b/src/libstore/legacy-ssh-store.cc index 777ba7520..5a08fbba3 100644 --- a/src/libstore/legacy-ssh-store.cc +++ b/src/libstore/legacy-ssh-store.cc @@ -9,15 +9,21 @@ namespace nix { -struct LegacySSHStore : public Store +struct LegacySSHStoreConfig : virtual StoreConfig { + using StoreConfig::StoreConfig; const Setting maxConnections{this, 1, "max-connections", "maximum number of concurrent SSH connections"}; const Setting sshKey{this, "", "ssh-key", "path to an SSH private key"}; const Setting compress{this, false, "compress", "whether to compress the connection"}; const Setting remoteProgram{this, "nix-store", "remote-program", "path to the nix-store executable on the remote system"}; const Setting remoteStore{this, "", "remote-store", "URI of the store on the remote system"}; +}; +struct LegacySSHStore : public Store, public virtual LegacySSHStoreConfig +{ // Hack for getting remote build log output. + // Intentionally not in `StoreConfig` so that it doesn't appear in the + // documentation const Setting logFD{this, -1, "log-fd", "file descriptor to which SSH's stderr is connected"}; struct Connection @@ -37,12 +43,9 @@ struct LegacySSHStore : public Store static std::vector uriPrefixes() { return {"ssh"}; } - LegacySSHStore(const Params & params) - : LegacySSHStore("dummy", params) - {} - LegacySSHStore(const string & host, const Params & params) - : Store(params) + : LegacySSHStoreConfig(params) + , Store(params) , host(host) , connections(make_ref>( std::max(1, (int) maxConnections), @@ -329,6 +332,6 @@ public: } }; -static RegisterStoreImplementation regStore; +static RegisterStoreImplementation regStore; } diff --git a/src/libstore/local-binary-cache-store.cc b/src/libstore/local-binary-cache-store.cc index bcd23eb82..fc3ae94c2 100644 --- a/src/libstore/local-binary-cache-store.cc +++ b/src/libstore/local-binary-cache-store.cc @@ -4,7 +4,12 @@ namespace nix { -class LocalBinaryCacheStore : public BinaryCacheStore +struct LocalBinaryCacheStoreConfig : virtual BinaryCacheStoreConfig +{ + using BinaryCacheStoreConfig::BinaryCacheStoreConfig; +}; + +class LocalBinaryCacheStore : public BinaryCacheStore, public virtual LocalBinaryCacheStoreConfig { private: @@ -12,16 +17,11 @@ private: public: - LocalBinaryCacheStore( - const Params & params) - : LocalBinaryCacheStore("dummy", params) - { - } - LocalBinaryCacheStore( const Path & binaryCacheDir, const Params & params) - : BinaryCacheStore(params) + : LocalBinaryCacheStoreConfig(params) + , BinaryCacheStore(params) , binaryCacheDir(binaryCacheDir) { } @@ -102,6 +102,6 @@ std::vector LocalBinaryCacheStore::uriPrefixes() return {"file"}; } -static RegisterStoreImplementation regStore; +static RegisterStoreImplementation regStore; } diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index d5e6d68ef..8b6972847 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -95,7 +95,7 @@ public: private: - Setting requireSigs{(Store*) this, + Setting requireSigs{(StoreConfig*) this, settings.requireSigs, "require-sigs", "whether store paths should have a trusted signature on import"}; diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 5f455a62a..619e9a7f1 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -94,6 +94,7 @@ void write(const Store & store, Sink & out, const std::optional & sto /* TODO: Separate these store impls into different files, give them better names */ RemoteStore::RemoteStore(const Params & params) : Store(params) + , RemoteStoreConfig(params) , connections(make_ref>( std::max(1, (int) maxConnections), [this]() { return openConnectionWrapper(); }, @@ -124,6 +125,7 @@ ref RemoteStore::openConnectionWrapper() UDSRemoteStore::UDSRemoteStore(const Params & params) : Store(params) + , UDSRemoteStoreConfig(params) , LocalFSStore(params) , RemoteStore(params) { @@ -131,11 +133,9 @@ UDSRemoteStore::UDSRemoteStore(const Params & params) UDSRemoteStore::UDSRemoteStore(std::string socket_path, const Params & params) - : Store(params) - , LocalFSStore(params) - , RemoteStore(params) - , path(socket_path) + : UDSRemoteStore(params) { + path.emplace(socket_path); } @@ -982,6 +982,6 @@ std::exception_ptr RemoteStore::Connection::processStderr(Sink * sink, Source * return nullptr; } -static RegisterStoreImplementation regStore; +static RegisterStoreImplementation regStore; } diff --git a/src/libstore/remote-store.hh b/src/libstore/remote-store.hh index 07cd5daf8..ba2f69427 100644 --- a/src/libstore/remote-store.hh +++ b/src/libstore/remote-store.hh @@ -16,19 +16,23 @@ struct FdSource; template class Pool; struct ConnectionHandle; +struct RemoteStoreConfig : virtual StoreConfig +{ + using StoreConfig::StoreConfig; + + const Setting maxConnections{(StoreConfig*) this, 1, + "max-connections", "maximum number of concurrent connections to the Nix daemon"}; + + const Setting maxConnectionAge{(StoreConfig*) this, std::numeric_limits::max(), + "max-connection-age", "number of seconds to reuse a connection"}; +}; /* FIXME: RemoteStore is a misnomer - should be something like DaemonStore. */ -class RemoteStore : public virtual Store +class RemoteStore : public virtual Store, public virtual RemoteStoreConfig { public: - const Setting maxConnections{(Store*) this, 1, - "max-connections", "maximum number of concurrent connections to the Nix daemon"}; - - const Setting maxConnectionAge{(Store*) this, std::numeric_limits::max(), - "max-connection-age", "number of seconds to reuse a connection"}; - virtual bool sameMachine() = 0; RemoteStore(const Params & params); @@ -141,7 +145,21 @@ private: }; -class UDSRemoteStore : public LocalFSStore, public RemoteStore +struct UDSRemoteStoreConfig : virtual LocalFSStoreConfig, virtual RemoteStoreConfig +{ + UDSRemoteStoreConfig(const Store::Params & params) + : LocalFSStoreConfig(params) + , RemoteStoreConfig(params) + { + } + + UDSRemoteStoreConfig() + : UDSRemoteStoreConfig(Store::Params({})) + { + } +}; + +class UDSRemoteStore : public LocalFSStore, public RemoteStore, public virtual UDSRemoteStoreConfig { public: diff --git a/src/libstore/s3-binary-cache-store.cc b/src/libstore/s3-binary-cache-store.cc index ab27f203f..b75d05555 100644 --- a/src/libstore/s3-binary-cache-store.cc +++ b/src/libstore/s3-binary-cache-store.cc @@ -172,8 +172,9 @@ S3Helper::FileTransferResult S3Helper::getObject( return res; } -struct S3BinaryCacheStoreImpl : public S3BinaryCacheStore +struct S3BinaryCacheStoreConfig : virtual BinaryCacheStoreConfig { + using BinaryCacheStoreConfig::BinaryCacheStoreConfig; const Setting profile{this, "", "profile", "The name of the AWS configuration profile to use."}; const Setting region{this, Aws::Region::US_EAST_1, "region", {"aws-region"}}; const Setting scheme{this, "", "scheme", "The scheme to use for S3 requests, https by default."}; @@ -185,20 +186,21 @@ struct S3BinaryCacheStoreImpl : public S3BinaryCacheStore this, false, "multipart-upload", "whether to use multi-part uploads"}; const Setting bufferSize{ this, 5 * 1024 * 1024, "buffer-size", "size (in bytes) of each part in multi-part uploads"}; +}; +struct S3BinaryCacheStoreImpl : public S3BinaryCacheStore, virtual S3BinaryCacheStoreConfig +{ std::string bucketName; Stats stats; S3Helper s3Helper; - S3BinaryCacheStoreImpl(const Params & params) - : S3BinaryCacheStoreImpl("dummy-bucket", params) {} - S3BinaryCacheStoreImpl( const std::string & bucketName, const Params & params) - : S3BinaryCacheStore(params) + : S3BinaryCacheStoreConfig(params) + , S3BinaryCacheStore(params) , bucketName(bucketName) , s3Helper(profile, region, scheme, endpoint) { @@ -434,7 +436,7 @@ struct S3BinaryCacheStoreImpl : public S3BinaryCacheStore }; -static RegisterStoreImplementation regStore; +static RegisterStoreImplementation regStore; } diff --git a/src/libstore/ssh-store.cc b/src/libstore/ssh-store.cc index 85ddce8be..2b6e2a298 100644 --- a/src/libstore/ssh-store.cc +++ b/src/libstore/ssh-store.cc @@ -8,23 +8,22 @@ namespace nix { -class SSHStore : public RemoteStore +struct SSHStoreConfig : virtual RemoteStoreConfig +{ + using RemoteStoreConfig::RemoteStoreConfig; + const Setting sshKey{(StoreConfig*) this, "", "ssh-key", "path to an SSH private key"}; + const Setting compress{(StoreConfig*) this, false, "compress", "whether to compress the connection"}; + const Setting remoteProgram{(StoreConfig*) this, "nix-daemon", "remote-program", "path to the nix-daemon executable on the remote system"}; + const Setting remoteStore{(StoreConfig*) this, "", "remote-store", "URI of the store on the remote system"}; +}; + +class SSHStore : public RemoteStore, public virtual SSHStoreConfig { public: - const Setting sshKey{(Store*) this, "", "ssh-key", "path to an SSH private key"}; - const Setting compress{(Store*) this, false, "compress", "whether to compress the connection"}; - const Setting remoteProgram{(Store*) this, "nix-daemon", "remote-program", "path to the nix-daemon executable on the remote system"}; - const Setting remoteStore{(Store*) this, "", "remote-store", "URI of the store on the remote system"}; - - SSHStore( - const Params & params) - : SSHStore("dummy", params) - { - } - SSHStore(const std::string & host, const Params & params) : Store(params) + , RemoteStoreConfig(params) , RemoteStore(params) , host(host) , master( @@ -82,6 +81,6 @@ ref SSHStore::openConnection() return conn; } -static RegisterStoreImplementation regStore; +static RegisterStoreImplementation regStore; } diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 831d4f30f..fe3f5221c 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -346,7 +346,7 @@ ValidPathInfo Store::addToStoreSlow(std::string_view name, const Path & srcPath, Store::Store(const Params & params) - : Config(params) + : StoreConfig(params) , state({(size_t) pathInfoCacheSize}) { } diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 6e081da41..61552f74e 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -145,12 +145,9 @@ struct BuildResult } }; - -class Store : public std::enable_shared_from_this, public Config +struct StoreConfig : public Config { -public: - - typedef std::map Params; + using Config::Config; const PathSetting storeDir_{this, false, settings.nixStore, "store", "path to the Nix store"}; @@ -168,6 +165,14 @@ public: "system-features", "Optional features that the system this store builds on implements (like \"kvm\")."}; +}; + +class Store : public std::enable_shared_from_this, public virtual StoreConfig +{ +public: + + typedef std::map Params; + protected: struct PathInfoCacheValue { @@ -632,22 +637,25 @@ protected: }; - -class LocalFSStore : public virtual Store +struct LocalFSStoreConfig : virtual StoreConfig { -public: - - // FIXME: the (Store*) cast works around a bug in gcc that causes + using StoreConfig::StoreConfig; + // FIXME: the (StoreConfig*) cast works around a bug in gcc that causes // it to omit the call to the Setting constructor. Clang works fine // either way. - const PathSetting rootDir{(Store*) this, true, "", + const PathSetting rootDir{(StoreConfig*) this, true, "", "root", "directory prefixed to all other paths"}; - const PathSetting stateDir{(Store*) this, false, + const PathSetting stateDir{(StoreConfig*) this, false, rootDir != "" ? rootDir + "/nix/var/nix" : settings.nixStateDir, "state", "directory where Nix will store state"}; - const PathSetting logDir{(Store*) this, false, + const PathSetting logDir{(StoreConfig*) this, false, rootDir != "" ? rootDir + "/nix/var/log/nix" : settings.nixLogDir, "log", "directory where Nix will store state"}; +}; + +class LocalFSStore : public virtual Store, public LocalFSStoreConfig +{ +public: const static string drvsLogDir; @@ -753,13 +761,13 @@ std::list> getDefaultSubstituters(); struct StoreFactory { std::function (const std::string & uri, const Store::Params & params)> create; - std::function (const Store::Params & params)> createDummy; + std::function ()> getConfig; }; struct Implementations { static std::vector * registered; - template + template static void add() { if (!registered) registered = new std::vector(); @@ -768,21 +776,21 @@ struct Implementations ([](const std::string & uri, const Store::Params & params) -> std::shared_ptr { return std::make_shared(uri, params); }), - .createDummy = - ([](const Store::Params & params) - -> std::shared_ptr - { return std::make_shared(params); }) + .getConfig = + ([]() + -> std::shared_ptr + { return std::make_shared(); }) }; registered->push_back(factory); } }; -template +template struct RegisterStoreImplementation { RegisterStoreImplementation() { - Implementations::add(); + Implementations::add(); } }; From dae39f0a7abfc41c122f4f2d54d6cf9e26cafe7a Mon Sep 17 00:00:00 2001 From: regnat Date: Thu, 10 Sep 2020 11:03:36 +0200 Subject: [PATCH 32/52] Make `nix describe-stores` functional Using the `*Config` class hierarchy --- src/nix/describe-stores.cc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/nix/describe-stores.cc b/src/nix/describe-stores.cc index d84378316..f019ec9c2 100644 --- a/src/nix/describe-stores.cc +++ b/src/nix/describe-stores.cc @@ -20,7 +20,11 @@ struct CmdDescribeStores : Command, MixJSON { if (json) { - auto availableStores = *implementations; + auto res = nlohmann::json::array(); + for (auto & implem : *Implementations::registered) { + auto storeConfig = implem.getConfig(); + std::cout << storeConfig->toJSON().dump() << std::endl; + } } else { throw Error("Only json is available for describe-stores"); } From d184ad1d276006d4d003046710a56a019034a6a1 Mon Sep 17 00:00:00 2001 From: regnat Date: Fri, 11 Sep 2020 11:02:38 +0200 Subject: [PATCH 33/52] fixup! Make the store plugins more introspectable --- result | 1 - 1 file changed, 1 deletion(-) delete mode 120000 result diff --git a/result b/result deleted file mode 120000 index af7bbb6da..000000000 --- a/result +++ /dev/null @@ -1 +0,0 @@ -/nix/store/9pqfirjppd91mzhkgh8xnn66iwh53zk2-hello-2.10 \ No newline at end of file From 5895184df44e86ae55270390402c8263b0f24ae2 Mon Sep 17 00:00:00 2001 From: regnat Date: Fri, 11 Sep 2020 11:06:18 +0200 Subject: [PATCH 34/52] Correctly call all the parent contructors of the stores Using virtual inheritance means that only the default constructors of the parent classes will be called, which isn't what we want --- src/libstore/build.cc | 2 +- src/libstore/dummy-store.cc | 13 +++++++++---- src/libstore/http-binary-cache-store.cc | 4 +++- src/libstore/legacy-ssh-store.cc | 3 ++- src/libstore/local-binary-cache-store.cc | 4 +++- src/libstore/local-store.cc | 5 ++++- src/libstore/local-store.hh | 15 ++++++++++----- src/libstore/remote-store.cc | 5 ++++- src/libstore/remote-store.hh | 3 ++- src/libstore/s3-binary-cache-store.cc | 4 +++- src/libstore/ssh-store.cc | 7 +++++-- src/libstore/store-api.hh | 2 +- 12 files changed, 47 insertions(+), 20 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index ce973862d..4040a3fac 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -2883,7 +2883,7 @@ struct RestrictedStore : public LocalFSStore DerivationGoal & goal; RestrictedStore(const Params & params, ref next, DerivationGoal & goal) - : Store(params), LocalFSStore(params), next(next), goal(goal) + : StoreConfig(params), LocalFSStoreConfig(params), Store(params), LocalFSStore(params), next(next), goal(goal) { } Path getRealStoreDir() override diff --git a/src/libstore/dummy-store.cc b/src/libstore/dummy-store.cc index d4560c13d..68d03d934 100644 --- a/src/libstore/dummy-store.cc +++ b/src/libstore/dummy-store.cc @@ -2,17 +2,22 @@ namespace nix { -struct DummyStoreConfig : StoreConfig {}; +struct DummyStoreConfig : StoreConfig { + using StoreConfig::StoreConfig; +}; -struct DummyStore : public Store +struct DummyStore : public Store, public virtual DummyStoreConfig { DummyStore(const std::string uri, const Params & params) : DummyStore(params) { } DummyStore(const Params & params) - : Store(params) - { } + : StoreConfig(params) + , DummyStoreConfig(params) + , Store(params) + { + } string getUri() override { diff --git a/src/libstore/http-binary-cache-store.cc b/src/libstore/http-binary-cache-store.cc index c3fb455fa..2c9d61d51 100644 --- a/src/libstore/http-binary-cache-store.cc +++ b/src/libstore/http-binary-cache-store.cc @@ -31,7 +31,9 @@ public: HttpBinaryCacheStore( const Path & _cacheUri, const Params & params) - : BinaryCacheStore(params) + : StoreConfig(params) + , BinaryCacheStoreConfig(params) + , BinaryCacheStore(params) , HttpBinaryCacheStoreConfig(params) , cacheUri(_cacheUri) { diff --git a/src/libstore/legacy-ssh-store.cc b/src/libstore/legacy-ssh-store.cc index 5a08fbba3..bbab452ab 100644 --- a/src/libstore/legacy-ssh-store.cc +++ b/src/libstore/legacy-ssh-store.cc @@ -44,7 +44,8 @@ struct LegacySSHStore : public Store, public virtual LegacySSHStoreConfig static std::vector uriPrefixes() { return {"ssh"}; } LegacySSHStore(const string & host, const Params & params) - : LegacySSHStoreConfig(params) + : StoreConfig(params) + , LegacySSHStoreConfig(params) , Store(params) , host(host) , connections(make_ref>( diff --git a/src/libstore/local-binary-cache-store.cc b/src/libstore/local-binary-cache-store.cc index fc3ae94c2..becd74c75 100644 --- a/src/libstore/local-binary-cache-store.cc +++ b/src/libstore/local-binary-cache-store.cc @@ -20,7 +20,9 @@ public: LocalBinaryCacheStore( const Path & binaryCacheDir, const Params & params) - : LocalBinaryCacheStoreConfig(params) + : StoreConfig(params) + , BinaryCacheStoreConfig(params) + , LocalBinaryCacheStoreConfig(params) , BinaryCacheStore(params) , binaryCacheDir(binaryCacheDir) { diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 0086bb13e..4e9840f3b 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -42,7 +42,10 @@ namespace nix { LocalStore::LocalStore(const Params & params) - : Store(params) + : StoreConfig(params) + , Store(params) + , LocalFSStoreConfig(params) + , LocalStoreConfig(params) , LocalFSStore(params) , realStoreDir_{this, false, rootDir != "" ? rootDir + "/nix/store" : storeDir, "real", "physical path to the Nix store"} diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 8b6972847..0265f0837 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -30,8 +30,17 @@ struct OptimiseStats uint64_t blocksFreed = 0; }; +struct LocalStoreConfig : virtual LocalFSStoreConfig +{ + using LocalFSStoreConfig::LocalFSStoreConfig; -class LocalStore : public LocalFSStore + Setting requireSigs{(StoreConfig*) this, + settings.requireSigs, + "require-sigs", "whether store paths should have a trusted signature on import"}; +}; + + +class LocalStore : public LocalFSStore, public virtual LocalStoreConfig { private: @@ -95,10 +104,6 @@ public: private: - Setting requireSigs{(StoreConfig*) this, - settings.requireSigs, - "require-sigs", "whether store paths should have a trusted signature on import"}; - const PublicKeys & getPublicKeys(); public: diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 619e9a7f1..9bbb69b99 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -124,7 +124,10 @@ ref RemoteStore::openConnectionWrapper() UDSRemoteStore::UDSRemoteStore(const Params & params) - : Store(params) + : StoreConfig(params) + , Store(params) + , LocalFSStoreConfig(params) + , RemoteStoreConfig(params) , UDSRemoteStoreConfig(params) , LocalFSStore(params) , RemoteStore(params) diff --git a/src/libstore/remote-store.hh b/src/libstore/remote-store.hh index ba2f69427..32daab59a 100644 --- a/src/libstore/remote-store.hh +++ b/src/libstore/remote-store.hh @@ -148,7 +148,8 @@ private: struct UDSRemoteStoreConfig : virtual LocalFSStoreConfig, virtual RemoteStoreConfig { UDSRemoteStoreConfig(const Store::Params & params) - : LocalFSStoreConfig(params) + : StoreConfig(params) + , LocalFSStoreConfig(params) , RemoteStoreConfig(params) { } diff --git a/src/libstore/s3-binary-cache-store.cc b/src/libstore/s3-binary-cache-store.cc index b75d05555..30ef4082a 100644 --- a/src/libstore/s3-binary-cache-store.cc +++ b/src/libstore/s3-binary-cache-store.cc @@ -199,7 +199,9 @@ struct S3BinaryCacheStoreImpl : public S3BinaryCacheStore, virtual S3BinaryCache S3BinaryCacheStoreImpl( const std::string & bucketName, const Params & params) - : S3BinaryCacheStoreConfig(params) + : StoreConfig(params) + , BinaryCacheStoreConfig(params) + , S3BinaryCacheStoreConfig(params) , S3BinaryCacheStore(params) , bucketName(bucketName) , s3Helper(profile, region, scheme, endpoint) diff --git a/src/libstore/ssh-store.cc b/src/libstore/ssh-store.cc index 2b6e2a298..da201a9c3 100644 --- a/src/libstore/ssh-store.cc +++ b/src/libstore/ssh-store.cc @@ -11,20 +11,23 @@ namespace nix { struct SSHStoreConfig : virtual RemoteStoreConfig { using RemoteStoreConfig::RemoteStoreConfig; + const Setting sshKey{(StoreConfig*) this, "", "ssh-key", "path to an SSH private key"}; const Setting compress{(StoreConfig*) this, false, "compress", "whether to compress the connection"}; const Setting remoteProgram{(StoreConfig*) this, "nix-daemon", "remote-program", "path to the nix-daemon executable on the remote system"}; const Setting remoteStore{(StoreConfig*) this, "", "remote-store", "URI of the store on the remote system"}; }; -class SSHStore : public RemoteStore, public virtual SSHStoreConfig +class SSHStore : public virtual RemoteStore, public virtual SSHStoreConfig { public: SSHStore(const std::string & host, const Params & params) - : Store(params) + : StoreConfig(params) + , Store(params) , RemoteStoreConfig(params) , RemoteStore(params) + , SSHStoreConfig(params) , host(host) , master( host, diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 61552f74e..8e9cb786c 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -653,7 +653,7 @@ struct LocalFSStoreConfig : virtual StoreConfig "log", "directory where Nix will store state"}; }; -class LocalFSStore : public virtual Store, public LocalFSStoreConfig +class LocalFSStore : public virtual Store, public virtual LocalFSStoreConfig { public: From 7f103dcddd4cf3c0748b7a379a117f3262c4c5f7 Mon Sep 17 00:00:00 2001 From: regnat Date: Fri, 11 Sep 2020 11:11:05 +0200 Subject: [PATCH 35/52] Properly filter the stores according to their declared uriSchemes When opening a store, only try the stores whose `uriSchemes()` include the current one --- src/libstore/dummy-store.cc | 6 ++--- src/libstore/http-binary-cache-store.cc | 9 +++---- src/libstore/legacy-ssh-store.cc | 6 ++--- src/libstore/local-binary-cache-store.cc | 5 ++-- src/libstore/remote-store.cc | 5 +++- src/libstore/remote-store.hh | 4 ++-- src/libstore/s3-binary-cache-store.cc | 3 ++- src/libstore/ssh-store.cc | 6 ++--- src/libstore/store-api.cc | 30 ++++++++++++++++-------- src/libstore/store-api.hh | 10 ++++---- 10 files changed, 51 insertions(+), 33 deletions(-) diff --git a/src/libstore/dummy-store.cc b/src/libstore/dummy-store.cc index 68d03d934..210e5b11c 100644 --- a/src/libstore/dummy-store.cc +++ b/src/libstore/dummy-store.cc @@ -8,7 +8,7 @@ struct DummyStoreConfig : StoreConfig { struct DummyStore : public Store, public virtual DummyStoreConfig { - DummyStore(const std::string uri, const Params & params) + DummyStore(const std::string scheme, const std::string uri, const Params & params) : DummyStore(params) { } @@ -21,7 +21,7 @@ struct DummyStore : public Store, public virtual DummyStoreConfig string getUri() override { - return uriPrefixes()[0] + "://"; + return *uriSchemes().begin(); } void queryPathInfoUncached(const StorePath & path, @@ -30,7 +30,7 @@ struct DummyStore : public Store, public virtual DummyStoreConfig callback(nullptr); } - static std::vector uriPrefixes() { + static std::set uriSchemes() { return {"dummy"}; } diff --git a/src/libstore/http-binary-cache-store.cc b/src/libstore/http-binary-cache-store.cc index 2c9d61d51..095eaa82c 100644 --- a/src/libstore/http-binary-cache-store.cc +++ b/src/libstore/http-binary-cache-store.cc @@ -29,13 +29,14 @@ private: public: HttpBinaryCacheStore( + const std::string & scheme, const Path & _cacheUri, const Params & params) : StoreConfig(params) , BinaryCacheStoreConfig(params) , BinaryCacheStore(params) , HttpBinaryCacheStoreConfig(params) - , cacheUri(_cacheUri) + , cacheUri(scheme + "://" + _cacheUri) { if (cacheUri.back() == '/') cacheUri.pop_back(); @@ -64,11 +65,11 @@ public: } } - static std::vector uriPrefixes() + static std::set uriSchemes() { static bool forceHttp = getEnv("_NIX_FORCE_HTTP") == "1"; - auto ret = std::vector({"http", "https"}); - if (forceHttp) ret.push_back("file"); + auto ret = std::set({"http", "https"}); + if (forceHttp) ret.insert("file"); return ret; } protected: diff --git a/src/libstore/legacy-ssh-store.cc b/src/libstore/legacy-ssh-store.cc index bbab452ab..6dc83e288 100644 --- a/src/libstore/legacy-ssh-store.cc +++ b/src/libstore/legacy-ssh-store.cc @@ -41,9 +41,9 @@ struct LegacySSHStore : public Store, public virtual LegacySSHStoreConfig SSHMaster master; - static std::vector uriPrefixes() { return {"ssh"}; } + static std::set uriSchemes() { return {"ssh"}; } - LegacySSHStore(const string & host, const Params & params) + LegacySSHStore(const string & scheme, const string & host, const Params & params) : StoreConfig(params) , LegacySSHStoreConfig(params) , Store(params) @@ -92,7 +92,7 @@ struct LegacySSHStore : public Store, public virtual LegacySSHStoreConfig string getUri() override { - return uriPrefixes()[0] + "://" + host; + return *uriSchemes().begin() + "://" + host; } void queryPathInfoUncached(const StorePath & path, diff --git a/src/libstore/local-binary-cache-store.cc b/src/libstore/local-binary-cache-store.cc index becd74c75..4d11e0f2a 100644 --- a/src/libstore/local-binary-cache-store.cc +++ b/src/libstore/local-binary-cache-store.cc @@ -18,6 +18,7 @@ private: public: LocalBinaryCacheStore( + [[maybe_unused]] const std::string scheme, const Path & binaryCacheDir, const Params & params) : StoreConfig(params) @@ -35,7 +36,7 @@ public: return "file://" + binaryCacheDir; } - static std::vector uriPrefixes(); + static std::set uriSchemes(); protected: @@ -96,7 +97,7 @@ bool LocalBinaryCacheStore::fileExists(const std::string & path) return pathExists(binaryCacheDir + "/" + path); } -std::vector LocalBinaryCacheStore::uriPrefixes() +std::set LocalBinaryCacheStore::uriSchemes() { if (getEnv("_NIX_FORCE_HTTP_BINARY_CACHE_STORE") == "1") return {}; diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 9bbb69b99..561502cbe 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -135,7 +135,10 @@ UDSRemoteStore::UDSRemoteStore(const Params & params) } -UDSRemoteStore::UDSRemoteStore(std::string socket_path, const Params & params) +UDSRemoteStore::UDSRemoteStore( + [[maybe_unused]] const std::string scheme, + std::string socket_path, + const Params & params) : UDSRemoteStore(params) { path.emplace(socket_path); diff --git a/src/libstore/remote-store.hh b/src/libstore/remote-store.hh index 32daab59a..cceb8d185 100644 --- a/src/libstore/remote-store.hh +++ b/src/libstore/remote-store.hh @@ -165,11 +165,11 @@ class UDSRemoteStore : public LocalFSStore, public RemoteStore, public virtual U public: UDSRemoteStore(const Params & params); - UDSRemoteStore(std::string path, const Params & params); + UDSRemoteStore(const std::string scheme, std::string path, const Params & params); std::string getUri() override; - static std::vector uriPrefixes() + static std::set uriSchemes() { return {"unix"}; } bool sameMachine() override diff --git a/src/libstore/s3-binary-cache-store.cc b/src/libstore/s3-binary-cache-store.cc index 30ef4082a..020b20c37 100644 --- a/src/libstore/s3-binary-cache-store.cc +++ b/src/libstore/s3-binary-cache-store.cc @@ -197,6 +197,7 @@ struct S3BinaryCacheStoreImpl : public S3BinaryCacheStore, virtual S3BinaryCache S3Helper s3Helper; S3BinaryCacheStoreImpl( + [[maybe_unused]] const std::string & scheme, const std::string & bucketName, const Params & params) : StoreConfig(params) @@ -434,7 +435,7 @@ struct S3BinaryCacheStoreImpl : public S3BinaryCacheStore, virtual S3BinaryCache return paths; } - static std::vector uriPrefixes() { return {"s3"}; } + static std::set uriSchemes() { return {"s3"}; } }; diff --git a/src/libstore/ssh-store.cc b/src/libstore/ssh-store.cc index da201a9c3..13265e127 100644 --- a/src/libstore/ssh-store.cc +++ b/src/libstore/ssh-store.cc @@ -22,7 +22,7 @@ class SSHStore : public virtual RemoteStore, public virtual SSHStoreConfig { public: - SSHStore(const std::string & host, const Params & params) + SSHStore([[maybe_unused]] const std::string & scheme, const std::string & host, const Params & params) : StoreConfig(params) , Store(params) , RemoteStoreConfig(params) @@ -38,11 +38,11 @@ public: { } - static std::vector uriPrefixes() { return {"ssh-ng"}; } + static std::set uriSchemes() { return {"ssh-ng"}; } std::string getUri() override { - return uriPrefixes()[0] + "://" + host; + return *uriSchemes().begin() + "://" + host; } bool sameMachine() override diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index fe3f5221c..9252c85e8 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -1078,25 +1078,35 @@ std::shared_ptr openFromNonUri(const std::string & uri, const Store::Para ref openStore(const std::string & uri_, const Store::Params & extraParams) { - auto [uri, uriParams] = splitUriAndParams(uri_); auto params = extraParams; - params.insert(uriParams.begin(), uriParams.end()); + try { + auto parsedUri = parseURL(uri_); + params.insert(parsedUri.query.begin(), parsedUri.query.end()); - if (auto store = openFromNonUri(uri, params)) { - store->warnUnknownSettings(); - return ref(store); + auto baseURI = parsedUri.authority.value_or("") + parsedUri.path; + + for (auto implem : *Implementations::registered) { + if (implem.uriSchemes.count(parsedUri.scheme)) { + auto store = implem.create(parsedUri.scheme, baseURI, params); + if (store) { + store->init(); + store->warnUnknownSettings(); + return ref(store); + } + } + } } + catch (BadURL) { + auto [uri, uriParams] = splitUriAndParams(uri_); + params.insert(uriParams.begin(), uriParams.end()); - for (auto implem : *Implementations::registered) { - auto store = implem.create(uri, params); - if (store) { - store->init(); + if (auto store = openFromNonUri(uri, params)) { store->warnUnknownSettings(); return ref(store); } } - throw Error("don't know how to open Nix store '%s'", uri); + throw Error("don't know how to open Nix store '%s'", uri_); } std::list> getDefaultSubstituters() diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 8e9cb786c..bb7d8036b 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -760,7 +760,8 @@ std::list> getDefaultSubstituters(); struct StoreFactory { - std::function (const std::string & uri, const Store::Params & params)> create; + std::set uriSchemes; + std::function (const std::string & scheme, const std::string & uri, const Store::Params & params)> create; std::function ()> getConfig; }; struct Implementations @@ -773,13 +774,14 @@ struct Implementations if (!registered) registered = new std::vector(); StoreFactory factory{ .create = - ([](const std::string & uri, const Store::Params & params) + ([](const std::string & scheme, const std::string & uri, const Store::Params & params) -> std::shared_ptr - { return std::make_shared(uri, params); }), + { return std::make_shared(scheme, uri, params); }), .getConfig = ([]() -> std::shared_ptr - { return std::make_shared(); }) + { return std::make_shared(StringMap({})); }), + .uriSchemes = T::uriSchemes() }; registered->push_back(factory); } From 1129913c4e175f4890a7029d22a301676f4cc3ca Mon Sep 17 00:00:00 2001 From: regnat Date: Fri, 11 Sep 2020 11:12:25 +0200 Subject: [PATCH 36/52] fixup! Correctly call all the parent contructors of the stores --- src/libstore/store-api.hh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index bb7d8036b..6f7893e41 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -148,6 +148,8 @@ struct BuildResult struct StoreConfig : public Config { using Config::Config; + StoreConfig() = delete; + const PathSetting storeDir_{this, false, settings.nixStore, "store", "path to the Nix store"}; From 29a632386e358b8cd72cf0545917e324bcfba16e Mon Sep 17 00:00:00 2001 From: regnat Date: Fri, 11 Sep 2020 11:13:41 +0200 Subject: [PATCH 37/52] fixup! Make the store plugins more introspectable --- src/libstore/store.hh | 9 --------- 1 file changed, 9 deletions(-) delete mode 100644 src/libstore/store.hh diff --git a/src/libstore/store.hh b/src/libstore/store.hh deleted file mode 100644 index bc73963f3..000000000 --- a/src/libstore/store.hh +++ /dev/null @@ -1,9 +0,0 @@ -#pragma once - -namespace nix { - -template class BasicStore; -class StoreConfig; -typedef BasicStore Store; - -} From d65962db4d5c1a2c8f456e3ae6bb789eb3a1e744 Mon Sep 17 00:00:00 2001 From: regnat Date: Fri, 11 Sep 2020 14:07:02 +0200 Subject: [PATCH 38/52] Make uri schemes grammar more RFC-compliant Allow `-` and `.` in the RFC schemes as stated by [RFC3986](https://tools.ietf.org/html/rfc3986#section-3.1). Practically, this is needed so that `ssh-ng` is a valid URI scheme --- src/libutil/url.hh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libutil/url.hh b/src/libutil/url.hh index 2ef88ef2a..1f716ba10 100644 --- a/src/libutil/url.hh +++ b/src/libutil/url.hh @@ -31,7 +31,7 @@ ParsedURL parseURL(const std::string & url); // URI stuff. const static std::string pctEncoded = "(?:%[0-9a-fA-F][0-9a-fA-F])"; -const static std::string schemeRegex = "(?:[a-z+]+)"; +const static std::string schemeRegex = "(?:[a-z+.-]+)"; const static std::string ipv6AddressRegex = "(?:\\[[0-9a-fA-F:]+\\])"; const static std::string unreservedRegex = "(?:[a-zA-Z0-9-._~])"; const static std::string subdelimsRegex = "(?:[!$&'\"()*+,;=])"; From f24f0888f93b0881e2cefa1ceb4c8fc187c94f21 Mon Sep 17 00:00:00 2001 From: regnat Date: Mon, 14 Sep 2020 11:18:45 +0200 Subject: [PATCH 39/52] Document the new store hierarchy --- src/libstore/build.cc | 2 +- src/libstore/dummy-store.cc | 3 +- src/libstore/http-binary-cache-store.cc | 2 -- src/libstore/legacy-ssh-store.cc | 1 - src/libstore/local-binary-cache-store.cc | 2 -- src/libstore/local-store.cc | 2 -- src/libstore/remote-store.cc | 3 -- src/libstore/s3-binary-cache-store.cc | 2 -- src/libstore/ssh-store.cc | 2 -- src/libstore/store-api.hh | 46 +++++++++++++++++++++++- 10 files changed, 47 insertions(+), 18 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 4040a3fac..18bb9b12e 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -2883,7 +2883,7 @@ struct RestrictedStore : public LocalFSStore DerivationGoal & goal; RestrictedStore(const Params & params, ref next, DerivationGoal & goal) - : StoreConfig(params), LocalFSStoreConfig(params), Store(params), LocalFSStore(params), next(next), goal(goal) + : StoreConfig(params), Store(params), LocalFSStore(params), next(next), goal(goal) { } Path getRealStoreDir() override diff --git a/src/libstore/dummy-store.cc b/src/libstore/dummy-store.cc index 210e5b11c..074953b1d 100644 --- a/src/libstore/dummy-store.cc +++ b/src/libstore/dummy-store.cc @@ -2,7 +2,7 @@ namespace nix { -struct DummyStoreConfig : StoreConfig { +struct DummyStoreConfig : virtual StoreConfig { using StoreConfig::StoreConfig; }; @@ -14,7 +14,6 @@ struct DummyStore : public Store, public virtual DummyStoreConfig DummyStore(const Params & params) : StoreConfig(params) - , DummyStoreConfig(params) , Store(params) { } diff --git a/src/libstore/http-binary-cache-store.cc b/src/libstore/http-binary-cache-store.cc index 095eaa82c..bd65273eb 100644 --- a/src/libstore/http-binary-cache-store.cc +++ b/src/libstore/http-binary-cache-store.cc @@ -33,9 +33,7 @@ public: const Path & _cacheUri, const Params & params) : StoreConfig(params) - , BinaryCacheStoreConfig(params) , BinaryCacheStore(params) - , HttpBinaryCacheStoreConfig(params) , cacheUri(scheme + "://" + _cacheUri) { if (cacheUri.back() == '/') diff --git a/src/libstore/legacy-ssh-store.cc b/src/libstore/legacy-ssh-store.cc index 6dc83e288..ce9051d20 100644 --- a/src/libstore/legacy-ssh-store.cc +++ b/src/libstore/legacy-ssh-store.cc @@ -45,7 +45,6 @@ struct LegacySSHStore : public Store, public virtual LegacySSHStoreConfig LegacySSHStore(const string & scheme, const string & host, const Params & params) : StoreConfig(params) - , LegacySSHStoreConfig(params) , Store(params) , host(host) , connections(make_ref>( diff --git a/src/libstore/local-binary-cache-store.cc b/src/libstore/local-binary-cache-store.cc index 4d11e0f2a..b893befbc 100644 --- a/src/libstore/local-binary-cache-store.cc +++ b/src/libstore/local-binary-cache-store.cc @@ -22,8 +22,6 @@ public: const Path & binaryCacheDir, const Params & params) : StoreConfig(params) - , BinaryCacheStoreConfig(params) - , LocalBinaryCacheStoreConfig(params) , BinaryCacheStore(params) , binaryCacheDir(binaryCacheDir) { diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 4e9840f3b..c618203f0 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -44,8 +44,6 @@ namespace nix { LocalStore::LocalStore(const Params & params) : StoreConfig(params) , Store(params) - , LocalFSStoreConfig(params) - , LocalStoreConfig(params) , LocalFSStore(params) , realStoreDir_{this, false, rootDir != "" ? rootDir + "/nix/store" : storeDir, "real", "physical path to the Nix store"} diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 561502cbe..4c8ffe71e 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -126,9 +126,6 @@ ref RemoteStore::openConnectionWrapper() UDSRemoteStore::UDSRemoteStore(const Params & params) : StoreConfig(params) , Store(params) - , LocalFSStoreConfig(params) - , RemoteStoreConfig(params) - , UDSRemoteStoreConfig(params) , LocalFSStore(params) , RemoteStore(params) { diff --git a/src/libstore/s3-binary-cache-store.cc b/src/libstore/s3-binary-cache-store.cc index 020b20c37..e4548dd22 100644 --- a/src/libstore/s3-binary-cache-store.cc +++ b/src/libstore/s3-binary-cache-store.cc @@ -201,8 +201,6 @@ struct S3BinaryCacheStoreImpl : public S3BinaryCacheStore, virtual S3BinaryCache const std::string & bucketName, const Params & params) : StoreConfig(params) - , BinaryCacheStoreConfig(params) - , S3BinaryCacheStoreConfig(params) , S3BinaryCacheStore(params) , bucketName(bucketName) , s3Helper(profile, region, scheme, endpoint) diff --git a/src/libstore/ssh-store.cc b/src/libstore/ssh-store.cc index 13265e127..fac251d8f 100644 --- a/src/libstore/ssh-store.cc +++ b/src/libstore/ssh-store.cc @@ -25,9 +25,7 @@ public: SSHStore([[maybe_unused]] const std::string & scheme, const std::string & host, const Params & params) : StoreConfig(params) , Store(params) - , RemoteStoreConfig(params) , RemoteStore(params) - , SSHStoreConfig(params) , host(host) , master( host, diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 6f7893e41..a67123ff0 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -24,6 +24,31 @@ namespace nix { +/** + * About the class hierarchy of the store implementations: + * + * Each store type `Foo` consists of two classes: + * + * 1. A class `FooConfig : virtual StoreConfig` that contains the configuration + * for the store + * + * It should only contain members of type `const Setting` (or subclasses + * of it) and inherit the constructors of `StoreConfig` + * (`using StoreConfig::StoreConfig`). + * + * 2. A class `Foo : virtual Store, virtual FooConfig` that contains the + * implementation of the store. + * + * This class is expected to have a constructor `Foo(const Params & params)` + * that calls `StoreConfig(params)` (otherwise you're gonna encounter an + * `assertion failure` when trying to instantiate it). + * + * You can then register the new store using: + * + * ``` + * cpp static RegisterStoreImplementation regStore; + * ``` + */ MakeError(SubstError, Error); MakeError(BuildError, Error); // denotes a permanent build failure @@ -148,7 +173,26 @@ struct BuildResult struct StoreConfig : public Config { using Config::Config; - StoreConfig() = delete; + + /** + * When constructing a store implementation, we pass in a map `params` of + * parameters that's supposed to initialize the associated config. + * To do that, we must use the `StoreConfig(StringMap & params)` + * constructor, so we'd like to `delete` its default constructor to enforce + * it. + * + * However, actually deleting it means that all the subclasses of + * `StoreConfig` will have their default constructor deleted (because it's + * supposed to call the deleted default constructor of `StoreConfig`). But + * because we're always using virtual inheritance, the constructors of + * child classes will never implicitely call this one, so deleting it will + * be more painful than anything else. + * + * So we `assert(false)` here to ensure at runtime that the right + * constructor is always called without having to redefine a custom + * constructor for each `*Config` class. + */ + StoreConfig() { assert(false); } const PathSetting storeDir_{this, false, settings.nixStore, From b73adacc1ebda8a790cd8e0b0988c3b0607d947a Mon Sep 17 00:00:00 2001 From: regnat Date: Mon, 14 Sep 2020 14:04:02 +0200 Subject: [PATCH 40/52] Add a name to the stores So that it can be printed by `nix describe-stores` --- src/libstore/build.cc | 7 ++++++- src/libstore/dummy-store.cc | 2 ++ src/libstore/http-binary-cache-store.cc | 2 ++ src/libstore/legacy-ssh-store.cc | 2 ++ src/libstore/local-binary-cache-store.cc | 2 ++ src/libstore/local-store.hh | 2 ++ src/libstore/remote-store.hh | 2 ++ src/libstore/s3-binary-cache-store.cc | 2 ++ src/libstore/ssh-store.cc | 2 ++ src/libstore/store-api.hh | 1 + src/nix/describe-stores.cc | 13 +++++++------ 11 files changed, 30 insertions(+), 7 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 18bb9b12e..6e55f83d5 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -2872,11 +2872,16 @@ void DerivationGoal::writeStructuredAttrs() chownToBuilder(tmpDir + "/.attrs.sh"); } +struct RestrictedStoreConfig : LocalFSStoreConfig +{ + using LocalFSStoreConfig::LocalFSStoreConfig; + const std::string name() { return "Restricted Store"; } +}; /* A wrapper around LocalStore that only allows building/querying of paths that are in the input closures of the build or were added via recursive Nix calls. */ -struct RestrictedStore : public LocalFSStore +struct RestrictedStore : public LocalFSStore, public virtual RestrictedStoreConfig { ref next; diff --git a/src/libstore/dummy-store.cc b/src/libstore/dummy-store.cc index 074953b1d..128832e60 100644 --- a/src/libstore/dummy-store.cc +++ b/src/libstore/dummy-store.cc @@ -4,6 +4,8 @@ namespace nix { struct DummyStoreConfig : virtual StoreConfig { using StoreConfig::StoreConfig; + + const std::string name() override { return "Dummy Store"; } }; struct DummyStore : public Store, public virtual DummyStoreConfig diff --git a/src/libstore/http-binary-cache-store.cc b/src/libstore/http-binary-cache-store.cc index bd65273eb..f4ab15a10 100644 --- a/src/libstore/http-binary-cache-store.cc +++ b/src/libstore/http-binary-cache-store.cc @@ -10,6 +10,8 @@ MakeError(UploadToHTTP, Error); struct HttpBinaryCacheStoreConfig : virtual BinaryCacheStoreConfig { using BinaryCacheStoreConfig::BinaryCacheStoreConfig; + + const std::string name() override { return "Http Binary Cache Store"; } }; class HttpBinaryCacheStore : public BinaryCacheStore, public HttpBinaryCacheStoreConfig diff --git a/src/libstore/legacy-ssh-store.cc b/src/libstore/legacy-ssh-store.cc index ce9051d20..6563c001f 100644 --- a/src/libstore/legacy-ssh-store.cc +++ b/src/libstore/legacy-ssh-store.cc @@ -17,6 +17,8 @@ struct LegacySSHStoreConfig : virtual StoreConfig const Setting compress{this, false, "compress", "whether to compress the connection"}; const Setting remoteProgram{this, "nix-store", "remote-program", "path to the nix-store executable on the remote system"}; const Setting remoteStore{this, "", "remote-store", "URI of the store on the remote system"}; + + const std::string name() override { return "Legacy SSH Store"; } }; struct LegacySSHStore : public Store, public virtual LegacySSHStoreConfig diff --git a/src/libstore/local-binary-cache-store.cc b/src/libstore/local-binary-cache-store.cc index b893befbc..357bc74df 100644 --- a/src/libstore/local-binary-cache-store.cc +++ b/src/libstore/local-binary-cache-store.cc @@ -7,6 +7,8 @@ namespace nix { struct LocalBinaryCacheStoreConfig : virtual BinaryCacheStoreConfig { using BinaryCacheStoreConfig::BinaryCacheStoreConfig; + + const std::string name() override { return "Local Binary Cache Store"; } }; class LocalBinaryCacheStore : public BinaryCacheStore, public virtual LocalBinaryCacheStoreConfig diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 0265f0837..e7c9d1605 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -37,6 +37,8 @@ struct LocalStoreConfig : virtual LocalFSStoreConfig Setting requireSigs{(StoreConfig*) this, settings.requireSigs, "require-sigs", "whether store paths should have a trusted signature on import"}; + + const std::string name() override { return "Local Store"; } }; diff --git a/src/libstore/remote-store.hh b/src/libstore/remote-store.hh index cceb8d185..a23690830 100644 --- a/src/libstore/remote-store.hh +++ b/src/libstore/remote-store.hh @@ -158,6 +158,8 @@ struct UDSRemoteStoreConfig : virtual LocalFSStoreConfig, virtual RemoteStoreCon : UDSRemoteStoreConfig(Store::Params({})) { } + + const std::string name() override { return "Local Daemon Store"; } }; class UDSRemoteStore : public LocalFSStore, public RemoteStore, public virtual UDSRemoteStoreConfig diff --git a/src/libstore/s3-binary-cache-store.cc b/src/libstore/s3-binary-cache-store.cc index e4548dd22..0b9c9bb31 100644 --- a/src/libstore/s3-binary-cache-store.cc +++ b/src/libstore/s3-binary-cache-store.cc @@ -186,6 +186,8 @@ struct S3BinaryCacheStoreConfig : virtual BinaryCacheStoreConfig this, false, "multipart-upload", "whether to use multi-part uploads"}; const Setting bufferSize{ this, 5 * 1024 * 1024, "buffer-size", "size (in bytes) of each part in multi-part uploads"}; + + const std::string name() override { return "S3 Binary Cache Store"; } }; struct S3BinaryCacheStoreImpl : public S3BinaryCacheStore, virtual S3BinaryCacheStoreConfig diff --git a/src/libstore/ssh-store.cc b/src/libstore/ssh-store.cc index fac251d8f..df383bba6 100644 --- a/src/libstore/ssh-store.cc +++ b/src/libstore/ssh-store.cc @@ -16,6 +16,8 @@ struct SSHStoreConfig : virtual RemoteStoreConfig const Setting compress{(StoreConfig*) this, false, "compress", "whether to compress the connection"}; const Setting remoteProgram{(StoreConfig*) this, "nix-daemon", "remote-program", "path to the nix-daemon executable on the remote system"}; const Setting remoteStore{(StoreConfig*) this, "", "remote-store", "URI of the store on the remote system"}; + + const std::string name() override { return "SSH Store"; } }; class SSHStore : public virtual RemoteStore, public virtual SSHStoreConfig diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index a67123ff0..81b979b73 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -194,6 +194,7 @@ struct StoreConfig : public Config */ StoreConfig() { assert(false); } + virtual const std::string name() = 0; const PathSetting storeDir_{this, false, settings.nixStore, "store", "path to the Nix store"}; diff --git a/src/nix/describe-stores.cc b/src/nix/describe-stores.cc index f019ec9c2..027187bd9 100644 --- a/src/nix/describe-stores.cc +++ b/src/nix/describe-stores.cc @@ -18,13 +18,14 @@ struct CmdDescribeStores : Command, MixJSON void run() override { - + auto res = nlohmann::json::object(); + for (auto & implem : *Implementations::registered) { + auto storeConfig = implem.getConfig(); + auto storeName = storeConfig->name(); + res[storeName] = storeConfig->toJSON(); + } if (json) { - auto res = nlohmann::json::array(); - for (auto & implem : *Implementations::registered) { - auto storeConfig = implem.getConfig(); - std::cout << storeConfig->toJSON().dump() << std::endl; - } + std::cout << res; } else { throw Error("Only json is available for describe-stores"); } From 634cb2a5aec64164dd7fa9467d9cdb1569611c21 Mon Sep 17 00:00:00 2001 From: regnat Date: Mon, 14 Sep 2020 14:04:21 +0200 Subject: [PATCH 41/52] Add a markdown output to `nix describe-stores` --- src/nix/describe-stores.cc | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/nix/describe-stores.cc b/src/nix/describe-stores.cc index 027187bd9..0cc2d9337 100644 --- a/src/nix/describe-stores.cc +++ b/src/nix/describe-stores.cc @@ -27,7 +27,16 @@ struct CmdDescribeStores : Command, MixJSON if (json) { std::cout << res; } else { - throw Error("Only json is available for describe-stores"); + for (auto & [storeName, storeConfig] : res.items()) { + std::cout << "## " << storeName << std::endl << std::endl; + for (auto & [optionName, optionDesc] : storeConfig.items()) { + std::cout << "### " << optionName << std::endl << std::endl; + std::cout << optionDesc["description"].get() << std::endl; + std::cout << "default: " << optionDesc["defaultValue"] << std::endl < Date: Mon, 14 Sep 2020 14:12:47 +0200 Subject: [PATCH 42/52] Fix build issues with gcc --- src/libstore/local-binary-cache-store.cc | 2 +- src/libstore/remote-store.cc | 2 +- src/libstore/s3-binary-cache-store.cc | 2 +- src/libstore/ssh-store.cc | 2 +- src/libstore/store-api.hh | 4 ++-- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/libstore/local-binary-cache-store.cc b/src/libstore/local-binary-cache-store.cc index 357bc74df..b5744448e 100644 --- a/src/libstore/local-binary-cache-store.cc +++ b/src/libstore/local-binary-cache-store.cc @@ -20,7 +20,7 @@ private: public: LocalBinaryCacheStore( - [[maybe_unused]] const std::string scheme, + const std::string scheme, const Path & binaryCacheDir, const Params & params) : StoreConfig(params) diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 4c8ffe71e..1abe236f7 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -133,7 +133,7 @@ UDSRemoteStore::UDSRemoteStore(const Params & params) UDSRemoteStore::UDSRemoteStore( - [[maybe_unused]] const std::string scheme, + const std::string scheme, std::string socket_path, const Params & params) : UDSRemoteStore(params) diff --git a/src/libstore/s3-binary-cache-store.cc b/src/libstore/s3-binary-cache-store.cc index 0b9c9bb31..b3541d7b8 100644 --- a/src/libstore/s3-binary-cache-store.cc +++ b/src/libstore/s3-binary-cache-store.cc @@ -199,7 +199,7 @@ struct S3BinaryCacheStoreImpl : public S3BinaryCacheStore, virtual S3BinaryCache S3Helper s3Helper; S3BinaryCacheStoreImpl( - [[maybe_unused]] const std::string & scheme, + const std::string & scheme, const std::string & bucketName, const Params & params) : StoreConfig(params) diff --git a/src/libstore/ssh-store.cc b/src/libstore/ssh-store.cc index df383bba6..8b6e48fb0 100644 --- a/src/libstore/ssh-store.cc +++ b/src/libstore/ssh-store.cc @@ -24,7 +24,7 @@ class SSHStore : public virtual RemoteStore, public virtual SSHStoreConfig { public: - SSHStore([[maybe_unused]] const std::string & scheme, const std::string & host, const Params & params) + SSHStore(const std::string & scheme, const std::string & host, const Params & params) : StoreConfig(params) , Store(params) , RemoteStore(params) diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 81b979b73..1bea4837b 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -820,6 +820,7 @@ struct Implementations { if (!registered) registered = new std::vector(); StoreFactory factory{ + .uriSchemes = T::uriSchemes(), .create = ([](const std::string & scheme, const std::string & uri, const Store::Params & params) -> std::shared_ptr @@ -827,8 +828,7 @@ struct Implementations .getConfig = ([]() -> std::shared_ptr - { return std::make_shared(StringMap({})); }), - .uriSchemes = T::uriSchemes() + { return std::make_shared(StringMap({})); }) }; registered->push_back(factory); } From a1e82ba450f274966dc45375b42b5d4413fff77d Mon Sep 17 00:00:00 2001 From: regnat Date: Mon, 14 Sep 2020 15:35:17 +0200 Subject: [PATCH 43/52] fixup! Add a default value for the settings --- src/libutil/tests/config.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libutil/tests/config.cc b/src/libutil/tests/config.cc index c5abefe11..c7777a21f 100644 --- a/src/libutil/tests/config.cc +++ b/src/libutil/tests/config.cc @@ -161,7 +161,7 @@ namespace nix { Setting setting{&config, "", "name-of-the-setting", "description"}; setting.assign("value"); - ASSERT_EQ(config.toJSON().dump(), R"#({"name-of-the-setting":{"aliases":[],"description":"description\n","value":"value"}})#"); + ASSERT_EQ(config.toJSON().dump(), R"#({"name-of-the-setting":{"aliases":[],"defaultValue":"","description":"description\n","value":"value"}})#"); } TEST(Config, setSettingAlias) { From fc2d31c423377cea2355e7f7a4ce75e64a7f3620 Mon Sep 17 00:00:00 2001 From: regnat Date: Tue, 15 Sep 2020 09:10:37 +0200 Subject: [PATCH 44/52] Add `(StoreConfig*)` casts to work around a GCC bug Work around https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80431 that was already there in the code but was accidentally removed in the last commits --- src/libstore/binary-cache-store.hh | 12 ++++++------ src/libstore/legacy-ssh-store.cc | 16 ++++++++-------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/libstore/binary-cache-store.hh b/src/libstore/binary-cache-store.hh index f1fc82013..4b779cdd4 100644 --- a/src/libstore/binary-cache-store.hh +++ b/src/libstore/binary-cache-store.hh @@ -15,12 +15,12 @@ struct BinaryCacheStoreConfig : virtual StoreConfig { using StoreConfig::StoreConfig; - const Setting compression{this, "xz", "compression", "NAR compression method ('xz', 'bzip2', or 'none')"}; - const Setting writeNARListing{this, false, "write-nar-listing", "whether to write a JSON file listing the files in each NAR"}; - const Setting writeDebugInfo{this, false, "index-debug-info", "whether to index DWARF debug info files by build ID"}; - const Setting secretKeyFile{this, "", "secret-key", "path to secret key used to sign the binary cache"}; - const Setting localNarCache{this, "", "local-nar-cache", "path to a local cache of NARs"}; - const Setting parallelCompression{this, false, "parallel-compression", + const Setting compression{(StoreConfig*) this, "xz", "compression", "NAR compression method ('xz', 'bzip2', or 'none')"}; + const Setting writeNARListing{(StoreConfig*) this, false, "write-nar-listing", "whether to write a JSON file listing the files in each NAR"}; + const Setting writeDebugInfo{(StoreConfig*) this, false, "index-debug-info", "whether to index DWARF debug info files by build ID"}; + const Setting secretKeyFile{(StoreConfig*) this, "", "secret-key", "path to secret key used to sign the binary cache"}; + const Setting localNarCache{(StoreConfig*) this, "", "local-nar-cache", "path to a local cache of NARs"}; + const Setting parallelCompression{(StoreConfig*) this, false, "parallel-compression", "enable multi-threading compression, available for xz only currently"}; }; diff --git a/src/libstore/legacy-ssh-store.cc b/src/libstore/legacy-ssh-store.cc index 6563c001f..e9478c1d5 100644 --- a/src/libstore/legacy-ssh-store.cc +++ b/src/libstore/legacy-ssh-store.cc @@ -12,11 +12,11 @@ namespace nix { struct LegacySSHStoreConfig : virtual StoreConfig { using StoreConfig::StoreConfig; - const Setting maxConnections{this, 1, "max-connections", "maximum number of concurrent SSH connections"}; - const Setting sshKey{this, "", "ssh-key", "path to an SSH private key"}; - const Setting compress{this, false, "compress", "whether to compress the connection"}; - const Setting remoteProgram{this, "nix-store", "remote-program", "path to the nix-store executable on the remote system"}; - const Setting remoteStore{this, "", "remote-store", "URI of the store on the remote system"}; + const Setting maxConnections{(StoreConfig*) this, 1, "max-connections", "maximum number of concurrent SSH connections"}; + const Setting sshKey{(StoreConfig*) this, "", "ssh-key", "path to an SSH private key"}; + const Setting compress{(StoreConfig*) this, false, "compress", "whether to compress the connection"}; + const Setting remoteProgram{(StoreConfig*) this, "nix-store", "remote-program", "path to the nix-store executable on the remote system"}; + const Setting remoteStore{(StoreConfig*) this, "", "remote-store", "URI of the store on the remote system"}; const std::string name() override { return "Legacy SSH Store"; } }; @@ -24,9 +24,9 @@ struct LegacySSHStoreConfig : virtual StoreConfig struct LegacySSHStore : public Store, public virtual LegacySSHStoreConfig { // Hack for getting remote build log output. - // Intentionally not in `StoreConfig` so that it doesn't appear in the - // documentation - const Setting logFD{this, -1, "log-fd", "file descriptor to which SSH's stderr is connected"}; + // Intentionally not in `LegacySSHStoreConfig` so that it doesn't appear in + // the documentation + const Setting logFD{(StoreConfig*) this, -1, "log-fd", "file descriptor to which SSH's stderr is connected"}; struct Connection { From 93c0e14a308d513cf61daa7a003417e42a7dc2f8 Mon Sep 17 00:00:00 2001 From: regnat Date: Tue, 15 Sep 2020 11:34:20 +0200 Subject: [PATCH 45/52] Include the full nlohmann/json header in config.hh It is apparently required for using `toJSONObject()`, which we do inside the header file (because it's in a template). This was accidentally working when building Nix itself (presumably because `config.hh` was always included after `nlohman/json.hpp`) but caused a (pretty dirty) build failure in the perl bindings package. --- src/libutil/config.hh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libutil/config.hh b/src/libutil/config.hh index 42fc856e0..18fa9dea8 100644 --- a/src/libutil/config.hh +++ b/src/libutil/config.hh @@ -4,7 +4,7 @@ #include "types.hh" -#include +#include #pragma once From e0817cbcdcdbec81d7ce2f5141b4f99bbc2bece7 Mon Sep 17 00:00:00 2001 From: regnat Date: Wed, 16 Sep 2020 09:06:35 +0200 Subject: [PATCH 46/52] Don't include nlohmann/json.hpp in config.hh Instead make a separate header with the template implementation of `BaseSetting::toJSONObj` that can be included where needed --- src/libstore/globals.hh | 1 + src/libutil/abstractsettingtojson.hh | 15 +++++++++++++++ src/libutil/config.cc | 1 + src/libutil/config.hh | 10 ++-------- 4 files changed, 19 insertions(+), 8 deletions(-) create mode 100644 src/libutil/abstractsettingtojson.hh diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index 8a2d3ff75..02721285a 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -2,6 +2,7 @@ #include "types.hh" #include "config.hh" +#include "abstractsettingtojson.hh" #include "util.hh" #include diff --git a/src/libutil/abstractsettingtojson.hh b/src/libutil/abstractsettingtojson.hh new file mode 100644 index 000000000..b3fbc84f7 --- /dev/null +++ b/src/libutil/abstractsettingtojson.hh @@ -0,0 +1,15 @@ +#pragma once + +#include +#include "config.hh" + +namespace nix { +template +std::map BaseSetting::toJSONObject() +{ + auto obj = AbstractSetting::toJSONObject(); + obj.emplace("value", value); + obj.emplace("defaultValue", defaultValue); + return obj; +} +} diff --git a/src/libutil/config.cc b/src/libutil/config.cc index faa5cdbeb..309d23b40 100644 --- a/src/libutil/config.cc +++ b/src/libutil/config.cc @@ -1,5 +1,6 @@ #include "config.hh" #include "args.hh" +#include "abstractsettingtojson.hh" #include diff --git a/src/libutil/config.hh b/src/libutil/config.hh index 18fa9dea8..1f5f4e7b9 100644 --- a/src/libutil/config.hh +++ b/src/libutil/config.hh @@ -4,7 +4,7 @@ #include "types.hh" -#include +#include #pragma once @@ -255,13 +255,7 @@ public: void convertToArg(Args & args, const std::string & category) override; - std::map toJSONObject() override - { - auto obj = AbstractSetting::toJSONObject(); - obj.emplace("value", value); - obj.emplace("defaultValue", defaultValue); - return obj; - } + std::map toJSONObject() override; }; template From d72927aa7a16be56b134dfadf34d5bbdb751f264 Mon Sep 17 00:00:00 2001 From: regnat Date: Wed, 16 Sep 2020 13:51:53 +0200 Subject: [PATCH 47/52] Fix the s3 store Add some necessary casts in the initialisation of the store's config --- src/libstore/s3-binary-cache-store.cc | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/libstore/s3-binary-cache-store.cc b/src/libstore/s3-binary-cache-store.cc index b3541d7b8..d43f267e0 100644 --- a/src/libstore/s3-binary-cache-store.cc +++ b/src/libstore/s3-binary-cache-store.cc @@ -175,17 +175,17 @@ S3Helper::FileTransferResult S3Helper::getObject( struct S3BinaryCacheStoreConfig : virtual BinaryCacheStoreConfig { using BinaryCacheStoreConfig::BinaryCacheStoreConfig; - const Setting profile{this, "", "profile", "The name of the AWS configuration profile to use."}; - const Setting region{this, Aws::Region::US_EAST_1, "region", {"aws-region"}}; - const Setting scheme{this, "", "scheme", "The scheme to use for S3 requests, https by default."}; - const Setting endpoint{this, "", "endpoint", "An optional override of the endpoint to use when talking to S3."}; - const Setting narinfoCompression{this, "", "narinfo-compression", "compression method for .narinfo files"}; - const Setting lsCompression{this, "", "ls-compression", "compression method for .ls files"}; - const Setting logCompression{this, "", "log-compression", "compression method for log/* files"}; + const Setting profile{(StoreConfig*) this, "", "profile", "The name of the AWS configuration profile to use."}; + const Setting region{(StoreConfig*) this, Aws::Region::US_EAST_1, "region", {"aws-region"}}; + const Setting scheme{(StoreConfig*) this, "", "scheme", "The scheme to use for S3 requests, https by default."}; + const Setting endpoint{(StoreConfig*) this, "", "endpoint", "An optional override of the endpoint to use when talking to S3."}; + const Setting narinfoCompression{(StoreConfig*) this, "", "narinfo-compression", "compression method for .narinfo files"}; + const Setting lsCompression{(StoreConfig*) this, "", "ls-compression", "compression method for .ls files"}; + const Setting logCompression{(StoreConfig*) this, "", "log-compression", "compression method for log/* files"}; const Setting multipartUpload{ - this, false, "multipart-upload", "whether to use multi-part uploads"}; + (StoreConfig*) this, false, "multipart-upload", "whether to use multi-part uploads"}; const Setting bufferSize{ - this, 5 * 1024 * 1024, "buffer-size", "size (in bytes) of each part in multi-part uploads"}; + (StoreConfig*) this, 5 * 1024 * 1024, "buffer-size", "size (in bytes) of each part in multi-part uploads"}; const std::string name() override { return "S3 Binary Cache Store"; } }; From c29624bf7d58aeb698ddd12b983465d355fccf79 Mon Sep 17 00:00:00 2001 From: regnat Date: Wed, 16 Sep 2020 13:52:15 +0200 Subject: [PATCH 48/52] Add a test for `nix describe-stores` Doesn't test much, but at least ensures that the command runs properly --- tests/describe-stores.sh | 8 ++++++++ tests/local.mk | 1 + 2 files changed, 9 insertions(+) create mode 100644 tests/describe-stores.sh diff --git a/tests/describe-stores.sh b/tests/describe-stores.sh new file mode 100644 index 000000000..3fea61483 --- /dev/null +++ b/tests/describe-stores.sh @@ -0,0 +1,8 @@ +source common.sh + +# Query an arbitrary value in `nix describe-stores --json`'s output just to +# check that it has the right structure +[[ $(nix --experimental-features 'nix-command flakes' describe-stores --json | jq '.["SSH Store"]["compress"]["defaultValue"]') == false ]] + +# Ensure that the output of `nix describe-stores` isn't empty +[[ -n $(nix --experimental-features 'nix-command flakes' describe-stores) ]] diff --git a/tests/local.mk b/tests/local.mk index 5d25de019..594901504 100644 --- a/tests/local.mk +++ b/tests/local.mk @@ -32,6 +32,7 @@ nix_tests = \ post-hook.sh \ function-trace.sh \ recursive.sh \ + describe-stores.sh \ flakes.sh \ content-addressed.sh # parallel.sh From 77a0e2c5beba478f4cfc9f3c9516e516a5da43c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Wed, 16 Sep 2020 14:00:21 +0200 Subject: [PATCH 49/52] Remove useless exception copy Co-authored-by: Eelco Dolstra --- src/libstore/store-api.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 9252c85e8..76cbc0605 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -1096,7 +1096,7 @@ ref openStore(const std::string & uri_, } } } - catch (BadURL) { + catch (BadURL &) { auto [uri, uriParams] = splitUriAndParams(uri_); params.insert(uriParams.begin(), uriParams.end()); From 2eacc1bc00fcc3bccd470b846062c63bc408be05 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 16 Sep 2020 14:18:46 +0200 Subject: [PATCH 50/52] builtins.toFile: Fix indentation --- src/libexpr/primops.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 4a0dd5544..816980f10 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -1704,7 +1704,7 @@ static RegisterPrimOp primop_toFile({ ... cp ${configFile} $out/etc/foo.conf "; - ``` + ``` Note that `${configFile}` is an [antiquotation](language-values.md), so the result of the From 39bc49318f64d30b945d9dbb4f96f225d25dbd50 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 16 Sep 2020 14:55:24 +0200 Subject: [PATCH 51/52] jq -> nix --- doc/manual/generate-builtins.jq | 6 ---- doc/manual/generate-builtins.nix | 14 ++++++++ doc/manual/generate-manpage.jq | 44 ------------------------- doc/manual/generate-manpage.nix | 56 ++++++++++++++++++++++++++++++++ doc/manual/generate-options.jq | 16 --------- doc/manual/generate-options.nix | 21 ++++++++++++ doc/manual/local.mk | 16 ++++----- doc/manual/utils.nix | 7 ++++ 8 files changed, 106 insertions(+), 74 deletions(-) delete mode 100644 doc/manual/generate-builtins.jq create mode 100644 doc/manual/generate-builtins.nix delete mode 100644 doc/manual/generate-manpage.jq create mode 100644 doc/manual/generate-manpage.nix delete mode 100644 doc/manual/generate-options.jq create mode 100644 doc/manual/generate-options.nix create mode 100644 doc/manual/utils.nix diff --git a/doc/manual/generate-builtins.jq b/doc/manual/generate-builtins.jq deleted file mode 100644 index c38799479..000000000 --- a/doc/manual/generate-builtins.jq +++ /dev/null @@ -1,6 +0,0 @@ -. | to_entries | sort_by(.key) | map( - " - `builtins." + .key + "` " - + (.value.args | map("*" + . + "*") | join(" ")) - + " \n\n" - + (.value.doc | split("\n") | map(" " + . + "\n") | join("")) + "\n\n" -) | join("") diff --git a/doc/manual/generate-builtins.nix b/doc/manual/generate-builtins.nix new file mode 100644 index 000000000..416a7fdba --- /dev/null +++ b/doc/manual/generate-builtins.nix @@ -0,0 +1,14 @@ +with builtins; +with import ./utils.nix; + +builtins: + +concatStrings (map + (name: + let builtin = builtins.${name}; in + " - `builtins.${name}` " + concatStringsSep " " (map (s: "*${s}*") builtin.args) + + " \n\n" + + concatStrings (map (s: " ${s}\n") (splitLines builtin.doc)) + "\n\n" + ) + (attrNames builtins)) + diff --git a/doc/manual/generate-manpage.jq b/doc/manual/generate-manpage.jq deleted file mode 100644 index dd632f162..000000000 --- a/doc/manual/generate-manpage.jq +++ /dev/null @@ -1,44 +0,0 @@ -def show_flags: - .flags - | map_values(select(.category != "config")) - | to_entries - | map( - " - `--" + .key + "`" - + (if .value.shortName then " / `" + .value.shortName + "`" else "" end) - + (if .value.labels then " " + (.value.labels | map("*" + . + "*") | join(" ")) else "" end) - + " \n" - + " " + .value.description + "\n\n") - | join("") - ; - -def show_synopsis: - "`" + .command + "` [*flags*...] " + (.args | map("*" + .label + "*" + (if has("arity") then "" else "..." end)) | join(" ")) + "\n\n" - ; - -def show_command: - . as $top | - .section + " Name\n\n" - + "`" + .command + "` - " + .def.description + "\n\n" - + .section + " Synopsis\n\n" - + ({"command": .command, "args": .def.args} | show_synopsis) - + (if .def | has("doc") - then .section + " Description\n\n" + .def.doc + "\n\n" - else "" - end) - + (if (.def.flags | length) > 0 then - .section + " Flags\n\n" - + (.def | show_flags) - else "" end) - + (if (.def.examples | length) > 0 then - .section + " Examples\n\n" - + (.def.examples | map(.description + "\n\n```console\n" + .command + "\n```\n" ) | join("\n")) - + "\n" - else "" end) - + (if .def.commands then .def.commands | to_entries | map( - "# Subcommand `" + ($top.command + " " + .key) + "`\n\n" - + ({"command": ($top.command + " " + .key), "section": "##", "def": .value} | show_command) - ) | join("") else "" end) - ; - -"Title: nix\n\n" -+ ({"command": "nix", "section": "#", "def": .} | show_command) diff --git a/doc/manual/generate-manpage.nix b/doc/manual/generate-manpage.nix new file mode 100644 index 000000000..4709c0e2c --- /dev/null +++ b/doc/manual/generate-manpage.nix @@ -0,0 +1,56 @@ +with builtins; +with import ./utils.nix; + +let + + showCommand = + { command, section, def }: + "${section} Name\n\n" + + "`${command}` - ${def.description}\n\n" + + "${section} Synopsis\n\n" + + showSynopsis { inherit command; args = def.args; } + + (if def ? doc + then "${section} Description\n\n" + def.doc + "\n\n" + else "") + + (let s = showFlags def.flags; in + if s != "" + then "${section} Flags\n\n${s}" + else "") + + (if def.examples or [] != [] + then + "${section} Examples\n\n" + + concatStrings (map ({ description, command }: "${description}\n\n```console\n${command}\n```\n\n") def.examples) + else "") + + (if def.commands or [] != [] + then concatStrings ( + map (name: + "# Subcommand `${command} ${name}`\n\n" + + showCommand { command = command + " " + name; section = "##"; def = def.commands.${name}; }) + (attrNames def.commands)) + else ""); + + showFlags = flags: + concatStrings + (map (longName: + let flag = flags.${longName}; in + if flag.category or "" != "config" + then + " - `--${longName}`" + + (if flag ? shortName then " / `${flag.shortName}`" else "") + + (if flag ? labels then " " + (concatStringsSep " " (map (s: "*${s}*") flag.labels)) else "") + + " \n" + + " " + flag.description + "\n\n" + else "") + (attrNames flags)); + + showSynopsis = + { command, args }: + "`${command}` [*flags*...] ${concatStringsSep " " + (map (arg: "*${arg.label}*" + (if arg ? arity then "" else "...")) args)}\n\n"; + +in + +command: + +"Title: nix\n\n" ++ showCommand { command = "nix"; section = "#"; def = command; } diff --git a/doc/manual/generate-options.jq b/doc/manual/generate-options.jq deleted file mode 100644 index ccf62e8ed..000000000 --- a/doc/manual/generate-options.jq +++ /dev/null @@ -1,16 +0,0 @@ -. | to_entries | sort_by(.key) | map( - " - `" + .key + "` \n\n" - + (.value.description | split("\n") | map(" " + . + "\n") | join("")) + "\n\n" - + " **Default:** " + ( - if .value.value == "" or .value.value == [] - then "*empty*" - elif (.value.value | type) == "array" - then "`" + (.value.value | join(" ")) + "`" - else "`" + (.value.value | tostring) + "`" - end) - + "\n\n" - + (if (.value.aliases | length) > 0 - then " **Deprecated alias:** " + (.value.aliases | map("`" + . + "`") | join(", ")) + "\n\n" - else "" - end) -) | join("") diff --git a/doc/manual/generate-options.nix b/doc/manual/generate-options.nix new file mode 100644 index 000000000..7afe279c3 --- /dev/null +++ b/doc/manual/generate-options.nix @@ -0,0 +1,21 @@ +with builtins; +with import ./utils.nix; + +options: + +concatStrings (map + (name: + let option = options.${name}; in + " - `${name}` \n\n" + + concatStrings (map (s: " ${s}\n") (splitLines option.description)) + "\n\n" + + " **Default:** " + ( + if option.value == "" || option.value == [] + then "*empty*" + else if isBool option.value + then (if option.value then "`true`" else "`false`") + else "`" + toString option.value + "`") + "\n\n" + + (if option.aliases != [] + then " **Deprecated alias:** " + (concatStringsSep ", " (map (s: "`${s}`") option.aliases)) + "\n\n" + else "") + ) + (attrNames options)) diff --git a/doc/manual/local.mk b/doc/manual/local.mk index 297a73414..c219153e9 100644 --- a/doc/manual/local.mk +++ b/doc/manual/local.mk @@ -24,12 +24,12 @@ $(d)/%.8: $(d)/src/command-ref/%.md $(d)/nix.conf.5: $(d)/src/command-ref/conf-file.md $(trace-gen) lowdown -sT man $^ -o $@ -$(d)/src/command-ref/nix.md: $(d)/nix.json $(d)/generate-manpage.jq - jq -r -f doc/manual/generate-manpage.jq $< > $@ +$(d)/src/command-ref/nix.md: $(d)/nix.json $(d)/generate-manpage.nix $(bindir)/nix + $(trace-gen) $(bindir)/nix eval --experimental-features nix-command --impure --raw --expr 'import doc/manual/generate-manpage.nix (builtins.fromJSON (builtins.readFile $<))' > $@ -$(d)/src/command-ref/conf-file.md: $(d)/conf-file.json $(d)/generate-options.jq $(d)/src/command-ref/conf-file-prefix.md - cat doc/manual/src/command-ref/conf-file-prefix.md > $@ - jq -r -f doc/manual/generate-options.jq $< >> $@ +$(d)/src/command-ref/conf-file.md: $(d)/conf-file.json $(d)/generate-options.nix $(d)/src/command-ref/conf-file-prefix.md $(bindir)/nix + @cat doc/manual/src/command-ref/conf-file-prefix.md > $@ + $(trace-gen) $(bindir)/nix eval --experimental-features nix-command --impure --raw --expr 'import doc/manual/generate-options.nix (builtins.fromJSON (builtins.readFile $<))' >> $@ $(d)/nix.json: $(bindir)/nix $(trace-gen) $(bindir)/nix __dump-args > $@ @@ -37,9 +37,9 @@ $(d)/nix.json: $(bindir)/nix $(d)/conf-file.json: $(bindir)/nix $(trace-gen) env -i NIX_CONF_DIR=/dummy HOME=/dummy $(bindir)/nix show-config --json --experimental-features nix-command > $@ -$(d)/src/expressions/builtins.md: $(d)/builtins.json $(d)/generate-builtins.jq $(d)/src/expressions/builtins-prefix.md - cat doc/manual/src/expressions/builtins-prefix.md > $@ - jq -r -f doc/manual/generate-builtins.jq $< >> $@ +$(d)/src/expressions/builtins.md: $(d)/builtins.json $(d)/generate-builtins.nix $(d)/src/expressions/builtins-prefix.md $(bindir)/nix + @cat doc/manual/src/expressions/builtins-prefix.md > $@ + $(trace-gen) $(bindir)/nix eval --experimental-features nix-command --impure --raw --expr 'import doc/manual/generate-builtins.nix (builtins.fromJSON (builtins.readFile $<))' >> $@ $(d)/builtins.json: $(bindir)/nix $(trace-gen) $(bindir)/nix __dump-builtins > $@ diff --git a/doc/manual/utils.nix b/doc/manual/utils.nix new file mode 100644 index 000000000..50150bf3e --- /dev/null +++ b/doc/manual/utils.nix @@ -0,0 +1,7 @@ +with builtins; + +{ + splitLines = s: filter (x: !isList x) (split "\n" s); + + concatStrings = concatStringsSep ""; +} From 0066ef6c59c356d5714f2e26af9471937ba0c865 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 16 Sep 2020 16:56:28 +0200 Subject: [PATCH 52/52] Fix doc generation --- doc/manual/local.mk | 24 ++++++++++++++++-------- src/libexpr/eval.cc | 6 +++++- src/libexpr/primops.cc | 11 +++++++---- src/nix/main.cc | 1 + 4 files changed, 29 insertions(+), 13 deletions(-) diff --git a/doc/manual/local.mk b/doc/manual/local.mk index c219153e9..3b8e7e2df 100644 --- a/doc/manual/local.mk +++ b/doc/manual/local.mk @@ -15,6 +15,8 @@ clean-files += $(d)/*.1 $(d)/*.5 $(d)/*.8 dist-files += $(man-pages) +nix-eval = $(bindir)/nix eval --experimental-features nix-command -I nix/corepkgs=corepkgs --store dummy:// --impure --raw --expr + $(d)/%.1: $(d)/src/command-ref/%.md $(trace-gen) lowdown -sT man $^ -o $@ @@ -25,24 +27,30 @@ $(d)/nix.conf.5: $(d)/src/command-ref/conf-file.md $(trace-gen) lowdown -sT man $^ -o $@ $(d)/src/command-ref/nix.md: $(d)/nix.json $(d)/generate-manpage.nix $(bindir)/nix - $(trace-gen) $(bindir)/nix eval --experimental-features nix-command --impure --raw --expr 'import doc/manual/generate-manpage.nix (builtins.fromJSON (builtins.readFile $<))' > $@ + $(trace-gen) $(nix-eval) 'import doc/manual/generate-manpage.nix (builtins.fromJSON (builtins.readFile $<))' > $@.tmp + @mv $@.tmp $@ $(d)/src/command-ref/conf-file.md: $(d)/conf-file.json $(d)/generate-options.nix $(d)/src/command-ref/conf-file-prefix.md $(bindir)/nix - @cat doc/manual/src/command-ref/conf-file-prefix.md > $@ - $(trace-gen) $(bindir)/nix eval --experimental-features nix-command --impure --raw --expr 'import doc/manual/generate-options.nix (builtins.fromJSON (builtins.readFile $<))' >> $@ + @cat doc/manual/src/command-ref/conf-file-prefix.md > $@.tmp + $(trace-gen) $(nix-eval) 'import doc/manual/generate-options.nix (builtins.fromJSON (builtins.readFile $<))' >> $@.tmp + @mv $@.tmp $@ $(d)/nix.json: $(bindir)/nix - $(trace-gen) $(bindir)/nix __dump-args > $@ + $(trace-gen) $(bindir)/nix __dump-args > $@.tmp + @mv $@.tmp $@ $(d)/conf-file.json: $(bindir)/nix - $(trace-gen) env -i NIX_CONF_DIR=/dummy HOME=/dummy $(bindir)/nix show-config --json --experimental-features nix-command > $@ + $(trace-gen) env -i NIX_CONF_DIR=/dummy HOME=/dummy $(bindir)/nix show-config --json --experimental-features nix-command > $@.tmp + @mv $@.tmp $@ $(d)/src/expressions/builtins.md: $(d)/builtins.json $(d)/generate-builtins.nix $(d)/src/expressions/builtins-prefix.md $(bindir)/nix - @cat doc/manual/src/expressions/builtins-prefix.md > $@ - $(trace-gen) $(bindir)/nix eval --experimental-features nix-command --impure --raw --expr 'import doc/manual/generate-builtins.nix (builtins.fromJSON (builtins.readFile $<))' >> $@ + @cat doc/manual/src/expressions/builtins-prefix.md > $@.tmp + $(trace-gen) $(nix-eval) 'import doc/manual/generate-builtins.nix (builtins.fromJSON (builtins.readFile $<))' >> $@.tmp + @mv $@.tmp $@ $(d)/builtins.json: $(bindir)/nix - $(trace-gen) $(bindir)/nix __dump-builtins > $@ + $(trace-gen) NIX_PATH=nix/corepkgs=corepkgs $(bindir)/nix __dump-builtins > $@.tmp + mv $@.tmp $@ # Generate the HTML manual. install: $(docdir)/manual/index.html diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index d678231c6..139067f20 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -370,7 +370,11 @@ EvalState::EvalState(const Strings & _searchPath, ref store) for (auto & i : _searchPath) addToSearchPath(i); for (auto & i : evalSettings.nixPath.get()) addToSearchPath(i); } - addToSearchPath("nix=" + canonPath(settings.nixDataDir + "/nix/corepkgs", true)); + + try { + addToSearchPath("nix=" + canonPath(settings.nixDataDir + "/nix/corepkgs", true)); + } catch (Error &) { + } if (evalSettings.restrictEval || evalSettings.pureEval) { allowedPaths = PathSet(); diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 816980f10..d0b0c57b2 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -3565,10 +3565,13 @@ void EvalState::createBaseEnv() /* Add a wrapper around the derivation primop that computes the `drvPath' and `outPath' attributes lazily. */ - string path = canonPath(settings.nixDataDir + "/nix/corepkgs/derivation.nix", true); - sDerivationNix = symbols.create(path); - evalFile(path, v); - addConstant("derivation", v); + try { + string path = canonPath(settings.nixDataDir + "/nix/corepkgs/derivation.nix", true); + sDerivationNix = symbols.create(path); + evalFile(path, v); + addConstant("derivation", v); + } catch (SysError &) { + } /* Now that we've added all primops, sort the `builtins' set, because attribute lookups expect it to be sorted. */ diff --git a/src/nix/main.cc b/src/nix/main.cc index e9479f564..1e9e07bc0 100644 --- a/src/nix/main.cc +++ b/src/nix/main.cc @@ -185,6 +185,7 @@ void mainWrapped(int argc, char * * argv) } if (argc == 2 && std::string(argv[1]) == "__dump-builtins") { + evalSettings.pureEval = false; EvalState state({}, openStore("dummy://")); auto res = nlohmann::json::object(); auto builtins = state.baseEnv.values[0]->attrs;