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.

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

@@ -677,7 +677,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
             auto * localStoreP = dynamic_cast<LocalStore *>(&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 <fileName>. */
@@ -726,7 +748,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
                     *drvOptions,
                     inputPaths,
                     initialOutputs,
-                    std::move(extraEnv),
+                    std::move(finalEnv),
                     std::move(extraFiles),
                 });
         }

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<std::string, EnvEntry, std::less<>> extraEnv;
+    std::map<std::string, EnvEntry, std::less<>> 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<std::string, InitialOutput> & initialOutputs,
-        std::map<std::string, EnvEntry, std::less<>> extraEnv,
+        std::map<std::string, EnvEntry, std::less<>> 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)}
     {
     }

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) {
+    /* Write the final environment. Note that this is intentionally
-        writeBuilderFile(fileName, rewriteStrings(value, inputRewrites));
+       *not* `drv.env`, because we've desugared things like like
-        env[envVarName] = tmpDirInSandbox() + "/" + fileName;
+       "passAFile", "expandReferencesGraph", structured attrs, etc. */
-    };
+    for (const auto & [name, info] : finalEnv) {
-
-    /* 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) {
         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);
     }