Merge pull request #3875 from obsidiansystems/new-interface-for-path-pathOpt

Offer a safer interface for path and pathOpt

5 files changed, 95 insertions, 55 deletions

diff --git a/src/libstore/build.cc b/src/libstore/build.cc
index a373d6408..afb2bb096 100644
--- a/src/libstore/build.cc
+++ b/src/libstore/build.cc
@@ -1181,8 +1181,8 @@ void DerivationGoal::haveDerivation()
 
     retrySubstitution = false;
 
-    for (auto & i : drv->outputs)
-        worker.store.addTempRoot(i.second.path(worker.store, drv->name));
+    for (auto & i : drv->outputsAndPaths(worker.store))
+        worker.store.addTempRoot(i.second.second);
 
     /* Check what outputs paths are not already valid. */
     auto invalidOutputs = checkPathValidity(false, buildMode == bmRepair);
@@ -1288,14 +1288,14 @@ void DerivationGoal::repairClosure()
 
     /* Get the output closure. */
     StorePathSet outputClosure;
-    for (auto & i : drv->outputs) {
+    for (auto & i : drv->outputsAndPaths(worker.store)) {
         if (!wantOutput(i.first, wantedOutputs)) continue;
-        worker.store.computeFSClosure(i.second.path(worker.store, drv->name), outputClosure);
+        worker.store.computeFSClosure(i.second.second, outputClosure);
     }
 
     /* Filter out our own outputs (which we have already checked). */
-    for (auto & i : drv->outputs)
-        outputClosure.erase(i.second.path(worker.store, drv->name));
+    for (auto & i : drv->outputsAndPaths(worker.store))
+        outputClosure.erase(i.second.second);
 
     /* Get all dependencies of this derivation so that we know which
        derivation is responsible for which path in the output
@@ -1306,8 +1306,8 @@ void DerivationGoal::repairClosure()
     for (auto & i : inputClosure)
         if (i.isDerivation()) {
             Derivation drv = worker.store.derivationFromPath(i);
-            for (auto & j : drv.outputs)
-                outputsToDrv.insert_or_assign(j.second.path(worker.store, drv.name), i);
+            for (auto & j : drv.outputsAndPaths(worker.store))
+                outputsToDrv.insert_or_assign(j.second.second, i);
         }
 
     /* Check each path (slow!). */
@@ -1466,10 +1466,10 @@ void DerivationGoal::tryToBuild()
 
     /* If any of the outputs already exist but are not valid, delete
        them. */
-    for (auto & i : drv->outputs) {
-        if (worker.store.isValidPath(i.second.path(worker.store, drv->name))) continue;
-        debug("removing invalid path '%s'", worker.store.printStorePath(i.second.path(worker.store, drv->name)));
-        deletePath(worker.store.Store::toRealPath(i.second.path(worker.store, drv->name)));
+    for (auto & i : drv->outputsAndPaths(worker.store)) {
+        if (worker.store.isValidPath(i.second.second)) continue;
+        debug("removing invalid path '%s'", worker.store.printStorePath(i.second.second));
+        deletePath(worker.store.Store::toRealPath(i.second.second));
     }
 
     /* Don't do a remote build if the derivation has the attribute
@@ -1919,8 +1919,8 @@ StorePathSet DerivationGoal::exportReferences(const StorePathSet & storePaths)
     for (auto & j : paths2) {
         if (j.isDerivation()) {
             Derivation drv = worker.store.derivationFromPath(j);
-            for (auto & k : drv.outputs)
-                worker.store.computeFSClosure(k.second.path(worker.store, drv.name), paths);
+            for (auto & k : drv.outputsAndPaths(worker.store))
+                worker.store.computeFSClosure(k.second.second, paths);
         }
     }
 
@@ -2014,8 +2014,8 @@ void DerivationGoal::startBuilder()
     chownToBuilder(tmpDir);
 
     /* Substitute output placeholders with the actual output paths. */
-    for (auto & output : drv->outputs)
-        inputRewrites[hashPlaceholder(output.first)] = worker.store.printStorePath(output.second.path(worker.store, drv->name));
+    for (auto & output : drv->outputsAndPaths(worker.store))
+        inputRewrites[hashPlaceholder(output.first)] = worker.store.printStorePath(output.second.second);
 
     /* Construct the environment passed to the builder. */
     initEnv();
@@ -2199,8 +2199,8 @@ void DerivationGoal::startBuilder()
            rebuilding a path that is in settings.dirsInChroot
            (typically the dependencies of /bin/sh).  Throw them
            out. */
-        for (auto & i : drv->outputs)
-            dirsInChroot.erase(worker.store.printStorePath(i.second.path(worker.store, drv->name)));
+        for (auto & i : drv->outputsAndPaths(worker.store))
+            dirsInChroot.erase(worker.store.printStorePath(i.second.second));
 
 #elif __APPLE__
         /* We don't really have any parent prep work to do (yet?)
@@ -2612,8 +2612,8 @@ void DerivationGoal::writeStructuredAttrs()
 
     /* Add an "outputs" object containing the output paths. */
     nlohmann::json outputs;
-    for (auto & i : drv->outputs)
-        outputs[i.first] = rewriteStrings(worker.store.printStorePath(i.second.path(worker.store, drv->name)), inputRewrites);
+    for (auto & i : drv->outputsAndPaths(worker.store))
+        outputs[i.first] = rewriteStrings(worker.store.printStorePath(i.second.second), inputRewrites);
     json["outputs"] = outputs;
 
     /* Handle exportReferencesGraph. */
@@ -2815,9 +2815,9 @@ struct RestrictedStore : public LocalFSStore
                 if (!goal.isAllowed(path.path))
                     throw InvalidPath("cannot build unknown path '%s' in recursive Nix", printStorePath(path.path));
                 auto drv = derivationFromPath(path.path);
-                for (auto & output : drv.outputs)
+                for (auto & output : drv.outputsAndPaths(*this))
                     if (wantOutput(output.first, path.outputs))
-                        newPaths.insert(output.second.path(*this, drv.name));
+                        newPaths.insert(output.second.second);
             } else if (!goal.isAllowed(path.path))
                 throw InvalidPath("cannot build unknown path '%s' in recursive Nix", printStorePath(path.path));
         }
@@ -3617,8 +3617,8 @@ void DerivationGoal::registerOutputs()
        to do anything here. */
     if (hook) {
         bool allValid = true;
-        for (auto & i : drv->outputs)
-            if (!worker.store.isValidPath(i.second.path(worker.store, drv->name))) allValid = false;
+        for (auto & i : drv->outputsAndPaths(worker.store))
+            if (!worker.store.isValidPath(i.second.second)) allValid = false;
         if (allValid) return;
     }
 
@@ -3639,23 +3639,23 @@ void DerivationGoal::registerOutputs()
        Nix calls. */
     StorePathSet referenceablePaths;
     for (auto & p : inputPaths) referenceablePaths.insert(p);
-    for (auto & i : drv->outputs) referenceablePaths.insert(i.second.path(worker.store, drv->name));
+    for (auto & i : drv->outputsAndPaths(worker.store)) referenceablePaths.insert(i.second.second);
     for (auto & p : addedPaths) referenceablePaths.insert(p);
 
     /* Check whether the output paths were created, and grep each
        output path to determine what other paths it references.  Also make all
        output paths read-only. */
-    for (auto & i : drv->outputs) {
-        auto path = worker.store.printStorePath(i.second.path(worker.store, drv->name));
-        if (!missingPaths.count(i.second.path(worker.store, drv->name))) continue;
+    for (auto & i : drv->outputsAndPaths(worker.store)) {
+        auto path = worker.store.printStorePath(i.second.second);
+        if (!missingPaths.count(i.second.second)) continue;
 
         Path actualPath = path;
         if (needsHashRewrite()) {
-            auto r = redirectedOutputs.find(i.second.path(worker.store, drv->name));
+            auto r = redirectedOutputs.find(i.second.second);
             if (r != redirectedOutputs.end()) {
                 auto redirected = worker.store.Store::toRealPath(r->second);
                 if (buildMode == bmRepair
-                    && redirectedBadOutputs.count(i.second.path(worker.store, drv->name))
+                    && redirectedBadOutputs.count(i.second.second)
                     && pathExists(redirected))
                     replaceValidPath(path, redirected);
                 if (buildMode == bmCheck)
@@ -3722,7 +3722,7 @@ void DerivationGoal::registerOutputs()
            hash). */
         std::optional<ContentAddress> ca;
 
-        if (! std::holds_alternative<DerivationOutputInputAddressed>(i.second.output)) {
+        if (! std::holds_alternative<DerivationOutputInputAddressed>(i.second.first.output)) {
             DerivationOutputCAFloating outputHash;
             std::visit(overloaded {
                 [&](DerivationOutputInputAddressed doi) {
@@ -3737,7 +3737,7 @@ void DerivationGoal::registerOutputs()
                 [&](DerivationOutputCAFloating dof) {
                     outputHash = dof;
                 },
-            }, i.second.output);
+            }, i.second.first.output);
 
             if (outputHash.method == FileIngestionMethod::Flat) {
                 /* The output path should be a regular file without execute permission. */
@@ -3754,12 +3754,12 @@ void DerivationGoal::registerOutputs()
                 ? hashPath(outputHash.hashType, actualPath).first
                 : hashFile(outputHash.hashType, actualPath);
 
-            auto dest = worker.store.makeFixedOutputPath(outputHash.method, h2, i.second.path(worker.store, drv->name).name());
+            auto dest = worker.store.makeFixedOutputPath(outputHash.method, h2, i.second.second.name());
 
             // true if either floating CA, or incorrect fixed hash.
             bool needsMove = true;
 
-            if (auto p = std::get_if<DerivationOutputCAFixed>(& i.second.output)) {
+            if (auto p = std::get_if<DerivationOutputCAFixed>(& i.second.first.output)) {
               Hash & h = p->hash.hash;
               if (h != h2) {
 
@@ -3924,8 +3924,8 @@ void DerivationGoal::registerOutputs()
 
     /* If this is the first round of several, then move the output out of the way. */
     if (nrRounds > 1 && curRound == 1 && curRound < nrRounds && keepPreviousRound) {
-        for (auto & i : drv->outputs) {
-            auto path = worker.store.printStorePath(i.second.path(worker.store, drv->name));
+        for (auto & i : drv->outputsAndPaths(worker.store)) {
+            auto path = worker.store.printStorePath(i.second.second);
             Path prev = path + checkSuffix;
             deletePath(prev);
             Path dst = path + checkSuffix;
@@ -3942,8 +3942,8 @@ void DerivationGoal::registerOutputs()
     /* Remove the .check directories if we're done. FIXME: keep them
        if the result was not determistic? */
     if (curRound == nrRounds) {
-        for (auto & i : drv->outputs) {
-            Path prev = worker.store.printStorePath(i.second.path(worker.store, drv->name)) + checkSuffix;
+        for (auto & i : drv->outputsAndPaths(worker.store)) {
+            Path prev = worker.store.printStorePath(i.second.second) + checkSuffix;
             deletePath(prev);
         }
     }
@@ -4241,12 +4241,12 @@ void DerivationGoal::flushLine()
 StorePathSet DerivationGoal::checkPathValidity(bool returnValid, bool checkHash)
 {
     StorePathSet result;
-    for (auto & i : drv->outputs) {
+    for (auto & i : drv->outputsAndPaths(worker.store)) {
         if (!wantOutput(i.first, wantedOutputs)) continue;
         bool good =
-            worker.store.isValidPath(i.second.path(worker.store, drv->name)) &&
-            (!checkHash || worker.pathContentsGood(i.second.path(worker.store, drv->name)));
-        if (good == returnValid) result.insert(i.second.path(worker.store, drv->name));
+            worker.store.isValidPath(i.second.second) &&
+            (!checkHash || worker.pathContentsGood(i.second.second));
+        if (good == returnValid) result.insert(i.second.second);
     }
     return result;
 }
diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc
index e9e4b5173..a9fed2564 100644
--- a/src/libstore/derivations.cc
+++ b/src/libstore/derivations.cc
@@ -480,12 +480,12 @@ DrvHashModulo hashDerivationModulo(Store & store, const Derivation & drv, bool m
         throw Error("Regular input-addressed derivations are not yet allowed to depend on CA derivations");
     case DerivationType::CAFixed: {
         std::map<std::string, Hash> outputHashes;
-        for (const auto & i : drv.outputs) {
-            auto & dof = std::get<DerivationOutputCAFixed>(i.second.output);
+        for (const auto & i : drv.outputsAndPaths(store)) {
+            auto & dof = std::get<DerivationOutputCAFixed>(i.second.first.output);
             auto hash = hashString(htSHA256, "fixed:out:"
                 + dof.hash.printMethodAlgo() + ":"
                 + dof.hash.hash.to_string(Base16, false) + ":"
-                + store.printStorePath(i.second.path(store, drv.name)));
+                + store.printStorePath(i.second.second));
             outputHashes.insert_or_assign(i.first, std::move(hash));
         }
         return outputHashes;
@@ -539,8 +539,8 @@ bool wantOutput(const string & output, const std::set<string> & wanted)
 StorePathSet BasicDerivation::outputPaths(const Store & store) const
 {
     StorePathSet paths;
-    for (auto & i : outputs)
-        paths.insert(i.second.path(store, name));
+    for (auto & i : outputsAndPaths(store))
+        paths.insert(i.second.second);
     return paths;
 }
 
@@ -561,6 +561,27 @@ StringSet BasicDerivation::outputNames() const
     return names;
 }
 
+DerivationOutputsAndPaths BasicDerivation::outputsAndPaths(const Store & store) const {
+    DerivationOutputsAndPaths outsAndPaths;
+    for (auto output : outputs)
+        outsAndPaths.insert(std::make_pair(
+            output.first,
+            std::make_pair(output.second, output.second.path(store, name))
+            )
+        );
+    return outsAndPaths;
+}
+
+DerivationOutputsAndOptPaths BasicDerivation::outputsAndOptPaths(const Store & store) const {
+    DerivationOutputsAndOptPaths outsAndOptPaths;
+    for (auto output : outputs)
+        outsAndOptPaths.insert(std::make_pair(
+            output.first,
+            std::make_pair(output.second, output.second.pathOpt(store, output.first))
+            )
+        );
+    return outsAndOptPaths;
+}
 
 std::string_view BasicDerivation::nameFromPath(const StorePath & drvPath) {
     auto nameWithSuffix = drvPath.name();
@@ -601,9 +622,9 @@ Source & readDerivation(Source & in, const Store & store, BasicDerivation & drv,
 void writeDerivation(Sink & out, const Store & store, const BasicDerivation & drv)
 {
     out << drv.outputs.size();
-    for (auto & i : drv.outputs) {
+    for (auto & i : drv.outputsAndPaths(store)) {
         out << i.first
-            << store.printStorePath(i.second.path(store, drv.name));
+            << store.printStorePath(i.second.second);
         std::visit(overloaded {
             [&](DerivationOutputInputAddressed doi) {
                 out << "" << "";
@@ -616,7 +637,7 @@ void writeDerivation(Sink & out, const Store & store, const BasicDerivation & dr
                 out << (makeFileIngestionPrefix(dof.method) + printHashType(dof.hashType))
                     << "";
             },
-        }, i.second.output);
+        }, i.second.first.output);
     }
     writeStorePaths(store, out, drv.inputSrcs);
     out << drv.platform << drv.builder << drv.args;
diff --git a/src/libstore/derivations.hh b/src/libstore/derivations.hh
index 53dfc7f13..3aae30ab2 100644
--- a/src/libstore/derivations.hh
+++ b/src/libstore/derivations.hh
@@ -47,6 +47,9 @@ struct DerivationOutput
         DerivationOutputCAFloating
     > output;
     std::optional<HashType> hashAlgoOpt(const Store & store) const;
+    /* Note, when you use this function you should make sure that you're passing
+       the right derivation name. When in doubt, you should use the safer
+       interface provided by BasicDerivation::outputsAndPaths */
     std::optional<StorePath> pathOpt(const Store & store, std::string_view drvName) const;
     /* DEPRECATED: Remove after CA drvs are fully implemented */
     StorePath path(const Store & store, std::string_view drvName) const {
@@ -58,6 +61,15 @@ struct DerivationOutput
 
 typedef std::map<string, DerivationOutput> DerivationOutputs;
 
+/* These are analogues to the previous DerivationOutputs data type, but they
+   also contains, for each output, the (optional) store path in which it would
+   be written. To calculate values of these types, see the corresponding
+   functions in BasicDerivation */
+typedef std::map<string, std::pair<DerivationOutput, StorePath>>
+  DerivationOutputsAndPaths;
+typedef std::map<string, std::pair<DerivationOutput, std::optional<StorePath>>>
+  DerivationOutputsAndOptPaths;
+
 /* For inputs that are sub-derivations, we specify exactly which
    output IDs we are interested in. */
 typedef std::map<StorePath, StringSet> DerivationInputs;
@@ -107,6 +119,13 @@ struct BasicDerivation
     /* Return the output names of a derivation. */
     StringSet outputNames() const;
 
+    /* Calculates the maps that contains all the DerivationOutputs, but
+       augmented with knowledge of the Store paths they would be written into.
+       The first one of these functions will be removed when the CA work is
+       completed */
+    DerivationOutputsAndPaths outputsAndPaths(const Store & store) const;
+    DerivationOutputsAndOptPaths outputsAndOptPaths(const Store & store) const;
+
     static std::string_view nameFromPath(const StorePath & storePath);
 };
 
diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc
index 8cb1af7c7..bccd77b68 100644
--- a/src/libstore/local-store.cc
+++ b/src/libstore/local-store.cc
@@ -618,11 +618,11 @@ uint64_t LocalStore::addValidPath(State & state,
            registration above is undone. */
         if (checkOutputs) checkDerivationOutputs(info.path, drv);
 
-        for (auto & i : drv.outputs) {
+        for (auto & i : drv.outputsAndPaths(*this)) {
             state.stmtAddDerivationOutput.use()
                 (id)
                 (i.first)
-                (printStorePath(i.second.path(*this, drv.name)))
+                (printStorePath(i.second.second))
                 .exec();
         }
     }
diff --git a/src/libstore/misc.cc b/src/libstore/misc.cc
index 0ae1ceaad..f6aa570bb 100644
--- a/src/libstore/misc.cc
+++ b/src/libstore/misc.cc
@@ -207,10 +207,10 @@ void Store::queryMissing(const std::vector<StorePathWithOutputs> & targets,
             ParsedDerivation parsedDrv(StorePath(path.path), *drv);
 
             PathSet invalid;
-            for (auto & j : drv->outputs)
+            for (auto & j : drv->outputsAndPaths(*this))
                 if (wantOutput(j.first, path.outputs)
-                    && !isValidPath(j.second.path(*this, drv->name)))
-                    invalid.insert(printStorePath(j.second.path(*this, drv->name)));
+                    && !isValidPath(j.second.second))
+                    invalid.insert(printStorePath(j.second.second));
             if (invalid.empty()) return;
 
             if (settings.useSubstitutes && parsedDrv.substitutesAllowed()) {