From 31875bcfb7ccbbf6e88c2cc62714a2a3794994ec Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 12 Jan 2023 20:20:27 -0500 Subject: [PATCH] Split `OutputsSpec::merge` into `OuputsSpec::{union_, isSubsetOf}` Additionally get rid of the evil time we made an empty `OutputSpec::Names()`. --- src/libcmd/installables.cc | 26 ++++++++------- src/libstore/build/derivation-goal.cc | 5 +-- src/libstore/outputs-spec.cc | 46 +++++++++++++++++++-------- src/libstore/outputs-spec.hh | 9 +++--- 4 files changed, 56 insertions(+), 30 deletions(-) diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index d73109873..5090ea6d2 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -469,20 +469,14 @@ struct InstallableAttrPath : InstallableValue // Backward compatibility hack: group results by drvPath. This // helps keep .all output together. - std::map byDrvPath; + std::map byDrvPath; for (auto & drvInfo : drvInfos) { auto drvPath = drvInfo.queryDrvPath(); if (!drvPath) throw Error("'%s' is not a derivation", what()); - auto derivedPath = byDrvPath.emplace(*drvPath, DerivedPath::Built { - .drvPath = *drvPath, - // Not normally legal, but we will merge right below - .outputs = OutputsSpec::Names { StringSet { } }, - }).first; - - derivedPath->second.outputs.merge(std::visit(overloaded { + auto newOutputs = std::visit(overloaded { [&](const ExtendedOutputsSpec::Default & d) -> OutputsSpec { std::set outputsToInstall; for (auto & output : drvInfo.queryOutputs(false, true)) @@ -492,12 +486,22 @@ struct InstallableAttrPath : InstallableValue [&](const ExtendedOutputsSpec::Explicit & e) -> OutputsSpec { return e; }, - }, extendedOutputsSpec.raw())); + }, extendedOutputsSpec.raw()); + + auto [iter, didInsert] = byDrvPath.emplace(*drvPath, newOutputs); + + if (!didInsert) + iter->second = iter->second.union_(newOutputs); } DerivedPathsWithInfo res; - for (auto & [_, info] : byDrvPath) - res.push_back({ .path = { info } }); + for (auto & [drvPath, outputs] : byDrvPath) + res.push_back({ + .path = DerivedPath::Built { + .drvPath = drvPath, + .outputs = outputs, + }, + }); return res; } diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 61169635a..2021d0023 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -144,9 +144,10 @@ void DerivationGoal::work() void DerivationGoal::addWantedOutputs(const OutputsSpec & outputs) { - bool newOutputs = wantedOutputs.merge(outputs); - if (newOutputs) + auto newWanted = wantedOutputs.union_(outputs); + if (!newWanted.isSubsetOf(wantedOutputs)) needRestart = true; + wantedOutputs = newWanted; } diff --git a/src/libstore/outputs-spec.cc b/src/libstore/outputs-spec.cc index 8e6e40c2b..d0f39a854 100644 --- a/src/libstore/outputs-spec.cc +++ b/src/libstore/outputs-spec.cc @@ -96,24 +96,20 @@ std::string ExtendedOutputsSpec::to_string() const } -bool OutputsSpec::merge(const OutputsSpec & that) +OutputsSpec OutputsSpec::union_(const OutputsSpec & that) const { return std::visit(overloaded { - [&](OutputsSpec::All &) { - /* If we already refer to all outputs, there is nothing to do. */ - return false; + [&](const OutputsSpec::All &) -> OutputsSpec { + return OutputsSpec::All { }; }, - [&](OutputsSpec::Names & theseNames) { + [&](const OutputsSpec::Names & theseNames) -> OutputsSpec { return std::visit(overloaded { - [&](const OutputsSpec::All &) { - *this = OutputsSpec::All {}; - return true; + [&](const OutputsSpec::All &) -> OutputsSpec { + return OutputsSpec::All {}; }, - [&](const OutputsSpec::Names & thoseNames) { - bool ret = false; - for (auto & i : thoseNames) - if (theseNames.insert(i).second) - ret = true; + [&](const OutputsSpec::Names & thoseNames) -> OutputsSpec { + OutputsSpec::Names ret = theseNames; + ret.insert(thoseNames.begin(), thoseNames.end()); return ret; }, }, that.raw()); @@ -121,6 +117,30 @@ bool OutputsSpec::merge(const OutputsSpec & that) }, raw()); } + +bool OutputsSpec::isSubsetOf(const OutputsSpec & that) const +{ + return std::visit(overloaded { + [&](const OutputsSpec::All &) { + return true; + }, + [&](const OutputsSpec::Names & thoseNames) { + return std::visit(overloaded { + [&](const OutputsSpec::All &) { + return false; + }, + [&](const OutputsSpec::Names & theseNames) { + bool ret = true; + for (auto & o : theseNames) + if (thoseNames.count(o) == 0) + ret = false; + return ret; + }, + }, raw()); + }, + }, that.raw()); +} + } namespace nlohmann { diff --git a/src/libstore/outputs-spec.hh b/src/libstore/outputs-spec.hh index 9211a4fc6..82dfad479 100644 --- a/src/libstore/outputs-spec.hh +++ b/src/libstore/outputs-spec.hh @@ -49,10 +49,11 @@ struct OutputsSpec : _OutputsSpecRaw { 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); + /* Create a new OutputsSpec which is the union of this and that. */ + OutputsSpec union_(const OutputsSpec & that) const; + + /* Whether this OutputsSpec is a subset of that. */ + bool isSubsetOf(const OutputsSpec & outputs) const; /* Parse a string of the form 'output1,...outputN' or '*', returning the outputs spec. */