From 9355ecd54301372b6a919a2205340f904c7a51c6 Mon Sep 17 00:00:00 2001 From: regnat Date: Mon, 14 Dec 2020 17:24:30 +0100 Subject: [PATCH 1/5] Add a new Cmd type working on RealisedPaths Where a `RealisedPath` is a store path with its history, meaning either an opaque path for stuff that has been directly added to the store, or a `Realisation` for stuff that has been built by a derivation This is a low-level refactoring that doesn't bring anything by itself (except a few dozen extra lines of code :/ ), but raising the abstraction level a bit is important on a number of levels: - Commands like `nix build` have to query for the realisations after the build is finished which is fragile (see 27905f12e4a7207450abe37c9ed78e31603b67e1 for example). Having them oprate directly at the realisation level would avoid that - Others like `nix copy` currently operate directly on (built) store paths, but need a bit more information as they will need to register the realisations on the remote side --- src/libcmd/command.cc | 42 ++++++++++------- src/libcmd/command.hh | 23 ++++++++-- src/libcmd/installables.cc | 48 ++++++++++++++++---- src/libstore/realisation.cc | 31 +++++++++++++ src/libstore/realisation.hh | 90 +++++++++++++++++++++++++++++++++---- src/nix/copy.cc | 2 + 6 files changed, 200 insertions(+), 36 deletions(-) diff --git a/src/libcmd/command.cc b/src/libcmd/command.cc index 614dee788..efdc98d5a 100644 --- a/src/libcmd/command.cc +++ b/src/libcmd/command.cc @@ -54,7 +54,7 @@ void StoreCommand::run() run(getStore()); } -StorePathsCommand::StorePathsCommand(bool recursive) +RealisedPathsCommand::RealisedPathsCommand(bool recursive) : recursive(recursive) { if (recursive) @@ -81,30 +81,40 @@ StorePathsCommand::StorePathsCommand(bool recursive) }); } -void StorePathsCommand::run(ref store) +void RealisedPathsCommand::run(ref store) { - StorePaths storePaths; - + std::vector 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()) - storePaths.push_back(p); - } - - else { - for (auto & p : toStorePaths(store, realiseMode, operateOn, installables)) - storePaths.push_back(p); - + paths.push_back(p); + } else { + auto pathSet = toRealisedPaths(store, realiseMode, operateOn, installables); if (recursive) { - StorePathSet closure; - store->computeFSClosure(StorePathSet(storePaths.begin(), storePaths.end()), closure, false, false); - storePaths.clear(); - for (auto & p : closure) - storePaths.push_back(p); + auto roots = std::move(pathSet); + pathSet = {}; + RealisedPath::closure(*store, roots, pathSet); } + for (auto & path : pathSet) + paths.push_back(path); } + run(store, std::move(paths)); +} + +StorePathsCommand::StorePathsCommand(bool recursive) + : RealisedPathsCommand(recursive) +{ +} + +void StorePathsCommand::run(ref store, std::vector paths) +{ + StorePaths storePaths; + for (auto & p : paths) + storePaths.push_back(p.path()); + run(store, std::move(storePaths)); } diff --git a/src/libcmd/command.hh b/src/libcmd/command.hh index ed6980075..8c0b3a94a 100644 --- a/src/libcmd/command.hh +++ b/src/libcmd/command.hh @@ -141,7 +141,7 @@ private: }; /* A command that operates on zero or more store paths. */ -struct StorePathsCommand : public InstallablesCommand +struct RealisedPathsCommand : public InstallablesCommand { private: @@ -154,17 +154,28 @@ protected: public: - StorePathsCommand(bool recursive = false); + RealisedPathsCommand(bool recursive = false); using StoreCommand::run; - virtual void run(ref store, std::vector storePaths) = 0; + virtual void run(ref store, std::vector paths) = 0; void run(ref store) override; bool useDefaultInstallables() override { return !all; } }; +struct StorePathsCommand : public RealisedPathsCommand +{ + StorePathsCommand(bool recursive = false); + + using RealisedPathsCommand::run; + + virtual void run(ref store, std::vector storePaths) = 0; + + void run(ref store, std::vector paths) override; +}; + /* A command that operates on exactly one store path. */ struct StorePathCommand : public InstallablesCommand { @@ -218,6 +229,12 @@ std::set toDerivations(ref store, std::vector> installables, bool useDeriver = false); +std::set toRealisedPaths( + ref store, + Realise mode, + OperateOn operateOn, + std::vector> installables); + /* Helper function to generate args that invoke $EDITOR on filename:lineno. */ Strings editorFor(const Pos & pos); diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index 4e6bf4a9a..98a27ded9 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -704,23 +704,43 @@ Buildables build(ref store, Realise mode, return buildables; } -StorePathSet toStorePaths(ref store, - Realise mode, OperateOn operateOn, +std::set toRealisedPaths( + ref store, + Realise mode, + OperateOn operateOn, std::vector> installables) { - StorePathSet outPaths; - + std::set res; if (operateOn == OperateOn::Output) { for (auto & b : build(store, mode, installables)) std::visit(overloaded { [&](BuildableOpaque bo) { - outPaths.insert(bo.path); + res.insert(bo.path); }, [&](BuildableFromDrv bfd) { + auto drv = store->readDerivation(bfd.drvPath); + auto outputHashes = staticOutputHashes(*store, drv); for (auto & output : bfd.outputs) { - if (!output.second) - throw Error("Cannot operate on output of unbuilt CA drv"); - outPaths.insert(*output.second); + 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 output of unbuilt CA drv %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); @@ -731,9 +751,19 @@ StorePathSet toStorePaths(ref store, for (auto & i : installables) for (auto & b : i->toBuildables()) if (auto bfd = std::get_if(&b)) - outPaths.insert(bfd->drvPath); + res.insert(bfd->drvPath); } + return res; +} + +StorePathSet toStorePaths(ref store, + Realise mode, OperateOn operateOn, + std::vector> installables) +{ + StorePathSet outPaths; + for (auto & path : toRealisedPaths(store, mode, operateOn, installables)) + outPaths.insert(path.path()); return outPaths; } diff --git a/src/libstore/realisation.cc b/src/libstore/realisation.cc index 47ad90eee..c9b66186f 100644 --- a/src/libstore/realisation.cc +++ b/src/libstore/realisation.cc @@ -46,4 +46,35 @@ Realisation Realisation::fromJSON( }; } +StorePath RealisedPath::path() const { + return visit([](auto && arg) { return arg.getPath(); }); +} + +void RealisedPath::closure( + Store& store, + const RealisedPath::Set& startPaths, + RealisedPath::Set& ret) +{ + // FIXME: This only builds the store-path closure, not the real realisation + // closure + StorePathSet initialStorePaths, pathsClosure; + for (auto& path : startPaths) + initialStorePaths.insert(path.path()); + store.computeFSClosure(initialStorePaths, pathsClosure); + ret.insert(startPaths.begin(), startPaths.end()); + ret.insert(pathsClosure.begin(), pathsClosure.end()); +} + +void RealisedPath::closure(Store& store, RealisedPath::Set& ret) const +{ + RealisedPath::closure(store, {*this}, ret); +} + +RealisedPath::Set RealisedPath::closure(Store& store) const +{ + RealisedPath::Set ret; + closure(store, ret); + return ret; +} + } // namespace nix diff --git a/src/libstore/realisation.hh b/src/libstore/realisation.hh index 4b8ead3c5..1ecddc4d1 100644 --- a/src/libstore/realisation.hh +++ b/src/libstore/realisation.hh @@ -3,6 +3,34 @@ #include "path.hh" #include + +/* Awfull hacky generation of the comparison operators by doing a lexicographic + * comparison between the choosen fields + * ``` + * GENERATE_CMP(ClassName, my->field1, my->field2, ...) + * ``` + * + * will generate comparison operators semantically equivalent to: + * ``` + * bool operator<(const ClassName& other) { + * return field1 < other.field1 && field2 < other.field2 && ...; + * } + * ``` + */ +#define GENERATE_ONE_CMP(COMPARATOR, MY_TYPE, FIELDS...) \ + bool operator COMPARATOR(const MY_TYPE& other) const { \ + const MY_TYPE* me = this; \ + auto fields1 = std::make_tuple( FIELDS ); \ + me = &other; \ + auto fields2 = std::make_tuple( FIELDS ); \ + return fields1 COMPARATOR fields2; \ + } +#define GENERATE_EQUAL(args...) GENERATE_ONE_CMP(==, args) +#define GENERATE_LEQ(args...) GENERATE_ONE_CMP(<, args) +#define GENERATE_CMP(args...) \ + GENERATE_EQUAL(args) \ + GENERATE_LEQ(args) + namespace nix { struct DrvOutput { @@ -17,13 +45,7 @@ struct DrvOutput { static DrvOutput parse(const std::string &); - bool operator<(const DrvOutput& other) const { return to_pair() < other.to_pair(); } - bool operator==(const DrvOutput& other) const { return to_pair() == other.to_pair(); } - -private: - // Just to make comparison operators easier to write - std::pair to_pair() const - { return std::make_pair(drvHash, outputName); } + GENERATE_CMP(DrvOutput, me->drvHash, me->outputName); }; struct Realisation { @@ -32,8 +54,60 @@ struct Realisation { nlohmann::json toJSON() const; static Realisation fromJSON(const nlohmann::json& json, const std::string& whence); + + StorePath getPath() const { return outPath; } + + GENERATE_CMP(Realisation, me->id, me->outPath); }; -typedef std::map DrvOutputs; +struct OpaquePath { + StorePath path; + + StorePath getPath() const { return path; } + + GENERATE_CMP(OpaquePath, me->path); +}; + + +/** + * A store path with all the history of how it went into the store + */ +struct RealisedPath { + /* + * A path is either the result of the realisation of a derivation or + * an opaque blob that has been directly added to the store + */ + using Raw = std::variant; + Raw raw; + + using Set = std::set; + + RealisedPath(StorePath path) : raw(OpaquePath{path}) {} + RealisedPath(Realisation r) : raw(r) {} + + /** + * Syntactic sugar to run `std::visit` on the raw value: + * path.visit(blah) == std::visit(blah, path.raw) + */ + template + constexpr decltype(auto) visit(Visitor && vis) { + return std::visit(vis, raw); + } + template + constexpr decltype(auto) visit(Visitor && vis) const { + return std::visit(vis, raw); + } + + /** + * Get the raw store path associated to this + */ + StorePath path() const; + + void closure(Store& store, Set& ret) const; + static void closure(Store& store, const Set& startPaths, Set& ret); + Set closure(Store& store) const; + + GENERATE_CMP(RealisedPath, me->raw); +}; } diff --git a/src/nix/copy.cc b/src/nix/copy.cc index f15031a45..c56a1def1 100644 --- a/src/nix/copy.cc +++ b/src/nix/copy.cc @@ -16,6 +16,8 @@ struct CmdCopy : StorePathsCommand SubstituteFlag substitute = NoSubstitute; + using StorePathsCommand::run; + CmdCopy() : StorePathsCommand(true) { From ca8facefb6b6b0ffd6e22507111847dbfc9a3c75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Thu, 4 Feb 2021 14:47:28 +0100 Subject: [PATCH 2/5] Normalize some error messages Co-authored-by: Eelco Dolstra --- src/libcmd/installables.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index 98a27ded9..9ad02b5f0 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -724,14 +724,13 @@ std::set toRealisedPaths( if (settings.isExperimentalFeatureEnabled("ca-derivations")) { if (!outputHashes.count(output.first)) throw Error( - "The derivation %s doesn't have an output " - "named %s", + "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 output of unbuilt CA drv %s", outputId.to_string()); + throw Error("cannot operate on an output of unbuilt content-addresed derivation '%s'", outputId.to_string()); res.insert(RealisedPath{*realisation}); } else { From 43d409f6690b79b5d4e1ab5e9780de93eb0f677a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Thu, 4 Feb 2021 14:47:56 +0100 Subject: [PATCH 3/5] Fix a whitespace issue Co-authored-by: Eelco Dolstra --- src/libstore/realisation.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstore/realisation.cc b/src/libstore/realisation.cc index c9b66186f..e4276c040 100644 --- a/src/libstore/realisation.cc +++ b/src/libstore/realisation.cc @@ -65,7 +65,7 @@ void RealisedPath::closure( ret.insert(pathsClosure.begin(), pathsClosure.end()); } -void RealisedPath::closure(Store& store, RealisedPath::Set& ret) const +void RealisedPath::closure(Store& store, RealisedPath::Set & ret) const { RealisedPath::closure(store, {*this}, ret); } From d2091af231ab97b729c2486b55e520c565e59dd3 Mon Sep 17 00:00:00 2001 From: regnat Date: Thu, 4 Feb 2021 15:11:05 +0100 Subject: [PATCH 4/5] Move the GENERATE_CMP macro to its own file Despite being an ugly hack, it can probably be useful in a couple extra places --- src/libstore/realisation.hh | 29 +---------------------------- src/libutil/comparator.hh | 30 ++++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 28 deletions(-) create mode 100644 src/libutil/comparator.hh diff --git a/src/libstore/realisation.hh b/src/libstore/realisation.hh index 1ecddc4d1..557f54362 100644 --- a/src/libstore/realisation.hh +++ b/src/libstore/realisation.hh @@ -2,34 +2,7 @@ #include "path.hh" #include - - -/* Awfull hacky generation of the comparison operators by doing a lexicographic - * comparison between the choosen fields - * ``` - * GENERATE_CMP(ClassName, my->field1, my->field2, ...) - * ``` - * - * will generate comparison operators semantically equivalent to: - * ``` - * bool operator<(const ClassName& other) { - * return field1 < other.field1 && field2 < other.field2 && ...; - * } - * ``` - */ -#define GENERATE_ONE_CMP(COMPARATOR, MY_TYPE, FIELDS...) \ - bool operator COMPARATOR(const MY_TYPE& other) const { \ - const MY_TYPE* me = this; \ - auto fields1 = std::make_tuple( FIELDS ); \ - me = &other; \ - auto fields2 = std::make_tuple( FIELDS ); \ - return fields1 COMPARATOR fields2; \ - } -#define GENERATE_EQUAL(args...) GENERATE_ONE_CMP(==, args) -#define GENERATE_LEQ(args...) GENERATE_ONE_CMP(<, args) -#define GENERATE_CMP(args...) \ - GENERATE_EQUAL(args) \ - GENERATE_LEQ(args) +#include "comparator.hh" namespace nix { diff --git a/src/libutil/comparator.hh b/src/libutil/comparator.hh new file mode 100644 index 000000000..0315dc506 --- /dev/null +++ b/src/libutil/comparator.hh @@ -0,0 +1,30 @@ +#pragma once + +/* Awfull hacky generation of the comparison operators by doing a lexicographic + * comparison between the choosen fields. + * + * ``` + * GENERATE_CMP(ClassName, me->field1, me->field2, ...) + * ``` + * + * will generate comparison operators semantically equivalent to: + * + * ``` + * bool operator<(const ClassName& other) { + * return field1 < other.field1 && field2 < other.field2 && ...; + * } + * ``` + */ +#define GENERATE_ONE_CMP(COMPARATOR, MY_TYPE, FIELDS...) \ + bool operator COMPARATOR(const MY_TYPE& other) const { \ + const MY_TYPE* me = this; \ + auto fields1 = std::make_tuple( FIELDS ); \ + me = &other; \ + auto fields2 = std::make_tuple( FIELDS ); \ + return fields1 COMPARATOR fields2; \ + } +#define GENERATE_EQUAL(args...) GENERATE_ONE_CMP(==, args) +#define GENERATE_LEQ(args...) GENERATE_ONE_CMP(<, args) +#define GENERATE_CMP(args...) \ + GENERATE_EQUAL(args) \ + GENERATE_LEQ(args) From e69cfdebb090b3aabbff69a44504883d5b6fb866 Mon Sep 17 00:00:00 2001 From: regnat Date: Thu, 4 Feb 2021 15:15:22 +0100 Subject: [PATCH 5/5] Remove the `visit` machinery in `RealisedPath` In addition to being some ugly template trickery, it was also totally useless as it was used in only one place where I could replace it by just a few extra characters --- src/libstore/realisation.cc | 2 +- src/libstore/realisation.hh | 13 ------------- 2 files changed, 1 insertion(+), 14 deletions(-) diff --git a/src/libstore/realisation.cc b/src/libstore/realisation.cc index e4276c040..cd74af4ee 100644 --- a/src/libstore/realisation.cc +++ b/src/libstore/realisation.cc @@ -47,7 +47,7 @@ Realisation Realisation::fromJSON( } StorePath RealisedPath::path() const { - return visit([](auto && arg) { return arg.getPath(); }); + return std::visit([](auto && arg) { return arg.getPath(); }, raw); } void RealisedPath::closure( diff --git a/src/libstore/realisation.hh b/src/libstore/realisation.hh index 557f54362..7c91d802a 100644 --- a/src/libstore/realisation.hh +++ b/src/libstore/realisation.hh @@ -58,19 +58,6 @@ struct RealisedPath { RealisedPath(StorePath path) : raw(OpaquePath{path}) {} RealisedPath(Realisation r) : raw(r) {} - /** - * Syntactic sugar to run `std::visit` on the raw value: - * path.visit(blah) == std::visit(blah, path.raw) - */ - template - constexpr decltype(auto) visit(Visitor && vis) { - return std::visit(vis, raw); - } - template - constexpr decltype(auto) visit(Visitor && vis) const { - return std::visit(vis, raw); - } - /** * Get the raw store path associated to this */