From 21050846457f356346204dd52fb7a6d49f710688 Mon Sep 17 00:00:00 2001 From: regnat Date: Mon, 17 May 2021 08:45:08 +0200 Subject: [PATCH] Enfore the use of properly built paths in libcmd Replace `DerivedPathWithHints` by a new `BuiltPath` type that serves as a proof that the corresponding path has been built. --- src/libcmd/command.cc | 39 ++++----- src/libcmd/command.hh | 14 ++-- src/libcmd/installables.cc | 151 ++++++++++++++++++----------------- src/libcmd/installables.hh | 6 +- src/libstore/derived-path.cc | 43 +++++++++- src/libstore/derived-path.hh | 22 ++--- src/nix/copy.cc | 20 +++-- src/nix/develop.cc | 17 +--- src/nix/log.cc | 6 +- src/nix/realisation.cc | 15 +++- 10 files changed, 189 insertions(+), 144 deletions(-) diff --git a/src/libcmd/command.cc b/src/libcmd/command.cc index 25e4873e8..569c4b9e4 100644 --- a/src/libcmd/command.cc +++ b/src/libcmd/command.cc @@ -54,7 +54,7 @@ void StoreCommand::run() run(getStore()); } -RealisedPathsCommand::RealisedPathsCommand(bool recursive) +BuiltPathsCommand::BuiltPathsCommand(bool recursive) : recursive(recursive) { if (recursive) @@ -81,39 +81,45 @@ RealisedPathsCommand::RealisedPathsCommand(bool recursive) }); } -void RealisedPathsCommand::run(ref store) +void BuiltPathsCommand::run(ref store) { - std::vector paths; + BuiltPaths paths; if (all) { if (installables.size()) throw UsageError("'--all' does not expect arguments"); // XXX: Only uses opaque paths, ignores all the realisations for (auto & p : store->queryAllValidPaths()) - paths.push_back(p); + paths.push_back(BuiltPath::Opaque{p}); } else { - auto pathSet = toRealisedPaths(store, realiseMode, operateOn, installables); + paths = toBuiltPaths(store, realiseMode, operateOn, installables); if (recursive) { - auto roots = std::move(pathSet); - pathSet = {}; - RealisedPath::closure(*store, roots, pathSet); + // XXX: This only computes the store path closure, ignoring + // intermediate realisations + StorePathSet pathsRoots, pathsClosure; + for (auto & root: paths) { + auto rootFromThis = root.outPaths(); + pathsRoots.insert(rootFromThis.begin(), rootFromThis.end()); + } + store->computeFSClosure(pathsRoots, pathsClosure); + for (auto & path : pathsClosure) + paths.push_back(BuiltPath::Opaque{path}); } - for (auto & path : pathSet) - paths.push_back(path); } run(store, std::move(paths)); } StorePathsCommand::StorePathsCommand(bool recursive) - : RealisedPathsCommand(recursive) + : BuiltPathsCommand(recursive) { } -void StorePathsCommand::run(ref store, std::vector paths) +void StorePathsCommand::run(ref store, BuiltPaths paths) { StorePaths storePaths; - for (auto & p : paths) - storePaths.push_back(p.path()); + for (auto& builtPath : paths) + for (auto& p : builtPath.outPaths()) + storePaths.push_back(p); run(store, std::move(storePaths)); } @@ -175,10 +181,7 @@ void MixProfile::updateProfile(const BuiltPaths & buildables) }, [&](BuiltPath::Built bfd) { for (auto & output : bfd.outputs) { - /* Output path should be known because we just tried to - build it. */ - assert(output.second); - result.push_back(*output.second); + result.push_back(output.second); } }, }, buildable.raw()); diff --git a/src/libcmd/command.hh b/src/libcmd/command.hh index 952279f7b..35b3a384b 100644 --- a/src/libcmd/command.hh +++ b/src/libcmd/command.hh @@ -143,7 +143,7 @@ private: }; /* A command that operates on zero or more store paths. */ -struct RealisedPathsCommand : public InstallablesCommand +struct BuiltPathsCommand : public InstallablesCommand { private: @@ -156,26 +156,26 @@ protected: public: - RealisedPathsCommand(bool recursive = false); + BuiltPathsCommand(bool recursive = false); using StoreCommand::run; - virtual void run(ref store, std::vector paths) = 0; + virtual void run(ref store, BuiltPaths paths) = 0; void run(ref store) override; bool useDefaultInstallables() override { return !all; } }; -struct StorePathsCommand : public RealisedPathsCommand +struct StorePathsCommand : public BuiltPathsCommand { StorePathsCommand(bool recursive = false); - using RealisedPathsCommand::run; + using BuiltPathsCommand::run; virtual void run(ref store, std::vector storePaths) = 0; - void run(ref store, std::vector paths) override; + void run(ref store, BuiltPaths paths) override; }; /* A command that operates on exactly one store path. */ @@ -231,7 +231,7 @@ std::set toDerivations(ref store, std::vector> installables, bool useDeriver = false); -std::set toRealisedPaths( +BuiltPaths toBuiltPaths( ref store, Realise mode, OperateOn operateOn, diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index 36d7ecc39..fe52912cf 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -285,9 +285,9 @@ void completeFlakeRef(ref store, std::string_view prefix) } } -BuiltPath Installable::toBuiltPath() +DerivedPath Installable::toDerivedPath() { - auto buildables = toBuiltPaths(); + auto buildables = toDerivedPaths(); if (buildables.size() != 1) throw Error("installable '%s' evaluates to %d derivations, where only one is expected", what(), buildables.size()); return std::move(buildables[0]); @@ -321,22 +321,19 @@ struct InstallableStorePath : Installable std::string what() override { return store->printStorePath(storePath); } - BuiltPaths toBuiltPaths() override + DerivedPaths toDerivedPaths() override { if (storePath.isDerivation()) { - std::map> outputs; auto drv = store->readDerivation(storePath); - for (auto & [name, output] : drv.outputsAndOptPaths(*store)) - outputs.emplace(name, output.second); return { - BuiltPath::Built { + DerivedPath::Built { .drvPath = storePath, - .outputs = std::move(outputs) + .outputs = drv.outputNames(), } }; } else { return { - BuiltPath::Opaque { + DerivedPath::Opaque { .path = storePath, } }; @@ -349,22 +346,22 @@ struct InstallableStorePath : Installable } }; -BuiltPaths InstallableValue::toBuiltPaths() +DerivedPaths InstallableValue::toDerivedPaths() { - BuiltPaths res; + DerivedPaths res; - std::map>> drvsToOutputs; + std::map> drvsToOutputs; // Group by derivation, helps with .all in particular for (auto & drv : toDerivations()) { auto outputName = drv.outputName; if (outputName == "") throw Error("derivation '%s' lacks an 'outputName' attribute", state->store->printStorePath(drv.drvPath)); - drvsToOutputs[drv.drvPath].insert_or_assign(outputName, drv.outPath); + drvsToOutputs[drv.drvPath].insert(outputName); } for (auto & i : drvsToOutputs) - res.push_back(BuiltPath::Built { i.first, i.second }); + res.push_back(DerivedPath::Built { i.first, i.second }); return res; } @@ -675,32 +672,67 @@ std::shared_ptr SourceExprCommand::parseInstallable( return installables.front(); } +BuiltPaths getBuiltPaths(ref store, DerivedPaths hopefullyBuiltPaths) +{ + BuiltPaths res; + for (auto& b : hopefullyBuiltPaths) + std::visit( + overloaded{ + [&](DerivedPath::Opaque bo) { + res.push_back(BuiltPath::Opaque{bo.path}); + }, + [&](DerivedPath::Built bfd) { + OutputPathMap outputs; + auto drv = store->readDerivation(bfd.drvPath); + auto outputHashes = staticOutputHashes(*store, drv); + auto drvOutputs = drv.outputsAndOptPaths(*store); + for (auto& output : bfd.outputs) { + if (!outputHashes.count(output)) + throw Error( + "the derivation '%s' doesn't have an output " + "named '%s'", + store->printStorePath(bfd.drvPath), output); + if (settings.isExperimentalFeatureEnabled( + "ca-derivations")) { + auto outputId = + DrvOutput{outputHashes.at(output), output}; + auto realisation = + store->queryRealisation(outputId); + if (!realisation) + throw Error( + "cannot operate on an output of unbuilt " + "content-addresed derivation '%s'", + outputId.to_string()); + outputs.insert_or_assign( + output, realisation->outPath); + } else { + // If ca-derivations isn't enabled, assume that + // the output path is statically known. + assert(drvOutputs.count(output)); + assert(drvOutputs.at(output).second); + outputs.insert_or_assign( + output, *drvOutputs.at(output).second); + } + } + res.push_back(BuiltPath::Built{bfd.drvPath, outputs}); + }, + }, + b.raw()); + + return res; +} + BuiltPaths build(ref store, Realise mode, std::vector> installables, BuildMode bMode) { if (mode == Realise::Nothing) settings.readOnlyMode = true; - BuiltPaths buildables; - std::vector pathsToBuild; for (auto & i : installables) { - for (auto & b : i->toBuiltPaths()) { - std::visit(overloaded { - [&](BuiltPath::Opaque bo) { - pathsToBuild.push_back(bo); - }, - [&](BuiltPath::Built bfd) { - StringSet outputNames; - for (auto & output : bfd.outputs) - outputNames.insert(output.first); - pathsToBuild.push_back( - DerivedPath::Built{bfd.drvPath, outputNames}); - }, - }, b.raw()); - buildables.push_back(std::move(b)); - } + auto b = i->toDerivedPaths(); + pathsToBuild.insert(pathsToBuild.end(), b.begin(), b.end()); } if (mode == Realise::Nothing) @@ -708,57 +740,26 @@ BuiltPaths build(ref store, Realise mode, else if (mode == Realise::Outputs) store->buildPaths(pathsToBuild, bMode); - return buildables; + return getBuiltPaths(store, pathsToBuild); } -std::set toRealisedPaths( +BuiltPaths toBuiltPaths( ref store, Realise mode, OperateOn operateOn, std::vector> installables) { - std::set res; if (operateOn == OperateOn::Output) { - for (auto & b : build(store, mode, installables)) - std::visit(overloaded { - [&](BuiltPath::Opaque bo) { - res.insert(bo.path); - }, - [&](BuiltPath::Built bfd) { - auto drv = store->readDerivation(bfd.drvPath); - auto outputHashes = staticOutputHashes(*store, drv); - for (auto & output : bfd.outputs) { - if (settings.isExperimentalFeatureEnabled("ca-derivations")) { - if (!outputHashes.count(output.first)) - throw Error( - "the derivation '%s' doesn't have an output named '%s'", - store->printStorePath(bfd.drvPath), - output.first); - auto outputId = DrvOutput{outputHashes.at(output.first), output.first}; - auto realisation = store->queryRealisation(outputId); - if (!realisation) - throw Error("cannot operate on an output of unbuilt content-addresed derivation '%s'", outputId.to_string()); - res.insert(RealisedPath{*realisation}); - } - else { - // If ca-derivations isn't enabled, behave as if - // all the paths are opaque to keep the default - // behavior - assert(output.second); - res.insert(*output.second); - } - } - }, - }, b.raw()); + return build(store, mode, installables); } else { if (mode == Realise::Nothing) settings.readOnlyMode = true; - auto drvPaths = toDerivations(store, installables, true); - res.insert(drvPaths.begin(), drvPaths.end()); + BuiltPaths res; + for (auto & drvPath : toDerivations(store, installables, true)) + res.push_back(BuiltPath::Opaque{drvPath}); + return res; } - - return res; } StorePathSet toStorePaths(ref store, @@ -766,8 +767,10 @@ StorePathSet toStorePaths(ref store, std::vector> installables) { StorePathSet outPaths; - for (auto & path : toRealisedPaths(store, mode, operateOn, installables)) - outPaths.insert(path.path()); + for (auto & path : toBuiltPaths(store, mode, operateOn, installables)) { + auto thisOutPaths = path.outPaths(); + outPaths.insert(thisOutPaths.begin(), thisOutPaths.end()); + } return outPaths; } @@ -789,9 +792,9 @@ StorePathSet toDerivations(ref store, StorePathSet drvPaths; for (auto & i : installables) - for (auto & b : i->toBuiltPaths()) + for (auto & b : i->toDerivedPaths()) std::visit(overloaded { - [&](BuiltPath::Opaque bo) { + [&](DerivedPath::Opaque bo) { if (!useDeriver) throw Error("argument '%s' did not evaluate to a derivation", i->what()); auto derivers = store->queryValidDerivers(bo.path); @@ -800,7 +803,7 @@ StorePathSet toDerivations(ref store, // FIXME: use all derivers? drvPaths.insert(*derivers.begin()); }, - [&](BuiltPath::Built bfd) { + [&](DerivedPath::Built bfd) { drvPaths.insert(bfd.drvPath); }, }, b.raw()); diff --git a/src/libcmd/installables.hh b/src/libcmd/installables.hh index 7d79efaad..16bc4ae68 100644 --- a/src/libcmd/installables.hh +++ b/src/libcmd/installables.hh @@ -29,9 +29,9 @@ struct Installable virtual std::string what() = 0; - virtual BuiltPaths toBuiltPaths() = 0; + virtual DerivedPaths toDerivedPaths() = 0; - BuiltPath toBuiltPath(); + DerivedPath toDerivedPath(); App toApp(EvalState & state); @@ -74,7 +74,7 @@ struct InstallableValue : Installable virtual std::vector toDerivations() = 0; - BuiltPaths toBuiltPaths() override; + DerivedPaths toDerivedPaths() override; }; struct InstallableFlake : InstallableValue diff --git a/src/libstore/derived-path.cc b/src/libstore/derived-path.cc index d38613bd3..f188ef3c1 100644 --- a/src/libstore/derived-path.cc +++ b/src/libstore/derived-path.cc @@ -15,11 +15,26 @@ nlohmann::json BuiltPath::Built::toJSON(ref store) const { nlohmann::json res; res["drvPath"] = store->printStorePath(drvPath); for (const auto& [output, path] : outputs) { - res["outputs"][output] = path ? store->printStorePath(*path) : ""; + res["outputs"][output] = store->printStorePath(path); } return res; } +StorePathSet BuiltPath::outPaths() const +{ + return std::visit( + overloaded{ + [](BuiltPath::Opaque p) { return StorePathSet{p.path}; }, + [](BuiltPath::Built b) { + StorePathSet res; + for (auto & [_, path] : b.outputs) + res.insert(path); + return res; + }, + }, raw() + ); +} + nlohmann::json derivedPathsWithHintsToJSON(const BuiltPaths & buildables, ref store) { auto res = nlohmann::json::array(); for (const BuiltPath & buildable : buildables) { @@ -74,4 +89,30 @@ DerivedPath DerivedPath::parse(const Store & store, std::string_view s) : (DerivedPath) DerivedPath::Built::parse(store, s); } +RealisedPath::Set BuiltPath::toRealisedPaths(Store & store) const +{ + RealisedPath::Set res; + std::visit( + overloaded{ + [&](BuiltPath::Opaque p) { res.insert(p.path); }, + [&](BuiltPath::Built p) { + auto drvHashes = + staticOutputHashes(store, store.readDerivation(p.drvPath)); + for (auto& [outputName, outputPath] : p.outputs) { + if (settings.isExperimentalFeatureEnabled( + "ca-derivations")) { + auto thisRealisation = store.queryRealisation( + DrvOutput{drvHashes.at(outputName), outputName}); + assert(thisRealisation); // We’ve built it, so we must h + // ve the realisation + res.insert(*thisRealisation); + } else { + res.insert(outputPath); + } + } + }, + }, + raw()); + return res; +} } diff --git a/src/libstore/derived-path.hh b/src/libstore/derived-path.hh index 72304f4fa..9d6ace069 100644 --- a/src/libstore/derived-path.hh +++ b/src/libstore/derived-path.hh @@ -2,6 +2,7 @@ #include "util.hh" #include "path.hh" +#include "realisation.hh" #include @@ -83,7 +84,7 @@ struct DerivedPath : _DerivedPathRaw { */ struct BuiltPathBuilt { StorePath drvPath; - std::map> outputs; + std::map outputs; nlohmann::json toJSON(ref store) const; static BuiltPathBuilt parse(const Store & store, std::string_view); @@ -95,20 +96,9 @@ using _BuiltPathRaw = std::variant< >; /** - * A derived path with hints in the form of optional concrete output paths in the built case. - * - * This type is currently just used by the CLI. The paths are filled in - * during evaluation for derivations that know what paths they will - * produce in advanced, i.e. input-addressed or fixed-output content - * addressed derivations. - * - * That isn't very good, because it puts floating content-addressed - * derivations "at a disadvantage". It would be better to never rely on - * the output path of unbuilt derivations, and exclusively use the - * realizations types to work with built derivations' concrete output - * paths. + * A built path. Similar to a `DerivedPath`, but enriched with the corresponding + * output path(s). */ -// FIXME Stop using and delete this, or if that is not possible move out of libstore to libcmd. struct BuiltPath : _BuiltPathRaw { using Raw = _BuiltPathRaw; using Raw::Raw; @@ -120,8 +110,12 @@ struct BuiltPath : _BuiltPathRaw { return static_cast(*this); } + StorePathSet outPaths() const; + RealisedPath::Set toRealisedPaths(Store & store) const; + }; +typedef std::vector DerivedPaths; typedef std::vector BuiltPaths; nlohmann::json derivedPathsWithHintsToJSON(const BuiltPaths & buildables, ref store); diff --git a/src/nix/copy.cc b/src/nix/copy.cc index f59f7c76b..674cce4b4 100644 --- a/src/nix/copy.cc +++ b/src/nix/copy.cc @@ -8,7 +8,7 @@ using namespace nix; -struct CmdCopy : RealisedPathsCommand +struct CmdCopy : BuiltPathsCommand { std::string srcUri, dstUri; @@ -16,10 +16,10 @@ struct CmdCopy : RealisedPathsCommand SubstituteFlag substitute = NoSubstitute; - using RealisedPathsCommand::run; + using BuiltPathsCommand::run; CmdCopy() - : RealisedPathsCommand(true) + : BuiltPathsCommand(true) { addFlag({ .longName = "from", @@ -75,16 +75,22 @@ struct CmdCopy : RealisedPathsCommand if (srcUri.empty() && dstUri.empty()) throw UsageError("you must pass '--from' and/or '--to'"); - RealisedPathsCommand::run(store); + BuiltPathsCommand::run(store); } - void run(ref srcStore, std::vector paths) override + void run(ref srcStore, BuiltPaths paths) override { ref dstStore = dstUri.empty() ? openStore() : openStore(dstUri); + RealisedPath::Set stuffToCopy; + + for (auto & builtPath : paths) { + auto theseRealisations = builtPath.toRealisedPaths(*srcStore); + stuffToCopy.insert(theseRealisations.begin(), theseRealisations.end()); + } + copyPaths( - srcStore, dstStore, RealisedPath::Set(paths.begin(), paths.end()), - NoRepair, checkSigs, substitute); + srcStore, dstStore, stuffToCopy, NoRepair, checkSigs, substitute); } }; diff --git a/src/nix/develop.cc b/src/nix/develop.cc index 3762e5bcc..2b64d9a31 100644 --- a/src/nix/develop.cc +++ b/src/nix/develop.cc @@ -265,9 +265,9 @@ struct Common : InstallableCommand, MixProfile for (auto & [installable_, dir_] : redirects) { auto dir = absPath(dir_); auto installable = parseInstallable(store, installable_); - auto buildable = installable->toBuiltPath(); - auto doRedirect = [&](const StorePath & path) - { + auto builtPaths = toStorePaths( + store, Realise::Nothing, OperateOn::Output, {installable}); + for (auto & path: builtPaths) { auto from = store->printStorePath(path); if (script.find(from) == std::string::npos) warn("'%s' (path '%s') is not used by this build environment", installable->what(), from); @@ -275,16 +275,7 @@ struct Common : InstallableCommand, MixProfile printInfo("redirecting '%s' to '%s'", from, dir); rewrites.insert({from, dir}); } - }; - std::visit(overloaded { - [&](const BuiltPath::Opaque & bo) { - doRedirect(bo.path); - }, - [&](const BuiltPath::Built & bfd) { - for (auto & [outputName, path] : bfd.outputs) - if (path) doRedirect(*path); - }, - }, buildable.raw()); + } } return rewriteStrings(script, rewrites); diff --git a/src/nix/log.cc b/src/nix/log.cc index d87fda0b8..962c47525 100644 --- a/src/nix/log.cc +++ b/src/nix/log.cc @@ -30,15 +30,15 @@ struct CmdLog : InstallableCommand subs.push_front(store); - auto b = installable->toBuiltPath(); + auto b = installable->toDerivedPath(); RunPager pager; for (auto & sub : subs) { auto log = std::visit(overloaded { - [&](BuiltPath::Opaque bo) { + [&](DerivedPath::Opaque bo) { return sub->getBuildLog(bo.path); }, - [&](BuiltPath::Built bfd) { + [&](DerivedPath::Built bfd) { return sub->getBuildLog(bfd.drvPath); }, }, b.raw()); diff --git a/src/nix/realisation.cc b/src/nix/realisation.cc index 9ee9ccb91..d59e594df 100644 --- a/src/nix/realisation.cc +++ b/src/nix/realisation.cc @@ -28,7 +28,7 @@ struct CmdRealisation : virtual NixMultiCommand static auto rCmdRealisation = registerCommand("realisation"); -struct CmdRealisationInfo : RealisedPathsCommand, MixJSON +struct CmdRealisationInfo : BuiltPathsCommand, MixJSON { std::string description() override { @@ -44,12 +44,19 @@ struct CmdRealisationInfo : RealisedPathsCommand, MixJSON Category category() override { return catSecondary; } - void run(ref store, std::vector paths) override + void run(ref store, BuiltPaths paths) override { settings.requireExperimentalFeature("ca-derivations"); + RealisedPath::Set realisations; + + for (auto & builtPath : paths) { + auto theseRealisations = builtPath.toRealisedPaths(*store); + realisations.insert(theseRealisations.begin(), theseRealisations.end()); + } + if (json) { nlohmann::json res = nlohmann::json::array(); - for (auto & path : paths) { + for (auto & path : realisations) { nlohmann::json currentPath; if (auto realisation = std::get_if(&path.raw)) currentPath = realisation->toJSON(); @@ -61,7 +68,7 @@ struct CmdRealisationInfo : RealisedPathsCommand, MixJSON std::cout << res.dump(); } else { - for (auto & path : paths) { + for (auto & path : realisations) { if (auto realisation = std::get_if(&path.raw)) { std::cout << realisation->id.to_string() << " " <<