From ce2f91d356438297fd795bd3edb8f9f4536db7da Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 11 Jan 2023 18:57:18 -0500 Subject: [PATCH] Split `OutputsSpec` and `ExtendedOutputsSpec`, use the former more `DerivedPath::Built` and `DerivationGoal` were previously using a regular set with the convention that the empty set means all outputs. But it is easy to forget about this rule when processing those sets. Using `OutputSpec` forces us to get it right. --- src/libcmd/installables.cc | 107 ++++++-------- src/libexpr/flake/flakeref.cc | 2 +- src/libexpr/primops.cc | 16 +-- src/libstore/build/derivation-goal.cc | 47 ++++--- src/libstore/build/derivation-goal.hh | 9 +- src/libstore/build/entry-points.cc | 3 +- src/libstore/build/local-derivation-goal.cc | 2 +- src/libstore/build/worker.cc | 6 +- src/libstore/build/worker.hh | 6 +- src/libstore/derivations.cc | 6 - src/libstore/derivations.hh | 2 - src/libstore/derived-path.cc | 25 ++-- src/libstore/derived-path.hh | 3 +- src/libstore/misc.cc | 45 +++++- src/libstore/outputs-spec.cc | 148 ++++++++++++++++---- src/libstore/outputs-spec.hh | 46 +++++- src/libstore/path-with-outputs.cc | 23 ++- src/libstore/path.hh | 1 - src/libstore/remote-store.cc | 11 +- src/libstore/store-api.hh | 10 ++ src/libstore/tests/outputs-spec.cc | 42 ++++-- src/nix-build/nix-build.cc | 4 +- src/nix/app.cc | 6 +- 23 files changed, 377 insertions(+), 193 deletions(-) diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index b80ae4df1..58b5ef9b9 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -488,18 +488,23 @@ struct InstallableAttrPath : InstallableValue if (!drvPath) throw Error("'%s' is not a derivation", what()); - std::set outputsToInstall; + auto derivedPath = byDrvPath.emplace(*drvPath, DerivedPath::Built { + .drvPath = *drvPath, + // Not normally legal, but we will merge right below + .outputs = OutputsSpec::Names { }, + }).first; - if (auto outputNames = std::get_if(&extendedOutputsSpec)) - outputsToInstall = *outputNames; - else - for (auto & output : drvInfo.queryOutputs(false, std::get_if(&extendedOutputsSpec))) - outputsToInstall.insert(output.first); - - auto derivedPath = byDrvPath.emplace(*drvPath, DerivedPath::Built { .drvPath = *drvPath }).first; - - for (auto & output : outputsToInstall) - derivedPath->second.outputs.insert(output); + derivedPath->second.outputs.merge(std::visit(overloaded { + [&](const ExtendedOutputsSpec::Default & d) -> OutputsSpec { + std::set outputsToInstall; + for (auto & output : drvInfo.queryOutputs(false, true)) + outputsToInstall.insert(output.first); + return OutputsSpec::Names { std::move(outputsToInstall) }; + }, + [&](const ExtendedOutputsSpec::Explicit & e) -> OutputsSpec { + return e; + }, + }, extendedOutputsSpec.raw())); } DerivedPathsWithInfo res; @@ -639,41 +644,40 @@ DerivedPathsWithInfo InstallableFlake::toDerivedPaths() auto drvPath = attr->forceDerivation(); - std::set outputsToInstall; std::optional priority; - if (auto aOutputSpecified = attr->maybeGetAttr(state->sOutputSpecified)) { - if (aOutputSpecified->getBool()) { - if (auto aOutputName = attr->maybeGetAttr("outputName")) - outputsToInstall = { aOutputName->getString() }; - } - } - - else if (auto aMeta = attr->maybeGetAttr(state->sMeta)) { - if (auto aOutputsToInstall = aMeta->maybeGetAttr("outputsToInstall")) - for (auto & s : aOutputsToInstall->getListOfStrings()) - outputsToInstall.insert(s); + if (attr->maybeGetAttr(state->sOutputSpecified)) { + } else if (auto aMeta = attr->maybeGetAttr(state->sMeta)) { if (auto aPriority = aMeta->maybeGetAttr("priority")) priority = aPriority->getInt(); } - if (outputsToInstall.empty() || std::get_if(&extendedOutputsSpec)) { - outputsToInstall.clear(); - if (auto aOutputs = attr->maybeGetAttr(state->sOutputs)) - for (auto & s : aOutputs->getListOfStrings()) - outputsToInstall.insert(s); - } - - if (outputsToInstall.empty()) - outputsToInstall.insert("out"); - - if (auto outputNames = std::get_if(&extendedOutputsSpec)) - outputsToInstall = *outputNames; - return {{ .path = DerivedPath::Built { .drvPath = std::move(drvPath), - .outputs = std::move(outputsToInstall), + .outputs = std::visit(overloaded { + [&](const ExtendedOutputsSpec::Default & d) -> OutputsSpec { + std::set outputsToInstall; + if (auto aOutputSpecified = attr->maybeGetAttr(state->sOutputSpecified)) { + if (aOutputSpecified->getBool()) { + if (auto aOutputName = attr->maybeGetAttr("outputName")) + outputsToInstall = { aOutputName->getString() }; + } + } else if (auto aMeta = attr->maybeGetAttr(state->sMeta)) { + if (auto aOutputsToInstall = aMeta->maybeGetAttr("outputsToInstall")) + for (auto & s : aOutputsToInstall->getListOfStrings()) + outputsToInstall.insert(s); + } + + if (outputsToInstall.empty()) + outputsToInstall.insert("out"); + + return OutputsSpec::Names { std::move(outputsToInstall) }; + }, + [&](const ExtendedOutputsSpec::Explicit & e) -> OutputsSpec { + return e; + }, + }, extendedOutputsSpec.raw()), }, .info = { .priority = priority, @@ -801,7 +805,7 @@ std::vector> SourceExprCommand::parseInstallables( result.push_back( std::make_shared( state, *this, vFile, - prefix == "." ? "" : prefix, + prefix == "." ? "" : std::string { prefix }, extendedOutputsSpec)); } @@ -918,32 +922,7 @@ std::vector, BuiltPathWithResult>> Instal for (auto & aux : backmap[path]) { std::visit(overloaded { [&](const DerivedPath::Built & bfd) { - OutputPathMap outputs; - auto drv = evalStore->readDerivation(bfd.drvPath); - auto outputHashes = staticOutputHashes(*evalStore, drv); // FIXME: expensive - auto drvOutputs = drv.outputsAndOptPaths(*store); - for (auto & output : bfd.outputs) { - auto outputHash = get(outputHashes, output); - if (!outputHash) - throw Error( - "the derivation '%s' doesn't have an output named '%s'", - store->printStorePath(bfd.drvPath), output); - if (settings.isExperimentalFeatureEnabled(Xp::CaDerivations)) { - DrvOutput outputId { *outputHash, output }; - auto realisation = store->queryRealisation(outputId); - if (!realisation) - throw MissingRealisation(outputId); - outputs.insert_or_assign(output, realisation->outPath); - } else { - // If ca-derivations isn't enabled, assume that - // the output path is statically known. - auto drvOutput = get(drvOutputs, output); - assert(drvOutput); - assert(drvOutput->second); - outputs.insert_or_assign( - output, *drvOutput->second); - } - } + auto outputs = resolveDerivedPath(*store, bfd, &*evalStore); res.push_back({aux.installable, { .path = BuiltPath::Built { bfd.drvPath, outputs }, .info = aux.info}}); diff --git a/src/libexpr/flake/flakeref.cc b/src/libexpr/flake/flakeref.cc index bc61e2c9a..08adbe0c9 100644 --- a/src/libexpr/flake/flakeref.cc +++ b/src/libexpr/flake/flakeref.cc @@ -245,7 +245,7 @@ std::tuple parseFlakeRefWithFragment bool isFlake) { auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse(url); - auto [flakeRef, fragment] = parseFlakeRefWithFragment(prefix, baseDir, allowMissing, isFlake); + auto [flakeRef, fragment] = parseFlakeRefWithFragment(std::string { prefix }, baseDir, allowMissing, isFlake); return {std::move(flakeRef), fragment, extendedOutputsSpec}; } diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index a08fef011..9cff4b365 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -53,7 +53,7 @@ StringMap EvalState::realiseContext(const PathSet & context) [&](const NixStringContextElem::Built & b) { drvs.push_back(DerivedPath::Built { .drvPath = b.drvPath, - .outputs = std::set { b.output }, + .outputs = OutputsSpec::Names { b.output }, }); ensureValid(b.drvPath); }, @@ -84,16 +84,12 @@ StringMap EvalState::realiseContext(const PathSet & context) store->buildPaths(buildReqs); /* Get all the output paths corresponding to the placeholders we had */ - for (auto & [drvPath, outputs] : drvs) { - const auto outputPaths = store->queryDerivationOutputMap(drvPath); - for (auto & outputName : outputs) { - auto outputPath = get(outputPaths, outputName); - if (!outputPath) - debugThrowLastTrace(Error("derivation '%s' does not have an output named '%s'", - store->printStorePath(drvPath), outputName)); + for (auto & drv : drvs) { + auto outputs = resolveDerivedPath(*store, drv); + for (auto & [outputName, outputPath] : outputs) { res.insert_or_assign( - downstreamPlaceholder(*store, drvPath, outputName), - store->printStorePath(*outputPath) + downstreamPlaceholder(*store, drv.drvPath, outputName), + store->printStorePath(outputPath) ); } } diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 5e86b5269..98c1ddaae 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -63,7 +63,7 @@ namespace nix { DerivationGoal::DerivationGoal(const StorePath & drvPath, - const StringSet & wantedOutputs, Worker & worker, BuildMode buildMode) + const OutputsSpec & wantedOutputs, Worker & worker, BuildMode buildMode) : Goal(worker, DerivedPath::Built { .drvPath = drvPath, .outputs = wantedOutputs }) , useDerivation(true) , drvPath(drvPath) @@ -82,7 +82,7 @@ DerivationGoal::DerivationGoal(const StorePath & drvPath, DerivationGoal::DerivationGoal(const StorePath & drvPath, const BasicDerivation & drv, - const StringSet & wantedOutputs, Worker & worker, BuildMode buildMode) + const OutputsSpec & wantedOutputs, Worker & worker, BuildMode buildMode) : Goal(worker, DerivedPath::Built { .drvPath = drvPath, .outputs = wantedOutputs }) , useDerivation(false) , drvPath(drvPath) @@ -142,18 +142,11 @@ void DerivationGoal::work() (this->*state)(); } -void DerivationGoal::addWantedOutputs(const StringSet & outputs) +void DerivationGoal::addWantedOutputs(const OutputsSpec & outputs) { - /* If we already want all outputs, there is nothing to do. */ - if (wantedOutputs.empty()) return; - - if (outputs.empty()) { - wantedOutputs.clear(); + bool newOutputs = wantedOutputs.merge(outputs); + if (newOutputs) needRestart = true; - } else - for (auto & i : outputs) - if (wantedOutputs.insert(i).second) - needRestart = true; } @@ -390,7 +383,7 @@ void DerivationGoal::repairClosure() auto outputs = queryDerivationOutputMap(); StorePathSet outputClosure; for (auto & i : outputs) { - if (!wantOutput(i.first, wantedOutputs)) continue; + if (!wantedOutputs.contains(i.first)) continue; worker.store.computeFSClosure(i.second, outputClosure); } @@ -422,7 +415,7 @@ void DerivationGoal::repairClosure() if (drvPath2 == outputsToDrv.end()) addWaitee(upcast_goal(worker.makePathSubstitutionGoal(i, Repair))); else - addWaitee(worker.makeDerivationGoal(drvPath2->second, StringSet(), bmRepair)); + addWaitee(worker.makeDerivationGoal(drvPath2->second, OutputsSpec::All(), bmRepair)); } if (waitees.empty()) { @@ -991,10 +984,15 @@ void DerivationGoal::resolvedFinished() StorePathSet outputPaths; - // `wantedOutputs` might be empty, which means “all the outputs” - auto realWantedOutputs = wantedOutputs; - if (realWantedOutputs.empty()) - realWantedOutputs = resolvedDrv.outputNames(); + // `wantedOutputs` might merely indicate “all the outputs” + auto realWantedOutputs = std::visit(overloaded { + [&](const OutputsSpec::All &) { + return resolvedDrv.outputNames(); + }, + [&](const OutputsSpec::Names & names) { + return names; + }, + }, wantedOutputs.raw()); for (auto & wantedOutput : realWantedOutputs) { auto initialOutput = get(initialOutputs, wantedOutput); @@ -1322,7 +1320,14 @@ std::pair DerivationGoal::checkPathValidity() if (!drv->type().isPure()) return { false, {} }; bool checkHash = buildMode == bmRepair; - auto wantedOutputsLeft = wantedOutputs; + auto wantedOutputsLeft = std::visit(overloaded { + [&](const OutputsSpec::All &) { + return StringSet {}; + }, + [&](const OutputsSpec::Names & names) { + return names; + }, + }, wantedOutputs.raw()); DrvOutputs validOutputs; for (auto & i : queryPartialDerivationOutputMap()) { @@ -1331,7 +1336,7 @@ std::pair DerivationGoal::checkPathValidity() // this is an invalid output, gets catched with (!wantedOutputsLeft.empty()) continue; auto & info = *initialOutput; - info.wanted = wantOutput(i.first, wantedOutputs); + info.wanted = wantedOutputs.contains(i.first); if (info.wanted) wantedOutputsLeft.erase(i.first); if (i.second) { @@ -1369,7 +1374,7 @@ std::pair DerivationGoal::checkPathValidity() validOutputs.emplace(drvOutput, Realisation { drvOutput, info.known->path }); } - // If we requested all the outputs via the empty set, we are always fine. + // If we requested all the outputs, we are always fine. // If we requested specific elements, the loop above removes all the valid // ones, so any that are left must be invalid. if (!wantedOutputsLeft.empty()) diff --git a/src/libstore/build/derivation-goal.hh b/src/libstore/build/derivation-goal.hh index d33e04cbc..707e38b4b 100644 --- a/src/libstore/build/derivation-goal.hh +++ b/src/libstore/build/derivation-goal.hh @@ -2,6 +2,7 @@ #include "parsed-derivations.hh" #include "lock.hh" +#include "outputs-spec.hh" #include "store-api.hh" #include "pathlocks.hh" #include "goal.hh" @@ -55,7 +56,7 @@ struct DerivationGoal : public Goal /* The specific outputs that we need to build. Empty means all of them. */ - StringSet wantedOutputs; + OutputsSpec wantedOutputs; /* Mapping from input derivations + output names to actual store paths. This is filled in by waiteeDone() as each dependency @@ -128,10 +129,10 @@ struct DerivationGoal : public Goal std::string machineName; DerivationGoal(const StorePath & drvPath, - const StringSet & wantedOutputs, Worker & worker, + const OutputsSpec & wantedOutputs, Worker & worker, BuildMode buildMode = bmNormal); DerivationGoal(const StorePath & drvPath, const BasicDerivation & drv, - const StringSet & wantedOutputs, Worker & worker, + const OutputsSpec & wantedOutputs, Worker & worker, BuildMode buildMode = bmNormal); virtual ~DerivationGoal(); @@ -142,7 +143,7 @@ struct DerivationGoal : public Goal void work() override; /* Add wanted outputs to an already existing derivation goal. */ - void addWantedOutputs(const StringSet & outputs); + void addWantedOutputs(const OutputsSpec & outputs); /* The states. */ void getDerivation(); diff --git a/src/libstore/build/entry-points.cc b/src/libstore/build/entry-points.cc index e1b80165e..df7722733 100644 --- a/src/libstore/build/entry-points.cc +++ b/src/libstore/build/entry-points.cc @@ -130,7 +130,8 @@ void LocalStore::repairPath(const StorePath & path) auto info = queryPathInfo(path); if (info->deriver && isValidPath(*info->deriver)) { goals.clear(); - goals.insert(worker.makeDerivationGoal(*info->deriver, StringSet(), bmRepair)); + // FIXME: Should just build the specific output we need. + goals.insert(worker.makeDerivationGoal(*info->deriver, OutputsSpec::All { }, bmRepair)); worker.run(goals); } else throw Error(worker.exitStatus(), "cannot repair path '%s'", printStorePath(path)); diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 488e06d8c..09cb6b233 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -2735,7 +2735,7 @@ DrvOutputs LocalDerivationGoal::registerOutputs() signRealisation(thisRealisation); worker.store.registerDrvOutput(thisRealisation); } - if (wantOutput(outputName, wantedOutputs)) + if (wantedOutputs.contains(outputName)) builtOutputs.emplace(thisRealisation.id, thisRealisation); } diff --git a/src/libstore/build/worker.cc b/src/libstore/build/worker.cc index b192fbc77..b94fb8416 100644 --- a/src/libstore/build/worker.cc +++ b/src/libstore/build/worker.cc @@ -42,7 +42,7 @@ Worker::~Worker() std::shared_ptr Worker::makeDerivationGoalCommon( const StorePath & drvPath, - const StringSet & wantedOutputs, + const OutputsSpec & wantedOutputs, std::function()> mkDrvGoal) { std::weak_ptr & goal_weak = derivationGoals[drvPath]; @@ -59,7 +59,7 @@ std::shared_ptr Worker::makeDerivationGoalCommon( std::shared_ptr Worker::makeDerivationGoal(const StorePath & drvPath, - const StringSet & wantedOutputs, BuildMode buildMode) + const OutputsSpec & wantedOutputs, BuildMode buildMode) { return makeDerivationGoalCommon(drvPath, wantedOutputs, [&]() -> std::shared_ptr { return !dynamic_cast(&store) @@ -70,7 +70,7 @@ std::shared_ptr Worker::makeDerivationGoal(const StorePath & drv std::shared_ptr Worker::makeBasicDerivationGoal(const StorePath & drvPath, - const BasicDerivation & drv, const StringSet & wantedOutputs, BuildMode buildMode) + const BasicDerivation & drv, const OutputsSpec & wantedOutputs, BuildMode buildMode) { return makeDerivationGoalCommon(drvPath, wantedOutputs, [&]() -> std::shared_ptr { return !dynamic_cast(&store) diff --git a/src/libstore/build/worker.hh b/src/libstore/build/worker.hh index a1e036a96..6d68d3cf1 100644 --- a/src/libstore/build/worker.hh +++ b/src/libstore/build/worker.hh @@ -140,15 +140,15 @@ public: /* derivation goal */ private: std::shared_ptr makeDerivationGoalCommon( - const StorePath & drvPath, const StringSet & wantedOutputs, + const StorePath & drvPath, const OutputsSpec & wantedOutputs, std::function()> mkDrvGoal); public: std::shared_ptr makeDerivationGoal( const StorePath & drvPath, - const StringSet & wantedOutputs, BuildMode buildMode = bmNormal); + const OutputsSpec & wantedOutputs, BuildMode buildMode = bmNormal); std::shared_ptr makeBasicDerivationGoal( const StorePath & drvPath, const BasicDerivation & drv, - const StringSet & wantedOutputs, BuildMode buildMode = bmNormal); + const OutputsSpec & wantedOutputs, BuildMode buildMode = bmNormal); /* substitution goal */ std::shared_ptr makePathSubstitutionGoal(const StorePath & storePath, RepairFlag repair = NoRepair, std::optional ca = std::nullopt); diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index 42a53912e..cf18724ef 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -688,12 +688,6 @@ std::map staticOutputHashes(Store & store, const Derivation & } -bool wantOutput(const std::string & output, const std::set & wanted) -{ - return wanted.empty() || wanted.find(output) != wanted.end(); -} - - static DerivationOutput readDerivationOutput(Source & in, const Store & store) { const auto pathS = readString(in); diff --git a/src/libstore/derivations.hh b/src/libstore/derivations.hh index f3cd87fb1..7ee3ded6a 100644 --- a/src/libstore/derivations.hh +++ b/src/libstore/derivations.hh @@ -294,8 +294,6 @@ typedef std::map DrvHashes; // FIXME: global, though at least thread-safe. extern Sync drvHashes; -bool wantOutput(const std::string & output, const std::set & wanted); - struct Source; struct Sink; diff --git a/src/libstore/derived-path.cc b/src/libstore/derived-path.cc index 3fa5ae4f7..e0d86a42f 100644 --- a/src/libstore/derived-path.cc +++ b/src/libstore/derived-path.cc @@ -19,11 +19,11 @@ nlohmann::json DerivedPath::Built::toJSON(ref store) const { res["drvPath"] = store->printStorePath(drvPath); // Fallback for the input-addressed derivation case: We expect to always be // able to print the output paths, so let’s do it - const auto knownOutputs = store->queryPartialDerivationOutputMap(drvPath); - for (const auto & output : outputs) { - auto knownOutput = get(knownOutputs, output); - if (knownOutput && *knownOutput) - res["outputs"][output] = store->printStorePath(**knownOutput); + const auto outputMap = store->queryPartialDerivationOutputMap(drvPath); + for (const auto & [output, outputPathOpt] : outputMap) { + if (!outputs.contains(output)) continue; + if (outputPathOpt) + res["outputs"][output] = store->printStorePath(*outputPathOpt); else res["outputs"][output] = nullptr; } @@ -63,7 +63,7 @@ std::string DerivedPath::Built::to_string(const Store & store) const { return store.printStorePath(drvPath) + "!" - + (outputs.empty() ? std::string { "*" } : concatStringsSep(",", outputs)); + + outputs.to_string(); } std::string DerivedPath::to_string(const Store & store) const @@ -81,15 +81,10 @@ DerivedPath::Opaque DerivedPath::Opaque::parse(const Store & store, std::string_ DerivedPath::Built DerivedPath::Built::parse(const Store & store, std::string_view drvS, std::string_view outputsS) { - auto drvPath = store.parseStorePath(drvS); - std::set outputs; - if (outputsS != "*") { - outputs = tokenizeString>(outputsS, ","); - if (outputs.empty()) - throw Error( - "Explicit list of wanted outputs '%s' must not be empty. Consider using '*' as a wildcard meaning all outputs if no output in particular is wanted.", outputsS); - } - return {drvPath, outputs}; + return { + .drvPath = store.parseStorePath(drvS), + .outputs = OutputsSpec::parse(outputsS), + }; } DerivedPath DerivedPath::parse(const Store & store, std::string_view s) diff --git a/src/libstore/derived-path.hh b/src/libstore/derived-path.hh index 706e5dcb4..4edff7467 100644 --- a/src/libstore/derived-path.hh +++ b/src/libstore/derived-path.hh @@ -3,6 +3,7 @@ #include "util.hh" #include "path.hh" #include "realisation.hh" +#include "outputs-spec.hh" #include @@ -44,7 +45,7 @@ struct DerivedPathOpaque { */ struct DerivedPathBuilt { StorePath drvPath; - std::set outputs; + OutputsSpec outputs; std::string to_string(const Store & store) const; static DerivedPathBuilt parse(const Store & store, std::string_view, std::string_view); diff --git a/src/libstore/misc.cc b/src/libstore/misc.cc index fb985c97b..1ff855cd0 100644 --- a/src/libstore/misc.cc +++ b/src/libstore/misc.cc @@ -185,7 +185,7 @@ void Store::queryMissing(const std::vector & targets, knownOutputPaths = false; break; } - if (wantOutput(outputName, bfd.outputs) && !isValidPath(*pathOpt)) + if (bfd.outputs.contains(outputName) && !isValidPath(*pathOpt)) invalid.insert(*pathOpt); } if (knownOutputPaths && invalid.empty()) return; @@ -301,4 +301,47 @@ std::map drvOutputReferences( return drvOutputReferences(Realisation::closure(store, inputRealisations), info->references); } +OutputPathMap resolveDerivedPath(Store & store, const DerivedPath::Built & bfd, Store * evalStore_) +{ + auto & evalStore = evalStore_ ? *evalStore_ : store; + + OutputPathMap outputs; + auto drv = evalStore.readDerivation(bfd.drvPath); + auto outputHashes = staticOutputHashes(store, drv); + auto drvOutputs = drv.outputsAndOptPaths(store); + auto outputNames = std::visit(overloaded { + [&](const OutputsSpec::All &) { + StringSet names; + for (auto & [outputName, _] : drv.outputs) + names.insert(outputName); + return names; + }, + [&](const OutputsSpec::Names & names) { + return names; + }, + }, bfd.outputs); + for (auto & output : outputNames) { + auto outputHash = get(outputHashes, output); + if (!outputHash) + throw Error( + "the derivation '%s' doesn't have an output named '%s'", + store.printStorePath(bfd.drvPath), output); + if (settings.isExperimentalFeatureEnabled(Xp::CaDerivations)) { + DrvOutput outputId { *outputHash, output }; + auto realisation = store.queryRealisation(outputId); + if (!realisation) + throw MissingRealisation(outputId); + outputs.insert_or_assign(output, realisation->outPath); + } else { + // If ca-derivations isn't enabled, assume that + // the output path is statically known. + auto drvOutput = get(drvOutputs, output); + assert(drvOutput); + assert(drvOutput->second); + outputs.insert_or_assign(output, *drvOutput->second); + } + } + return outputs; +} + } diff --git a/src/libstore/outputs-spec.cc b/src/libstore/outputs-spec.cc index c446976bc..e7bd8ebd8 100644 --- a/src/libstore/outputs-spec.cc +++ b/src/libstore/outputs-spec.cc @@ -6,57 +6,153 @@ namespace nix { -std::pair ExtendedOutputsSpec::parse(std::string s) +bool OutputsSpec::contains(const std::string & outputName) const { - static std::regex regex(R"((.*)\^((\*)|([a-z]+(,[a-z]+)*)))"); + return std::visit(overloaded { + [&](const OutputsSpec::All &) { + return true; + }, + [&](const OutputsSpec::Names & outputNames) { + return outputNames.count(outputName) > 0; + }, + }, raw()); +} + + +std::optional OutputsSpec::parseOpt(std::string_view s) +{ + static std::regex regex(R"((\*)|([a-z]+(,[a-z]+)*))"); std::smatch match; - if (!std::regex_match(s, match, regex)) - return {s, DefaultOutputs()}; + std::string s2 { s }; // until some improves std::regex + if (!std::regex_match(s2, match, regex)) + return std::nullopt; - if (match[3].matched) - return {match[1], AllOutputs()}; + if (match[1].matched) + return { OutputsSpec::All {} }; - return {match[1], tokenizeString(match[4].str(), ",")}; + if (match[2].matched) + return { tokenizeString(match[2].str(), ",") }; + + assert(false); } + +OutputsSpec OutputsSpec::parse(std::string_view s) +{ + std::optional spec = OutputsSpec::parseOpt(s); + if (!spec) + throw Error("Invalid outputs specifier: '%s'", s); + return *spec; +} + + +std::pair ExtendedOutputsSpec::parse(std::string_view s) +{ + auto found = s.rfind('^'); + + if (found == std::string::npos) + return { s, ExtendedOutputsSpec::Default {} }; + + auto spec = OutputsSpec::parse(s.substr(found + 1)); + return { s.substr(0, found), ExtendedOutputsSpec::Explicit { spec } }; +} + + +std::string OutputsSpec::to_string() const +{ + return std::visit(overloaded { + [&](const OutputsSpec::All &) -> std::string { + return "*"; + }, + [&](const OutputsSpec::Names & outputNames) -> std::string { + return concatStringsSep(",", outputNames); + }, + }, raw()); +} + + std::string ExtendedOutputsSpec::to_string() const { return std::visit(overloaded { [&](const ExtendedOutputsSpec::Default &) -> std::string { return ""; }, - [&](const ExtendedOutputsSpec::All &) -> std::string { - return "*"; - }, - [&](const ExtendedOutputsSpec::Names & outputNames) -> std::string { - return "^" + concatStringsSep(",", outputNames); + [&](const ExtendedOutputsSpec::Explicit & outputSpec) -> std::string { + return "^" + outputSpec.to_string(); }, }, raw()); } + +bool OutputsSpec::merge(const OutputsSpec & that) +{ + return std::visit(overloaded { + [&](OutputsSpec::All &) { + /* If we already refer to all outputs, there is nothing to do. */ + return false; + }, + [&](OutputsSpec::Names & theseNames) { + return std::visit(overloaded { + [&](const OutputsSpec::All &) { + *this = OutputsSpec::All {}; + return true; + }, + [&](const OutputsSpec::Names & thoseNames) { + bool ret = false; + for (auto & i : thoseNames) + if (theseNames.insert(i).second) + ret = true; + return ret; + }, + }, that.raw()); + }, + }, raw()); +} + + +void to_json(nlohmann::json & json, const OutputsSpec & outputsSpec) +{ + std::visit(overloaded { + [&](const OutputsSpec::All &) { + json = std::vector({"*"}); + }, + [&](const OutputsSpec::Names & names) { + json = names; + }, + }, outputsSpec); +} + + void to_json(nlohmann::json & json, const ExtendedOutputsSpec & extendedOutputsSpec) { - if (std::get_if(&extendedOutputsSpec)) - json = nullptr; - - else if (std::get_if(&extendedOutputsSpec)) - json = std::vector({"*"}); - - else if (auto outputNames = std::get_if(&extendedOutputsSpec)) - json = *outputNames; + std::visit(overloaded { + [&](const ExtendedOutputsSpec::Default &) { + json = nullptr; + }, + [&](const ExtendedOutputsSpec::Explicit & e) { + to_json(json, e); + }, + }, extendedOutputsSpec); } + +void from_json(const nlohmann::json & json, OutputsSpec & outputsSpec) +{ + auto names = json.get(); + if (names == OutputNames({"*"})) + outputsSpec = OutputsSpec::All {}; + else + outputsSpec = names; +} + + void from_json(const nlohmann::json & json, ExtendedOutputsSpec & extendedOutputsSpec) { if (json.is_null()) - extendedOutputsSpec = DefaultOutputs(); + extendedOutputsSpec = ExtendedOutputsSpec::Default {}; else { - auto names = json.get(); - if (names == OutputNames({"*"})) - extendedOutputsSpec = AllOutputs(); - else - extendedOutputsSpec = names; + extendedOutputsSpec = ExtendedOutputsSpec::Explicit { json.get() }; } } diff --git a/src/libstore/outputs-spec.hh b/src/libstore/outputs-spec.hh index 5ed711a62..e81695da9 100644 --- a/src/libstore/outputs-spec.hh +++ b/src/libstore/outputs-spec.hh @@ -1,5 +1,6 @@ #pragma once +#include #include #include @@ -13,31 +14,66 @@ struct AllOutputs { bool operator < (const AllOutputs & _) const { return false; } }; +typedef std::variant _OutputsSpecRaw; + +struct OutputsSpec : _OutputsSpecRaw { + using Raw = _OutputsSpecRaw; + using Raw::Raw; + + using Names = OutputNames; + using All = AllOutputs; + + inline const Raw & raw() const { + return static_cast(*this); + } + + inline Raw & raw() { + return static_cast(*this); + } + + bool contains(const std::string & output) const; + + /* Modify the receiver outputs spec so it is the union of it's old value + and the argument. Return whether the output spec needed to be modified + --- if it didn't it was already "large enough". */ + bool merge(const OutputsSpec & outputs); + + /* Parse a string of the form 'output1,...outputN' or + '*', returning the outputs spec. */ + static OutputsSpec parse(std::string_view s); + static std::optional parseOpt(std::string_view s); + + std::string to_string() const; +}; + struct DefaultOutputs { bool operator < (const DefaultOutputs & _) const { return false; } }; -typedef std::variant _ExtendedOutputsSpecRaw; +typedef std::variant _ExtendedOutputsSpecRaw; struct ExtendedOutputsSpec : _ExtendedOutputsSpecRaw { using Raw = _ExtendedOutputsSpecRaw; using Raw::Raw; - using Names = OutputNames; - using All = AllOutputs; using Default = DefaultOutputs; + using Explicit = OutputsSpec; inline const Raw & raw() const { return static_cast(*this); } /* Parse a string of the form 'prefix^output1,...outputN' or - 'prefix^*', returning the prefix and the outputs spec. */ - static std::pair parse(std::string s); + 'prefix^*', returning the prefix and the extended outputs spec. */ + static std::pair parse(std::string_view s); std::string to_string() const; }; + +void to_json(nlohmann::json &, const OutputsSpec &); +void from_json(const nlohmann::json &, OutputsSpec &); + void to_json(nlohmann::json &, const ExtendedOutputsSpec &); void from_json(const nlohmann::json &, ExtendedOutputsSpec &); diff --git a/src/libstore/path-with-outputs.cc b/src/libstore/path-with-outputs.cc index 17230ffc4..10e0cc63e 100644 --- a/src/libstore/path-with-outputs.cc +++ b/src/libstore/path-with-outputs.cc @@ -15,10 +15,14 @@ std::string StorePathWithOutputs::to_string(const Store & store) const DerivedPath StorePathWithOutputs::toDerivedPath() const { - if (!outputs.empty() || path.isDerivation()) - return DerivedPath::Built { path, outputs }; - else + if (!outputs.empty()) { + return DerivedPath::Built { path, OutputsSpec::Names { outputs } }; + } else if (path.isDerivation()) { + assert(outputs.empty()); + return DerivedPath::Built { path, OutputsSpec::All { } }; + } else { return DerivedPath::Opaque { path }; + } } @@ -41,7 +45,18 @@ std::variant StorePathWithOutputs::tryFromDeriv return StorePathWithOutputs { bo.path }; }, [&](const DerivedPath::Built & bfd) -> std::variant { - return StorePathWithOutputs { bfd.drvPath, bfd.outputs }; + return StorePathWithOutputs { + .path = bfd.drvPath, + // Use legacy encoding of wildcard as empty set + .outputs = std::visit(overloaded { + [&](const OutputsSpec::All &) -> StringSet { + return {}; + }, + [&](const OutputsSpec::Names & outputs) { + return outputs; + }, + }, bfd.outputs.raw()), + }; }, }, p.raw()); } diff --git a/src/libstore/path.hh b/src/libstore/path.hh index 77fd0f8dc..0694b4c18 100644 --- a/src/libstore/path.hh +++ b/src/libstore/path.hh @@ -64,7 +64,6 @@ public: typedef std::set StorePathSet; typedef std::vector StorePaths; -typedef std::map OutputPathMap; typedef std::map> StorePathCAMap; diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index ccf7d7e8b..832be08f7 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -867,8 +867,8 @@ std::vector RemoteStore::buildPathsWithResults( OutputPathMap outputs; auto drv = evalStore->readDerivation(bfd.drvPath); const auto outputHashes = staticOutputHashes(*evalStore, drv); // FIXME: expensive - const auto drvOutputs = drv.outputsAndOptPaths(*this); - for (auto & output : bfd.outputs) { + auto built = resolveDerivedPath(*this, bfd, &*evalStore); + for (auto & [output, outputPath] : built) { auto outputHash = get(outputHashes, output); if (!outputHash) throw Error( @@ -882,16 +882,11 @@ std::vector RemoteStore::buildPathsWithResults( throw MissingRealisation(outputId); res.builtOutputs.emplace(realisation->id, *realisation); } else { - // If ca-derivations isn't enabled, assume that - // the output path is statically known. - const auto drvOutput = get(drvOutputs, output); - assert(drvOutput); - assert(drvOutput->second); res.builtOutputs.emplace( outputId, Realisation { .id = outputId, - .outPath = *drvOutput->second, + .outPath = outputPath, }); } } diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 4a88d7216..ad3a7c8c5 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -71,6 +71,9 @@ class NarInfoDiskCache; class Store; +typedef std::map OutputPathMap; + + enum CheckSigsFlag : bool { NoCheckSigs = false, CheckSigs = true }; enum SubstituteFlag : bool { NoSubstitute = false, Substitute = true }; enum AllowInvalidFlag : bool { DisallowInvalid = false, AllowInvalid = true }; @@ -120,6 +123,8 @@ public: typedef std::map Params; + + protected: struct PathInfoCacheValue { @@ -719,6 +724,11 @@ void copyClosure( void removeTempRoots(); +/* Resolve the derived path completely, failing if any derivation output + is unknown. */ +OutputPathMap resolveDerivedPath(Store &, const DerivedPath::Built &, Store * evalStore = nullptr); + + /* Return a Store object to access the Nix store denoted by ‘uri’ (slight misnomer...). Supported values are: diff --git a/src/libstore/tests/outputs-spec.cc b/src/libstore/tests/outputs-spec.cc index a3dd42341..03259e7c7 100644 --- a/src/libstore/tests/outputs-spec.cc +++ b/src/libstore/tests/outputs-spec.cc @@ -4,42 +4,62 @@ namespace nix { +TEST(OutputsSpec_parse, basic) +{ + { + auto outputsSpec = OutputsSpec::parse("*"); + ASSERT_TRUE(std::get_if(&outputsSpec)); + } + + { + auto outputsSpec = OutputsSpec::parse("out"); + ASSERT_TRUE(std::get(outputsSpec) == OutputsSpec::Names({"out"})); + } + + { + auto outputsSpec = OutputsSpec::parse("out,bin"); + ASSERT_TRUE(std::get(outputsSpec) == OutputsSpec::Names({"out", "bin"})); + } + + { + std::optional outputsSpecOpt = OutputsSpec::parseOpt("&*()"); + ASSERT_FALSE(outputsSpecOpt); + } +} + + TEST(ExtendedOutputsSpec_parse, basic) { { auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo"); ASSERT_EQ(prefix, "foo"); - ASSERT_TRUE(std::get_if(&extendedOutputsSpec)); + ASSERT_TRUE(std::get_if(&extendedOutputsSpec)); } { auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo^*"); ASSERT_EQ(prefix, "foo"); - ASSERT_TRUE(std::get_if(&extendedOutputsSpec)); + auto * explicit_p = std::get_if(&extendedOutputsSpec); + ASSERT_TRUE(explicit_p); + ASSERT_TRUE(std::get_if(explicit_p)); } { auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo^out"); ASSERT_EQ(prefix, "foo"); - ASSERT_TRUE(std::get(extendedOutputsSpec) == OutputNames({"out"})); + ASSERT_TRUE(std::get(std::get(extendedOutputsSpec)) == OutputsSpec::Names({"out"})); } { auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo^out,bin"); ASSERT_EQ(prefix, "foo"); - ASSERT_TRUE(std::get(extendedOutputsSpec) == OutputNames({"out", "bin"})); + ASSERT_TRUE(std::get(std::get(extendedOutputsSpec)) == OutputsSpec::Names({"out", "bin"})); } { auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo^bar^out,bin"); ASSERT_EQ(prefix, "foo^bar"); - ASSERT_TRUE(std::get(extendedOutputsSpec) == OutputNames({"out", "bin"})); - } - - { - auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo^&*()"); - ASSERT_EQ(prefix, "foo^&*()"); - ASSERT_TRUE(std::get_if(&extendedOutputsSpec)); + ASSERT_TRUE(std::get(std::get(extendedOutputsSpec)) == OutputsSpec::Names({"out", "bin"})); } } diff --git a/src/nix-build/nix-build.cc b/src/nix-build/nix-build.cc index adcaab686..0a7a21de2 100644 --- a/src/nix-build/nix-build.cc +++ b/src/nix-build/nix-build.cc @@ -397,7 +397,7 @@ static void main_nix_build(int argc, char * * argv) auto bashDrv = drv->requireDrvPath(); pathsToBuild.push_back(DerivedPath::Built { .drvPath = bashDrv, - .outputs = {"out"}, + .outputs = OutputsSpec::Names {"out"}, }); pathsToCopy.insert(bashDrv); shellDrv = bashDrv; @@ -591,7 +591,7 @@ static void main_nix_build(int argc, char * * argv) if (outputName == "") throw Error("derivation '%s' lacks an 'outputName' attribute", store->printStorePath(drvPath)); - pathsToBuild.push_back(DerivedPath::Built{drvPath, {outputName}}); + pathsToBuild.push_back(DerivedPath::Built{drvPath, OutputsSpec::Names{outputName}}); pathsToBuildOrdered.push_back({drvPath, {outputName}}); drvsToCopy.insert(drvPath); diff --git a/src/nix/app.cc b/src/nix/app.cc index c9637dcf5..08cd0ccd4 100644 --- a/src/nix/app.cc +++ b/src/nix/app.cc @@ -86,13 +86,13 @@ UnresolvedApp Installable::toApp(EvalState & state) /* We want all outputs of the drv */ return DerivedPath::Built { .drvPath = d.drvPath, - .outputs = {}, + .outputs = OutputsSpec::All {}, }; }, [&](const NixStringContextElem::Built & b) -> DerivedPath { return DerivedPath::Built { .drvPath = b.drvPath, - .outputs = { b.output }, + .outputs = OutputsSpec::Names { b.output }, }; }, [&](const NixStringContextElem::Opaque & o) -> DerivedPath { @@ -127,7 +127,7 @@ UnresolvedApp Installable::toApp(EvalState & state) return UnresolvedApp { App { .context = { DerivedPath::Built { .drvPath = drvPath, - .outputs = {outputName}, + .outputs = OutputsSpec::Names { outputName }, } }, .program = program, }};