From 1d3ddb21faf95ee72af3126bc475f706839a8669 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 18 Aug 2025 18:08:35 -0400 Subject: [PATCH] Further consolidate environment variable processing outside `DerivationBuilder` Now, `DerivationBuilder` only concerns itself with `finalEnv` and `extraFiles`, in straightforward unconditional code. All the fancy desugaring logic is consolidated in `DerivationBuildingGoal`. We should better share the pulled-out logic with `nix-shell`/`nix develop`, which would fill in some missing features, arguably fixing bugs. --- .../build/derivation-building-goal.cc | 30 ++++++++++++++--- .../nix/store/build/derivation-builder.hh | 8 ++--- src/libstore/unix/build/derivation-builder.cc | 33 +++++-------------- 3 files changed, 38 insertions(+), 33 deletions(-) diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index dae13c728..965ffa525 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -677,7 +677,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild() auto * localStoreP = dynamic_cast(&worker.store); assert(localStoreP); - decltype(DerivationBuilderParams::extraEnv) extraEnv; + decltype(DerivationBuilderParams::finalEnv) finalEnv; decltype(DerivationBuilderParams::extraFiles) extraFiles; try { @@ -685,19 +685,41 @@ Goal::Co DerivationBuildingGoal::tryToBuild() auto json = drv->structuredAttrs->prepareStructuredAttrs( worker.store, *drvOptions, inputPaths, drv->outputs); - extraEnv.insert_or_assign( + finalEnv.insert_or_assign( "NIX_ATTRS_SH_FILE", DerivationBuilderParams::EnvEntry{ .nameOfPassAsFile = ".attrs.sh", .value = StructuredAttrs::writeShell(json), }); - extraEnv.insert_or_assign( + finalEnv.insert_or_assign( "NIX_ATTRS_JSON_FILE", DerivationBuilderParams::EnvEntry{ .nameOfPassAsFile = ".attrs.json", .value = json.dump(), }); } else { + /* In non-structured mode, set all bindings either directory in the + environment or via a file, as specified by + `DerivationOptions::passAsFile`. */ + for (auto & [envName, envValue] : drv->env) { + if (drvOptions->passAsFile.find(envName) == drvOptions->passAsFile.end()) { + finalEnv.insert_or_assign( + envName, + DerivationBuilderParams::EnvEntry{ + .nameOfPassAsFile = std::nullopt, + .value = envValue, + }); + } else { + auto hash = hashString(HashAlgorithm::SHA256, envName); + finalEnv.insert_or_assign( + envName + "Path", + DerivationBuilderParams::EnvEntry{ + .nameOfPassAsFile = ".attr-" + hash.to_string(HashFormat::Nix32, false), + .value = envValue, + }); + } + } + /* Handle exportReferencesGraph(), if set. */ for (auto & [fileName, storePaths] : drvOptions->getParsedExportReferencesGraph(worker.store)) { /* Write closure info to . */ @@ -726,7 +748,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild() *drvOptions, inputPaths, initialOutputs, - std::move(extraEnv), + std::move(finalEnv), std::move(extraFiles), }); } diff --git a/src/libstore/include/nix/store/build/derivation-builder.hh b/src/libstore/include/nix/store/build/derivation-builder.hh index da8f74a09..301283cdc 100644 --- a/src/libstore/include/nix/store/build/derivation-builder.hh +++ b/src/libstore/include/nix/store/build/derivation-builder.hh @@ -74,13 +74,13 @@ struct DerivationBuilderParams }; /** - * Extra environment variables to additionally set, possibly + * The final environment variables to additionally set, possibly * indirectly via a file. * * This is used by the caller to desugar the "structured attrs" * mechanism, so `DerivationBuilder` doesn't need to know about it. */ - std::map> extraEnv; + std::map> finalEnv; /** * Inserted in the temp dir, but no file names placed in env, unlike @@ -96,7 +96,7 @@ struct DerivationBuilderParams const DerivationOptions & drvOptions, const StorePathSet & inputPaths, std::map & initialOutputs, - std::map> extraEnv, + std::map> finalEnv, StringMap extraFiles) : drvPath{drvPath} , buildResult{buildResult} @@ -105,7 +105,7 @@ struct DerivationBuilderParams , inputPaths{inputPaths} , initialOutputs{initialOutputs} , buildMode{buildMode} - , extraEnv{std::move(extraEnv)} + , finalEnv{std::move(finalEnv)} , extraFiles{std::move(extraFiles)} { } diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index 2c735a7c6..62af9cd85 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -1016,37 +1016,20 @@ void DerivationBuilderImpl::initEnv() /* The maximum number of cores to utilize for parallel building. */ env["NIX_BUILD_CORES"] = fmt("%d", settings.buildCores ? settings.buildCores : settings.getDefaultCores()); - auto writeEnv = [&](const std::string & envVarName, const std::string & fileName, const std::string & value) { - writeBuilderFile(fileName, rewriteStrings(value, inputRewrites)); - env[envVarName] = tmpDirInSandbox() + "/" + fileName; - }; - - /* In non-structured mode, set all bindings either directory in the - environment or via a file, as specified by - `DerivationOptions::passAsFile`. */ - if (!drv.structuredAttrs) { - for (auto & i : drv.env) { - if (drvOptions.passAsFile.find(i.first) == drvOptions.passAsFile.end()) { - env[i.first] = i.second; - } else { - auto hash = hashString(HashAlgorithm::SHA256, i.first); - std::string fn = ".attr-" + hash.to_string(HashFormat::Nix32, false); - writeEnv(i.first + "Path", fn, i.second); - } - } - } - - /* Do this with or without structured attrs --- actually, this is - used to desugar structured attrs. */ - for (const auto & [name, info] : extraEnv) { + /* Write the final environment. Note that this is intentionally + *not* `drv.env`, because we've desugared things like like + "passAFile", "expandReferencesGraph", structured attrs, etc. */ + for (const auto & [name, info] : finalEnv) { if (info.nameOfPassAsFile) { - writeEnv(name, *info.nameOfPassAsFile, info.value); + auto & fileName = *info.nameOfPassAsFile; + writeBuilderFile(fileName, rewriteStrings(info.value, inputRewrites)); + env[name] = tmpDirInSandbox() + "/" + fileName; } else { env[name] = info.value; } } - /* Add extra files, analogous to `extraEnv` */ + /* Add extra files, similar to `finalEnv` */ for (const auto & [fileName, value] : extraFiles) { writeBuilderFile(fileName, value); }