Fix the detection of already built drv outputs

thufschmitt · thufschmitt · commit 4d458394991f · 2020-12-17T11:35:24.000+01:00
PRs #4370 and #4348 had a bad interaction in that the second broke the fist one in a not trivial way. The issue was that since #4348 the logic for detecting whether a derivation output is already built requires some logic that was specific to the `LocalStore`. It happens though that most of this logic could be upstreamed to any `Store`, which is what this commit does.
diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc
@@ -745,7 +745,7 @@ static void rewriteDerivation(Store & store, BasicDerivation & drv, const String
 
 }
 
-std::optional<BasicDerivation> Derivation::tryResolve(Store & store) {
+std::optional<BasicDerivation> Derivation::tryResolveUncached(Store & store) {
     BasicDerivation resolved { *this };
 
     // Input paths that we'll want to rewrite in the derivation
@@ -771,4 +771,34 @@ std::optional<BasicDerivation> Derivation::tryResolve(Store & store) {
     return resolved;
 }
 
+std::optional<BasicDerivation> Derivation::tryResolve(Store& store)
+{
+    auto drvPath = writeDerivation(store, *this, NoRepair, false);
+    return Derivation::tryResolve(store, drvPath);
+}
+
+std::optional<BasicDerivation> Derivation::tryResolve(Store& store, const StorePath& drvPath)
+{
+    // This is quite dirty and leaky, but will disappear once #4340 is merged
+    static Sync<std::map<StorePath, std::optional<Derivation>>> resolutionsCache;
+
+    {
+        auto resolutions = resolutionsCache.lock();
+        auto resolvedDrvIter = resolutions->find(drvPath);
+        if (resolvedDrvIter != resolutions->end()) {
+            auto & [_, resolvedDrv] = *resolvedDrvIter;
+                return *resolvedDrv;
+        }
+    }
+
+    /* Try resolve drv and use that path instead. */
+    auto drv = store.readDerivation(drvPath);
+    auto attempt = drv.tryResolveUncached(store);
+    if (!attempt)
+        return std::nullopt;
+    /* Store in memo table. */
+    resolutionsCache.lock()->insert_or_assign(drvPath, *attempt);
+    return *attempt;
+}
+
 }
diff --git a/src/libstore/derivations.hh b/src/libstore/derivations.hh
@@ -138,10 +138,14 @@ struct Derivation : BasicDerivation
 
        2. Input placeholders are replaced with realized input store paths. */
     std::optional<BasicDerivation> tryResolve(Store & store);
+    static std::optional<BasicDerivation> tryResolve(Store & store, const StorePath & drvPath);
 
     Derivation() = default;
     Derivation(const BasicDerivation & bd) : BasicDerivation(bd) { }
     Derivation(BasicDerivation && bd) : BasicDerivation(std::move(bd)) { }
+
+private:
+    std::optional<BasicDerivation> tryResolveUncached(Store & store);
 };
 
 
diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc
@@ -877,35 +877,9 @@ StorePathSet LocalStore::queryValidDerivers(const StorePath & path)
     });
 }
 
-// Try to resolve the derivation at path `original`, with a caching layer
-// to make it more efficient
-std::optional<Derivation> cachedResolve(
-    LocalStore& store,
-    const StorePath& original)
-{
-    // This is quite dirty and leaky, but will disappear once #4340 is merged
-    static Sync<std::map<StorePath, std::optional<Derivation>>> resolutionsCache;
-    {
-        auto resolutions = resolutionsCache.lock();
-        auto resolvedDrvIter = resolutions->find(original);
-        if (resolvedDrvIter != resolutions->end()) {
-            auto & [_, resolvedDrv] = *resolvedDrvIter;
-                return *resolvedDrv;
-        }
-    }
-
-    /* Try resolve drv and use that path instead. */
-    auto drv = store.readDerivation(original);
-    auto attempt = drv.tryResolve(store);
-    if (!attempt)
-        return std::nullopt;
-    /* Store in memo table. */
-    resolutionsCache.lock()->insert_or_assign(original, *attempt);
-    return *attempt;
-}
 
 std::map<std::string, std::optional<StorePath>>
-LocalStore::queryPartialDerivationOutputMap(const StorePath& path_)
+LocalStore::queryDerivationOutputMapNoResolve(const StorePath& path_)
 {
     auto path = path_;
     auto outputs = retrySQLite<std::map<std::string, std::optional<StorePath>>>([&]() {
@@ -924,20 +898,9 @@ LocalStore::queryPartialDerivationOutputMap(const StorePath& path_)
     if (!settings.isExperimentalFeatureEnabled("ca-derivations"))
         return outputs;
 
-    auto drv = readDerivation(path);
-
-    auto resolvedDrv = cachedResolve(*this, path);
-
-    if (!resolvedDrv) {
-        for (auto& [outputName, _] : drv.outputsAndOptPaths(*this)) {
-            if (!outputs.count(outputName))
-                outputs.emplace(outputName, std::nullopt);
-        }
-        return outputs;
-    }
-
-    auto resolvedDrvHashes = staticOutputHashes(*this, *resolvedDrv);
-    for (auto& [outputName, hash] : resolvedDrvHashes) {
+    auto drv = readInvalidDerivation(path);
+    auto drvHashes = staticOutputHashes(*this, drv);
+    for (auto& [outputName, hash] : drvHashes) {
         auto realisation = queryRealisation(DrvOutput{hash, outputName});
         if (realisation)
             outputs.insert_or_assign(outputName, realisation->outPath);
diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh
@@ -127,7 +127,7 @@ public:
 
     StorePathSet queryValidDerivers(const StorePath & path) override;
 
-    std::map<std::string, std::optional<StorePath>> queryPartialDerivationOutputMap(const StorePath & path) override;
+    std::map<std::string, std::optional<StorePath>> queryDerivationOutputMapNoResolve(const StorePath & path) override;
 
     std::optional<StorePath> queryPathFromHashPart(const std::string & hashPart) override;
 
diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc
@@ -366,6 +366,29 @@ bool Store::PathInfoCacheValue::isKnownNow()
     return std::chrono::steady_clock::now() < time_point + ttl;
 }
 
+std::map<std::string, std::optional<StorePath>> Store::queryDerivationOutputMapNoResolve(const StorePath & path)
+{
+    std::map<std::string, std::optional<StorePath>> outputs;
+    auto drv = readInvalidDerivation(path);
+    for (auto& [outputName, output] : drv.outputsAndOptPaths(*this)) {
+        outputs.emplace(outputName, output.second);
+    }
+    return outputs;
+}
+
+std::map<std::string, std::optional<StorePath>> Store::queryPartialDerivationOutputMap(const StorePath & path)
+{
+    if (settings.isExperimentalFeatureEnabled("ca-derivations")) {
+        auto resolvedDrv = Derivation::tryResolve(*this, path);
+        if (resolvedDrv) {
+            auto resolvedDrvPath = writeDerivation(*this, *resolvedDrv, NoRepair, true);
+            if (isValidPath(resolvedDrvPath))
+                return queryDerivationOutputMapNoResolve(resolvedDrvPath);
+        }
+    }
+    return queryDerivationOutputMapNoResolve(path);
+}
+
 OutputPathMap Store::queryDerivationOutputMap(const StorePath & path) {
     auto resp = queryPartialDerivationOutputMap(path);
     OutputPathMap result;
@@ -730,14 +753,14 @@ void Store::buildPaths(const std::vector<StorePathWithOutputs> & paths, BuildMod
 
     for (auto & path : paths) {
         if (path.path.isDerivation()) {
-            if (settings.isExperimentalFeatureEnabled("ca-derivations")) {
-                for (auto & outputName : path.outputs) {
-                    if (!queryRealisation({path.path, outputName}))
-                        unsupported("buildPaths");
-                }
-            } else
-                unsupported("buildPaths");
-
+            auto outPaths = queryPartialDerivationOutputMap(path.path);
+            for (auto & outputName : path.outputs) {
+                auto currentOutputPathIter = outPaths.find(outputName);
+                if (currentOutputPathIter == outPaths.end() ||
+                    !currentOutputPathIter->second ||
+                    !isValidPath(*currentOutputPathIter->second))
+                    unsupported("buildPaths");
+            }
         } else
             paths2.insert(path.path);
     }
diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh
@@ -416,8 +416,13 @@ public:
     /* Query the mapping outputName => outputPath for the given derivation. All
        outputs are mentioned so ones mising the mapping are mapped to
        `std::nullopt`.  */
-    virtual std::map<std::string, std::optional<StorePath>> queryPartialDerivationOutputMap(const StorePath & path)
-    { unsupported("queryPartialDerivationOutputMap"); }
+    virtual std::map<std::string, std::optional<StorePath>> queryPartialDerivationOutputMap(const StorePath & path);
+
+    /*
+     * Similar to `queryPartialDerivationOutputMap`, but doesn't try to resolve
+     * the derivation
+     */
+    virtual std::map<std::string, std::optional<StorePath>> queryDerivationOutputMapNoResolve(const StorePath & path);
 
     /* Query the mapping outputName=>outputPath for the given derivation.
        Assume every output has a mapping and throw an exception otherwise. */
