From e3c74f5a134061f44b91fccaf6ec87786c790bae Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 18 Aug 2025 16:37:06 -0400 Subject: [PATCH] Desugar structured attrs, "export reference graph" outside `DerivationBuilder` I think this is a better separation of concerns. `DerivationBuilder` doesn't need to to the final, query-heavy details about how these things are constructed. It just operates on the level of "simple, stupid" files and environment variables. --- .../build/derivation-building-goal.cc | 38 +++++++++++++ .../nix/store/build/derivation-builder.hh | 35 +++++++++++- src/libstore/unix/build/derivation-builder.cc | 54 ++++++++----------- 3 files changed, 93 insertions(+), 34 deletions(-) diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index e0a8717a0..dae13c728 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -677,6 +677,42 @@ Goal::Co DerivationBuildingGoal::tryToBuild() auto * localStoreP = dynamic_cast(&worker.store); assert(localStoreP); + decltype(DerivationBuilderParams::extraEnv) extraEnv; + decltype(DerivationBuilderParams::extraFiles) extraFiles; + + try { + if (drv->structuredAttrs) { + auto json = drv->structuredAttrs->prepareStructuredAttrs( + worker.store, *drvOptions, inputPaths, drv->outputs); + + extraEnv.insert_or_assign( + "NIX_ATTRS_SH_FILE", + DerivationBuilderParams::EnvEntry{ + .nameOfPassAsFile = ".attrs.sh", + .value = StructuredAttrs::writeShell(json), + }); + extraEnv.insert_or_assign( + "NIX_ATTRS_JSON_FILE", + DerivationBuilderParams::EnvEntry{ + .nameOfPassAsFile = ".attrs.json", + .value = json.dump(), + }); + } else { + /* 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)); + } + } + } catch (BuildError & e) { + outputLocks.unlock(); + worker.permanentFailure = true; + co_return done(BuildResult::InputRejected, {}, std::move(e)); + } + /* If we have to wait and retry (see below), then `builder` will already be created, so we don't need to create it again. */ builder = makeDerivationBuilder( @@ -690,6 +726,8 @@ Goal::Co DerivationBuildingGoal::tryToBuild() *drvOptions, inputPaths, initialOutputs, + std::move(extraEnv), + 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 462352c76..da8f74a09 100644 --- a/src/libstore/include/nix/store/build/derivation-builder.hh +++ b/src/libstore/include/nix/store/build/derivation-builder.hh @@ -59,6 +59,35 @@ struct DerivationBuilderParams const BuildMode & buildMode; + 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; + }; + + /** + * Extra 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; + + /** + * Inserted in the temp dir, but no file names placed in env, unlike + * `EnvEntry::nameOfPassAsFile` above. + */ + StringMap extraFiles; + DerivationBuilderParams( const StorePath & drvPath, const BuildMode & buildMode, @@ -66,7 +95,9 @@ struct DerivationBuilderParams const Derivation & drv, const DerivationOptions & drvOptions, const StorePathSet & inputPaths, - std::map & initialOutputs) + std::map & initialOutputs, + std::map> extraEnv, + StringMap extraFiles) : drvPath{drvPath} , buildResult{buildResult} , drv{drv} @@ -74,6 +105,8 @@ struct DerivationBuilderParams , inputPaths{inputPaths} , initialOutputs{initialOutputs} , buildMode{buildMode} + , extraEnv{std::move(extraEnv)} + , extraFiles{std::move(extraFiles)} { } diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index 0b6dbe670..2c735a7c6 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -273,11 +273,6 @@ protected: private: - /** - * Write a JSON file containing the derivation attributes. - */ - void writeStructuredAttrs(); - /** * Start an in-process nix daemon thread for recursive-nix. */ @@ -781,18 +776,6 @@ void DerivationBuilderImpl::startBuilder() /* Construct the environment passed to the builder. */ initEnv(); - writeStructuredAttrs(); - - /* Handle exportReferencesGraph(), if set. */ - if (!drv.structuredAttrs) { - for (auto & [fileName, storePaths] : drvOptions.getParsedExportReferencesGraph(store)) { - /* Write closure info to . */ - writeFile( - tmpDir + "/" + fileName, - store.makeValidityRegistration(store.exportReferences(storePaths, inputPaths), false, false)); - } - } - prepareSandbox(); if (needsHashRewrite() && pathExists(homeDir)) @@ -1033,6 +1016,11 @@ 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`. */ @@ -1043,12 +1031,26 @@ void DerivationBuilderImpl::initEnv() } else { auto hash = hashString(HashAlgorithm::SHA256, i.first); std::string fn = ".attr-" + hash.to_string(HashFormat::Nix32, false); - writeBuilderFile(fn, rewriteStrings(i.second, inputRewrites)); - env[i.first + "Path"] = tmpDirInSandbox() + "/" + fn; + 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) { + if (info.nameOfPassAsFile) { + writeEnv(name, *info.nameOfPassAsFile, info.value); + } else { + env[name] = info.value; + } + } + + /* Add extra files, analogous to `extraEnv` */ + for (const auto & [fileName, value] : extraFiles) { + writeBuilderFile(fileName, value); + } + /* For convenience, set an environment pointing to the top build directory. */ env["NIX_BUILD_TOP"] = tmpDirInSandbox(); @@ -1102,20 +1104,6 @@ void DerivationBuilderImpl::initEnv() env["TERM"] = "xterm-256color"; } -void DerivationBuilderImpl::writeStructuredAttrs() -{ - if (drv.structuredAttrs) { - auto json = drv.structuredAttrs->prepareStructuredAttrs(store, drvOptions, inputPaths, drv.outputs); - - auto jsonSh = StructuredAttrs::writeShell(json); - - writeBuilderFile(".attrs.sh", rewriteStrings(jsonSh, inputRewrites)); - env["NIX_ATTRS_SH_FILE"] = tmpDirInSandbox() + "/.attrs.sh"; - writeBuilderFile(".attrs.json", rewriteStrings(json.dump(), inputRewrites)); - env["NIX_ATTRS_JSON_FILE"] = tmpDirInSandbox() + "/.attrs.json"; - } -} - void DerivationBuilderImpl::startDaemon() { experimentalFeatureSettings.require(Xp::RecursiveNix);