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 <[email protected]>

Author: Ericson2314

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 <fileName>. */
-                        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),
                 });
         }
 

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"

src/libstore/build/derivation-env-desugar.cc

@@ -0,0 +1,62 @@
+#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)
+{
+    if (drv.structuredAttrs) {
+        auto json = drv.structuredAttrs->prepareStructuredAttrs(store, drvOptions, inputPaths, drv.outputs);
+
+        DesugaredEnv res;
+        res.atFileEnvPair("NIX_ATTRS_SH_FILE", ".attrs.sh") = StructuredAttrs::writeShell(json);
+        res.atFileEnvPair("NIX_ATTRS_JSON_FILE", ".attrs.json") = json.dump();
+        return res;
+    } else {
+        DesugaredEnv res;
+
+        /* 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 <fileName>. */
+            res.extraFiles.insert_or_assign(
+                fileName, store.makeValidityRegistration(store.exportReferences(storePaths, inputPaths), false, false));
+        }
+
+        return res;
+    }
+}
+
+} // namespace nix

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<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;
+    DesugaredEnv desugaredEnv;
 };
 
 /**

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<std::string, EnvEntry, std::less<>> 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

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',

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',

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 <queue>
 
@@ -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