Merge pull request #13769 from obsidiansystems/simplify-derivation-building-goal

Handle structured attrs, "export references graph" outside of `DerivationBuilder`

src/libstore/build/derivation-building-goal.cc

@@ -677,6 +677,64 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
             auto * localStoreP = dynamic_cast<LocalStore *>(&worker.store);
             assert(localStoreP);
 
+            decltype(DerivationBuilderParams::finalEnv) finalEnv;
+            decltype(DerivationBuilderParams::extraFiles) extraFiles;
+
+            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 <fileName>. */
+                        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 +748,8 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
                     *drvOptions,
                     inputPaths,
                     initialOutputs,
+                    std::move(finalEnv),
+                    std::move(extraFiles),
                 });
         }
 

src/libstore/derivation-options.cc

@@ -256,6 +256,24 @@ DerivationOptions::fromStructuredAttrs(const StringMap & env, const StructuredAt
     };
 }
 
+std::map<std::string, StorePathSet>
+DerivationOptions::getParsedExportReferencesGraph(const StoreDirConfig & store) const
+{
+    std::map<std::string, StorePathSet> 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?

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<std::string> 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<std::string, EnvEntry, std::less<>> finalEnv;
+
+    /**
+     * 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<std::string, InitialOutput> & initialOutputs)
+        std::map<std::string, InitialOutput> & initialOutputs,
+        std::map<std::string, EnvEntry, std::less<>> finalEnv,
+        StringMap extraFiles)
         : drvPath{drvPath}
         , buildResult{buildResult}
         , drv{drv}
@@ -74,6 +105,8 @@ struct DerivationBuilderParams
         , inputPaths{inputPaths}
         , initialOutputs{initialOutputs}
         , buildMode{buildMode}
+        , finalEnv{std::move(finalEnv)}
+        , extraFiles{std::move(extraFiles)}
     {
     }
 

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<std::string, StringSet> 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<std::string, StorePathSet> getParsedExportReferencesGraph(const StoreDirConfig & store) const;
+
     /**
      * env: __sandboxProfile
      *

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) {
+    for (auto & [key, storePaths] : drvOptions.getParsedExportReferencesGraph(store)) {
-        StorePathSet storePaths;
-        for (auto & p : inputPaths)
-            storePaths.insert(store.toStorePath(p).first);
         json[key] = pathInfoToJSON(store, store.exportReferences(storePaths, storePaths));
     }
 

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,24 +776,6 @@ void DerivationBuilderImpl::startBuilder()
     /* Construct the environment passed to the builder. */
     initEnv();
 
-    writeStructuredAttrs();
-
-    /* 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);
-            }
-            /* Write closure info to <fileName>. */
-            writeFile(
-                tmpDir + "/" + fileName,
-                store.makeValidityRegistration(store.exportReferences(storePathSet, inputPaths), false, false));
-        }
-    }
-
     prepareSandbox();
 
     if (needsHashRewrite() && pathExists(homeDir))
@@ -1039,20 +1016,22 @@ 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());
 
-    /* In non-structured mode, set all bindings either directory in the
+    /* Write the final environment. Note that this is intentionally
-       environment or via a file, as specified by
+       *not* `drv.env`, because we've desugared things like like
-       `DerivationOptions::passAsFile`. */
+       "passAFile", "expandReferencesGraph", structured attrs, etc. */
-    if (!drv.structuredAttrs) {
+    for (const auto & [name, info] : finalEnv) {
-        for (auto & i : drv.env) {
+        if (info.nameOfPassAsFile) {
-            if (drvOptions.passAsFile.find(i.first) == drvOptions.passAsFile.end()) {
+            auto & fileName = *info.nameOfPassAsFile;
-                env[i.first] = i.second;
+            writeBuilderFile(fileName, rewriteStrings(info.value, inputRewrites));
+            env[name] = tmpDirInSandbox() + "/" + fileName;
         } else {
-                auto hash = hashString(HashAlgorithm::SHA256, i.first);
+            env[name] = info.value;
-                std::string fn = ".attr-" + hash.to_string(HashFormat::Nix32, false);
-                writeBuilderFile(fn, rewriteStrings(i.second, inputRewrites));
-                env[i.first + "Path"] = tmpDirInSandbox() + "/" + fn;
         }
     }
+
+    /* Add extra files, similar to `finalEnv` */
+    for (const auto & [fileName, value] : extraFiles) {
+        writeBuilderFile(fileName, value);
     }
 
     /* For convenience, set an environment pointing to the top build
@@ -1108,28 +1087,6 @@ void DerivationBuilderImpl::initEnv()
     env["TERM"] = "xterm-256color";
 }
 
-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<nlohmann::json::object_t>()) {
-            /* 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);
-
-        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);