From 53c31c8b2956c1510026bb90132b817ae5b86217 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 27 Aug 2025 15:52:51 -0400 Subject: [PATCH] Factor out a new `DesugaredEnv` from `DerivationBuildingGoal` Now we have better separation of the core logic --- an integral part of the store layer spec even --- from the goal mechanism and other minutiae. Co-authored-by: Jeremy Kolb --- .../build/derivation-building-goal.cc | 56 +------------ src/libstore/build/derivation-check.hh | 3 + src/libstore/build/derivation-env-desugar.cc | 59 +++++++++++++ .../nix/store/build/derivation-builder.hh | 30 +------ .../nix/store/build/derivation-env-desugar.hh | 83 +++++++++++++++++++ src/libstore/include/nix/store/meson.build | 1 + src/libstore/meson.build | 1 + src/libstore/unix/build/derivation-builder.cc | 15 ++-- 8 files changed, 158 insertions(+), 90 deletions(-) create mode 100644 src/libstore/build/derivation-env-desugar.cc create mode 100644 src/libstore/include/nix/store/build/derivation-env-desugar.hh diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index c290852fc..3d6595012 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -1,4 +1,5 @@ #include "nix/store/build/derivation-building-goal.hh" +#include "nix/store/build/derivation-env-desugar.hh" #include "nix/store/build/derivation-trampoline-goal.hh" #ifndef _WIN32 // TODO enable build hook on Windows # include "nix/store/build/hook-instance.hh" @@ -681,8 +682,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild() assert(localStoreP); decltype(DerivationBuilderParams::defaultPathsInChroot) defaultPathsInChroot = settings.sandboxPaths.get(); - decltype(DerivationBuilderParams::finalEnv) finalEnv; - decltype(DerivationBuilderParams::extraFiles) extraFiles; + DesugaredEnv desugaredEnv; /* Add the closure of store paths to the chroot. */ StorePathSet closure; @@ -701,54 +701,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild() } try { - if (drv->structuredAttrs) { - auto json = drv->structuredAttrs->prepareStructuredAttrs( - worker.store, *drvOptions, inputPaths, drv->outputs); - - finalEnv.insert_or_assign( - "NIX_ATTRS_SH_FILE", - DerivationBuilderParams::EnvEntry{ - .nameOfPassAsFile = ".attrs.sh", - .value = StructuredAttrs::writeShell(json), - }); - 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 . */ - extraFiles.insert_or_assign( - fileName, - worker.store.makeValidityRegistration( - worker.store.exportReferences(storePaths, inputPaths), false, false)); - } - } + desugaredEnv = DesugaredEnv::create(worker.store, *drv, *drvOptions, inputPaths); } catch (BuildError & e) { outputLocks.unlock(); worker.permanentFailure = true; @@ -770,8 +723,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild() .buildMode = buildMode, .defaultPathsInChroot = std::move(defaultPathsInChroot), .systemFeatures = worker.store.config.systemFeatures.get(), - .finalEnv = std::move(finalEnv), - .extraFiles = std::move(extraFiles), + .desugaredEnv = std::move(desugaredEnv), }); } diff --git a/src/libstore/build/derivation-check.hh b/src/libstore/build/derivation-check.hh index 249e176c5..25310bd83 100644 --- a/src/libstore/build/derivation-check.hh +++ b/src/libstore/build/derivation-check.hh @@ -1,3 +1,6 @@ +#pragma once +///@file + #include "nix/store/derivations.hh" #include "nix/store/derivation-options.hh" #include "nix/store/path-info.hh" diff --git a/src/libstore/build/derivation-env-desugar.cc b/src/libstore/build/derivation-env-desugar.cc new file mode 100644 index 000000000..d6e002d91 --- /dev/null +++ b/src/libstore/build/derivation-env-desugar.cc @@ -0,0 +1,59 @@ +#include "nix/store/build/derivation-env-desugar.hh" +#include "nix/store/store-api.hh" +#include "nix/store/derivations.hh" +#include "nix/store/derivation-options.hh" + +namespace nix { + +std::string & DesugaredEnv::atFileEnvPair(std::string_view name, std::string fileName) +{ + auto & ret = extraFiles[fileName]; + variables.insert_or_assign( + std::string{name}, + EnvEntry{ + .prependBuildDirectory = true, + .value = std::move(fileName), + }); + return ret; +} + +DesugaredEnv DesugaredEnv::create( + Store & store, const Derivation & drv, const DerivationOptions & drvOptions, const StorePathSet & inputPaths) +{ + DesugaredEnv res; + + if (drv.structuredAttrs) { + auto json = drv.structuredAttrs->prepareStructuredAttrs(store, drvOptions, inputPaths, drv.outputs); + res.atFileEnvPair("NIX_ATTRS_SH_FILE", ".attrs.sh") = StructuredAttrs::writeShell(json); + res.atFileEnvPair("NIX_ATTRS_JSON_FILE", ".attrs.json") = 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.contains(envName)) { + res.variables.insert_or_assign( + envName, + EnvEntry{ + .value = envValue, + }); + } else { + res.atFileEnvPair( + envName + "Path", + ".attr-" + hashString(HashAlgorithm::SHA256, envName).to_string(HashFormat::Nix32, false)) = + envValue; + } + } + + /* Handle exportReferencesGraph(), if set. */ + for (auto & [fileName, storePaths] : drvOptions.getParsedExportReferencesGraph(store)) { + /* Write closure info to . */ + res.extraFiles.insert_or_assign( + fileName, store.makeValidityRegistration(store.exportReferences(storePaths, inputPaths), false, false)); + } + } + + return res; +} + +} // namespace nix diff --git a/src/libstore/include/nix/store/build/derivation-builder.hh b/src/libstore/include/nix/store/build/derivation-builder.hh index f00d4db25..94a3ffae8 100644 --- a/src/libstore/include/nix/store/build/derivation-builder.hh +++ b/src/libstore/include/nix/store/build/derivation-builder.hh @@ -8,6 +8,7 @@ #include "nix/store/parsed-derivations.hh" #include "nix/util/processes.hh" #include "nix/store/restricted-store.hh" +#include "nix/store/build/derivation-env-desugar.hh" namespace nix { @@ -73,34 +74,7 @@ struct DerivationBuilderParams */ StringSet systemFeatures; - struct EnvEntry - { - /** - * Actually, this should be passed as a file, but with a custom - * name (rather than hash-derived name for usual "pass as file"). - */ - std::optional nameOfPassAsFile; - - /** - * String value of env var, or contents of the file - */ - std::string value; - }; - - /** - * 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> finalEnv; - - /** - * Inserted in the temp dir, but no file names placed in env, unlike - * `EnvEntry::nameOfPassAsFile` above. - */ - StringMap extraFiles; + DesugaredEnv desugaredEnv; }; /** diff --git a/src/libstore/include/nix/store/build/derivation-env-desugar.hh b/src/libstore/include/nix/store/build/derivation-env-desugar.hh new file mode 100644 index 000000000..6e2efa6bb --- /dev/null +++ b/src/libstore/include/nix/store/build/derivation-env-desugar.hh @@ -0,0 +1,83 @@ +#pragma once +///@file + +#include "nix/util/types.hh" +#include "nix/store/path.hh" + +namespace nix { + +class Store; +struct Derivation; +struct DerivationOptions; + +/** + * Derivations claim to "just" specify their environment variables, but + * actually do a number of different features, such as "structured + * attrs", "pass as file", and "export references graph", things are + * more complicated then they appear. + * + * The good news is that we can simplify all that to the following view, + * where environment variables and extra files are specified exactly, + * with no special cases. + * + * Because we have `DesugaredEnv`, `DerivationBuilder` doesn't need to + * know about any of those above features, and their special case. + */ +struct DesugaredEnv +{ + struct EnvEntry + { + /** + * Whether to prepend the (inside via) path to the sandbox build + * directory to `value`. This is useful for when the env var + * should point to a file visible to the builder. + */ + bool prependBuildDirectory = false; + + /** + * String value of env var, or contents of the file. + */ + std::string value; + }; + + /** + * The final environment variables to set. + */ + std::map> variables; + + /** + * Extra file to be placed in the build directory. + * + * @note `EnvEntry::prependBuildDirectory` can be used to refer to + * those files without knowing what the build directory is. + */ + StringMap extraFiles; + + /** + * A common case is to define an environment variable that points to + * a file, which contains some contents. + * + * In base: + * ``` + * export VAR=FILE_NAME + * echo CONTENTS >FILE_NAME + * ``` + * + * This function assists in doing both parts, so the file name is + * kept in sync. + */ + std::string & atFileEnvPair(std::string_view name, std::string fileName); + + /** + * Given a (resolved) derivation, its options, and the closure of + * its inputs (which we can get since the derivation is resolved), + * desugar the environment to create a `DesguaredEnv`. + * + * @todo drvOptions will go away as a separate argument when it is + * just part of `Derivation`. + */ + static DesugaredEnv create( + Store & store, const Derivation & drv, const DerivationOptions & drvOptions, const StorePathSet & inputPaths); +}; + +} // namespace nix diff --git a/src/libstore/include/nix/store/meson.build b/src/libstore/include/nix/store/meson.build index cba5d9ca5..776c7521d 100644 --- a/src/libstore/include/nix/store/meson.build +++ b/src/libstore/include/nix/store/meson.build @@ -15,6 +15,7 @@ headers = [ config_pub_h ] + files( 'build/derivation-builder.hh', 'build/derivation-building-goal.hh', 'build/derivation-building-misc.hh', + 'build/derivation-env-desugar.hh', 'build/derivation-goal.hh', 'build/derivation-trampoline-goal.hh', 'build/drv-output-substitution-goal.hh', diff --git a/src/libstore/meson.build b/src/libstore/meson.build index ca8eac12b..2b0106ff3 100644 --- a/src/libstore/meson.build +++ b/src/libstore/meson.build @@ -266,6 +266,7 @@ sources = files( 'build-result.cc', 'build/derivation-building-goal.cc', 'build/derivation-check.cc', + 'build/derivation-env-desugar.cc', 'build/derivation-goal.cc', 'build/derivation-trampoline-goal.cc', 'build/drv-output-substitution-goal.cc', diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index bd5f975fb..3140c716d 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -17,6 +17,7 @@ #include "nix/store/restricted-store.hh" #include "nix/store/user-lock.hh" #include "nix/store/globals.hh" +#include "nix/store/build/derivation-env-desugar.hh" #include @@ -992,19 +993,13 @@ void DerivationBuilderImpl::initEnv() /* 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) { - auto & fileName = *info.nameOfPassAsFile; - writeBuilderFile(fileName, rewriteStrings(info.value, inputRewrites)); - env[name] = tmpDirInSandbox() + "/" + fileName; - } else { - env[name] = info.value; - } + for (const auto & [name, info] : desugaredEnv.variables) { + env[name] = info.prependBuildDirectory ? tmpDirInSandbox() + "/" + info.value : info.value; } /* Add extra files, similar to `finalEnv` */ - for (const auto & [fileName, value] : extraFiles) { - writeBuilderFile(fileName, value); + for (const auto & [fileName, value] : desugaredEnv.extraFiles) { + writeBuilderFile(fileName, rewriteStrings(value, inputRewrites)); } /* For convenience, set an environment pointing to the top build