From a8f45b5e5a42daa9bdee640255464d4dbb431352 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 11 Jan 2023 01:51:14 -0500 Subject: [PATCH 01/11] Improve `OutputsSpec` slightly A few little changes preparing for the rest. --- src/libcmd/installables.cc | 3 ++- src/libexpr/flake/flakeref.cc | 2 +- src/libstore/outputs-spec.cc | 26 ++++++++++++++------------ src/libstore/outputs-spec.hh | 26 +++++++++++++++++++------- src/libstore/path-with-outputs.hh | 1 - src/libstore/tests/outputs-spec.cc | 14 +++++++------- src/nix/profile.cc | 6 +++--- 7 files changed, 46 insertions(+), 32 deletions(-) diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index c0db2a715..ce40986d0 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -1,5 +1,6 @@ #include "globals.hh" #include "installables.hh" +#include "outputs-spec.hh" #include "util.hh" #include "command.hh" #include "attr-path.hh" @@ -796,7 +797,7 @@ std::vector> SourceExprCommand::parseInstallables( } for (auto & s : ss) { - auto [prefix, outputsSpec] = parseOutputsSpec(s); + auto [prefix, outputsSpec] = OutputsSpec::parse(s); result.push_back( std::make_shared( state, *this, vFile, diff --git a/src/libexpr/flake/flakeref.cc b/src/libexpr/flake/flakeref.cc index eede493f8..cfa279fb4 100644 --- a/src/libexpr/flake/flakeref.cc +++ b/src/libexpr/flake/flakeref.cc @@ -244,7 +244,7 @@ std::tuple parseFlakeRefWithFragmentAndOutpu bool allowMissing, bool isFlake) { - auto [prefix, outputsSpec] = parseOutputsSpec(url); + auto [prefix, outputsSpec] = OutputsSpec::parse(url); auto [flakeRef, fragment] = parseFlakeRefWithFragment(prefix, baseDir, allowMissing, isFlake); return {std::move(flakeRef), fragment, outputsSpec}; } diff --git a/src/libstore/outputs-spec.cc b/src/libstore/outputs-spec.cc index 76779d193..b5ea7e325 100644 --- a/src/libstore/outputs-spec.cc +++ b/src/libstore/outputs-spec.cc @@ -1,3 +1,4 @@ +#include "util.hh" #include "outputs-spec.hh" #include "nlohmann/json.hpp" @@ -5,7 +6,7 @@ namespace nix { -std::pair parseOutputsSpec(const std::string & s) +std::pair OutputsSpec::parse(std::string s) { static std::regex regex(R"((.*)\^((\*)|([a-z]+(,[a-z]+)*)))"); @@ -19,18 +20,19 @@ std::pair parseOutputsSpec(const std::string & s) return {match[1], tokenizeString(match[4].str(), ",")}; } -std::string printOutputsSpec(const OutputsSpec & outputsSpec) +std::string OutputsSpec::to_string() const { - if (std::get_if(&outputsSpec)) - return ""; - - if (std::get_if(&outputsSpec)) - return "^*"; - - if (auto outputNames = std::get_if(&outputsSpec)) - return "^" + concatStringsSep(",", *outputNames); - - assert(false); + return std::visit(overloaded { + [&](const OutputsSpec::Default &) -> std::string { + return ""; + }, + [&](const OutputsSpec::All &) -> std::string { + return "*"; + }, + [&](const OutputsSpec::Names & outputNames) -> std::string { + return "^" + concatStringsSep(",", outputNames); + }, + }, raw()); } void to_json(nlohmann::json & json, const OutputsSpec & outputsSpec) diff --git a/src/libstore/outputs-spec.hh b/src/libstore/outputs-spec.hh index e2cf1d12b..6f886ccb6 100644 --- a/src/libstore/outputs-spec.hh +++ b/src/libstore/outputs-spec.hh @@ -1,9 +1,8 @@ #pragma once +#include #include -#include "util.hh" - #include "nlohmann/json_fwd.hpp" namespace nix { @@ -18,13 +17,26 @@ struct DefaultOutputs { bool operator < (const DefaultOutputs & _) const { return false; } }; -typedef std::variant OutputsSpec; +typedef std::variant _OutputsSpecRaw; -/* Parse a string of the form 'prefix^output1,...outputN' or - 'prefix^*', returning the prefix and the outputs spec. */ -std::pair parseOutputsSpec(const std::string & s); +struct OutputsSpec : _OutputsSpecRaw { + using Raw = _OutputsSpecRaw; + using Raw::Raw; -std::string printOutputsSpec(const OutputsSpec & outputsSpec); + using Names = OutputNames; + using All = AllOutputs; + using Default = DefaultOutputs; + + 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); + + std::string to_string() const; +}; void to_json(nlohmann::json &, const OutputsSpec &); void from_json(const nlohmann::json &, OutputsSpec &); diff --git a/src/libstore/path-with-outputs.hh b/src/libstore/path-with-outputs.hh index ed55cc333..5d25656a5 100644 --- a/src/libstore/path-with-outputs.hh +++ b/src/libstore/path-with-outputs.hh @@ -2,7 +2,6 @@ #include "path.hh" #include "derived-path.hh" -#include "nlohmann/json_fwd.hpp" namespace nix { diff --git a/src/libstore/tests/outputs-spec.cc b/src/libstore/tests/outputs-spec.cc index d781a930e..552380837 100644 --- a/src/libstore/tests/outputs-spec.cc +++ b/src/libstore/tests/outputs-spec.cc @@ -4,40 +4,40 @@ namespace nix { -TEST(parseOutputsSpec, basic) +TEST(OutputsSpec_parse, basic) { { - auto [prefix, outputsSpec] = parseOutputsSpec("foo"); + auto [prefix, outputsSpec] = OutputsSpec::parse("foo"); ASSERT_EQ(prefix, "foo"); ASSERT_TRUE(std::get_if(&outputsSpec)); } { - auto [prefix, outputsSpec] = parseOutputsSpec("foo^*"); + auto [prefix, outputsSpec] = OutputsSpec::parse("foo^*"); ASSERT_EQ(prefix, "foo"); ASSERT_TRUE(std::get_if(&outputsSpec)); } { - auto [prefix, outputsSpec] = parseOutputsSpec("foo^out"); + auto [prefix, outputsSpec] = OutputsSpec::parse("foo^out"); ASSERT_EQ(prefix, "foo"); ASSERT_TRUE(std::get(outputsSpec) == OutputNames({"out"})); } { - auto [prefix, outputsSpec] = parseOutputsSpec("foo^out,bin"); + auto [prefix, outputsSpec] = OutputsSpec::parse("foo^out,bin"); ASSERT_EQ(prefix, "foo"); ASSERT_TRUE(std::get(outputsSpec) == OutputNames({"out", "bin"})); } { - auto [prefix, outputsSpec] = parseOutputsSpec("foo^bar^out,bin"); + auto [prefix, outputsSpec] = OutputsSpec::parse("foo^bar^out,bin"); ASSERT_EQ(prefix, "foo^bar"); ASSERT_TRUE(std::get(outputsSpec) == OutputNames({"out", "bin"})); } { - auto [prefix, outputsSpec] = parseOutputsSpec("foo^&*()"); + auto [prefix, outputsSpec] = OutputsSpec::parse("foo^&*()"); ASSERT_EQ(prefix, "foo^&*()"); ASSERT_TRUE(std::get_if(&outputsSpec)); } diff --git a/src/nix/profile.cc b/src/nix/profile.cc index 22ee51ab9..346c4e117 100644 --- a/src/nix/profile.cc +++ b/src/nix/profile.cc @@ -44,7 +44,7 @@ struct ProfileElement std::string describe() const { if (source) - return fmt("%s#%s%s", source->originalRef, source->attrPath, printOutputsSpec(source->outputs)); + return fmt("%s#%s%s", source->originalRef, source->attrPath, source->outputs.to_string()); StringSet names; for (auto & path : storePaths) names.insert(DrvName(path.name()).name); @@ -553,8 +553,8 @@ struct CmdProfileList : virtual EvalCommand, virtual StoreCommand, MixDefaultPro for (size_t i = 0; i < manifest.elements.size(); ++i) { auto & element(manifest.elements[i]); logger->cout("%d %s %s %s", i, - element.source ? element.source->originalRef.to_string() + "#" + element.source->attrPath + printOutputsSpec(element.source->outputs) : "-", - element.source ? element.source->resolvedRef.to_string() + "#" + element.source->attrPath + printOutputsSpec(element.source->outputs) : "-", + element.source ? element.source->originalRef.to_string() + "#" + element.source->attrPath + element.source->outputs.to_string() : "-", + element.source ? element.source->resolvedRef.to_string() + "#" + element.source->attrPath + element.source->outputs.to_string() : "-", concatStringsSep(" ", store->printStorePathSet(element.storePaths))); } } From a7c0cff07f3e1af60bdbcd5bf7e13f8ae768da90 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 11 Jan 2023 02:00:44 -0500 Subject: [PATCH 02/11] Rename `OutputPath` -> `ExtendedOutputPath` Do this prior to making a new more limitted `OutputPath` we will use in more places. --- src/libcmd/installables.cc | 28 ++++++++++++++-------------- src/libcmd/installables.hh | 6 +++--- src/libexpr/flake/flakeref.cc | 6 +++--- src/libexpr/flake/flakeref.hh | 2 +- src/libstore/outputs-spec.cc | 26 +++++++++++++------------- src/libstore/outputs-spec.hh | 12 ++++++------ src/libstore/tests/outputs-spec.cc | 26 +++++++++++++------------- src/nix/bundle.cc | 4 ++-- src/nix/profile.cc | 10 +++++----- 9 files changed, 60 insertions(+), 60 deletions(-) diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index ce40986d0..b80ae4df1 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -446,19 +446,19 @@ struct InstallableAttrPath : InstallableValue SourceExprCommand & cmd; RootValue v; std::string attrPath; - OutputsSpec outputsSpec; + ExtendedOutputsSpec extendedOutputsSpec; InstallableAttrPath( ref state, SourceExprCommand & cmd, Value * v, const std::string & attrPath, - OutputsSpec outputsSpec) + ExtendedOutputsSpec extendedOutputsSpec) : InstallableValue(state) , cmd(cmd) , v(allocRootValue(v)) , attrPath(attrPath) - , outputsSpec(std::move(outputsSpec)) + , extendedOutputsSpec(std::move(extendedOutputsSpec)) { } std::string what() const override { return attrPath; } @@ -490,10 +490,10 @@ struct InstallableAttrPath : InstallableValue std::set outputsToInstall; - if (auto outputNames = std::get_if(&outputsSpec)) + if (auto outputNames = std::get_if(&extendedOutputsSpec)) outputsToInstall = *outputNames; else - for (auto & output : drvInfo.queryOutputs(false, std::get_if(&outputsSpec))) + for (auto & output : drvInfo.queryOutputs(false, std::get_if(&extendedOutputsSpec))) outputsToInstall.insert(output.first); auto derivedPath = byDrvPath.emplace(*drvPath, DerivedPath::Built { .drvPath = *drvPath }).first; @@ -581,7 +581,7 @@ InstallableFlake::InstallableFlake( ref state, FlakeRef && flakeRef, std::string_view fragment, - OutputsSpec outputsSpec, + ExtendedOutputsSpec extendedOutputsSpec, Strings attrPaths, Strings prefixes, const flake::LockFlags & lockFlags) @@ -589,7 +589,7 @@ InstallableFlake::InstallableFlake( flakeRef(flakeRef), attrPaths(fragment == "" ? attrPaths : Strings{(std::string) fragment}), prefixes(fragment == "" ? Strings{} : prefixes), - outputsSpec(std::move(outputsSpec)), + extendedOutputsSpec(std::move(extendedOutputsSpec)), lockFlags(lockFlags) { if (cmd && cmd->getAutoArgs(*state)->size()) @@ -657,7 +657,7 @@ DerivedPathsWithInfo InstallableFlake::toDerivedPaths() priority = aPriority->getInt(); } - if (outputsToInstall.empty() || std::get_if(&outputsSpec)) { + if (outputsToInstall.empty() || std::get_if(&extendedOutputsSpec)) { outputsToInstall.clear(); if (auto aOutputs = attr->maybeGetAttr(state->sOutputs)) for (auto & s : aOutputs->getListOfStrings()) @@ -667,7 +667,7 @@ DerivedPathsWithInfo InstallableFlake::toDerivedPaths() if (outputsToInstall.empty()) outputsToInstall.insert("out"); - if (auto outputNames = std::get_if(&outputsSpec)) + if (auto outputNames = std::get_if(&extendedOutputsSpec)) outputsToInstall = *outputNames; return {{ @@ -680,7 +680,7 @@ DerivedPathsWithInfo InstallableFlake::toDerivedPaths() .originalRef = flakeRef, .resolvedRef = getLockedFlake()->flake.lockedRef, .attrPath = attrPath, - .outputsSpec = outputsSpec, + .extendedOutputsSpec = extendedOutputsSpec, } }}; } @@ -797,12 +797,12 @@ std::vector> SourceExprCommand::parseInstallables( } for (auto & s : ss) { - auto [prefix, outputsSpec] = OutputsSpec::parse(s); + auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse(s); result.push_back( std::make_shared( state, *this, vFile, prefix == "." ? "" : prefix, - outputsSpec)); + extendedOutputsSpec)); } } else { @@ -837,13 +837,13 @@ std::vector> SourceExprCommand::parseInstallables( } try { - auto [flakeRef, fragment, outputsSpec] = parseFlakeRefWithFragmentAndOutputsSpec(s, absPath(".")); + auto [flakeRef, fragment, extendedOutputsSpec] = parseFlakeRefWithFragmentAndExtendedOutputsSpec(s, absPath(".")); result.push_back(std::make_shared( this, getEvalState(), std::move(flakeRef), fragment, - outputsSpec, + extendedOutputsSpec, getDefaultFlakeAttrPaths(), getDefaultFlakeAttrPathPrefixes(), lockFlags)); diff --git a/src/libcmd/installables.hh b/src/libcmd/installables.hh index 9b92cc4be..3d12639b0 100644 --- a/src/libcmd/installables.hh +++ b/src/libcmd/installables.hh @@ -59,7 +59,7 @@ struct ExtraPathInfo std::optional resolvedRef; std::optional attrPath; // FIXME: merge with DerivedPath's 'outputs' field? - std::optional outputsSpec; + std::optional extendedOutputsSpec; }; /* A derived path with any additional info that commands might @@ -169,7 +169,7 @@ struct InstallableFlake : InstallableValue FlakeRef flakeRef; Strings attrPaths; Strings prefixes; - OutputsSpec outputsSpec; + ExtendedOutputsSpec extendedOutputsSpec; const flake::LockFlags & lockFlags; mutable std::shared_ptr _lockedFlake; @@ -178,7 +178,7 @@ struct InstallableFlake : InstallableValue ref state, FlakeRef && flakeRef, std::string_view fragment, - OutputsSpec outputsSpec, + ExtendedOutputsSpec extendedOutputsSpec, Strings attrPaths, Strings prefixes, const flake::LockFlags & lockFlags); diff --git a/src/libexpr/flake/flakeref.cc b/src/libexpr/flake/flakeref.cc index cfa279fb4..bc61e2c9a 100644 --- a/src/libexpr/flake/flakeref.cc +++ b/src/libexpr/flake/flakeref.cc @@ -238,15 +238,15 @@ std::pair FlakeRef::fetchTree(ref store) const return {std::move(tree), FlakeRef(std::move(lockedInput), subdir)}; } -std::tuple parseFlakeRefWithFragmentAndOutputsSpec( +std::tuple parseFlakeRefWithFragmentAndExtendedOutputsSpec( const std::string & url, const std::optional & baseDir, bool allowMissing, bool isFlake) { - auto [prefix, outputsSpec] = OutputsSpec::parse(url); + auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse(url); auto [flakeRef, fragment] = parseFlakeRefWithFragment(prefix, baseDir, allowMissing, isFlake); - return {std::move(flakeRef), fragment, outputsSpec}; + return {std::move(flakeRef), fragment, extendedOutputsSpec}; } } diff --git a/src/libexpr/flake/flakeref.hh b/src/libexpr/flake/flakeref.hh index 4ec79fb73..c4142fc20 100644 --- a/src/libexpr/flake/flakeref.hh +++ b/src/libexpr/flake/flakeref.hh @@ -80,7 +80,7 @@ std::pair parseFlakeRefWithFragment( std::optional> maybeParseFlakeRefWithFragment( const std::string & url, const std::optional & baseDir = {}); -std::tuple parseFlakeRefWithFragmentAndOutputsSpec( +std::tuple parseFlakeRefWithFragmentAndExtendedOutputsSpec( const std::string & url, const std::optional & baseDir = {}, bool allowMissing = false, diff --git a/src/libstore/outputs-spec.cc b/src/libstore/outputs-spec.cc index b5ea7e325..c446976bc 100644 --- a/src/libstore/outputs-spec.cc +++ b/src/libstore/outputs-spec.cc @@ -6,7 +6,7 @@ namespace nix { -std::pair OutputsSpec::parse(std::string s) +std::pair ExtendedOutputsSpec::parse(std::string s) { static std::regex regex(R"((.*)\^((\*)|([a-z]+(,[a-z]+)*)))"); @@ -20,43 +20,43 @@ std::pair OutputsSpec::parse(std::string s) return {match[1], tokenizeString(match[4].str(), ",")}; } -std::string OutputsSpec::to_string() const +std::string ExtendedOutputsSpec::to_string() const { return std::visit(overloaded { - [&](const OutputsSpec::Default &) -> std::string { + [&](const ExtendedOutputsSpec::Default &) -> std::string { return ""; }, - [&](const OutputsSpec::All &) -> std::string { + [&](const ExtendedOutputsSpec::All &) -> std::string { return "*"; }, - [&](const OutputsSpec::Names & outputNames) -> std::string { + [&](const ExtendedOutputsSpec::Names & outputNames) -> std::string { return "^" + concatStringsSep(",", outputNames); }, }, raw()); } -void to_json(nlohmann::json & json, const OutputsSpec & outputsSpec) +void to_json(nlohmann::json & json, const ExtendedOutputsSpec & extendedOutputsSpec) { - if (std::get_if(&outputsSpec)) + if (std::get_if(&extendedOutputsSpec)) json = nullptr; - else if (std::get_if(&outputsSpec)) + else if (std::get_if(&extendedOutputsSpec)) json = std::vector({"*"}); - else if (auto outputNames = std::get_if(&outputsSpec)) + else if (auto outputNames = std::get_if(&extendedOutputsSpec)) json = *outputNames; } -void from_json(const nlohmann::json & json, OutputsSpec & outputsSpec) +void from_json(const nlohmann::json & json, ExtendedOutputsSpec & extendedOutputsSpec) { if (json.is_null()) - outputsSpec = DefaultOutputs(); + extendedOutputsSpec = DefaultOutputs(); else { auto names = json.get(); if (names == OutputNames({"*"})) - outputsSpec = AllOutputs(); + extendedOutputsSpec = AllOutputs(); else - outputsSpec = names; + extendedOutputsSpec = names; } } diff --git a/src/libstore/outputs-spec.hh b/src/libstore/outputs-spec.hh index 6f886ccb6..5ed711a62 100644 --- a/src/libstore/outputs-spec.hh +++ b/src/libstore/outputs-spec.hh @@ -17,10 +17,10 @@ struct DefaultOutputs { bool operator < (const DefaultOutputs & _) const { return false; } }; -typedef std::variant _OutputsSpecRaw; +typedef std::variant _ExtendedOutputsSpecRaw; -struct OutputsSpec : _OutputsSpecRaw { - using Raw = _OutputsSpecRaw; +struct ExtendedOutputsSpec : _ExtendedOutputsSpecRaw { + using Raw = _ExtendedOutputsSpecRaw; using Raw::Raw; using Names = OutputNames; @@ -33,12 +33,12 @@ struct OutputsSpec : _OutputsSpecRaw { /* 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); + static std::pair parse(std::string 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/tests/outputs-spec.cc b/src/libstore/tests/outputs-spec.cc index 552380837..a3dd42341 100644 --- a/src/libstore/tests/outputs-spec.cc +++ b/src/libstore/tests/outputs-spec.cc @@ -4,42 +4,42 @@ namespace nix { -TEST(OutputsSpec_parse, basic) +TEST(ExtendedOutputsSpec_parse, basic) { { - auto [prefix, outputsSpec] = OutputsSpec::parse("foo"); + auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo"); ASSERT_EQ(prefix, "foo"); - ASSERT_TRUE(std::get_if(&outputsSpec)); + ASSERT_TRUE(std::get_if(&extendedOutputsSpec)); } { - auto [prefix, outputsSpec] = OutputsSpec::parse("foo^*"); + auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo^*"); ASSERT_EQ(prefix, "foo"); - ASSERT_TRUE(std::get_if(&outputsSpec)); + ASSERT_TRUE(std::get_if(&extendedOutputsSpec)); } { - auto [prefix, outputsSpec] = OutputsSpec::parse("foo^out"); + auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo^out"); ASSERT_EQ(prefix, "foo"); - ASSERT_TRUE(std::get(outputsSpec) == OutputNames({"out"})); + ASSERT_TRUE(std::get(extendedOutputsSpec) == OutputNames({"out"})); } { - auto [prefix, outputsSpec] = OutputsSpec::parse("foo^out,bin"); + auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo^out,bin"); ASSERT_EQ(prefix, "foo"); - ASSERT_TRUE(std::get(outputsSpec) == OutputNames({"out", "bin"})); + ASSERT_TRUE(std::get(extendedOutputsSpec) == OutputNames({"out", "bin"})); } { - auto [prefix, outputsSpec] = OutputsSpec::parse("foo^bar^out,bin"); + auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo^bar^out,bin"); ASSERT_EQ(prefix, "foo^bar"); - ASSERT_TRUE(std::get(outputsSpec) == OutputNames({"out", "bin"})); + ASSERT_TRUE(std::get(extendedOutputsSpec) == OutputNames({"out", "bin"})); } { - auto [prefix, outputsSpec] = OutputsSpec::parse("foo^&*()"); + auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo^&*()"); ASSERT_EQ(prefix, "foo^&*()"); - ASSERT_TRUE(std::get_if(&outputsSpec)); + ASSERT_TRUE(std::get_if(&extendedOutputsSpec)); } } diff --git a/src/nix/bundle.cc b/src/nix/bundle.cc index 74a7973b0..a09dadff4 100644 --- a/src/nix/bundle.cc +++ b/src/nix/bundle.cc @@ -75,10 +75,10 @@ struct CmdBundle : InstallableCommand auto val = installable->toValue(*evalState).first; - auto [bundlerFlakeRef, bundlerName, outputsSpec] = parseFlakeRefWithFragmentAndOutputsSpec(bundler, absPath(".")); + auto [bundlerFlakeRef, bundlerName, extendedOutputsSpec] = parseFlakeRefWithFragmentAndExtendedOutputsSpec(bundler, absPath(".")); const flake::LockFlags lockFlags{ .writeLockFile = false }; InstallableFlake bundler{this, - evalState, std::move(bundlerFlakeRef), bundlerName, outputsSpec, + evalState, std::move(bundlerFlakeRef), bundlerName, extendedOutputsSpec, {"bundlers." + settings.thisSystem.get() + ".default", "defaultBundler." + settings.thisSystem.get() }, diff --git a/src/nix/profile.cc b/src/nix/profile.cc index 346c4e117..32364e720 100644 --- a/src/nix/profile.cc +++ b/src/nix/profile.cc @@ -22,7 +22,7 @@ struct ProfileElementSource // FIXME: record original attrpath. FlakeRef resolvedRef; std::string attrPath; - OutputsSpec outputs; + ExtendedOutputsSpec outputs; bool operator < (const ProfileElementSource & other) const { @@ -126,7 +126,7 @@ struct ProfileManifest parseFlakeRef(e[sOriginalUrl]), parseFlakeRef(e[sUrl]), e["attrPath"], - e["outputs"].get() + e["outputs"].get() }; } elements.emplace_back(std::move(element)); @@ -308,12 +308,12 @@ struct CmdProfileInstall : InstallablesCommand, MixDefaultProfile auto & [res, info] = builtPaths[installable.get()]; - if (info.originalRef && info.resolvedRef && info.attrPath && info.outputsSpec) { + if (info.originalRef && info.resolvedRef && info.attrPath && info.extendedOutputsSpec) { element.source = ProfileElementSource { .originalRef = *info.originalRef, .resolvedRef = *info.resolvedRef, .attrPath = *info.attrPath, - .outputs = *info.outputsSpec, + .outputs = *info.extendedOutputsSpec, }; } @@ -497,7 +497,7 @@ struct CmdProfileUpgrade : virtual SourceExprCommand, MixDefaultProfile, MixProf .originalRef = installable->flakeRef, .resolvedRef = *info.resolvedRef, .attrPath = *info.attrPath, - .outputs = installable->outputsSpec, + .outputs = installable->extendedOutputsSpec, }; installables.push_back(installable); From ce2f91d356438297fd795bd3edb8f9f4536db7da Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 11 Jan 2023 18:57:18 -0500 Subject: [PATCH 03/11] 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, }}; From 8a3b1b7ced3e00d29a0274dde110801dea4a1e0e Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 13 Dec 2022 13:00:34 -0500 Subject: [PATCH 04/11] Simplify and document store path installable parsing --- src/libcmd/installables.cc | 66 ++++++++++++++++++++++---------------- 1 file changed, 38 insertions(+), 28 deletions(-) diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index 58b5ef9b9..c791eef39 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -402,18 +402,6 @@ struct InstallableStorePath : Installable ref store; DerivedPath req; - InstallableStorePath(ref store, StorePath && storePath) - : store(store), - req(storePath.isDerivation() - ? (DerivedPath) DerivedPath::Built { - .drvPath = std::move(storePath), - .outputs = {}, - } - : (DerivedPath) DerivedPath::Opaque { - .path = std::move(storePath), - }) - { } - InstallableStorePath(ref store, DerivedPath && req) : store(store), req(std::move(req)) { } @@ -814,24 +802,46 @@ std::vector> SourceExprCommand::parseInstallables( for (auto & s : ss) { std::exception_ptr ex; - auto found = s.rfind('^'); - if (found != std::string::npos) { - try { - result.push_back(std::make_shared( - store, - DerivedPath::Built::parse(*store, s.substr(0, found), s.substr(found + 1)))); - continue; - } catch (BadStorePath &) { - } catch (...) { - if (!ex) - ex = std::current_exception(); - } - } + auto [prefix_, extendedOutputsSpec_] = ExtendedOutputsSpec::parse(s); + // To avoid clang's pedantry + auto prefix = std::move(prefix_); + auto extendedOutputsSpec = std::move(extendedOutputsSpec_); - found = s.find('/'); + auto found = prefix.find('/'); if (found != std::string::npos) { try { - result.push_back(std::make_shared(store, store->followLinksToStorePath(s))); + auto derivedPath = std::visit(overloaded { + // If the user did not use ^, we treat the output more liberally. + [&](const ExtendedOutputsSpec::Default &) -> DerivedPath { + // First, we accept a symlink chain or an actual store path. + auto storePath = store->followLinksToStorePath(prefix); + // Second, we see if the store path ends in `.drv` to decide what sort + // of derived path they want. + // + // This handling predates the `^` syntax. The `^*` in + // `/nix/store/hash-foo.drv^*` unambiguously means "do the + // `DerivedPath::Built` case", so plain `/nix/store/hash-foo.drv` could + // also unambiguously mean "do the DerivedPath::Opaque` case". + // + // Issue #7261 tracks reconsidering this `.drv` dispatching. + return storePath.isDerivation() + ? (DerivedPath) DerivedPath::Built { + .drvPath = std::move(storePath), + .outputs = {}, + } + : (DerivedPath) DerivedPath::Opaque { + .path = std::move(storePath), + }; + }, + // If the user did use ^, we just do exactly what is written. + [&](const ExtendedOutputsSpec::Explicit & outputSpec) -> DerivedPath { + return DerivedPath::Built { + .drvPath = store->parseStorePath(prefix), + .outputs = outputSpec, + }; + }, + }, extendedOutputsSpec.raw()); + result.push_back(std::make_shared(store, std::move(derivedPath))); continue; } catch (BadStorePath &) { } catch (...) { @@ -841,7 +851,7 @@ std::vector> SourceExprCommand::parseInstallables( } try { - auto [flakeRef, fragment, extendedOutputsSpec] = parseFlakeRefWithFragmentAndExtendedOutputsSpec(s, absPath(".")); + auto [flakeRef, fragment] = parseFlakeRefWithFragment(std::string { prefix }, absPath(".")); result.push_back(std::make_shared( this, getEvalState(), From 114a6e2b09eda7f23e7776e1cdf77715044e073e Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 11 Jan 2023 11:54:43 -0500 Subject: [PATCH 05/11] Make it hard to construct an empty `OutputsSpec::Names` This should be a non-empty set, and so we don't want people doing this by accident. We remove the zero-0 constructor with a little inheritance trickery. --- src/libcmd/installables.cc | 2 +- src/libstore/build/derivation-goal.cc | 4 ++-- src/libstore/misc.cc | 2 +- src/libstore/outputs-spec.cc | 8 ++++---- src/libstore/outputs-spec.hh | 17 ++++++++++++++++- src/libstore/path-with-outputs.cc | 2 +- src/nix-build/nix-build.cc | 2 +- 7 files changed, 26 insertions(+), 11 deletions(-) diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index c791eef39..3457f2e47 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -479,7 +479,7 @@ struct InstallableAttrPath : InstallableValue auto derivedPath = byDrvPath.emplace(*drvPath, DerivedPath::Built { .drvPath = *drvPath, // Not normally legal, but we will merge right below - .outputs = OutputsSpec::Names { }, + .outputs = OutputsSpec::Names { StringSet { } }, }).first; derivedPath->second.outputs.merge(std::visit(overloaded { diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 98c1ddaae..61169635a 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -990,7 +990,7 @@ void DerivationGoal::resolvedFinished() return resolvedDrv.outputNames(); }, [&](const OutputsSpec::Names & names) { - return names; + return static_cast>(names); }, }, wantedOutputs.raw()); @@ -1325,7 +1325,7 @@ std::pair DerivationGoal::checkPathValidity() return StringSet {}; }, [&](const OutputsSpec::Names & names) { - return names; + return static_cast(names); }, }, wantedOutputs.raw()); DrvOutputs validOutputs; diff --git a/src/libstore/misc.cc b/src/libstore/misc.cc index 1ff855cd0..5758c3d93 100644 --- a/src/libstore/misc.cc +++ b/src/libstore/misc.cc @@ -317,7 +317,7 @@ OutputPathMap resolveDerivedPath(Store & store, const DerivedPath::Built & bfd, return names; }, [&](const OutputsSpec::Names & names) { - return names; + return static_cast>(names); }, }, bfd.outputs); for (auto & output : outputNames) { diff --git a/src/libstore/outputs-spec.cc b/src/libstore/outputs-spec.cc index e7bd8ebd8..891252990 100644 --- a/src/libstore/outputs-spec.cc +++ b/src/libstore/outputs-spec.cc @@ -32,7 +32,7 @@ std::optional OutputsSpec::parseOpt(std::string_view s) return { OutputsSpec::All {} }; if (match[2].matched) - return { tokenizeString(match[2].str(), ",") }; + return OutputsSpec::Names { tokenizeString(match[2].str(), ",") }; assert(false); } @@ -139,11 +139,11 @@ void to_json(nlohmann::json & json, const ExtendedOutputsSpec & extendedOutputsS void from_json(const nlohmann::json & json, OutputsSpec & outputsSpec) { - auto names = json.get(); - if (names == OutputNames({"*"})) + auto names = json.get(); + if (names == StringSet({"*"})) outputsSpec = OutputsSpec::All {}; else - outputsSpec = names; + outputsSpec = OutputsSpec::Names { std::move(names) }; } diff --git a/src/libstore/outputs-spec.hh b/src/libstore/outputs-spec.hh index e81695da9..9c477ee2b 100644 --- a/src/libstore/outputs-spec.hh +++ b/src/libstore/outputs-spec.hh @@ -8,7 +8,22 @@ namespace nix { -typedef std::set OutputNames; +struct OutputNames : std::set { + using std::set::set; + + // These need to be "inherited manually" + OutputNames(const std::set & s) + : std::set(s) + { } + OutputNames(std::set && s) + : std::set(s) + { } + + /* This set should always be non-empty, so we delete this + constructor in order make creating empty ones by mistake harder. + */ + OutputNames() = delete; +}; struct AllOutputs { bool operator < (const AllOutputs & _) const { return false; } diff --git a/src/libstore/path-with-outputs.cc b/src/libstore/path-with-outputs.cc index 10e0cc63e..6c0704ed9 100644 --- a/src/libstore/path-with-outputs.cc +++ b/src/libstore/path-with-outputs.cc @@ -53,7 +53,7 @@ std::variant StorePathWithOutputs::tryFromDeriv return {}; }, [&](const OutputsSpec::Names & outputs) { - return outputs; + return static_cast(outputs); }, }, bfd.outputs.raw()), }; diff --git a/src/nix-build/nix-build.cc b/src/nix-build/nix-build.cc index 0a7a21de2..049838bb1 100644 --- a/src/nix-build/nix-build.cc +++ b/src/nix-build/nix-build.cc @@ -421,7 +421,7 @@ static void main_nix_build(int argc, char * * argv) { pathsToBuild.push_back(DerivedPath::Built { .drvPath = inputDrv, - .outputs = inputOutputs + .outputs = OutputsSpec::Names { inputOutputs }, }); pathsToCopy.insert(inputDrv); } From 5ba6e5d0d9bed2806ddb59c8a3305b3cb5784d53 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 11 Jan 2023 16:32:30 -0500 Subject: [PATCH 06/11] Remove default constructor from `OutputsSpec` This forces us to be explicit. It also requires to rework how `from_json` works. A `JSON_IMPL` is added to assist with this. --- src/libcmd/installables.cc | 2 +- src/libcmd/repl.cc | 7 +++- src/libstore/build/entry-points.cc | 7 ++-- src/libstore/legacy-ssh-store.cc | 7 +++- src/libstore/outputs-spec.cc | 53 +++++++++++++++--------------- src/libstore/outputs-spec.hh | 15 ++++----- src/libstore/remote-store.cc | 7 +++- src/libutil/json-impls.hh | 14 ++++++++ src/nix-env/nix-env.cc | 18 +++++++--- src/nix/bundle.cc | 7 +++- src/nix/develop.cc | 7 +++- src/nix/flake.cc | 8 +++-- 12 files changed, 103 insertions(+), 49 deletions(-) create mode 100644 src/libutil/json-impls.hh diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index 3457f2e47..d73109873 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -827,7 +827,7 @@ std::vector> SourceExprCommand::parseInstallables( return storePath.isDerivation() ? (DerivedPath) DerivedPath::Built { .drvPath = std::move(storePath), - .outputs = {}, + .outputs = OutputsSpec::All {}, } : (DerivedPath) DerivedPath::Opaque { .path = std::move(storePath), diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index 71a7e079a..9b12f8fa2 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -641,7 +641,12 @@ bool NixRepl::processLine(std::string line) Path drvPathRaw = state->store->printStorePath(drvPath); if (command == ":b" || command == ":bl") { - state->store->buildPaths({DerivedPath::Built{drvPath}}); + state->store->buildPaths({ + DerivedPath::Built { + .drvPath = drvPath, + .outputs = OutputsSpec::All { }, + }, + }); auto drv = state->store->readDerivation(drvPath); logger->cout("\nThis derivation produced the following outputs:"); for (auto & [outputName, outputPath] : state->store->queryDerivationOutputMap(drvPath)) { diff --git a/src/libstore/build/entry-points.cc b/src/libstore/build/entry-points.cc index df7722733..2925fe3ca 100644 --- a/src/libstore/build/entry-points.cc +++ b/src/libstore/build/entry-points.cc @@ -80,7 +80,7 @@ BuildResult Store::buildDerivation(const StorePath & drvPath, const BasicDerivat BuildMode buildMode) { Worker worker(*this, *this); - auto goal = worker.makeBasicDerivationGoal(drvPath, drv, {}, buildMode); + auto goal = worker.makeBasicDerivationGoal(drvPath, drv, OutputsSpec::All {}, buildMode); try { worker.run(Goals{goal}); @@ -89,7 +89,10 @@ BuildResult Store::buildDerivation(const StorePath & drvPath, const BasicDerivat return BuildResult { .status = BuildResult::MiscFailure, .errorMsg = e.msg(), - .path = DerivedPath::Built { .drvPath = drvPath }, + .path = DerivedPath::Built { + .drvPath = drvPath, + .outputs = OutputsSpec::All { }, + }, }; }; } diff --git a/src/libstore/legacy-ssh-store.cc b/src/libstore/legacy-ssh-store.cc index 4d398b21d..e1a4e13a3 100644 --- a/src/libstore/legacy-ssh-store.cc +++ b/src/libstore/legacy-ssh-store.cc @@ -279,7 +279,12 @@ public: conn->to.flush(); - BuildResult status { .path = DerivedPath::Built { .drvPath = drvPath } }; + BuildResult status { + .path = DerivedPath::Built { + .drvPath = drvPath, + .outputs = OutputsSpec::All { }, + }, + }; status.status = (BuildResult::Status) readInt(conn->from); conn->from >> status.errorMsg; diff --git a/src/libstore/outputs-spec.cc b/src/libstore/outputs-spec.cc index 891252990..1eeab1aa8 100644 --- a/src/libstore/outputs-spec.cc +++ b/src/libstore/outputs-spec.cc @@ -110,9 +110,21 @@ bool OutputsSpec::merge(const OutputsSpec & that) }, raw()); } +} -void to_json(nlohmann::json & json, const OutputsSpec & outputsSpec) -{ +namespace nlohmann { + +using namespace nix; + +OutputsSpec adl_serializer::from_json(const json & json) { + auto names = json.get(); + if (names == StringSet({"*"})) + return OutputsSpec::All {}; + else + return OutputsSpec::Names { std::move(names) }; +} + +void adl_serializer::to_json(json & json, OutputsSpec t) { std::visit(overloaded { [&](const OutputsSpec::All &) { json = std::vector({"*"}); @@ -120,40 +132,27 @@ void to_json(nlohmann::json & json, const OutputsSpec & outputsSpec) [&](const OutputsSpec::Names & names) { json = names; }, - }, outputsSpec); + }, t); } -void to_json(nlohmann::json & json, const ExtendedOutputsSpec & extendedOutputsSpec) -{ +ExtendedOutputsSpec adl_serializer::from_json(const json & json) { + if (json.is_null()) + return ExtendedOutputsSpec::Default {}; + else { + return ExtendedOutputsSpec::Explicit { json.get() }; + } +} + +void adl_serializer::to_json(json & json, ExtendedOutputsSpec t) { std::visit(overloaded { [&](const ExtendedOutputsSpec::Default &) { json = nullptr; }, [&](const ExtendedOutputsSpec::Explicit & e) { - to_json(json, e); + adl_serializer::to_json(json, e); }, - }, extendedOutputsSpec); -} - - -void from_json(const nlohmann::json & json, OutputsSpec & outputsSpec) -{ - auto names = json.get(); - if (names == StringSet({"*"})) - outputsSpec = OutputsSpec::All {}; - else - outputsSpec = OutputsSpec::Names { std::move(names) }; -} - - -void from_json(const nlohmann::json & json, ExtendedOutputsSpec & extendedOutputsSpec) -{ - if (json.is_null()) - extendedOutputsSpec = ExtendedOutputsSpec::Default {}; - else { - extendedOutputsSpec = ExtendedOutputsSpec::Explicit { json.get() }; - } + }, t); } } diff --git a/src/libstore/outputs-spec.hh b/src/libstore/outputs-spec.hh index 9c477ee2b..babf29d16 100644 --- a/src/libstore/outputs-spec.hh +++ b/src/libstore/outputs-spec.hh @@ -4,7 +4,7 @@ #include #include -#include "nlohmann/json_fwd.hpp" +#include "json-impls.hh" namespace nix { @@ -35,6 +35,9 @@ struct OutputsSpec : _OutputsSpecRaw { using Raw = _OutputsSpecRaw; using Raw::Raw; + /* Force choosing a variant */ + OutputsSpec() = delete; + using Names = OutputNames; using All = AllOutputs; @@ -85,11 +88,7 @@ struct ExtendedOutputsSpec : _ExtendedOutputsSpecRaw { 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 &); - } + +JSON_IMPL(OutputsSpec) +JSON_IMPL(ExtendedOutputsSpec) diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 832be08f7..ff57a77ca 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -910,7 +910,12 @@ BuildResult RemoteStore::buildDerivation(const StorePath & drvPath, const BasicD writeDerivation(conn->to, *this, drv); conn->to << buildMode; conn.processStderr(); - BuildResult res { .path = DerivedPath::Built { .drvPath = drvPath } }; + BuildResult res { + .path = DerivedPath::Built { + .drvPath = drvPath, + .outputs = OutputsSpec::All { }, + }, + }; res.status = (BuildResult::Status) readInt(conn->from); conn->from >> res.errorMsg; if (GET_PROTOCOL_MINOR(conn->daemonVersion) >= 29) { diff --git a/src/libutil/json-impls.hh b/src/libutil/json-impls.hh new file mode 100644 index 000000000..bd75748ad --- /dev/null +++ b/src/libutil/json-impls.hh @@ -0,0 +1,14 @@ +#pragma once + +#include "nlohmann/json_fwd.hpp" + +// Following https://github.com/nlohmann/json#how-can-i-use-get-for-non-default-constructiblenon-copyable-types +#define JSON_IMPL(TYPE) \ + namespace nlohmann { \ + using namespace nix; \ + template <> \ + struct adl_serializer { \ + static TYPE from_json(const json & json); \ + static void to_json(json & json, TYPE t); \ + }; \ + } diff --git a/src/nix-env/nix-env.cc b/src/nix-env/nix-env.cc index 31823a966..406e548c0 100644 --- a/src/nix-env/nix-env.cc +++ b/src/nix-env/nix-env.cc @@ -478,9 +478,14 @@ static void printMissing(EvalState & state, DrvInfos & elems) std::vector targets; for (auto & i : elems) if (auto drvPath = i.queryDrvPath()) - targets.push_back(DerivedPath::Built{*drvPath}); + targets.push_back(DerivedPath::Built{ + .drvPath = *drvPath, + .outputs = OutputsSpec::All { }, + }); else - targets.push_back(DerivedPath::Opaque{i.queryOutPath()}); + targets.push_back(DerivedPath::Opaque{ + .path = i.queryOutPath(), + }); printMissing(state.store, targets); } @@ -751,8 +756,13 @@ static void opSet(Globals & globals, Strings opFlags, Strings opArgs) auto drvPath = drv.queryDrvPath(); std::vector paths { drvPath - ? (DerivedPath) (DerivedPath::Built { *drvPath }) - : (DerivedPath) (DerivedPath::Opaque { drv.queryOutPath() }), + ? (DerivedPath) (DerivedPath::Built { + .drvPath = *drvPath, + .outputs = OutputsSpec::All { }, + }) + : (DerivedPath) (DerivedPath::Opaque { + .path = drv.queryOutPath(), + }), }; printMissing(globals.state->store, paths); if (globals.dryRun) return; diff --git a/src/nix/bundle.cc b/src/nix/bundle.cc index a09dadff4..6ae9460f6 100644 --- a/src/nix/bundle.cc +++ b/src/nix/bundle.cc @@ -105,7 +105,12 @@ struct CmdBundle : InstallableCommand auto outPath = evalState->coerceToStorePath(attr2->pos, *attr2->value, context2, ""); - store->buildPaths({ DerivedPath::Built { drvPath } }); + store->buildPaths({ + DerivedPath::Built { + .drvPath = drvPath, + .outputs = OutputsSpec::All { }, + }, + }); auto outPathS = store->printStorePath(outPath); diff --git a/src/nix/develop.cc b/src/nix/develop.cc index 6aa675386..16bbd8613 100644 --- a/src/nix/develop.cc +++ b/src/nix/develop.cc @@ -232,7 +232,12 @@ static StorePath getDerivationEnvironment(ref store, ref evalStore auto shellDrvPath = writeDerivation(*evalStore, drv); /* Build the derivation. */ - store->buildPaths({DerivedPath::Built{shellDrvPath}}, bmNormal, evalStore); + store->buildPaths( + { DerivedPath::Built { + .drvPath = shellDrvPath, + .outputs = OutputsSpec::All { }, + }}, + bmNormal, evalStore); for (auto & [_0, optPath] : evalStore->queryPartialDerivationOutputMap(shellDrvPath)) { assert(optPath); diff --git a/src/nix/flake.cc b/src/nix/flake.cc index 33ce3f401..d16d88ef8 100644 --- a/src/nix/flake.cc +++ b/src/nix/flake.cc @@ -513,8 +513,12 @@ struct CmdFlakeCheck : FlakeCommand auto drvPath = checkDerivation( fmt("%s.%s.%s", name, attr_name, state->symbols[attr2.name]), *attr2.value, attr2.pos); - if (drvPath && attr_name == settings.thisSystem.get()) - drvPaths.push_back(DerivedPath::Built{*drvPath}); + if (drvPath && attr_name == settings.thisSystem.get()) { + drvPaths.push_back(DerivedPath::Built { + .drvPath = *drvPath, + .outputs = OutputsSpec::All { }, + }); + } } } } From 0faf5326bd333eeef126730683dc02009a06402f Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 11 Jan 2023 17:31:32 -0500 Subject: [PATCH 07/11] Improve tests for `OutputsSpec` --- src/libstore/outputs-spec.cc | 21 +++-- src/libstore/outputs-spec.hh | 9 +-- src/libstore/tests/outputs-spec.cc | 123 ++++++++++++++++++----------- 3 files changed, 97 insertions(+), 56 deletions(-) diff --git a/src/libstore/outputs-spec.cc b/src/libstore/outputs-spec.cc index 1eeab1aa8..8e6e40c2b 100644 --- a/src/libstore/outputs-spec.cc +++ b/src/libstore/outputs-spec.cc @@ -40,22 +40,33 @@ std::optional OutputsSpec::parseOpt(std::string_view s) OutputsSpec OutputsSpec::parse(std::string_view s) { - std::optional spec = OutputsSpec::parseOpt(s); + std::optional spec = parseOpt(s); if (!spec) throw Error("Invalid outputs specifier: '%s'", s); return *spec; } -std::pair ExtendedOutputsSpec::parse(std::string_view s) +std::optional> ExtendedOutputsSpec::parseOpt(std::string_view s) { auto found = s.rfind('^'); if (found == std::string::npos) - return { s, ExtendedOutputsSpec::Default {} }; + return std::pair { s, ExtendedOutputsSpec::Default {} }; - auto spec = OutputsSpec::parse(s.substr(found + 1)); - return { s.substr(0, found), ExtendedOutputsSpec::Explicit { spec } }; + auto specOpt = OutputsSpec::parseOpt(s.substr(found + 1)); + if (!specOpt) + return std::nullopt; + return std::pair { s.substr(0, found), ExtendedOutputsSpec::Explicit { *std::move(specOpt) } }; +} + + +std::pair ExtendedOutputsSpec::parse(std::string_view s) +{ + std::optional spec = parseOpt(s); + if (!spec) + throw Error("Invalid extended outputs specifier: '%s'", s); + return *spec; } diff --git a/src/libstore/outputs-spec.hh b/src/libstore/outputs-spec.hh index babf29d16..9211a4fc6 100644 --- a/src/libstore/outputs-spec.hh +++ b/src/libstore/outputs-spec.hh @@ -25,9 +25,7 @@ struct OutputNames : std::set { OutputNames() = delete; }; -struct AllOutputs { - bool operator < (const AllOutputs & _) const { return false; } -}; +struct AllOutputs : std::monostate { }; typedef std::variant _OutputsSpecRaw; @@ -64,9 +62,7 @@ struct OutputsSpec : _OutputsSpecRaw { std::string to_string() const; }; -struct DefaultOutputs { - bool operator < (const DefaultOutputs & _) const { return false; } -}; +struct DefaultOutputs : std::monostate { }; typedef std::variant _ExtendedOutputsSpecRaw; @@ -84,6 +80,7 @@ struct ExtendedOutputsSpec : _ExtendedOutputsSpecRaw { /* Parse a string of the form 'prefix^output1,...outputN' or 'prefix^*', returning the prefix and the extended outputs spec. */ static std::pair parse(std::string_view s); + static std::optional> parseOpt(std::string_view s); std::string to_string() const; }; diff --git a/src/libstore/tests/outputs-spec.cc b/src/libstore/tests/outputs-spec.cc index 03259e7c7..6d07795aa 100644 --- a/src/libstore/tests/outputs-spec.cc +++ b/src/libstore/tests/outputs-spec.cc @@ -4,63 +4,96 @@ namespace nix { -TEST(OutputsSpec_parse, basic) -{ - { - auto outputsSpec = OutputsSpec::parse("*"); - ASSERT_TRUE(std::get_if(&outputsSpec)); +#define TEST_DONT_PARSE(NAME, STR) \ + TEST(OutputsSpec, bad_ ## NAME) { \ + std::optional OutputsSpecOpt = \ + OutputsSpec::parseOpt(STR); \ + ASSERT_FALSE(OutputsSpecOpt); \ } - { - auto outputsSpec = OutputsSpec::parse("out"); - ASSERT_TRUE(std::get(outputsSpec) == OutputsSpec::Names({"out"})); - } +TEST_DONT_PARSE(empty, "") +TEST_DONT_PARSE(garbage, "&*()") +TEST_DONT_PARSE(double_star, "**") +TEST_DONT_PARSE(star_first, "*,foo") +TEST_DONT_PARSE(star_second, "foo,*") - { - auto outputsSpec = OutputsSpec::parse("out,bin"); - ASSERT_TRUE(std::get(outputsSpec) == OutputsSpec::Names({"out", "bin"})); - } +#undef TEST_DONT_PARSE - { - std::optional outputsSpecOpt = OutputsSpec::parseOpt("&*()"); - ASSERT_FALSE(outputsSpecOpt); - } +TEST(OutputsSpec, all) { + std::string_view str = "*"; + OutputsSpec expected = OutputsSpec::All { }; + ASSERT_EQ(OutputsSpec::parse(str), expected); + ASSERT_EQ(expected.to_string(), str); +} + +TEST(OutputsSpec, names_out) { + std::string_view str = "out"; + OutputsSpec expected = OutputsSpec::Names { "out" }; + ASSERT_EQ(OutputsSpec::parse(str), expected); + ASSERT_EQ(expected.to_string(), str); +} + +TEST(OutputsSpec, names_out_bin) { + OutputsSpec expected = OutputsSpec::Names { "out", "bin" }; + ASSERT_EQ(OutputsSpec::parse("out,bin"), expected); + // N.B. This normalization is OK. + ASSERT_EQ(expected.to_string(), "bin,out"); } -TEST(ExtendedOutputsSpec_parse, basic) -{ - { - auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo"); - ASSERT_EQ(prefix, "foo"); - ASSERT_TRUE(std::get_if(&extendedOutputsSpec)); +#define TEST_DONT_PARSE(NAME, STR) \ + TEST(ExtendedOutputsSpec, bad_ ## NAME) { \ + std::optional extendedOutputsSpecOpt = \ + ExtendedOutputsSpec::parseOpt(STR); \ + ASSERT_FALSE(extendedOutputsSpecOpt); \ } - { - auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo^*"); - ASSERT_EQ(prefix, "foo"); - auto * explicit_p = std::get_if(&extendedOutputsSpec); - ASSERT_TRUE(explicit_p); - ASSERT_TRUE(std::get_if(explicit_p)); - } +TEST_DONT_PARSE(carot_empty, "^") +TEST_DONT_PARSE(prefix_carot_empty, "foo^") - { - auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo^out"); - ASSERT_EQ(prefix, "foo"); - ASSERT_TRUE(std::get(std::get(extendedOutputsSpec)) == OutputsSpec::Names({"out"})); - } +#undef TEST_DONT_PARSE - { - auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo^out,bin"); - ASSERT_EQ(prefix, "foo"); - ASSERT_TRUE(std::get(std::get(extendedOutputsSpec)) == OutputsSpec::Names({"out", "bin"})); - } +TEST(ExtendedOutputsSpec, defeault) { + std::string_view str = "foo"; + auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse(str); + ASSERT_EQ(prefix, "foo"); + ExtendedOutputsSpec expected = ExtendedOutputsSpec::Default { }; + ASSERT_EQ(extendedOutputsSpec, expected); + ASSERT_EQ(std::string { prefix } + expected.to_string(), str); +} - { - auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo^bar^out,bin"); - ASSERT_EQ(prefix, "foo^bar"); - ASSERT_TRUE(std::get(std::get(extendedOutputsSpec)) == OutputsSpec::Names({"out", "bin"})); - } +TEST(ExtendedOutputsSpec, all) { + std::string_view str = "foo^*"; + auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse(str); + ASSERT_EQ(prefix, "foo"); + ExtendedOutputsSpec expected = OutputsSpec::All { }; + ASSERT_EQ(extendedOutputsSpec, expected); + ASSERT_EQ(std::string { prefix } + expected.to_string(), str); +} + +TEST(ExtendedOutputsSpec, out) { + std::string_view str = "foo^out"; + auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse(str); + ASSERT_EQ(prefix, "foo"); + ExtendedOutputsSpec expected = OutputsSpec::Names { "out" }; + ASSERT_EQ(extendedOutputsSpec, expected); + ASSERT_EQ(std::string { prefix } + expected.to_string(), str); +} + +TEST(ExtendedOutputsSpec, out_bin) { + auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo^out,bin"); + ASSERT_EQ(prefix, "foo"); + ExtendedOutputsSpec expected = OutputsSpec::Names { "out", "bin" }; + ASSERT_EQ(extendedOutputsSpec, expected); + ASSERT_EQ(std::string { prefix } + expected.to_string(), "foo^bin,out"); +} + +TEST(ExtendedOutputsSpec, many_carrot) { + auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse("foo^bar^out,bin"); + ASSERT_EQ(prefix, "foo^bar"); + ExtendedOutputsSpec expected = OutputsSpec::Names { "out", "bin" }; + ASSERT_EQ(extendedOutputsSpec, expected); + ASSERT_EQ(std::string { prefix } + expected.to_string(), "foo^bar^bin,out"); } } From 31875bcfb7ccbbf6e88c2cc62714a2a3794994ec Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 12 Jan 2023 20:20:27 -0500 Subject: [PATCH 08/11] 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. */ From e947aa540129441ebb3df1980c9ba05a935473ca Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 12 Jan 2023 20:33:50 -0500 Subject: [PATCH 09/11] Unit test `OuputsSpec::{union_, isSubsetOf}` --- src/libstore/tests/outputs-spec.cc | 49 ++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/src/libstore/tests/outputs-spec.cc b/src/libstore/tests/outputs-spec.cc index 6d07795aa..5daac9234 100644 --- a/src/libstore/tests/outputs-spec.cc +++ b/src/libstore/tests/outputs-spec.cc @@ -40,6 +40,55 @@ TEST(OutputsSpec, names_out_bin) { ASSERT_EQ(expected.to_string(), "bin,out"); } +#define TEST_SUBSET(X, THIS, THAT) \ + X((OutputsSpec { THIS }).isSubsetOf(THAT)); + +TEST(OutputsSpec, subsets_all_all) { + TEST_SUBSET(ASSERT_TRUE, OutputsSpec::All { }, OutputsSpec::All { }); +} + +TEST(OutputsSpec, subsets_names_all) { + TEST_SUBSET(ASSERT_TRUE, OutputsSpec::Names { "a" }, OutputsSpec::All { }); +} + +TEST(OutputsSpec, subsets_names_names_eq) { + TEST_SUBSET(ASSERT_TRUE, OutputsSpec::Names { "a" }, OutputsSpec::Names { "a" }); +} + +TEST(OutputsSpec, subsets_names_names_noneq) { + TEST_SUBSET(ASSERT_TRUE, OutputsSpec::Names { "a" }, (OutputsSpec::Names { "a", "b" })); +} + +TEST(OutputsSpec, not_subsets_all_names) { + TEST_SUBSET(ASSERT_FALSE, OutputsSpec::All { }, OutputsSpec::Names { "a" }); +} + +TEST(OutputsSpec, not_subsets_names_names) { + TEST_SUBSET(ASSERT_FALSE, (OutputsSpec::Names { "a", "b" }), (OutputsSpec::Names { "a" })); +} + +#undef TEST_SUBSET + +#define TEST_UNION(RES, THIS, THAT) \ + ASSERT_EQ(OutputsSpec { RES }, (OutputsSpec { THIS }).union_(THAT)); + +TEST(OutputsSpec, union_all_all) { + TEST_UNION(OutputsSpec::All { }, OutputsSpec::All { }, OutputsSpec::All { }); +} + +TEST(OutputsSpec, union_all_names) { + TEST_UNION(OutputsSpec::All { }, OutputsSpec::All { }, OutputsSpec::Names { "a" }); +} + +TEST(OutputsSpec, union_names_all) { + TEST_UNION(OutputsSpec::All { }, OutputsSpec::Names { "a" }, OutputsSpec::All { }); +} + +TEST(OutputsSpec, union_names_names) { + TEST_UNION((OutputsSpec::Names { "a", "b" }), OutputsSpec::Names { "a" }, OutputsSpec::Names { "b" }); +} + +#undef TEST_UNION #define TEST_DONT_PARSE(NAME, STR) \ TEST(ExtendedOutputsSpec, bad_ ## NAME) { \ From d29eb085630aac6cbefeafe51937314ce0263593 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 12 Jan 2023 20:41:29 -0500 Subject: [PATCH 10/11] Assert on construction that `OutputsSpec::Names` is non-empty --- src/libstore/outputs-spec.hh | 9 ++++++--- src/libstore/tests/outputs-spec.cc | 6 ++++++ 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/libstore/outputs-spec.hh b/src/libstore/outputs-spec.hh index 82dfad479..46bc35ebc 100644 --- a/src/libstore/outputs-spec.hh +++ b/src/libstore/outputs-spec.hh @@ -1,5 +1,6 @@ #pragma once +#include #include #include #include @@ -11,13 +12,15 @@ namespace nix { struct OutputNames : std::set { using std::set::set; - // These need to be "inherited manually" + /* These need to be "inherited manually" */ + OutputNames(const std::set & s) : std::set(s) - { } + { assert(!empty()); } + OutputNames(std::set && s) : std::set(s) - { } + { assert(!empty()); } /* This set should always be non-empty, so we delete this constructor in order make creating empty ones by mistake harder. diff --git a/src/libstore/tests/outputs-spec.cc b/src/libstore/tests/outputs-spec.cc index 5daac9234..c5e3f382b 100644 --- a/src/libstore/tests/outputs-spec.cc +++ b/src/libstore/tests/outputs-spec.cc @@ -4,6 +4,12 @@ namespace nix { +#ifndef NDEBUG +TEST(OutputsSpec, no_empty_names) { + ASSERT_DEATH(OutputsSpec::Names { std::set { } }, ""); +} +#endif + #define TEST_DONT_PARSE(NAME, STR) \ TEST(OutputsSpec, bad_ ## NAME) { \ std::optional OutputsSpecOpt = \ From d8512653d480acf69aae820f8b9d4b674dd6fc2f Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 12 Jan 2023 22:05:55 -0500 Subject: [PATCH 11/11] Write more (extended) output spec tests --- src/libstore/tests/outputs-spec.cc | 33 ++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/src/libstore/tests/outputs-spec.cc b/src/libstore/tests/outputs-spec.cc index c5e3f382b..c9c2cafd0 100644 --- a/src/libstore/tests/outputs-spec.cc +++ b/src/libstore/tests/outputs-spec.cc @@ -1,5 +1,6 @@ #include "outputs-spec.hh" +#include #include namespace nix { @@ -105,6 +106,10 @@ TEST(OutputsSpec, union_names_names) { TEST_DONT_PARSE(carot_empty, "^") TEST_DONT_PARSE(prefix_carot_empty, "foo^") +TEST_DONT_PARSE(garbage, "^&*()") +TEST_DONT_PARSE(double_star, "^**") +TEST_DONT_PARSE(star_first, "^*,foo") +TEST_DONT_PARSE(star_second, "^foo,*") #undef TEST_DONT_PARSE @@ -151,4 +156,32 @@ TEST(ExtendedOutputsSpec, many_carrot) { ASSERT_EQ(std::string { prefix } + expected.to_string(), "foo^bar^bin,out"); } + +#define TEST_JSON(TYPE, NAME, STR, VAL) \ + \ + TEST(TYPE, NAME ## _to_json) { \ + using nlohmann::literals::operator "" _json; \ + ASSERT_EQ( \ + STR ## _json, \ + ((nlohmann::json) TYPE { VAL })); \ + } \ + \ + TEST(TYPE, NAME ## _from_json) { \ + using nlohmann::literals::operator "" _json; \ + ASSERT_EQ( \ + TYPE { VAL }, \ + (STR ## _json).get()); \ + } + +TEST_JSON(OutputsSpec, all, R"(["*"])", OutputsSpec::All { }) +TEST_JSON(OutputsSpec, name, R"(["a"])", OutputsSpec::Names { "a" }) +TEST_JSON(OutputsSpec, names, R"(["a","b"])", (OutputsSpec::Names { "a", "b" })) + +TEST_JSON(ExtendedOutputsSpec, def, R"(null)", ExtendedOutputsSpec::Default { }) +TEST_JSON(ExtendedOutputsSpec, all, R"(["*"])", ExtendedOutputsSpec::Explicit { OutputsSpec::All { } }) +TEST_JSON(ExtendedOutputsSpec, name, R"(["a"])", ExtendedOutputsSpec::Explicit { OutputsSpec::Names { "a" } }) +TEST_JSON(ExtendedOutputsSpec, names, R"(["a","b"])", (ExtendedOutputsSpec::Explicit { OutputsSpec::Names { "a", "b" } })) + +#undef TEST_JSON + }