From 25300c0ecdedcd8720b996b41e78dfe1037bfcff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Baylac=20Jacqu=C3=A9?= Date: Wed, 1 Mar 2023 20:01:36 +0100 Subject: [PATCH 1/4] Treat empty env var paths as unset We make sure the env var paths are actually set (ie. not "") before sending them to the canonicalization function. If we forget to do so, the user will end up facing a puzzled failed assertion internal error. We issue a non-failing warning as a stop-gap measure. We could want to revisit this to issue a detailed failing error message in the future. --- src/libstore/build/local-derivation-goal.cc | 2 +- src/libstore/globals.cc | 14 +++++++------- src/libutil/util.cc | 12 ++++++++++++ src/libutil/util.hh | 4 ++++ 4 files changed, 24 insertions(+), 8 deletions(-) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index a961d8eed..1c0860993 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -2063,7 +2063,7 @@ void LocalDerivationGoal::runChild() /* The tmpDir in scope points at the temporary build directory for our derivation. Some packages try different mechanisms to find temporary directories, so we want to open up a broader place for them to dump their files, if needed. */ - Path globalTmpDir = canonPath(getEnv("TMPDIR").value_or("/tmp"), true); + Path globalTmpDir = canonPath(getEnvNonEmpty("TMPDIR").value_or("/tmp"), true); /* They don't like trailing slashes on subpath directives */ if (globalTmpDir.back() == '/') globalTmpDir.pop_back(); diff --git a/src/libstore/globals.cc b/src/libstore/globals.cc index 8e33a3dec..fae79c1a0 100644 --- a/src/libstore/globals.cc +++ b/src/libstore/globals.cc @@ -30,15 +30,15 @@ static GlobalConfig::Register rSettings(&settings); Settings::Settings() : nixPrefix(NIX_PREFIX) - , nixStore(canonPath(getEnv("NIX_STORE_DIR").value_or(getEnv("NIX_STORE").value_or(NIX_STORE_DIR)))) - , nixDataDir(canonPath(getEnv("NIX_DATA_DIR").value_or(NIX_DATA_DIR))) - , nixLogDir(canonPath(getEnv("NIX_LOG_DIR").value_or(NIX_LOG_DIR))) - , nixStateDir(canonPath(getEnv("NIX_STATE_DIR").value_or(NIX_STATE_DIR))) - , nixConfDir(canonPath(getEnv("NIX_CONF_DIR").value_or(NIX_CONF_DIR))) + , nixStore(canonPath(getEnvNonEmpty("NIX_STORE_DIR").value_or(getEnvNonEmpty("NIX_STORE").value_or(NIX_STORE_DIR)))) + , nixDataDir(canonPath(getEnvNonEmpty("NIX_DATA_DIR").value_or(NIX_DATA_DIR))) + , nixLogDir(canonPath(getEnvNonEmpty("NIX_LOG_DIR").value_or(NIX_LOG_DIR))) + , nixStateDir(canonPath(getEnvNonEmpty("NIX_STATE_DIR").value_or(NIX_STATE_DIR))) + , nixConfDir(canonPath(getEnvNonEmpty("NIX_CONF_DIR").value_or(NIX_CONF_DIR))) , nixUserConfFiles(getUserConfigFiles()) - , nixBinDir(canonPath(getEnv("NIX_BIN_DIR").value_or(NIX_BIN_DIR))) + , nixBinDir(canonPath(getEnvNonEmpty("NIX_BIN_DIR").value_or(NIX_BIN_DIR))) , nixManDir(canonPath(NIX_MAN_DIR)) - , nixDaemonSocketFile(canonPath(getEnv("NIX_DAEMON_SOCKET_PATH").value_or(nixStateDir + DEFAULT_SOCKET_PATH))) + , nixDaemonSocketFile(canonPath(getEnvNonEmpty("NIX_DAEMON_SOCKET_PATH").value_or(nixStateDir + DEFAULT_SOCKET_PATH))) { buildUsersGroup = getuid() == 0 ? "nixbld" : ""; lockCPU = getEnv("NIX_AFFINITY_HACK") == "1"; diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 885bae69c..5377f093b 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -54,6 +54,18 @@ std::optional getEnv(const std::string & key) return std::string(value); } +std::optional getEnvNonEmpty(const std::string & key) { + auto value = getEnv(key); + if (value == "") { + // TODO: determine whether this should be a warning or an error. + logWarning({ + .msg = hintfmt("ignoring the '%1%' env variable, its value has been set to \"\"", key) + }); + return std::nullopt; + } else { + return value; + } +} std::map getEnv() { diff --git a/src/libutil/util.hh b/src/libutil/util.hh index b5625ecef..3293c758f 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -39,6 +39,10 @@ extern const std::string nativeSystem; /* Return an environment variable. */ std::optional getEnv(const std::string & key); +/* Return a non empty environment variable. Returns nullopt if the env +variable is set to "" */ +std::optional getEnvNonEmpty(const std::string & key); + /* Get the entire environment. */ std::map getEnv(); From 72e1e230517b1e774d2db97cf9d4750e31ebcaa3 Mon Sep 17 00:00:00 2001 From: Jonas Chevalier Date: Thu, 2 Mar 2023 16:17:20 +0100 Subject: [PATCH 2/4] Update src/libutil/util.cc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Théophane Hufschmitt <7226587+thufschmitt@users.noreply.github.com> --- src/libutil/util.cc | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 5377f093b..c8965baa8 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -58,9 +58,7 @@ std::optional getEnvNonEmpty(const std::string & key) { auto value = getEnv(key); if (value == "") { // TODO: determine whether this should be a warning or an error. - logWarning({ - .msg = hintfmt("ignoring the '%1%' env variable, its value has been set to \"\"", key) - }); + warn("ignoring the '%1%' env variable, its value has been set to \"\"", key); return std::nullopt; } else { return value; From b96d9c1687066045ce1a045798b64e914cc51fd4 Mon Sep 17 00:00:00 2001 From: zimbatm Date: Fri, 3 Mar 2023 11:32:04 +0100 Subject: [PATCH 3/4] fixup: remove warning entirely fixes https://github.com/NixOS/nix/pull/7918/files/72e1e230517b1e774d2db97cf9d4750e31ebcaa3#r1124211067 --- src/libutil/util.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/libutil/util.cc b/src/libutil/util.cc index c8965baa8..b70723634 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -57,8 +57,6 @@ std::optional getEnv(const std::string & key) std::optional getEnvNonEmpty(const std::string & key) { auto value = getEnv(key); if (value == "") { - // TODO: determine whether this should be a warning or an error. - warn("ignoring the '%1%' env variable, its value has been set to \"\"", key); return std::nullopt; } else { return value; From dc8820c71f841df49568099bf3889c7cfb2d92a9 Mon Sep 17 00:00:00 2001 From: zimbatm Date: Fri, 3 Mar 2023 11:34:36 +0100 Subject: [PATCH 4/4] fixup: use same style as getEnv --- src/libutil/util.cc | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/libutil/util.cc b/src/libutil/util.cc index b70723634..c0b8f77b0 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -56,11 +56,8 @@ std::optional getEnv(const std::string & key) std::optional getEnvNonEmpty(const std::string & key) { auto value = getEnv(key); - if (value == "") { - return std::nullopt; - } else { - return value; - } + if (value == "") return {}; + return value; } std::map getEnv()