From 7a04839ea53986a0175bf085e7d061ab8a32d59b Mon Sep 17 00:00:00 2001 From: Maximilian Bosch Date: Mon, 31 Jan 2022 22:05:28 +0100 Subject: [PATCH 01/31] ssh-ng: also store build logs to make them accessible by `nix log` Right now when building a derivation remotely via $ nix build -j0 -f . hello -L --builders 'ssh://builder' it's possible later to read through the entire build-log by running `nix log -f . hello`. This isn't possible however when using `ssh-ng` rather than `ssh`. The reason for that is that there are two different ways to transfer logs in Nix through e.g. an SSH tunnel (that are used by `ssh`/`ssh-ng` respectively): * `ssh://` receives its logs from the fd pointing to `builderOut`. This is directly passed to the "log-sink" (and to the logger on each `\n`), hence `nix log` works here. * `ssh-ng://` however expects JSON-like messages (i.e. `@nix {log data in here}`) and passes it directly to the logger without doing anything with the `logSink`. However it's certainly possible to extract log-lines from this format as these have their own message-type in the JSON payload (i.e. `resBuildLogLine`). This is basically what I changed in this patch: if the code-path for `builderOut` is not reached and a `logSink` is initialized, the message was successfully processed by the JSON logger (i.e. it's in the expected format) and the line is of the expected type (i.e. `resBuildLogLine`), the line will be written to the log-sink as well. Closes #5079 --- src/libstore/build/derivation-goal.cc | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 95da1841d..ef1135601 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -1175,10 +1175,10 @@ bool DerivationGoal::isReadDesc(int fd) return fd == hook->builderOut.readSide.get(); } - void DerivationGoal::handleChildOutput(int fd, std::string_view data) { - if (isReadDesc(fd)) + auto isWrittenToLog = isReadDesc(fd); + if (isWrittenToLog) { logSize += data.size(); if (settings.maxLogSize && logSize > settings.maxLogSize) { @@ -1207,7 +1207,14 @@ void DerivationGoal::handleChildOutput(int fd, std::string_view data) if (hook && fd == hook->fromHook.readSide.get()) { for (auto c : data) if (c == '\n') { - handleJSONLogMessage(currentHookLine, worker.act, hook->activities, true); + auto s = handleJSONLogMessage(currentHookLine, worker.act, hook->activities, true); + if (s && !isWrittenToLog && logSink) { + auto json = nlohmann::json::parse(std::string(currentHookLine, 5)); + if (json["type"] == resBuildLogLine) { + auto f = json["fields"]; + (*logSink)((f.size() > 0 ? f.at(0).get() : "") + "\n"); + } + } currentHookLine.clear(); } else currentHookLine += c; From cd92ea588504db697103d3ff56166fe61e6da131 Mon Sep 17 00:00:00 2001 From: Maximilian Bosch Date: Sat, 19 Feb 2022 22:34:50 +0100 Subject: [PATCH 02/31] libstore/derivation-goal: avoid double-parsing of JSON messages To avoid that JSON messages are parsed twice in case of remote builds with `ssh-ng://`, I split up the original `handleJSONLogMessage` into three parts: * `parseJSONMessage(const std::string&)` checks if it's a message in the form of `@nix {...}` and tries to parse it (and prints an error if the parsing fails). * `handleJSONLogMessage(nlohmann::json&, ...)` reads the fields from the message and passes them to the logger. * `handleJSONLogMessage(const std::string&, ...)` behaves as before, but uses the two functions mentioned above as implementation. In case of `ssh-ng://`-logs the first two methods are invoked manually. --- src/libstore/build/derivation-goal.cc | 14 +++-- src/libutil/logging.cc | 84 +++++++++++++++------------ src/libutil/logging.hh | 8 +++ 3 files changed, 64 insertions(+), 42 deletions(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index ef1135601..c49be5e79 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -1207,12 +1207,14 @@ void DerivationGoal::handleChildOutput(int fd, std::string_view data) if (hook && fd == hook->fromHook.readSide.get()) { for (auto c : data) if (c == '\n') { - auto s = handleJSONLogMessage(currentHookLine, worker.act, hook->activities, true); - if (s && !isWrittenToLog && logSink) { - auto json = nlohmann::json::parse(std::string(currentHookLine, 5)); - if (json["type"] == resBuildLogLine) { - auto f = json["fields"]; - (*logSink)((f.size() > 0 ? f.at(0).get() : "") + "\n"); + auto json = parseJSONMessage(currentHookLine); + if (json) { + auto s = handleJSONLogMessage(*json, worker.act, hook->activities, true); + if (s && !isWrittenToLog && logSink) { + if ((*json)["type"] == resBuildLogLine) { + auto f = (*json)["fields"]; + (*logSink)((f.size() > 0 ? f.at(0).get() : "") + "\n"); + } } } currentHookLine.clear(); diff --git a/src/libutil/logging.cc b/src/libutil/logging.cc index 74ee2f063..cb2b15b41 100644 --- a/src/libutil/logging.cc +++ b/src/libutil/logging.cc @@ -266,51 +266,63 @@ static Logger::Fields getFields(nlohmann::json & json) return fields; } -bool handleJSONLogMessage(const std::string & msg, - const Activity & act, std::map & activities, bool trusted) +std::optional parseJSONMessage(const std::string & msg) { - if (!hasPrefix(msg, "@nix ")) return false; - + if (!hasPrefix(msg, "@nix ")) return std::nullopt; try { - auto json = nlohmann::json::parse(std::string(msg, 5)); - - std::string action = json["action"]; - - if (action == "start") { - auto type = (ActivityType) json["type"]; - if (trusted || type == actFileTransfer) - activities.emplace(std::piecewise_construct, - std::forward_as_tuple(json["id"]), - std::forward_as_tuple(*logger, (Verbosity) json["level"], type, - json["text"], getFields(json["fields"]), act.id)); - } - - else if (action == "stop") - activities.erase((ActivityId) json["id"]); - - else if (action == "result") { - auto i = activities.find((ActivityId) json["id"]); - if (i != activities.end()) - i->second.result((ResultType) json["type"], getFields(json["fields"])); - } - - else if (action == "setPhase") { - std::string phase = json["phase"]; - act.result(resSetPhase, phase); - } - - else if (action == "msg") { - std::string msg = json["msg"]; - logger->log((Verbosity) json["level"], msg); - } - + return nlohmann::json::parse(std::string(msg, 5)); } catch (std::exception & e) { printError("bad JSON log message from builder: %s", e.what()); } + return std::nullopt; +} + +bool handleJSONLogMessage(nlohmann::json & json, + const Activity & act, std::map & activities, + bool trusted) +{ + std::string action = json["action"]; + + if (action == "start") { + auto type = (ActivityType) json["type"]; + if (trusted || type == actFileTransfer) + activities.emplace(std::piecewise_construct, + std::forward_as_tuple(json["id"]), + std::forward_as_tuple(*logger, (Verbosity) json["level"], type, + json["text"], getFields(json["fields"]), act.id)); + } + + else if (action == "stop") + activities.erase((ActivityId) json["id"]); + + else if (action == "result") { + auto i = activities.find((ActivityId) json["id"]); + if (i != activities.end()) + i->second.result((ResultType) json["type"], getFields(json["fields"])); + } + + else if (action == "setPhase") { + std::string phase = json["phase"]; + act.result(resSetPhase, phase); + } + + else if (action == "msg") { + std::string msg = json["msg"]; + logger->log((Verbosity) json["level"], msg); + } return true; } +bool handleJSONLogMessage(const std::string & msg, + const Activity & act, std::map & activities, bool trusted) +{ + auto json = parseJSONMessage(msg); + if (!json) return false; + + return handleJSONLogMessage(*json, act, activities, trusted); +} + Activity::~Activity() { try { diff --git a/src/libutil/logging.hh b/src/libutil/logging.hh index bd28036ae..9ab2091c7 100644 --- a/src/libutil/logging.hh +++ b/src/libutil/logging.hh @@ -4,6 +4,8 @@ #include "error.hh" #include "config.hh" +#include + namespace nix { typedef enum { @@ -166,6 +168,12 @@ Logger * makeSimpleLogger(bool printBuildLogs = true); Logger * makeJSONLogger(Logger & prevLogger); +std::optional parseJSONMessage(const std::string & msg); + +bool handleJSONLogMessage(nlohmann::json & json, + const Activity & act, std::map & activities, + bool trusted); + bool handleJSONLogMessage(const std::string & msg, const Activity & act, std::map & activities, bool trusted); From 287642f132d8141269bd7c23b0bc7c1ee512b171 Mon Sep 17 00:00:00 2001 From: Maximilian Bosch Date: Sun, 20 Feb 2022 15:56:22 +0100 Subject: [PATCH 03/31] tests: implement test for `nix log` with `ssh-ng://` builds A few notes: * The `echo hi` is needed to make sure that a file that can be read by `nix log` is properly created (i.e. some output is needed). This is known and to be fixed in #6051. * We explicitly ignore the floating-CA case here: the `$out` of `input3` depends on `$out` of `input2`. This means that there are actually two derivations - I assume that this is because at eval time (i.e. `nix-instantiate -A`) the hash of `input2` isn't known yet and the other .drv is created as soon as `input2` was built. This is another issue on its own, so we ignore the case here explicitly. --- tests/build-hook-ca-fixed.nix | 10 ++++++---- tests/build-hook.nix | 10 ++++++---- tests/build-remote.sh | 8 ++++++++ 3 files changed, 20 insertions(+), 8 deletions(-) diff --git a/tests/build-hook-ca-fixed.nix b/tests/build-hook-ca-fixed.nix index ec7171ac9..899b610a8 100644 --- a/tests/build-hook-ca-fixed.nix +++ b/tests/build-hook-ca-fixed.nix @@ -11,13 +11,13 @@ let args = ["sh" "-e" args.builder or (builtins.toFile "builder-${args.name}.sh" "if [ -e .attrs.sh ]; then source .attrs.sh; fi; eval \"$buildCommand\"")]; outputHashMode = "recursive"; outputHashAlgo = "sha256"; - } // removeAttrs args ["builder" "meta"]) - // { meta = args.meta or {}; }; + } // removeAttrs args ["builder" "meta" "passthru"]) + // { meta = args.meta or {}; passthru = args.passthru or {}; }; input1 = mkDerivation { shell = busybox; name = "build-remote-input-1"; - buildCommand = "echo FOO > $out"; + buildCommand = "echo hi; echo FOO > $out"; requiredSystemFeatures = ["foo"]; outputHash = "sha256-FePFYIlMuycIXPZbWi7LGEiMmZSX9FMbaQenWBzm1Sc="; }; @@ -25,7 +25,7 @@ let input2 = mkDerivation { shell = busybox; name = "build-remote-input-2"; - buildCommand = "echo BAR > $out"; + buildCommand = "echo hi; echo BAR > $out"; requiredSystemFeatures = ["bar"]; outputHash = "sha256-XArauVH91AVwP9hBBQNlkX9ccuPpSYx9o0zeIHb6e+Q="; }; @@ -34,6 +34,7 @@ let shell = busybox; name = "build-remote-input-3"; buildCommand = '' + echo hi read x < ${input2} echo $x BAZ > $out ''; @@ -46,6 +47,7 @@ in mkDerivation { shell = busybox; name = "build-remote"; + passthru = { inherit input1 input2 input3; }; buildCommand = '' read x < ${input1} diff --git a/tests/build-hook.nix b/tests/build-hook.nix index eb16676f0..3c83e0475 100644 --- a/tests/build-hook.nix +++ b/tests/build-hook.nix @@ -9,20 +9,20 @@ let inherit system; builder = busybox; args = ["sh" "-e" args.builder or (builtins.toFile "builder-${args.name}.sh" "if [ -e .attrs.sh ]; then source .attrs.sh; fi; eval \"$buildCommand\"")]; - } // removeAttrs args ["builder" "meta"]) - // { meta = args.meta or {}; }; + } // removeAttrs args ["builder" "meta" "passthru"]) + // { meta = args.meta or {}; passthru = args.passthru or {}; }; input1 = mkDerivation { shell = busybox; name = "build-remote-input-1"; - buildCommand = "echo FOO > $out"; + buildCommand = "echo hi; echo FOO > $out"; requiredSystemFeatures = ["foo"]; }; input2 = mkDerivation { shell = busybox; name = "build-remote-input-2"; - buildCommand = "echo BAR > $out"; + buildCommand = "echo hi; echo BAR > $out"; requiredSystemFeatures = ["bar"]; }; @@ -30,6 +30,7 @@ let shell = busybox; name = "build-remote-input-3"; buildCommand = '' + echo hi read x < ${input2} echo $x BAZ > $out ''; @@ -41,6 +42,7 @@ in mkDerivation { shell = busybox; name = "build-remote"; + passthru = { inherit input1 input2 input3; }; buildCommand = '' read x < ${input1} diff --git a/tests/build-remote.sh b/tests/build-remote.sh index 806c6d261..4ecc788a3 100644 --- a/tests/build-remote.sh +++ b/tests/build-remote.sh @@ -54,6 +54,14 @@ nix path-info --store $TEST_ROOT/machine3 --all \ | grep -v builder-build-remote-input-2.sh \ | grep builder-build-remote-input-3.sh + +if [[ -z "$CONTENT_ADDRESSED" ]]; then + for i in input1 input3; do + drv="$(nix-instantiate $file -A passthru.$i --store $TEST_ROOT/machine0 --arg busybox $busybox)" + nix log --store $TEST_ROOT/machine0 "$drv" + done +fi + # Behavior of keep-failed out="$(nix-build 2>&1 failing.nix \ --builders "$(join_by '; ' "${builders[@]}")" \ From 102cb390864f8a142ad6730d3456784e2778a493 Mon Sep 17 00:00:00 2001 From: Maximilian Bosch Date: Sun, 20 Feb 2022 16:02:44 +0100 Subject: [PATCH 04/31] libstore/build: add a few explanatory comments; simplify --- src/libstore/build/derivation-goal.cc | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index c49be5e79..ae250ffaf 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -1177,6 +1177,7 @@ bool DerivationGoal::isReadDesc(int fd) void DerivationGoal::handleChildOutput(int fd, std::string_view data) { + // local & `ssh://`-builds are dealt with here. auto isWrittenToLog = isReadDesc(fd); if (isWrittenToLog) { @@ -1210,11 +1211,11 @@ void DerivationGoal::handleChildOutput(int fd, std::string_view data) auto json = parseJSONMessage(currentHookLine); if (json) { auto s = handleJSONLogMessage(*json, worker.act, hook->activities, true); - if (s && !isWrittenToLog && logSink) { - if ((*json)["type"] == resBuildLogLine) { - auto f = (*json)["fields"]; - (*logSink)((f.size() > 0 ? f.at(0).get() : "") + "\n"); - } + // ensure that logs from a builder using `ssh-ng://` as protocol + // are also available to `nix log`. + if (s && !isWrittenToLog && logSink && (*json)["type"] == resBuildLogLine) { + auto f = (*json)["fields"]; + (*logSink)((f.size() > 0 ? f.at(0).get() : "") + "\n"); } } currentHookLine.clear(); From 6a8f1b548fc85af7e065feee93920839ec94fa40 Mon Sep 17 00:00:00 2001 From: Maximilian Bosch Date: Mon, 28 Feb 2022 16:05:21 +0100 Subject: [PATCH 05/31] logging.hh: json.hpp -> json_fwd.hpp --- src/libutil/logging.hh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libutil/logging.hh b/src/libutil/logging.hh index 9ab2091c7..6f81b92de 100644 --- a/src/libutil/logging.hh +++ b/src/libutil/logging.hh @@ -4,7 +4,7 @@ #include "error.hh" #include "config.hh" -#include +#include namespace nix { From ecff9d969acef5a08d3963d16eac83a5210de86e Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 3 Mar 2022 13:07:50 +0100 Subject: [PATCH 06/31] printValue(): Don't show repeated values Fixes #6157. --- src/libexpr/eval.cc | 25 +++++++++++++------------ src/libexpr/value.hh | 4 ++-- src/nix/repl.cc | 2 +- 3 files changed, 16 insertions(+), 15 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 193358161..d9d36fab2 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -86,15 +86,10 @@ RootValue allocRootValue(Value * v) } -void printValue(std::ostream & str, std::set & active, const Value & v) +void printValue(std::ostream & str, std::set & seen, const Value & v) { checkInterrupt(); - if (!active.insert(&v).second) { - str << ""; - return; - } - switch (v.internalType) { case tInt: str << v.integer; @@ -120,10 +115,14 @@ void printValue(std::ostream & str, std::set & active, const Valu str << "null"; break; case tAttrs: { + seen.insert(&v); str << "{ "; for (auto & i : v.attrs->lexicographicOrder()) { str << i->name << " = "; - printValue(str, active, *i->value); + if (seen.count(i->value)) + str << ""; + else + printValue(str, seen, *i->value); str << "; "; } str << "}"; @@ -132,9 +131,13 @@ void printValue(std::ostream & str, std::set & active, const Valu case tList1: case tList2: case tListN: + seen.insert(&v); str << "[ "; for (auto v2 : v.listItems()) { - printValue(str, active, *v2); + if (seen.count(v2)) + str << ""; + else + printValue(str, seen, *v2); str << " "; } str << "]"; @@ -161,15 +164,13 @@ void printValue(std::ostream & str, std::set & active, const Valu default: abort(); } - - active.erase(&v); } std::ostream & operator << (std::ostream & str, const Value & v) { - std::set active; - printValue(str, active, v); + std::set seen; + printValue(str, seen, v); return str; } diff --git a/src/libexpr/value.hh b/src/libexpr/value.hh index 3fdff71a5..84c4d09f8 100644 --- a/src/libexpr/value.hh +++ b/src/libexpr/value.hh @@ -114,8 +114,8 @@ struct Value private: InternalType internalType; -friend std::string showType(const Value & v); -friend void printValue(std::ostream & str, std::set & active, const Value & v); + friend std::string showType(const Value & v); + friend void printValue(std::ostream & str, std::set & seen, const Value & v); public: diff --git a/src/nix/repl.cc b/src/nix/repl.cc index 5c0d44c68..3a51a13e6 100644 --- a/src/nix/repl.cc +++ b/src/nix/repl.cc @@ -800,7 +800,7 @@ std::ostream & NixRepl::printValue(std::ostream & str, Value & v, unsigned int m else printStringValue(str, i.first.c_str()); str << " = "; - if (seen.find(i.second) != seen.end()) + if (seen.count(i.second)) str << "«repeated»"; else try { From e9c04c3351b4b3acae2d154b9aba808c3600054f Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 3 Mar 2022 13:29:14 +0100 Subject: [PATCH 07/31] Be more aggressive in hiding repeated values We now memoize on Bindings / list element vectors rather than Values, so that e.g. two Values that point to the same Bindings will be printed only once. --- src/libexpr/eval.cc | 38 +++++++++++++++++++------------------- src/libexpr/value.hh | 2 +- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index d9d36fab2..2d7309738 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -86,7 +86,7 @@ RootValue allocRootValue(Value * v) } -void printValue(std::ostream & str, std::set & seen, const Value & v) +void printValue(std::ostream & str, std::set & seen, const Value & v) { checkInterrupt(); @@ -115,32 +115,32 @@ void printValue(std::ostream & str, std::set & seen, const Value str << "null"; break; case tAttrs: { - seen.insert(&v); - str << "{ "; - for (auto & i : v.attrs->lexicographicOrder()) { - str << i->name << " = "; - if (seen.count(i->value)) - str << ""; - else + if (!v.attrs->empty() && !seen.insert(v.attrs).second) + str << ""; + else { + str << "{ "; + for (auto & i : v.attrs->lexicographicOrder()) { + str << i->name << " = "; printValue(str, seen, *i->value); - str << "; "; + str << "; "; + } + str << "}"; } - str << "}"; break; } case tList1: case tList2: case tListN: - seen.insert(&v); - str << "[ "; - for (auto v2 : v.listItems()) { - if (seen.count(v2)) - str << ""; - else + if (v.listSize() && !seen.insert(v.listElems()).second) + str << ""; + else { + str << "[ "; + for (auto v2 : v.listItems()) { printValue(str, seen, *v2); - str << " "; + str << " "; + } + str << "]"; } - str << "]"; break; case tThunk: case tApp: @@ -169,7 +169,7 @@ void printValue(std::ostream & str, std::set & seen, const Value std::ostream & operator << (std::ostream & str, const Value & v) { - std::set seen; + std::set seen; printValue(str, seen, v); return str; } diff --git a/src/libexpr/value.hh b/src/libexpr/value.hh index 84c4d09f8..d0fa93e92 100644 --- a/src/libexpr/value.hh +++ b/src/libexpr/value.hh @@ -115,7 +115,7 @@ private: InternalType internalType; friend std::string showType(const Value & v); - friend void printValue(std::ostream & str, std::set & seen, const Value & v); + friend void printValue(std::ostream & str, std::set & seen, const Value & v); public: From 6636202356b94ca4128462493770e7fedf997b0e Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 1 Mar 2022 18:31:36 +0000 Subject: [PATCH 08/31] Factor out a `GcStore` interface Starts progress on #5729. The idea is that we should not have these default methods throwing "unimplemented". This is a small step in that direction. I kept `addTempRoot` because it is a no-op, rather than failure. Also, as a practical matter, it is called all over the place, while doing other tasks, so the downcasting would be annoying. Maybe in the future I could move the "real" `addTempRoot` to `GcStore`, and the existing usecases use a `tryAddTempRoot` wrapper to downcast or do nothing, but I wasn't sure whether that was a good idea so with a bias to less churn I didn't do it yet. --- src/libmain/shared.cc | 1 + src/libstore/build/local-derivation-goal.cc | 3 +- src/libstore/daemon.cc | 12 ++- src/libstore/gc-store.cc | 13 +++ src/libstore/gc-store.hh | 84 +++++++++++++++++++ src/libstore/local-fs-store.hh | 3 +- src/libstore/local-store.hh | 3 +- src/libstore/path-with-outputs.cc | 6 +- src/libstore/remote-store.cc | 1 + src/libstore/remote-store.hh | 3 +- src/libstore/store-api.hh | 73 ---------------- src/libutil/tests/logging.cc | 2 +- .../nix-collect-garbage.cc | 4 +- src/nix-store/nix-store.cc | 18 ++-- src/nix/store-delete.cc | 5 +- src/nix/store-gc.cc | 5 +- 16 files changed, 143 insertions(+), 93 deletions(-) create mode 100644 src/libstore/gc-store.cc create mode 100644 src/libstore/gc-store.hh diff --git a/src/libmain/shared.cc b/src/libmain/shared.cc index a0b0f4cb3..562d1b414 100644 --- a/src/libmain/shared.cc +++ b/src/libmain/shared.cc @@ -1,6 +1,7 @@ #include "globals.hh" #include "shared.hh" #include "store-api.hh" +#include "gc-store.hh" #include "util.hh" #include "loggers.hh" diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 7e69d4145..581d63d0e 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -1,4 +1,5 @@ #include "local-derivation-goal.hh" +#include "gc-store.hh" #include "hook-instance.hh" #include "worker.hh" #include "builtins.hh" @@ -1127,7 +1128,7 @@ struct RestrictedStoreConfig : virtual LocalFSStoreConfig /* A wrapper around LocalStore that only allows building/querying of paths that are in the input closures of the build or were added via recursive Nix calls. */ -struct RestrictedStore : public virtual RestrictedStoreConfig, public virtual LocalFSStore +struct RestrictedStore : public virtual RestrictedStoreConfig, public virtual LocalFSStore, public virtual GcStore { ref next; diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc index 9d4f6b4a4..89d9487da 100644 --- a/src/libstore/daemon.cc +++ b/src/libstore/daemon.cc @@ -3,6 +3,7 @@ #include "worker-protocol.hh" #include "build-result.hh" #include "store-api.hh" +#include "gc-store.hh" #include "path-with-outputs.hh" #include "finally.hh" #include "archive.hh" @@ -623,9 +624,12 @@ static void performOp(TunnelLogger * logger, ref store, case wopAddIndirectRoot: { Path path = absPath(readString(from)); + logger->startWork(); - store->addIndirectRoot(path); + auto & gcStore = requireGcStore(*store); + gcStore.addIndirectRoot(path); logger->stopWork(); + to << 1; break; } @@ -640,7 +644,8 @@ static void performOp(TunnelLogger * logger, ref store, case wopFindRoots: { logger->startWork(); - Roots roots = store->findRoots(!trusted); + auto & gcStore = requireGcStore(*store); + Roots roots = gcStore.findRoots(!trusted); logger->stopWork(); size_t size = 0; @@ -671,7 +676,8 @@ static void performOp(TunnelLogger * logger, ref store, logger->startWork(); if (options.ignoreLiveness) throw Error("you are not allowed to ignore liveness"); - store->collectGarbage(options, results); + auto & gcStore = requireGcStore(*store); + gcStore.collectGarbage(options, results); logger->stopWork(); to << results.paths << results.bytesFreed << 0 /* obsolete */; diff --git a/src/libstore/gc-store.cc b/src/libstore/gc-store.cc new file mode 100644 index 000000000..3dbdec53b --- /dev/null +++ b/src/libstore/gc-store.cc @@ -0,0 +1,13 @@ +#include "gc-store.hh" + +namespace nix { + +GcStore & requireGcStore(Store & store) +{ + auto * gcStore = dynamic_cast(&store); + if (!gcStore) + throw UsageError("Garbage collection not supported by this store"); + return *gcStore; +} + +} diff --git a/src/libstore/gc-store.hh b/src/libstore/gc-store.hh new file mode 100644 index 000000000..829f70dc4 --- /dev/null +++ b/src/libstore/gc-store.hh @@ -0,0 +1,84 @@ +#pragma once + +#include "store-api.hh" + + +namespace nix { + + +typedef std::unordered_map> Roots; + + +struct GCOptions +{ + /* Garbage collector operation: + + - `gcReturnLive': return the set of paths reachable from + (i.e. in the closure of) the roots. + + - `gcReturnDead': return the set of paths not reachable from + the roots. + + - `gcDeleteDead': actually delete the latter set. + + - `gcDeleteSpecific': delete the paths listed in + `pathsToDelete', insofar as they are not reachable. + */ + typedef enum { + gcReturnLive, + gcReturnDead, + gcDeleteDead, + gcDeleteSpecific, + } GCAction; + + GCAction action{gcDeleteDead}; + + /* If `ignoreLiveness' is set, then reachability from the roots is + ignored (dangerous!). However, the paths must still be + unreferenced *within* the store (i.e., there can be no other + store paths that depend on them). */ + bool ignoreLiveness{false}; + + /* For `gcDeleteSpecific', the paths to delete. */ + StorePathSet pathsToDelete; + + /* Stop after at least `maxFreed' bytes have been freed. */ + uint64_t maxFreed{std::numeric_limits::max()}; +}; + + +struct GCResults +{ + /* Depending on the action, the GC roots, or the paths that would + be or have been deleted. */ + PathSet paths; + + /* For `gcReturnDead', `gcDeleteDead' and `gcDeleteSpecific', the + number of bytes that would be or was freed. */ + uint64_t bytesFreed = 0; +}; + + +struct GcStore : public virtual Store +{ + /* Add an indirect root, which is merely a symlink to `path' from + /nix/var/nix/gcroots/auto/. `path' is supposed + to be a symlink to a store path. The garbage collector will + automatically remove the indirect root when it finds that + `path' has disappeared. */ + virtual void addIndirectRoot(const Path & path) = 0; + + /* Find the roots of the garbage collector. Each root is a pair + (link, storepath) where `link' is the path of the symlink + outside of the Nix store that point to `storePath'. If + 'censor' is true, privacy-sensitive information about roots + found in /proc is censored. */ + virtual Roots findRoots(bool censor) = 0; + + /* Perform a garbage collection. */ + virtual void collectGarbage(const GCOptions & options, GCResults & results) = 0; +}; + +GcStore & requireGcStore(Store & store); + +} diff --git a/src/libstore/local-fs-store.hh b/src/libstore/local-fs-store.hh index d34f0cb62..fbd49dc2c 100644 --- a/src/libstore/local-fs-store.hh +++ b/src/libstore/local-fs-store.hh @@ -1,6 +1,7 @@ #pragma once #include "store-api.hh" +#include "gc-store.hh" namespace nix { @@ -23,7 +24,7 @@ struct LocalFSStoreConfig : virtual StoreConfig "physical path to the Nix store"}; }; -class LocalFSStore : public virtual LocalFSStoreConfig, public virtual Store +class LocalFSStore : public virtual LocalFSStoreConfig, public virtual Store, virtual GcStore { public: diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 1a278c9a8..70d225be3 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -5,6 +5,7 @@ #include "pathlocks.hh" #include "store-api.hh" #include "local-fs-store.hh" +#include "gc-store.hh" #include "sync.hh" #include "util.hh" @@ -43,7 +44,7 @@ struct LocalStoreConfig : virtual LocalFSStoreConfig }; -class LocalStore : public virtual LocalStoreConfig, public virtual LocalFSStore +class LocalStore : public virtual LocalStoreConfig, public virtual LocalFSStore, public virtual GcStore { private: diff --git a/src/libstore/path-with-outputs.cc b/src/libstore/path-with-outputs.cc index 97aa01b57..078c117bd 100644 --- a/src/libstore/path-with-outputs.cc +++ b/src/libstore/path-with-outputs.cc @@ -22,9 +22,9 @@ DerivedPath StorePathWithOutputs::toDerivedPath() const std::vector toDerivedPaths(const std::vector ss) { - std::vector reqs; - for (auto & s : ss) reqs.push_back(s.toDerivedPath()); - return reqs; + std::vector reqs; + for (auto & s : ss) reqs.push_back(s.toDerivedPath()); + return reqs; } diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index cbcb75c00..a25398ef2 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -1,6 +1,7 @@ #include "serialise.hh" #include "util.hh" #include "path-with-outputs.hh" +#include "gc-store.hh" #include "remote-fs-accessor.hh" #include "build-result.hh" #include "remote-store.hh" diff --git a/src/libstore/remote-store.hh b/src/libstore/remote-store.hh index b94216d31..2628206b1 100644 --- a/src/libstore/remote-store.hh +++ b/src/libstore/remote-store.hh @@ -4,6 +4,7 @@ #include #include "store-api.hh" +#include "gc-store.hh" namespace nix { @@ -29,7 +30,7 @@ struct RemoteStoreConfig : virtual StoreConfig /* FIXME: RemoteStore is a misnomer - should be something like DaemonStore. */ -class RemoteStore : public virtual RemoteStoreConfig, public virtual Store +class RemoteStore : public virtual RemoteStoreConfig, public virtual Store, public virtual GcStore { public: diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 8c57596d0..7bd21519c 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -76,59 +76,6 @@ enum AllowInvalidFlag : bool { DisallowInvalid = false, AllowInvalid = true }; const uint32_t exportMagic = 0x4558494e; -typedef std::unordered_map> Roots; - - -struct GCOptions -{ - /* Garbage collector operation: - - - `gcReturnLive': return the set of paths reachable from - (i.e. in the closure of) the roots. - - - `gcReturnDead': return the set of paths not reachable from - the roots. - - - `gcDeleteDead': actually delete the latter set. - - - `gcDeleteSpecific': delete the paths listed in - `pathsToDelete', insofar as they are not reachable. - */ - typedef enum { - gcReturnLive, - gcReturnDead, - gcDeleteDead, - gcDeleteSpecific, - } GCAction; - - GCAction action{gcDeleteDead}; - - /* If `ignoreLiveness' is set, then reachability from the roots is - ignored (dangerous!). However, the paths must still be - unreferenced *within* the store (i.e., there can be no other - store paths that depend on them). */ - bool ignoreLiveness{false}; - - /* For `gcDeleteSpecific', the paths to delete. */ - StorePathSet pathsToDelete; - - /* Stop after at least `maxFreed' bytes have been freed. */ - uint64_t maxFreed{std::numeric_limits::max()}; -}; - - -struct GCResults -{ - /* Depending on the action, the GC roots, or the paths that would - be or have been deleted. */ - PathSet paths; - - /* For `gcReturnDead', `gcDeleteDead' and `gcDeleteSpecific', the - number of bytes that would be or was freed. */ - uint64_t bytesFreed = 0; -}; - - enum BuildMode { bmNormal, bmRepair, bmCheck }; struct BuildResult; @@ -531,26 +478,6 @@ public: virtual void addTempRoot(const StorePath & path) { debug("not creating temporary root, store doesn't support GC"); } - /* Add an indirect root, which is merely a symlink to `path' from - /nix/var/nix/gcroots/auto/. `path' is supposed - to be a symlink to a store path. The garbage collector will - automatically remove the indirect root when it finds that - `path' has disappeared. */ - virtual void addIndirectRoot(const Path & path) - { unsupported("addIndirectRoot"); } - - /* Find the roots of the garbage collector. Each root is a pair - (link, storepath) where `link' is the path of the symlink - outside of the Nix store that point to `storePath'. If - 'censor' is true, privacy-sensitive information about roots - found in /proc is censored. */ - virtual Roots findRoots(bool censor) - { unsupported("findRoots"); } - - /* Perform a garbage collection. */ - virtual void collectGarbage(const GCOptions & options, GCResults & results) - { unsupported("collectGarbage"); } - /* Return a string representing information about the path that can be loaded into the database using `nix-store --load-db' or `nix-store --register-validity'. */ diff --git a/src/libutil/tests/logging.cc b/src/libutil/tests/logging.cc index cef3bd481..2ffdc2e9b 100644 --- a/src/libutil/tests/logging.cc +++ b/src/libutil/tests/logging.cc @@ -359,7 +359,7 @@ namespace nix { // constructing without access violation. ErrPos ep(invalid); - + // assignment without access violation. ep = invalid; diff --git a/src/nix-collect-garbage/nix-collect-garbage.cc b/src/nix-collect-garbage/nix-collect-garbage.cc index 48c030b18..4b28ea6a4 100644 --- a/src/nix-collect-garbage/nix-collect-garbage.cc +++ b/src/nix-collect-garbage/nix-collect-garbage.cc @@ -1,4 +1,5 @@ #include "store-api.hh" +#include "gc-store.hh" #include "profiles.hh" #include "shared.hh" #include "globals.hh" @@ -80,10 +81,11 @@ static int main_nix_collect_garbage(int argc, char * * argv) // Run the actual garbage collector. if (!dryRun) { auto store = openStore(); + auto & gcStore = requireGcStore(*store); options.action = GCOptions::gcDeleteDead; GCResults results; PrintFreed freed(true, results); - store->collectGarbage(options, results); + gcStore.collectGarbage(options, results); } return 0; diff --git a/src/nix-store/nix-store.cc b/src/nix-store/nix-store.cc index 1ebc177f5..8ebaf9387 100644 --- a/src/nix-store/nix-store.cc +++ b/src/nix-store/nix-store.cc @@ -3,6 +3,7 @@ #include "dotgraph.hh" #include "globals.hh" #include "build-result.hh" +#include "gc-store.hh" #include "local-store.hh" #include "monitor-fd.hh" #include "serve-protocol.hh" @@ -428,11 +429,12 @@ static void opQuery(Strings opFlags, Strings opArgs) store->computeFSClosure( args, referrers, true, settings.gcKeepOutputs, settings.gcKeepDerivations); - Roots roots = store->findRoots(false); + auto & gcStore = requireGcStore(*store); + Roots roots = gcStore.findRoots(false); for (auto & [target, links] : roots) if (referrers.find(target) != referrers.end()) for (auto & link : links) - cout << fmt("%1% -> %2%\n", link, store->printStorePath(target)); + cout << fmt("%1% -> %2%\n", link, gcStore.printStorePath(target)); break; } @@ -588,20 +590,22 @@ static void opGC(Strings opFlags, Strings opArgs) if (!opArgs.empty()) throw UsageError("no arguments expected"); + auto & gcStore = requireGcStore(*store); + if (printRoots) { - Roots roots = store->findRoots(false); + Roots roots = gcStore.findRoots(false); std::set> roots2; // Transpose and sort the roots. for (auto & [target, links] : roots) for (auto & link : links) roots2.emplace(link, target); for (auto & [link, target] : roots2) - std::cout << link << " -> " << store->printStorePath(target) << "\n"; + std::cout << link << " -> " << gcStore.printStorePath(target) << "\n"; } else { PrintFreed freed(options.action == GCOptions::gcDeleteDead, results); - store->collectGarbage(options, results); + gcStore.collectGarbage(options, results); if (options.action != GCOptions::gcDeleteDead) for (auto & i : results.paths) @@ -625,9 +629,11 @@ static void opDelete(Strings opFlags, Strings opArgs) for (auto & i : opArgs) options.pathsToDelete.insert(store->followLinksToStorePath(i)); + auto & gcStore = requireGcStore(*store); + GCResults results; PrintFreed freed(true, results); - store->collectGarbage(options, results); + gcStore.collectGarbage(options, results); } diff --git a/src/nix/store-delete.cc b/src/nix/store-delete.cc index e4a3cb554..aa7a8b12f 100644 --- a/src/nix/store-delete.cc +++ b/src/nix/store-delete.cc @@ -2,6 +2,7 @@ #include "common-args.hh" #include "shared.hh" #include "store-api.hh" +#include "gc-store.hh" using namespace nix; @@ -32,12 +33,14 @@ struct CmdStoreDelete : StorePathsCommand void run(ref store, std::vector && storePaths) override { + auto & gcStore = requireGcStore(*store); + for (auto & path : storePaths) options.pathsToDelete.insert(path); GCResults results; PrintFreed freed(true, results); - store->collectGarbage(options, results); + gcStore.collectGarbage(options, results); } }; diff --git a/src/nix/store-gc.cc b/src/nix/store-gc.cc index a2d74066e..21718dc0c 100644 --- a/src/nix/store-gc.cc +++ b/src/nix/store-gc.cc @@ -2,6 +2,7 @@ #include "common-args.hh" #include "shared.hh" #include "store-api.hh" +#include "gc-store.hh" using namespace nix; @@ -33,10 +34,12 @@ struct CmdStoreGC : StoreCommand, MixDryRun void run(ref store) override { + auto & gcStore = requireGcStore(*store); + options.action = dryRun ? GCOptions::gcReturnDead : GCOptions::gcDeleteDead; GCResults results; PrintFreed freed(options.action == GCOptions::gcDeleteDead, results); - store->collectGarbage(options, results); + gcStore.collectGarbage(options, results); } }; From 697d1dac0164001a10ab990a382ad83f2e7628d4 Mon Sep 17 00:00:00 2001 From: Maximilian Bosch Date: Fri, 4 Mar 2022 16:58:27 +0100 Subject: [PATCH 09/31] tests: grep for string in nix log for remote-builds --- tests/build-hook-ca-fixed.nix | 4 ++-- tests/build-hook.nix | 4 ++-- tests/build-remote.sh | 3 +-- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/tests/build-hook-ca-fixed.nix b/tests/build-hook-ca-fixed.nix index 899b610a8..4cb9e85d1 100644 --- a/tests/build-hook-ca-fixed.nix +++ b/tests/build-hook-ca-fixed.nix @@ -17,7 +17,7 @@ let input1 = mkDerivation { shell = busybox; name = "build-remote-input-1"; - buildCommand = "echo hi; echo FOO > $out"; + buildCommand = "echo hi-input1; echo FOO > $out"; requiredSystemFeatures = ["foo"]; outputHash = "sha256-FePFYIlMuycIXPZbWi7LGEiMmZSX9FMbaQenWBzm1Sc="; }; @@ -34,7 +34,7 @@ let shell = busybox; name = "build-remote-input-3"; buildCommand = '' - echo hi + echo hi-input3 read x < ${input2} echo $x BAZ > $out ''; diff --git a/tests/build-hook.nix b/tests/build-hook.nix index 3c83e0475..643334caa 100644 --- a/tests/build-hook.nix +++ b/tests/build-hook.nix @@ -15,7 +15,7 @@ let input1 = mkDerivation { shell = busybox; name = "build-remote-input-1"; - buildCommand = "echo hi; echo FOO > $out"; + buildCommand = "echo hi-input1; echo FOO > $out"; requiredSystemFeatures = ["foo"]; }; @@ -30,7 +30,7 @@ let shell = busybox; name = "build-remote-input-3"; buildCommand = '' - echo hi + echo hi-input3 read x < ${input2} echo $x BAZ > $out ''; diff --git a/tests/build-remote.sh b/tests/build-remote.sh index 4ecc788a3..a2c3b9ce7 100644 --- a/tests/build-remote.sh +++ b/tests/build-remote.sh @@ -57,8 +57,7 @@ nix path-info --store $TEST_ROOT/machine3 --all \ if [[ -z "$CONTENT_ADDRESSED" ]]; then for i in input1 input3; do - drv="$(nix-instantiate $file -A passthru.$i --store $TEST_ROOT/machine0 --arg busybox $busybox)" - nix log --store $TEST_ROOT/machine0 "$drv" + nix log --store $TEST_ROOT/machine0 --file "$file" --arg busybox $busybox passthru."$i" | grep hi-$i done fi From 314852a10e3e741e84510f90f5db75f797145f0a Mon Sep 17 00:00:00 2001 From: Nicholas Sielicki Date: Sun, 6 Mar 2022 17:01:14 -0600 Subject: [PATCH 10/31] Point to new github oauth docs url Previous URL was 404'ing. --- src/libfetchers/fetch-settings.hh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libfetchers/fetch-settings.hh b/src/libfetchers/fetch-settings.hh index 58a2aded3..04c9feda0 100644 --- a/src/libfetchers/fetch-settings.hh +++ b/src/libfetchers/fetch-settings.hh @@ -29,7 +29,7 @@ struct FetchSettings : public Config * Github: the token value is the OAUTH-TOKEN string obtained as the Personal Access Token from the Github server (see - https://docs.github.com/en/developers/apps/authorizing-oath-apps). + https://docs.github.com/en/developers/apps/building-oauth-apps/authorizing-oauth-apps). * Gitlab: the token value is either the OAuth2 token or the Personal Access Token (these are different types tokens From 860016bcbf1ea91289ebc5d495eccfe43f22ac62 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= <7226587+thufschmitt@users.noreply.github.com> Date: Mon, 7 Mar 2022 09:15:34 +0100 Subject: [PATCH 11/31] Explain why the log tests are disabled for CA derivations --- tests/build-remote.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/build-remote.sh b/tests/build-remote.sh index a2c3b9ce7..094366872 100644 --- a/tests/build-remote.sh +++ b/tests/build-remote.sh @@ -55,6 +55,7 @@ nix path-info --store $TEST_ROOT/machine3 --all \ | grep builder-build-remote-input-3.sh +# Temporarily disabled because of https://github.com/NixOS/nix/issues/6209 if [[ -z "$CONTENT_ADDRESSED" ]]; then for i in input1 input3; do nix log --store $TEST_ROOT/machine0 --file "$file" --arg busybox $busybox passthru."$i" | grep hi-$i From c0792b1546ceed1c02a02ca1286ead55f79d5798 Mon Sep 17 00:00:00 2001 From: regnat Date: Thu, 3 Mar 2022 10:50:35 +0100 Subject: [PATCH 12/31] Implement a suggestions mechanism Each `Error` class now includes a set of suggestions, and these are printed by the top-level handler. --- src/libcmd/installables.cc | 11 +++- src/libexpr/attr-path.cc | 10 +++- src/libutil/args.cc | 9 ++- src/libutil/error.cc | 7 +++ src/libutil/error.hh | 8 +++ src/libutil/suggestions.cc | 111 +++++++++++++++++++++++++++++++++++++ src/libutil/suggestions.hh | 41 ++++++++++++++ 7 files changed, 191 insertions(+), 6 deletions(-) create mode 100644 src/libutil/suggestions.cc create mode 100644 src/libutil/suggestions.hh diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index 3209456bf..888d863ff 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -610,17 +610,24 @@ std::pair InstallableFlake::toValue(EvalState & state) auto emptyArgs = state.allocBindings(0); + Suggestions suggestions; + for (auto & attrPath : getActualAttrPaths()) { try { auto [v, pos] = findAlongAttrPath(state, attrPath, *emptyArgs, *vOutputs); state.forceValue(*v, pos); return {v, pos}; } catch (AttrPathNotFound & e) { + suggestions += e.info().suggestions; } } - throw Error("flake '%s' does not provide attribute %s", - flakeRef, showAttrPaths(getActualAttrPaths())); + throw Error( + suggestions, + "flake '%s' does not provide attribute %s", + flakeRef, + showAttrPaths(getActualAttrPaths()) + ); } std::vector, std::string>> diff --git a/src/libexpr/attr-path.cc b/src/libexpr/attr-path.cc index eb0e706c7..32deecfae 100644 --- a/src/libexpr/attr-path.cc +++ b/src/libexpr/attr-path.cc @@ -74,8 +74,14 @@ std::pair findAlongAttrPath(EvalState & state, const std::string & throw Error("empty attribute name in selection path '%1%'", attrPath); Bindings::iterator a = v->attrs->find(state.symbols.create(attr)); - if (a == v->attrs->end()) - throw AttrPathNotFound("attribute '%1%' in selection path '%2%' not found", attr, attrPath); + if (a == v->attrs->end()) { + std::set attrNames; + for (auto & attr : *v->attrs) + attrNames.insert(attr.name); + + auto suggestions = Suggestions::bestMatches(attrNames, attr); + throw AttrPathNotFound(suggestions, "attribute '%1%' in selection path '%2%' not found", attr, attrPath); + } v = &*a->value; pos = *a->pos; } diff --git a/src/libutil/args.cc b/src/libutil/args.cc index f970c0e9e..69aa0d094 100644 --- a/src/libutil/args.cc +++ b/src/libutil/args.cc @@ -328,8 +328,13 @@ MultiCommand::MultiCommand(const Commands & commands_) completions->add(name); } auto i = commands.find(s); - if (i == commands.end()) - throw UsageError("'%s' is not a recognised command", s); + if (i == commands.end()) { + std::set commandNames; + for (auto & [name, _] : commands) + commandNames.insert(name); + auto suggestions = Suggestions::bestMatches(commandNames, s); + throw UsageError(suggestions, "'%s' is not a recognised command", s); + } command = {s, i->second()}; command->second->parent = this; }} diff --git a/src/libutil/error.cc b/src/libutil/error.cc index dcd2f82a5..15ae7a759 100644 --- a/src/libutil/error.cc +++ b/src/libutil/error.cc @@ -282,6 +282,13 @@ std::ostream & showErrorInfo(std::ostream & out, const ErrorInfo & einfo, bool s } } + auto suggestions = einfo.suggestions.trim(); + if (! suggestions.suggestions.empty()){ + oss << "Did you mean " << + suggestions.trim().pretty_print() << + "?" << std::endl; + } + // traces if (showTrace && !einfo.traces.empty()) { for (auto iter = einfo.traces.rbegin(); iter != einfo.traces.rend(); ++iter) { diff --git a/src/libutil/error.hh b/src/libutil/error.hh index d55e1d701..600e94888 100644 --- a/src/libutil/error.hh +++ b/src/libutil/error.hh @@ -1,5 +1,6 @@ #pragma once +#include "suggestions.hh" #include "ref.hh" #include "types.hh" #include "fmt.hh" @@ -112,6 +113,8 @@ struct ErrorInfo { std::optional errPos; std::list traces; + Suggestions suggestions; + static std::optional programName; }; @@ -141,6 +144,11 @@ public: : err { .level = lvlError, .msg = hintfmt(fs, args...) } { } + template + BaseError(const Suggestions & sug, const Args & ... args) + : err { .level = lvlError, .msg = hintfmt(args...), .suggestions = sug } + { } + BaseError(hintformat hint) : err { .level = lvlError, .msg = hint } { } diff --git a/src/libutil/suggestions.cc b/src/libutil/suggestions.cc new file mode 100644 index 000000000..96b48416b --- /dev/null +++ b/src/libutil/suggestions.cc @@ -0,0 +1,111 @@ +#include "suggestions.hh" +#include "ansicolor.hh" +#include "util.hh" +#include + +namespace nix { + +/** + * Return `some(distance)` where distance is an integer representing some + * notion of distance between both arguments. + * + * If the distance is too big, return none + */ +int distanceBetween(std::string_view first, std::string_view second) +{ + // Levenshtein distance. + // Implementation borrowed from + // https://en.wikipedia.org/wiki/Levenshtein_distance#Iterative_with_two_matrix_rows + + int m = first.size(); + int n = second.size(); + + auto v0 = std::vector(n+1); + auto v1 = std::vector(n+1); + + for (auto i = 0; i <= n; i++) + v0[i] = i; + + for (auto i = 0; i < m; i++) { + v1[0] = i+1; + + for (auto j = 0; j < n; j++) { + auto deletionCost = v0[j+1] + 1; + auto insertionCost = v1[j] + 1; + auto substitutionCost = first[i] == second[j] ? v0[j] : v0[j] + 1; + v1[j+1] = std::min({deletionCost, insertionCost, substitutionCost}); + } + + std::swap(v0, v1); + } + + return v0[n]; +} + +Suggestions Suggestions::bestMatches ( + std::set allMatches, + std::string query) +{ + std::set res; + for (const auto & possibleMatch : allMatches) { + res.insert(Suggestion { + .distance = distanceBetween(query, possibleMatch), + .suggestion = possibleMatch, + }); + } + return Suggestions { res }; +} + +Suggestions Suggestions::trim(int limit, int maxDistance) const +{ + std::set res; + + int count = 0; + + for (auto & elt : suggestions) { + if (count >= limit || elt.distance >= maxDistance) + break; + count++; + res.insert(elt); + } + + return Suggestions{res}; +} + +std::string Suggestion::pretty_print() const +{ + return ANSI_WARNING + filterANSIEscapes(suggestion) + ANSI_NORMAL; +} + +std::string Suggestions::pretty_print() const +{ + switch (suggestions.size()) { + case 0: + return ""; + case 1: + return suggestions.begin()->pretty_print(); + default: { + std::string res = "one of "; + auto iter = suggestions.begin(); + res += iter->pretty_print(); // Iter can’t be end() because the container isn’t null + iter++; + auto last = suggestions.end(); last--; + for ( ; iter != suggestions.end() ; iter++) { + res += (iter == last) ? " or " : ", "; + res += iter->pretty_print(); + } + return res; + } + } +} + +Suggestions & Suggestions::operator+=(const Suggestions & other) +{ + suggestions.insert( + other.suggestions.begin(), + other.suggestions.end() + ); + return *this; +} + +} diff --git a/src/libutil/suggestions.hh b/src/libutil/suggestions.hh new file mode 100644 index 000000000..3caed487a --- /dev/null +++ b/src/libutil/suggestions.hh @@ -0,0 +1,41 @@ +#pragma once + +#include "comparator.hh" +#include "types.hh" +#include + +namespace nix { + +/** + * A potential suggestion for the cli interface. + */ +class Suggestion { +public: + int distance; // The smaller the better + std::string suggestion; + + std::string pretty_print() const; + + GENERATE_CMP(Suggestion, me->distance, me->suggestion) +}; + +class Suggestions { +public: + std::set suggestions; + + std::string pretty_print() const; + + Suggestions trim( + int limit = 5, + int maxDistance = 2 + ) const; + + static Suggestions bestMatches ( + std::set allMatches, + std::string query + ); + + Suggestions& operator+=(const Suggestions & other); +}; + +} From 2405bbbb5edd204d6031edcd470a2057f4a25782 Mon Sep 17 00:00:00 2001 From: regnat Date: Thu, 3 Mar 2022 13:12:27 +0100 Subject: [PATCH 13/31] Add some tests for the suggestions --- src/libutil/suggestions.cc | 13 +++------- src/libutil/suggestions.hh | 2 ++ src/libutil/tests/suggestions.cc | 43 ++++++++++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 10 deletions(-) create mode 100644 src/libutil/tests/suggestions.cc diff --git a/src/libutil/suggestions.cc b/src/libutil/suggestions.cc index 96b48416b..bcd93aa6b 100644 --- a/src/libutil/suggestions.cc +++ b/src/libutil/suggestions.cc @@ -5,15 +5,8 @@ namespace nix { -/** - * Return `some(distance)` where distance is an integer representing some - * notion of distance between both arguments. - * - * If the distance is too big, return none - */ -int distanceBetween(std::string_view first, std::string_view second) +int levenshteinDistance(std::string_view first, std::string_view second) { - // Levenshtein distance. // Implementation borrowed from // https://en.wikipedia.org/wiki/Levenshtein_distance#Iterative_with_two_matrix_rows @@ -49,7 +42,7 @@ Suggestions Suggestions::bestMatches ( std::set res; for (const auto & possibleMatch : allMatches) { res.insert(Suggestion { - .distance = distanceBetween(query, possibleMatch), + .distance = levenshteinDistance(query, possibleMatch), .suggestion = possibleMatch, }); } @@ -63,7 +56,7 @@ Suggestions Suggestions::trim(int limit, int maxDistance) const int count = 0; for (auto & elt : suggestions) { - if (count >= limit || elt.distance >= maxDistance) + if (count >= limit || elt.distance > maxDistance) break; count++; res.insert(elt); diff --git a/src/libutil/suggestions.hh b/src/libutil/suggestions.hh index 3caed487a..76063a261 100644 --- a/src/libutil/suggestions.hh +++ b/src/libutil/suggestions.hh @@ -6,6 +6,8 @@ namespace nix { +int levenshteinDistance(std::string_view first, std::string_view second); + /** * A potential suggestion for the cli interface. */ diff --git a/src/libutil/tests/suggestions.cc b/src/libutil/tests/suggestions.cc new file mode 100644 index 000000000..279994abc --- /dev/null +++ b/src/libutil/tests/suggestions.cc @@ -0,0 +1,43 @@ +#include "suggestions.hh" +#include + +namespace nix { + + struct LevenshteinDistanceParam { + std::string s1, s2; + int distance; + }; + + class LevenshteinDistanceTest : + public testing::TestWithParam { + }; + + TEST_P(LevenshteinDistanceTest, CorrectlyComputed) { + auto params = GetParam(); + + ASSERT_EQ(levenshteinDistance(params.s1, params.s2), params.distance); + ASSERT_EQ(levenshteinDistance(params.s2, params.s1), params.distance); + } + + INSTANTIATE_TEST_SUITE_P(LevenshteinDistance, LevenshteinDistanceTest, + testing::Values( + LevenshteinDistanceParam{"foo", "foo", 0}, + LevenshteinDistanceParam{"foo", "", 3}, + LevenshteinDistanceParam{"", "", 0}, + LevenshteinDistanceParam{"foo", "fo", 1}, + LevenshteinDistanceParam{"foo", "oo", 1}, + LevenshteinDistanceParam{"foo", "fao", 1}, + LevenshteinDistanceParam{"foo", "abc", 3} + ) + ); + + TEST(Suggestions, Trim) { + auto suggestions = Suggestions::bestMatches({"foooo", "bar", "fo", "gao"}, "foo"); + auto onlyOne = suggestions.trim(1); + ASSERT_EQ(onlyOne.suggestions.size(), 1); + ASSERT_TRUE(onlyOne.suggestions.begin()->suggestion == "fo"); + + auto closest = suggestions.trim(999, 2); + ASSERT_EQ(closest.suggestions.size(), 3); + } +} From 98e361ad4c1a26d4ffe4762a6f33bb9e39321a39 Mon Sep 17 00:00:00 2001 From: regnat Date: Fri, 4 Mar 2022 09:44:00 +0100 Subject: [PATCH 14/31] Also display suggestions for the commands using the eval cache Make `nix build .#nix-armv8l-linux` work for example --- src/libcmd/installables.cc | 19 +++++++++---- src/libexpr/eval-cache.cc | 35 +++++++++++++++++------ src/libexpr/eval-cache.hh | 8 ++++-- src/libutil/suggestions.hh | 58 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 103 insertions(+), 17 deletions(-) diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index 888d863ff..03f3bd409 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -272,9 +272,9 @@ void completeFlakeRefWithFragment( auto attr = root->findAlongAttrPath(attrPath); if (!attr) continue; - for (auto & attr2 : attr->getAttrs()) { + for (auto & attr2 : (*attr)->getAttrs()) { if (hasPrefix(attr2, lastAttr)) { - auto attrPath2 = attr->getAttrPath(attr2); + auto attrPath2 = (*attr)->getAttrPath(attr2); /* Strip the attrpath prefix. */ attrPath2.erase(attrPath2.begin(), attrPath2.begin() + attrPathPrefix.size()); completions->add(flakeRefS + "#" + concatStringsSep(".", attrPath2)); @@ -568,15 +568,22 @@ std::tuple InstallableF auto cache = openEvalCache(*state, lockedFlake); auto root = cache->getRoot(); + Suggestions suggestions; + for (auto & attrPath : getActualAttrPaths()) { debug("trying flake output attribute '%s'", attrPath); - auto attr = root->findAlongAttrPath( + auto attrOrSuggestions = root->findAlongAttrPath( parseAttrPath(*state, attrPath), true ); - if (!attr) continue; + if (!attrOrSuggestions) { + suggestions += attrOrSuggestions.getSuggestions(); + continue; + } + + auto attr = *attrOrSuggestions; if (!attr->isDerivation()) throw Error("flake output attribute '%s' is not a derivation", attrPath); @@ -591,7 +598,7 @@ std::tuple InstallableF return {attrPath, lockedFlake->flake.lockedRef, std::move(drvInfo)}; } - throw Error("flake '%s' does not provide attribute %s", + throw Error(suggestions, "flake '%s' does not provide attribute %s", flakeRef, showAttrPaths(getActualAttrPaths())); } @@ -642,7 +649,7 @@ InstallableFlake::getCursors(EvalState & state) for (auto & attrPath : getActualAttrPaths()) { auto attr = root->findAlongAttrPath(parseAttrPath(state, attrPath)); - if (attr) res.push_back({attr, attrPath}); + if (attr) res.push_back({*attr, attrPath}); } return res; diff --git a/src/libexpr/eval-cache.cc b/src/libexpr/eval-cache.cc index 00d0749f9..188223957 100644 --- a/src/libexpr/eval-cache.cc +++ b/src/libexpr/eval-cache.cc @@ -406,6 +406,16 @@ Value & AttrCursor::forceValue() return v; } +Suggestions AttrCursor::getSuggestionsForAttr(Symbol name) +{ + auto attrNames = getAttrs(); + std::set strAttrNames; + for (auto & name : attrNames) + strAttrNames.insert(std::string(name)); + + return Suggestions::bestMatches(strAttrNames, name); +} + std::shared_ptr AttrCursor::maybeGetAttr(Symbol name, bool forceErrors) { if (root->db) { @@ -446,6 +456,11 @@ std::shared_ptr AttrCursor::maybeGetAttr(Symbol name, bool forceErro return nullptr; //throw TypeError("'%s' is not an attribute set", getAttrPathStr()); + for (auto & attr : *v.attrs) { + if (root->db) + root->db->setPlaceholder({cachedValue->first, attr.name}); + } + auto attr = v.attrs->get(name); if (!attr) { @@ -464,7 +479,7 @@ std::shared_ptr AttrCursor::maybeGetAttr(Symbol name, bool forceErro cachedValue2 = {root->db->setPlaceholder({cachedValue->first, name}), placeholder_t()}; } - return std::make_shared( + return make_ref( root, std::make_pair(shared_from_this(), name), attr->value, std::move(cachedValue2)); } @@ -473,27 +488,31 @@ std::shared_ptr AttrCursor::maybeGetAttr(std::string_view name) return maybeGetAttr(root->state.symbols.create(name)); } -std::shared_ptr AttrCursor::getAttr(Symbol name, bool forceErrors) +ref AttrCursor::getAttr(Symbol name, bool forceErrors) { auto p = maybeGetAttr(name, forceErrors); if (!p) throw Error("attribute '%s' does not exist", getAttrPathStr(name)); - return p; + return ref(p); } -std::shared_ptr AttrCursor::getAttr(std::string_view name) +ref AttrCursor::getAttr(std::string_view name) { return getAttr(root->state.symbols.create(name)); } -std::shared_ptr AttrCursor::findAlongAttrPath(const std::vector & attrPath, bool force) +OrSuggestions> AttrCursor::findAlongAttrPath(const std::vector & attrPath, bool force) { auto res = shared_from_this(); for (auto & attr : attrPath) { - res = res->maybeGetAttr(attr, force); - if (!res) return {}; + auto child = res->maybeGetAttr(attr, force); + if (!child) { + auto suggestions = res->getSuggestionsForAttr(attr); + return OrSuggestions>::failed(suggestions); + } + res = child; } - return res; + return ref(res); } std::string AttrCursor::getString() diff --git a/src/libexpr/eval-cache.hh b/src/libexpr/eval-cache.hh index 43b34ebcb..40f1d4ffc 100644 --- a/src/libexpr/eval-cache.hh +++ b/src/libexpr/eval-cache.hh @@ -94,15 +94,17 @@ public: std::string getAttrPathStr(Symbol name) const; + Suggestions getSuggestionsForAttr(Symbol name); + std::shared_ptr maybeGetAttr(Symbol name, bool forceErrors = false); std::shared_ptr maybeGetAttr(std::string_view name); - std::shared_ptr getAttr(Symbol name, bool forceErrors = false); + ref getAttr(Symbol name, bool forceErrors = false); - std::shared_ptr getAttr(std::string_view name); + ref getAttr(std::string_view name); - std::shared_ptr findAlongAttrPath(const std::vector & attrPath, bool force = false); + OrSuggestions> findAlongAttrPath(const std::vector & attrPath, bool force = false); std::string getString(); diff --git a/src/libutil/suggestions.hh b/src/libutil/suggestions.hh index 76063a261..63426259d 100644 --- a/src/libutil/suggestions.hh +++ b/src/libutil/suggestions.hh @@ -40,4 +40,62 @@ public: Suggestions& operator+=(const Suggestions & other); }; +// Either a value of type `T`, or some suggestions +template +class OrSuggestions { +public: + using Raw = std::variant; + + Raw raw; + + T* operator ->() + { + return &**this; + } + + T& operator *() + { + if (auto elt = std::get_if(&raw)) + return *elt; + throw Error("Invalid access to a failed value"); + } + + operator bool() const noexcept + { + return std::holds_alternative(raw); + } + + OrSuggestions(T t) + : raw(t) + { + } + + OrSuggestions() + : raw(Suggestions{}) + { + } + + static OrSuggestions failed(const Suggestions & s) + { + auto res = OrSuggestions(); + res.raw = s; + return res; + } + + static OrSuggestions failed() + { + return OrSuggestions::failed(Suggestions{}); + } + + const Suggestions & get_suggestions() + { + static Suggestions noSuggestions; + if (const auto & suggestions = std::get_if(&raw)) + return *suggestions; + else + return noSuggestions; + } + +}; + } From 91635206c0d33f9862b788487aa8a056a791af94 Mon Sep 17 00:00:00 2001 From: regnat Date: Fri, 4 Mar 2022 10:29:48 +0100 Subject: [PATCH 15/31] Add some end-to-end tests for the suggestions --- tests/local.mk | 5 +++-- tests/suggestions.sh | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 2 deletions(-) create mode 100644 tests/suggestions.sh diff --git a/tests/local.mk b/tests/local.mk index c3a6aa1fc..8032fc38a 100644 --- a/tests/local.mk +++ b/tests/local.mk @@ -92,8 +92,9 @@ nix_tests = \ bash-profile.sh \ pass-as-file.sh \ describe-stores.sh \ - store-ping.sh \ - nix-profile.sh + nix-profile.sh \ + suggestions.sh \ + store-ping.sh ifeq ($(HAVE_LIBCPUID), 1) nix_tests += compute-levels.sh diff --git a/tests/suggestions.sh b/tests/suggestions.sh new file mode 100644 index 000000000..16a5a7004 --- /dev/null +++ b/tests/suggestions.sh @@ -0,0 +1,36 @@ +source common.sh + +clearStore + +cd "$TEST_HOME" + +cat < flake.nix +{ + outputs = a: { + packages.$system = { + foo = 1; + fo1 = 1; + fo2 = 1; + fooo = 1; + foooo = 1; + fooooo = 1; + fooooo1 = 1; + fooooo2 = 1; + fooooo3 = 1; + fooooo4 = 1; + fooooo5 = 1; + fooooo6 = 1; + }; + }; +} +EOF + +# Probable typo in the requested attribute path. Suggest some close possibilities +NIX_BUILD_STDERR_WITH_SUGGESTIONS=$(! nix build .\#fob 2>&1 1>/dev/null) +[[ "$NIX_BUILD_STDERR_WITH_SUGGESTIONS" =~ "Did you mean one of fo1, fo2, foo or fooo?" ]] || \ + fail "The nix build stderr should suggest the three closest possiblities" + +# None of the possible attributes is close to `bar`, so shouldn’t suggest anything +NIX_BUILD_STDERR_WITH_NO_CLOSE_SUGGESTION=$(! nix build .\#bar 2>&1 1>/dev/null) +[[ ! "$NIX_BUILD_STDERR_WITH_NO_CLOSE_SUGGESTION" =~ "Did you mean" ]] || \ + fail "The nix build stderr shouldn’t suggest anything if there’s nothing relevant to suggest" From b44cebd1fd68f89bbc2317e2b7978042309e17bb Mon Sep 17 00:00:00 2001 From: regnat Date: Fri, 4 Mar 2022 10:47:58 +0100 Subject: [PATCH 16/31] Add a release-notes entry for the cli suggestions --- doc/manual/src/release-notes/rl-next.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/doc/manual/src/release-notes/rl-next.md b/doc/manual/src/release-notes/rl-next.md index 7dd8387d8..1a74dec95 100644 --- a/doc/manual/src/release-notes/rl-next.md +++ b/doc/manual/src/release-notes/rl-next.md @@ -26,3 +26,6 @@ * Templates can now define a `welcomeText` attribute, which is printed out by `nix flake {init,new} --template