From e1de1fe0d82d8ba702947dcad3b678cbb9ce9333 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 23 Jul 2020 19:02:57 +0000 Subject: [PATCH 1/2] Make `Buildable` a `std::variant` I think this better captures the intent of what's going on: we either have an opaque store path, or a drv path with some outputs. Having this structure will also help us support CA derivations: we'll have to allow the outpath paths to be optional, so the structure we gain now makes up for the structure we loose then. --- src/libstore/content-address.cc | 4 -- src/libstore/store-api.cc | 4 -- src/libutil/util.hh | 5 ++ src/nix/build.cc | 29 +++++++---- src/nix/command.cc | 17 ++++-- src/nix/installables.cc | 91 ++++++++++++++++----------------- src/nix/installables.hh | 14 +++-- src/nix/log.cc | 13 +++-- 8 files changed, 98 insertions(+), 79 deletions(-) diff --git a/src/libstore/content-address.cc b/src/libstore/content-address.cc index 6cb69d0a9..f45f77d5c 100644 --- a/src/libstore/content-address.cc +++ b/src/libstore/content-address.cc @@ -24,10 +24,6 @@ std::string makeFixedOutputCA(FileIngestionMethod method, const Hash & hash) + hash.to_string(Base32, true); } -// FIXME Put this somewhere? -template struct overloaded : Ts... { using Ts::operator()...; }; -template overloaded(Ts...) -> overloaded; - std::string renderContentAddress(ContentAddress ca) { return std::visit(overloaded { [](TextHash th) { diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index ab21b0bd5..0f6f3b98f 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -865,10 +865,6 @@ void ValidPathInfo::sign(const Store & store, const SecretKey & secretKey) sigs.insert(secretKey.signDetached(fingerprint(store))); } -// FIXME Put this somewhere? -template struct overloaded : Ts... { using Ts::operator()...; }; -template overloaded(Ts...) -> overloaded; - bool ValidPathInfo::isContentAddressed(const Store & store) const { if (! ca) return false; diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 42130f6dc..630303a5d 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -601,4 +601,9 @@ constexpr auto enumerate(T && iterable) } +// C++17 std::visit boilerplate +template struct overloaded : Ts... { using Ts::operator()...; }; +template overloaded(Ts...) -> overloaded; + + } diff --git a/src/nix/build.cc b/src/nix/build.cc index 0f7e0e123..927301314 100644 --- a/src/nix/build.cc +++ b/src/nix/build.cc @@ -57,17 +57,24 @@ struct CmdBuild : InstallablesCommand, MixDryRun, MixProfile if (dryRun) return; - if (outLink != "") { - for (size_t i = 0; i < buildables.size(); ++i) { - for (auto & output : buildables[i].outputs) - if (auto store2 = store.dynamic_pointer_cast()) { - std::string symlink = outLink; - if (i) symlink += fmt("-%d", i); - if (output.first != "out") symlink += fmt("-%s", output.first); - store2->addPermRoot(output.second, absPath(symlink), true); - } - } - } + if (outLink != "") + if (auto store2 = store.dynamic_pointer_cast()) + for (size_t i = 0; i < buildables.size(); ++i) + std::visit(overloaded { + [&](BuildableOpaque bo) { + std::string symlink = outLink; + if (i) symlink += fmt("-%d", i); + store2->addPermRoot(bo.path, absPath(symlink), true); + }, + [&](BuildableFromDrv bfd) { + for (auto & output : bfd.outputs) { + std::string symlink = outLink; + if (i) symlink += fmt("-%d", i); + if (output.first != "out") symlink += fmt("-%s", output.first); + store2->addPermRoot(output.second, absPath(symlink), true); + } + }, + }, buildables[i]); updateProfile(buildables); } diff --git a/src/nix/command.cc b/src/nix/command.cc index af36dda89..04715b43a 100644 --- a/src/nix/command.cc +++ b/src/nix/command.cc @@ -131,11 +131,18 @@ void MixProfile::updateProfile(const Buildables & buildables) std::optional result; for (auto & buildable : buildables) { - for (auto & output : buildable.outputs) { - if (result) - throw Error("'--profile' requires that the arguments produce a single store path, but there are multiple"); - result = output.second; - } + std::visit(overloaded { + [&](BuildableOpaque bo) { + result = bo.path; + }, + [&](BuildableFromDrv bfd) { + for (auto & output : bfd.outputs) { + if (result) + throw Error("'--profile' requires that the arguments produce a single store path, but there are multiple"); + result = output.second; + } + }, + }, buildable); } if (!result) diff --git a/src/nix/installables.cc b/src/nix/installables.cc index a13e5a3df..a051950cd 100644 --- a/src/nix/installables.cc +++ b/src/nix/installables.cc @@ -307,16 +307,15 @@ struct InstallableStorePath : Installable for (auto & [name, output] : store->readDerivation(storePath).outputs) outputs.emplace(name, output.path); return { - Buildable { + BuildableFromDrv { .drvPath = storePath, .outputs = std::move(outputs) } }; } else { return { - Buildable { - .drvPath = {}, - .outputs = {{"out", storePath}} + BuildableOpaque { + .path = storePath, } }; } @@ -332,33 +331,20 @@ Buildables InstallableValue::toBuildables() { Buildables res; - StorePathSet drvPaths; + std::map drvsToOutputs; + // Group by derivation, helps with .all in particular for (auto & drv : toDerivations()) { - Buildable b{.drvPath = drv.drvPath}; - drvPaths.insert(drv.drvPath); - auto outputName = drv.outputName; if (outputName == "") - throw Error("derivation '%s' lacks an 'outputName' attribute", state->store->printStorePath(*b.drvPath)); - - b.outputs.emplace(outputName, drv.outPath); - - res.push_back(std::move(b)); + throw Error("derivation '%s' lacks an 'outputName' attribute", state->store->printStorePath(drv.drvPath)); + drvsToOutputs[drv.drvPath].insert_or_assign(outputName, drv.outPath); } - // Hack to recognize .all: if all drvs have the same drvPath, - // merge the buildables. - if (drvPaths.size() == 1) { - Buildable b{.drvPath = *drvPaths.begin()}; - for (auto & b2 : res) - for (auto & output : b2.outputs) - b.outputs.insert_or_assign(output.first, output.second); - Buildables bs; - bs.push_back(std::move(b)); - return bs; - } else - return res; + for (auto & i : drvsToOutputs) + res.push_back(BuildableFromDrv { i.first, i.second }); + + return res; } struct InstallableAttrPath : InstallableValue @@ -653,14 +639,17 @@ Buildables build(ref store, Realise mode, for (auto & i : installables) { for (auto & b : i->toBuildables()) { - if (b.drvPath) { - StringSet outputNames; - for (auto & output : b.outputs) - outputNames.insert(output.first); - pathsToBuild.push_back({*b.drvPath, outputNames}); - } else - for (auto & output : b.outputs) - pathsToBuild.push_back({output.second}); + std::visit(overloaded { + [&](BuildableOpaque bo) { + pathsToBuild.push_back({bo.path}); + }, + [&](BuildableFromDrv bfd) { + StringSet outputNames; + for (auto & output : bfd.outputs) + outputNames.insert(output.first); + pathsToBuild.push_back({bfd.drvPath, outputNames}); + }, + }, b); buildables.push_back(std::move(b)); } } @@ -681,16 +670,23 @@ StorePathSet toStorePaths(ref store, if (operateOn == OperateOn::Output) { for (auto & b : build(store, mode, installables)) - for (auto & output : b.outputs) - outPaths.insert(output.second); + std::visit(overloaded { + [&](BuildableOpaque bo) { + outPaths.insert(bo.path); + }, + [&](BuildableFromDrv bfd) { + for (auto & output : bfd.outputs) + outPaths.insert(output.second); + }, + }, b); } else { if (mode == Realise::Nothing) settings.readOnlyMode = true; for (auto & i : installables) for (auto & b : i->toBuildables()) - if (b.drvPath) - outPaths.insert(*b.drvPath); + if (auto bfd = std::get_if(&b)) + outPaths.insert(bfd->drvPath); } return outPaths; @@ -714,20 +710,21 @@ StorePathSet toDerivations(ref store, StorePathSet drvPaths; for (auto & i : installables) - for (auto & b : i->toBuildables()) { - if (!b.drvPath) { - if (!useDeriver) - throw Error("argument '%s' did not evaluate to a derivation", i->what()); - for (auto & output : b.outputs) { - auto derivers = store->queryValidDerivers(output.second); + for (auto & b : i->toBuildables()) + std::visit(overloaded { + [&](BuildableOpaque bo) { + if (!useDeriver) + throw Error("argument '%s' did not evaluate to a derivation", i->what()); + auto derivers = store->queryValidDerivers(bo.path); if (derivers.empty()) throw Error("'%s' does not have a known deriver", i->what()); // FIXME: use all derivers? drvPaths.insert(*derivers.begin()); - } - } else - drvPaths.insert(*b.drvPath); - } + }, + [&](BuildableFromDrv bfd) { + drvPaths.insert(bfd.drvPath); + }, + }, b); return drvPaths; } diff --git a/src/nix/installables.hh b/src/nix/installables.hh index eb34365d4..9edff3331 100644 --- a/src/nix/installables.hh +++ b/src/nix/installables.hh @@ -14,12 +14,20 @@ struct SourceExprCommand; namespace eval_cache { class EvalCache; class AttrCursor; } -struct Buildable -{ - std::optional drvPath; +struct BuildableOpaque { + StorePath path; +}; + +struct BuildableFromDrv { + StorePath drvPath; std::map outputs; }; +typedef std::variant< + BuildableOpaque, + BuildableFromDrv +> Buildable; + typedef std::vector Buildables; struct App diff --git a/src/nix/log.cc b/src/nix/log.cc index 7e10d373a..33380dcf5 100644 --- a/src/nix/log.cc +++ b/src/nix/log.cc @@ -45,11 +45,14 @@ struct CmdLog : InstallableCommand RunPager pager; for (auto & sub : subs) { - auto log = b.drvPath ? sub->getBuildLog(*b.drvPath) : nullptr; - for (auto & output : b.outputs) { - if (log) break; - log = sub->getBuildLog(output.second); - } + auto log = std::visit(overloaded { + [&](BuildableOpaque bo) { + return sub->getBuildLog(bo.path); + }, + [&](BuildableFromDrv bfd) { + return sub->getBuildLog(bfd.drvPath); + }, + }, b); if (!log) continue; stopProgressBar(); printInfo("got build log for '%s' from '%s'", installable->what(), sub->getUri()); From a9bbfaa8515de7107457fda2a7aef87aa198788e Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 5 Aug 2020 16:27:15 +0000 Subject: [PATCH 2/2] Fix --profile with multiple opaque paths --- src/nix/command.cc | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/nix/command.cc b/src/nix/command.cc index 04715b43a..da32819da 100644 --- a/src/nix/command.cc +++ b/src/nix/command.cc @@ -128,27 +128,25 @@ void MixProfile::updateProfile(const Buildables & buildables) { if (!profile) return; - std::optional result; + std::vector result; for (auto & buildable : buildables) { std::visit(overloaded { [&](BuildableOpaque bo) { - result = bo.path; + result.push_back(bo.path); }, [&](BuildableFromDrv bfd) { for (auto & output : bfd.outputs) { - if (result) - throw Error("'--profile' requires that the arguments produce a single store path, but there are multiple"); - result = output.second; + result.push_back(output.second); } }, }, buildable); } - if (!result) - throw Error("'--profile' requires that the arguments produce a single store path, but there are none"); + if (result.size() != 1) + throw Error("'--profile' requires that the arguments produce a single store path, but there are %d", result.size()); - updateProfile(*result); + updateProfile(result[0]); } MixDefaultProfile::MixDefaultProfile()