From e1de1fe0d82d8ba702947dcad3b678cbb9ce9333 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 23 Jul 2020 19:02:57 +0000 Subject: [PATCH 1/6] 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/6] 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() From f1a47a96b6ab55d0b0017df5b94b3da69c65bf21 Mon Sep 17 00:00:00 2001 From: Ben Burdette Date: Wed, 5 Aug 2020 10:58:00 -0600 Subject: [PATCH 3/6] error messages for issue 2238 --- src/build-remote/build-remote.cc | 33 +++++++++++++++++++++++++++++++- src/libstore/build.cc | 13 +++++++++++-- 2 files changed, 43 insertions(+), 3 deletions(-) diff --git a/src/build-remote/build-remote.cc b/src/build-remote/build-remote.cc index 3579d8fff..2414a47c1 100644 --- a/src/build-remote/build-remote.cc +++ b/src/build-remote/build-remote.cc @@ -103,7 +103,7 @@ static int _main(int argc, char * * argv) drvPath = store->parseStorePath(readString(source)); auto requiredFeatures = readStrings>(source); - auto canBuildLocally = amWilling + auto canBuildLocally = amWilling && ( neededSystem == settings.thisSystem || settings.extraPlatforms.get().count(neededSystem) > 0) && allSupportedLocally(requiredFeatures); @@ -170,7 +170,38 @@ static int _main(int argc, char * * argv) if (rightType && !canBuildLocally) std::cerr << "# postpone\n"; else + { + // build the hint template. + string hintstring = "required (system, features): (%s, %s)"; + hintstring += "\n%s available machines:"; + hintstring += "\n(systems, maxjobs, supportedFeatures, mandatoryFeatures)"; + + for (unsigned int i = 0; i < machines.size(); ++i) { + hintstring += "\n(%s, %s, %s, %s)"; + } + + // add the template values. + auto hint = hintformat(hintstring); + hint + % neededSystem + % concatStringsSep(", ", requiredFeatures) + % machines.size(); + + for (auto & m : machines) { + hint % concatStringsSep>(", ", m.systemTypes) + % m.maxJobs + % concatStringsSep(", ", m.supportedFeatures) + % concatStringsSep(", ", m.mandatoryFeatures); + } + + logError({ + .name = "Remote build", + .description = "Failed to find a machine for remote build!", + .hint = hint + }); + std::cerr << "# decline\n"; + } break; } diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 94c398b2f..76baa1a6e 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -4876,8 +4876,17 @@ void Worker::run(const Goals & _topGoals) waitForInput(); else { if (awake.empty() && 0 == settings.maxBuildJobs) - throw Error("unable to start any build; either increase '--max-jobs' " - "or enable remote builds"); + { + if (getMachines().empty()) + throw Error("unable to start any build; either increase '--max-jobs' " + "or enable remote builds." + "\nhttps://nixos.org/nix/manual/#chap-distributed-builds"); + else + throw Error("unable to start any build; remote machines may not have " + "all required system features." + "\nhttps://nixos.org/nix/manual/#chap-distributed-builds"); + + } assert(!awake.empty()); } } From e4eae078a52fa4209bb5a46d21dbed09dd9c54ec Mon Sep 17 00:00:00 2001 From: Ben Burdette Date: Wed, 5 Aug 2020 11:21:36 -0600 Subject: [PATCH 4/6] add derivation path to hint --- src/build-remote/build-remote.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/build-remote/build-remote.cc b/src/build-remote/build-remote.cc index 2414a47c1..723e1463e 100644 --- a/src/build-remote/build-remote.cc +++ b/src/build-remote/build-remote.cc @@ -172,7 +172,7 @@ static int _main(int argc, char * * argv) else { // build the hint template. - string hintstring = "required (system, features): (%s, %s)"; + string hintstring = "derivation: %s\nrequired (system, features): (%s, %s)"; hintstring += "\n%s available machines:"; hintstring += "\n(systems, maxjobs, supportedFeatures, mandatoryFeatures)"; @@ -183,6 +183,7 @@ static int _main(int argc, char * * argv) // add the template values. auto hint = hintformat(hintstring); hint + % drvPath->to_string() % neededSystem % concatStringsSep(", ", requiredFeatures) % machines.size(); From 31f1af0cabb65e3c5f8f4539500c6b236c387780 Mon Sep 17 00:00:00 2001 From: Ben Burdette Date: Wed, 5 Aug 2020 11:26:06 -0600 Subject: [PATCH 5/6] don't crash if there's no drvPath --- src/build-remote/build-remote.cc | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/build-remote/build-remote.cc b/src/build-remote/build-remote.cc index 723e1463e..5247cefa6 100644 --- a/src/build-remote/build-remote.cc +++ b/src/build-remote/build-remote.cc @@ -181,9 +181,15 @@ static int _main(int argc, char * * argv) } // add the template values. + string drvstr; + if (drvPath.has_value()) + drvstr = drvPath->to_string(); + else + drvstr = ""; + auto hint = hintformat(hintstring); hint - % drvPath->to_string() + % drvstr % neededSystem % concatStringsSep(", ", requiredFeatures) % machines.size(); From 59067f0f587f75908c4f990bcbab29340c7f7652 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 6 Aug 2020 11:40:41 +0200 Subject: [PATCH 6/6] repl.cc: Check for HAVE_BOEHMGC Fixes #3906. --- src/nix/repl.cc | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/nix/repl.cc b/src/nix/repl.cc index fb9050d0d..c3c9e54a8 100644 --- a/src/nix/repl.cc +++ b/src/nix/repl.cc @@ -33,12 +33,17 @@ extern "C" { #include "command.hh" #include "finally.hh" +#if HAVE_BOEHMGC #define GC_INCLUDE_NEW #include +#endif namespace nix { -struct NixRepl : gc +struct NixRepl + #if HAVE_BOEHMGC + : gc + #endif { string curDir; std::unique_ptr state;