Fix the detection of already built drv outputs

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.
This commit is contained in:
regnat 2020-12-17 11:35:24 +01:00
parent ae3c3e3bb2
commit 4d45839499
6 changed files with 78 additions and 53 deletions

View file

@ -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 }; BasicDerivation resolved { *this };
// Input paths that we'll want to rewrite in the derivation // Input paths that we'll want to rewrite in the derivation
@ -771,4 +771,34 @@ std::optional<BasicDerivation> Derivation::tryResolve(Store & store) {
return resolved; 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;
}
} }

View file

@ -138,10 +138,14 @@ struct Derivation : BasicDerivation
2. Input placeholders are replaced with realized input store paths. */ 2. Input placeholders are replaced with realized input store paths. */
std::optional<BasicDerivation> tryResolve(Store & store); std::optional<BasicDerivation> tryResolve(Store & store);
static std::optional<BasicDerivation> tryResolve(Store & store, const StorePath & drvPath);
Derivation() = default; Derivation() = default;
Derivation(const BasicDerivation & bd) : BasicDerivation(bd) { } Derivation(const BasicDerivation & bd) : BasicDerivation(bd) { }
Derivation(BasicDerivation && bd) : BasicDerivation(std::move(bd)) { } Derivation(BasicDerivation && bd) : BasicDerivation(std::move(bd)) { }
private:
std::optional<BasicDerivation> tryResolveUncached(Store & store);
}; };

View file

@ -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>> std::map<std::string, std::optional<StorePath>>
LocalStore::queryPartialDerivationOutputMap(const StorePath& path_) LocalStore::queryDerivationOutputMapNoResolve(const StorePath& path_)
{ {
auto path = path_; auto path = path_;
auto outputs = retrySQLite<std::map<std::string, std::optional<StorePath>>>([&]() { auto outputs = retrySQLite<std::map<std::string, std::optional<StorePath>>>([&]() {
@ -924,20 +898,9 @@ LocalStore::queryPartialDerivationOutputMap(const StorePath& path_)
if (!settings.isExperimentalFeatureEnabled("ca-derivations")) if (!settings.isExperimentalFeatureEnabled("ca-derivations"))
return outputs; return outputs;
auto drv = readDerivation(path); auto drv = readInvalidDerivation(path);
auto drvHashes = staticOutputHashes(*this, drv);
auto resolvedDrv = cachedResolve(*this, path); for (auto& [outputName, hash] : drvHashes) {
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 realisation = queryRealisation(DrvOutput{hash, outputName}); auto realisation = queryRealisation(DrvOutput{hash, outputName});
if (realisation) if (realisation)
outputs.insert_or_assign(outputName, realisation->outPath); outputs.insert_or_assign(outputName, realisation->outPath);

View file

@ -127,7 +127,7 @@ public:
StorePathSet queryValidDerivers(const StorePath & path) override; 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; std::optional<StorePath> queryPathFromHashPart(const std::string & hashPart) override;

View file

@ -366,6 +366,29 @@ bool Store::PathInfoCacheValue::isKnownNow()
return std::chrono::steady_clock::now() < time_point + ttl; 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) { OutputPathMap Store::queryDerivationOutputMap(const StorePath & path) {
auto resp = queryPartialDerivationOutputMap(path); auto resp = queryPartialDerivationOutputMap(path);
OutputPathMap result; OutputPathMap result;
@ -730,14 +753,14 @@ void Store::buildPaths(const std::vector<StorePathWithOutputs> & paths, BuildMod
for (auto & path : paths) { for (auto & path : paths) {
if (path.path.isDerivation()) { if (path.path.isDerivation()) {
if (settings.isExperimentalFeatureEnabled("ca-derivations")) { auto outPaths = queryPartialDerivationOutputMap(path.path);
for (auto & outputName : path.outputs) { for (auto & outputName : path.outputs) {
if (!queryRealisation({path.path, outputName})) auto currentOutputPathIter = outPaths.find(outputName);
if (currentOutputPathIter == outPaths.end() ||
!currentOutputPathIter->second ||
!isValidPath(*currentOutputPathIter->second))
unsupported("buildPaths"); unsupported("buildPaths");
} }
} else
unsupported("buildPaths");
} else } else
paths2.insert(path.path); paths2.insert(path.path);
} }

View file

@ -416,8 +416,13 @@ public:
/* Query the mapping outputName => outputPath for the given derivation. All /* Query the mapping outputName => outputPath for the given derivation. All
outputs are mentioned so ones mising the mapping are mapped to outputs are mentioned so ones mising the mapping are mapped to
`std::nullopt`. */ `std::nullopt`. */
virtual std::map<std::string, std::optional<StorePath>> queryPartialDerivationOutputMap(const StorePath & path) virtual std::map<std::string, std::optional<StorePath>> queryPartialDerivationOutputMap(const StorePath & path);
{ unsupported("queryPartialDerivationOutputMap"); }
/*
* 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. /* Query the mapping outputName=>outputPath for the given derivation.
Assume every output has a mapping and throw an exception otherwise. */ Assume every output has a mapping and throw an exception otherwise. */