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 a961d8eedee..1c0860993a1 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 8e33a3deca6..fae79c1a098 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 885bae69c9b..5377f093b18 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 b5625ecefcf..3293c758fdb 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 5377f093b18..c8965baa8bd 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 c8965baa8bd..b7072363443 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 b7072363443..c0b8f77b08c 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()