From 2767ae35d9824e861df3211f4153bff7d3690023 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 15 Aug 2025 16:50:46 -0400 Subject: [PATCH 1/4] Deduplicate "export reference graph" logic a bit The first part on `drvOptions.exportReferencesGraph` is the same in both cases. It is just how the information is finally rendered that is different. --- src/libstore/derivation-options.cc | 18 ++++++++++++++++++ .../include/nix/store/derivation-options.hh | 18 ++++++++++++++++++ src/libstore/parsed-derivations.cc | 5 +---- src/libstore/unix/build/derivation-builder.cc | 10 ++-------- 4 files changed, 39 insertions(+), 12 deletions(-) diff --git a/src/libstore/derivation-options.cc b/src/libstore/derivation-options.cc index b41b97f4c..1acb9dc03 100644 --- a/src/libstore/derivation-options.cc +++ b/src/libstore/derivation-options.cc @@ -256,6 +256,24 @@ DerivationOptions::fromStructuredAttrs(const StringMap & env, const StructuredAt }; } +std::map +DerivationOptions::getParsedExportReferencesGraph(const StoreDirConfig & store) const +{ + std::map res; + + for (auto & [fileName, ss] : exportReferencesGraph) { + StorePathSet storePaths; + for (auto & storePathS : ss) { + if (!store.isInStore(storePathS)) + throw BuildError("'exportReferencesGraph' contains a non-store path '%1%'", storePathS); + storePaths.insert(store.toStorePath(storePathS).first); + } + res.insert_or_assign(fileName, storePaths); + } + + return res; +} + StringSet DerivationOptions::getRequiredSystemFeatures(const BasicDerivation & drv) const { // FIXME: cache this? diff --git a/src/libstore/include/nix/store/derivation-options.hh b/src/libstore/include/nix/store/derivation-options.hh index 98517e904..88694f730 100644 --- a/src/libstore/include/nix/store/derivation-options.hh +++ b/src/libstore/include/nix/store/derivation-options.hh @@ -8,10 +8,12 @@ #include "nix/util/types.hh" #include "nix/util/json-impls.hh" +#include "nix/store/path.hh" namespace nix { class Store; +struct StoreDirConfig; struct BasicDerivation; struct StructuredAttrs; @@ -116,6 +118,22 @@ struct DerivationOptions */ std::map exportReferencesGraph; + /** + * Once a derivations is resolved, the strings in in + * `exportReferencesGraph` should all be store paths (with possible + * suffix paths, but those are discarded). + * + * @return The parsed path set for for each key in the map. + * + * @todo Ideally, `exportReferencesGraph` would just store + * `StorePath`s for this, but we can't just do that, because for CA + * derivations they is actually in general `DerivedPath`s (via + * placeholder strings) until the derivation is resolved and exact + * inputs store paths are known. We can use better types for that + * too, but that is a longer project. + */ + std::map getParsedExportReferencesGraph(const StoreDirConfig & store) const; + /** * env: __sandboxProfile * diff --git a/src/libstore/parsed-derivations.cc b/src/libstore/parsed-derivations.cc index 1006bbc0a..9e8d44d6e 100644 --- a/src/libstore/parsed-derivations.cc +++ b/src/libstore/parsed-derivations.cc @@ -113,10 +113,7 @@ nlohmann::json StructuredAttrs::prepareStructuredAttrs( json["outputs"] = std::move(outputsJson); /* Handle exportReferencesGraph. */ - for (auto & [key, inputPaths] : drvOptions.exportReferencesGraph) { - StorePathSet storePaths; - for (auto & p : inputPaths) - storePaths.insert(store.toStorePath(p).first); + for (auto & [key, storePaths] : drvOptions.getParsedExportReferencesGraph(store)) { json[key] = pathInfoToJSON(store, store.exportReferences(storePaths, storePaths)); } diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index f6546ec62..76468cca0 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -785,17 +785,11 @@ void DerivationBuilderImpl::startBuilder() /* Handle exportReferencesGraph(), if set. */ if (!drv.structuredAttrs) { - for (auto & [fileName, ss] : drvOptions.exportReferencesGraph) { - StorePathSet storePathSet; - for (auto & storePathS : ss) { - if (!store.isInStore(storePathS)) - throw BuildError("'exportReferencesGraph' contains a non-store path '%1%'", storePathS); - storePathSet.insert(store.toStorePath(storePathS).first); - } + for (auto & [fileName, storePaths] : drvOptions.getParsedExportReferencesGraph(store)) { /* Write closure info to . */ writeFile( tmpDir + "/" + fileName, - store.makeValidityRegistration(store.exportReferences(storePathSet, inputPaths), false, false)); + store.makeValidityRegistration(store.exportReferences(storePaths, inputPaths), false, false)); } } From 92b10cf3f5061553b4f0400a64c661d57c6ef674 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 18 Aug 2025 15:26:54 -0400 Subject: [PATCH 2/4] `DerivationBuilderImpl::writeStructuredAttrs` remove a rewrite As much as I prefer rewriting the parsed rather than unparsed JSON for elegance, this gets in the way of the separation of concerns that I am trying to do. As a practical matter, any rewriting that this did will also be done by the second round of rewriting that remains below, so removing this code should have no effect. --- src/libstore/unix/build/derivation-builder.cc | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index 76468cca0..0b6dbe670 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -1106,14 +1106,6 @@ void DerivationBuilderImpl::writeStructuredAttrs() { if (drv.structuredAttrs) { auto json = drv.structuredAttrs->prepareStructuredAttrs(store, drvOptions, inputPaths, drv.outputs); - nlohmann::json rewritten; - for (auto & [i, v] : json["outputs"].get()) { - /* The placeholder must have a rewrite, so we use it to cover both the - cases where we know or don't know the output path ahead of time. */ - rewritten[i] = rewriteStrings((std::string) v, inputRewrites); - } - - json["outputs"] = rewritten; auto jsonSh = StructuredAttrs::writeShell(json); From e3c74f5a134061f44b91fccaf6ec87786c790bae Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 18 Aug 2025 16:37:06 -0400 Subject: [PATCH 3/4] 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); From 1d3ddb21faf95ee72af3126bc475f706839a8669 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 18 Aug 2025 18:08:35 -0400 Subject: [PATCH 4/4] 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); }