From 8a5d1c45d2636b781c05884c09d1cb584eac25cd Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Mon, 17 Feb 2025 14:38:46 -0800 Subject: [PATCH 1/3] daemon: complain much louder about unknown std::exception instances falling out This made debugging https://git.lix.systems/lix-project/lix/issues/681 a pain in the neck. This is partially a fix, in a certain sense, for https://git.lix.systems/lix-project/lix/issues/379, but that one also addresses expected exceptions from the daemon. Another instance of this error generation site being shit recently: https://git.lix.systems/lix-project/lix/issues/638 I don't know how we should improve that particular site but we definitely should complain about uncaught std exceptions with type ids. Change-Id: I68798300448ee9ebae65c6469ba69e3f933b2895 --- lix/libstore/daemon.cc | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/lix/libstore/daemon.cc b/lix/libstore/daemon.cc index 05037c921..a89d943a9 100644 --- a/lix/libstore/daemon.cc +++ b/lix/libstore/daemon.cc @@ -15,6 +15,7 @@ #include "lix/libutil/strings.hh" #include "lix/libutil/args.hh" +#include #include namespace nix::daemon { @@ -1100,9 +1101,15 @@ void processConnection( to.flush(); return; } catch (std::exception & e) { - auto ex = Error(e.what()); + auto ex = Error( + "Unexpected exception on the Lix daemon; this is a bug in Lix.\nWe would appreciate a report of the circumstances it happened in at https://git.lix.systems/lix-project/lix.\n%s: %s", + Uncolored(boost::core::demangle(typeid(e).name())), + e.what() + ); tunnelLogger->stopWork(&ex); to.flush(); + // Crash for good measure, so something winds up in system logs and a core dump is generated as well. + std::terminate(); return; } } From 24a6759940691b87e544dea0a05bc6df34d4c340 Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Mon, 17 Feb 2025 14:59:11 -0800 Subject: [PATCH 2/3] libstore/build: say what was failing when pre/post build hooks fail Fixes: https://git.lix.systems/lix-project/lix/issues/638 Change-Id: I4baa5721e79b425c37b9e7057ba4989003bced82 --- lix/libstore/build/derivation-goal.cc | 16 ++++++++++++++-- lix/libstore/build/local-derivation-goal.cc | 17 ++++++++++++++--- 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/lix/libstore/build/derivation-goal.cc b/lix/libstore/build/derivation-goal.cc index e1446f58e..5a442aed3 100644 --- a/lix/libstore/build/derivation-goal.cc +++ b/lix/libstore/build/derivation-goal.cc @@ -947,7 +947,8 @@ void runPostBuildHook( PushActivity pact(act.id); std::map hookEnvironment = getEnv(); - hookEnvironment.emplace("DRV_PATH", store.printStorePath(drvPath)); + auto drvPathPretty = store.printStorePath(drvPath); + hookEnvironment.emplace("DRV_PATH", drvPathPretty); hookEnvironment.emplace("OUT_PATHS", chomp(concatStringsSep(" ", store.printStorePathSet(outputPaths)))); hookEnvironment.emplace("NIX_CONFIG", globalConfig.toKeyValue()); @@ -987,7 +988,18 @@ void runPostBuildHook( .captureStdout = true, .mergeStderrToStdout = true, }); - Finally const _wait([&] { proc.wait(); }); + Finally const _wait([&] { + try { + proc.wait(); + } catch (nix::Error & e) { + e.addTrace(nullptr, + "while running the post-build-hook %s for derivation %s", + settings.postBuildHook, + drvPathPretty + ); + throw; + } + }); // FIXME just process the data, without a wrapper sink class proc.getStdout()->drainInto(sink); diff --git a/lix/libstore/build/local-derivation-goal.cc b/lix/libstore/build/local-derivation-goal.cc index ff11615df..0e4932949 100644 --- a/lix/libstore/build/local-derivation-goal.cc +++ b/lix/libstore/build/local-derivation-goal.cc @@ -665,14 +665,25 @@ try { if (useChroot && settings.preBuildHook != "" && dynamic_cast(drv.get())) { printMsg(lvlChatty, "executing pre-build hook '%1%'", settings.preBuildHook); - auto args = useChroot ? Strings({worker.store.printStorePath(drvPath), chrootRootDir}) : - Strings({ worker.store.printStorePath(drvPath) }); + auto drvPathPretty = worker.store.printStorePath(drvPath); + auto args = useChroot ? Strings({ drvPathPretty, chrootRootDir}) : + Strings({ drvPathPretty }); enum BuildHookState { stBegin, stExtraChrootDirs }; auto state = stBegin; - auto lines = runProgram(settings.preBuildHook, false, args); + std::string lines; + try { + runProgram(settings.preBuildHook, false, args); + } catch (nix::Error & e) { + e.addTrace(nullptr, + "while running pre-build-hook %s for derivation %s", + settings.preBuildHook, + drvPathPretty + ); + throw; + } auto lastPos = std::string::size_type{0}; for (auto nlPos = lines.find('\n'); nlPos != std::string::npos; nlPos = lines.find('\n', lastPos)) From c9b2e8b1d908e5208f8b59ca8853836968f88d99 Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Mon, 17 Feb 2025 15:54:26 -0800 Subject: [PATCH 3/3] fix: pre-build-hook did not run when useChroot == false It turns out there is actually some history here. Eight years ago, in 7f5b750b401e98e9e2a346552aba5bd2e0a9203f, the pre-build-hook condition got changed to check that useChroot is true (which we will put down as "eelco making a mistake" because it makes no sense) and that this->drv was a Derivation (as opposed to a BasicDerivation as would be the case if the derivation was not present on disk). The intent of this change was that pre-build-hook would not run when the derivation doesn't exist, so that some hydras would not explode. However, this broke later when both cases became Derivation such that AFAICT it will always run it if useChroot is true, which is absolutely not the intended behaviour, but it is a reasonable interpretation as well, just inconvenient for certain pre-build-hook usages. So, I think the safest and most compatible behaviour is to run the pre-build-hook but not guarantee that the derivation actually exists on disk, since it already didn't run in many cases. Maybe the more correct way is to actually write out the derivation if we are running a pre-build-hook, but post-build-hook has exactly the same situation and it's expected, so idk. Fixes: https://git.lix.systems/lix-project/lix/issues/674 Change-Id: I75b828ae2a07ab373f44083507eb243459bd081d --- lix/libstore/build/local-derivation-goal.cc | 12 +++++++- lix/libstore/settings/pre-build-hook.md | 2 ++ tests/functional/meson.build | 1 + tests/functional/pre-hook.sh | 31 +++++++++++++++++++++ 4 files changed, 45 insertions(+), 1 deletion(-) create mode 100644 tests/functional/pre-hook.sh diff --git a/lix/libstore/build/local-derivation-goal.cc b/lix/libstore/build/local-derivation-goal.cc index 0e4932949..5e35ebdb5 100644 --- a/lix/libstore/build/local-derivation-goal.cc +++ b/lix/libstore/build/local-derivation-goal.cc @@ -663,7 +663,17 @@ try { if (needsHashRewrite() && pathExists(homeDir)) throw Error("home directory '%1%' exists; please remove it to assure purity of builds without sandboxing", homeDir); - if (useChroot && settings.preBuildHook != "" && dynamic_cast(drv.get())) { + // Note that the derivation may or may not exist when running the + // pre-build-hook. In the past this was *supposed* to not run the + // hook in such cases, but at some intermediate point (since this->drv + // became always an instance of Derivation rather than BasicDerivation), it + // started being run in all cases on systems using the sandbox. + // https://git.lix.systems/lix-project/lix/commit/7f5b750b401e98e9e2a346552aba5bd2e0a9203f + // + // As such, we just run it every time. It might be reasonable (and more + // helpful behaviour for users) in the future to write out the derivation + // to disk if pre-build-hook is in use. + if (settings.preBuildHook != "") { printMsg(lvlChatty, "executing pre-build hook '%1%'", settings.preBuildHook); auto drvPathPretty = worker.store.printStorePath(drvPath); auto args = useChroot ? Strings({ drvPathPretty, chrootRootDir}) : diff --git a/lix/libstore/settings/pre-build-hook.md b/lix/libstore/settings/pre-build-hook.md index c3f1a0482..0e3be6059 100644 --- a/lix/libstore/settings/pre-build-hook.md +++ b/lix/libstore/settings/pre-build-hook.md @@ -9,6 +9,8 @@ settings for this system. This is used for settings that can't be captured by the derivation model itself and are too variable between different versions of the same system to be hard-coded into nix. +At the time of running the hook, the derivation may or may not exist on disk (and, e.g. won't exist in the case of many remote builds). + The hook is passed the derivation path and, if sandboxes are enabled, the sandbox directory. It can then modify the sandbox and send a series of commands to modify various settings to stdout. The diff --git a/tests/functional/meson.build b/tests/functional/meson.build index 2d905d79f..187841959 100644 --- a/tests/functional/meson.build +++ b/tests/functional/meson.build @@ -163,6 +163,7 @@ functional_tests_scripts = [ 'compression-levels.sh', 'nix-copy-ssh.sh', 'nix-copy-ssh-ng.sh', + 'pre-hook.sh', 'post-hook.sh', 'function-trace.sh', 'flakes/config.sh', diff --git a/tests/functional/pre-hook.sh b/tests/functional/pre-hook.sh new file mode 100644 index 000000000..82e2dc456 --- /dev/null +++ b/tests/functional/pre-hook.sh @@ -0,0 +1,31 @@ +source common.sh + +clearStore + +rm -f "$TEST_ROOT/result" + +cat > "$TEST_ROOT/pre-hook.sh" <<-'EOF' +#!/bin/sh + +# log the drvs that have been pre-hooked +echo "$1" >> "$(dirname "$0")"/drvs.txt +# sandbox: we have a sandbox path as second arg +if [[ $# == 2 ]]; then + # FIXME: verify this is actually present inside the derivation builder + # where sandbox is available. + echo extra-sandbox-paths + echo /foo=/bin/sh + echo +else + echo +fi +EOF + +chmod +x "$TEST_ROOT/pre-hook.sh" +drvPath="$(nix eval --raw -f dependencies.nix drvPath)" +nix-store -r "$drvPath" --pre-build-hook "$TEST_ROOT/pre-hook.sh" + +# We expect that the pre build hook got called on all the derivations in the closure we built +numDrvs="$(nix-store --query --requisites "$drvPath" | grep -c '\.drv$')" +[[ "$numDrvs" -gt 1 ]] +[[ "$numDrvs" == "$(< "$TEST_ROOT/drvs.txt" wc -l)" ]]