From d084c1cb410679ba2f80998e2263c60108aa0115 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 10 Apr 2024 12:46:21 +0200 Subject: [PATCH] Remove the "locked" flag from the fetcher cache This also reworks the Mercurial fetcher (which was still using the old cache interface) to have two distinct cache mappings: * A ref-to-rev mapping, which is store-independent. * A rev-to-store-path mapping. --- src/libfetchers/cache.cc | 9 ++-- src/libfetchers/cache.hh | 3 +- src/libfetchers/fetch-to-store.cc | 3 +- src/libfetchers/tarball.cc | 3 +- src/libfetchers/unix/mercurial.cc | 84 ++++++++++++++---------------- tests/functional/fetchMercurial.sh | 1 + 6 files changed, 45 insertions(+), 58 deletions(-) diff --git a/src/libfetchers/cache.cc b/src/libfetchers/cache.cc index e071b4717..83962856c 100644 --- a/src/libfetchers/cache.cc +++ b/src/libfetchers/cache.cc @@ -14,7 +14,7 @@ create table if not exists Cache ( input text not null, info text not null, path text not null, - immutable integer not null, + immutable integer not null, /* obsolete */ timestamp integer not null, primary key (input) ); @@ -45,7 +45,7 @@ struct CacheImpl : Cache state->db.exec(schema); state->add.create(state->db, - "insert or replace into Cache(input, info, path, immutable, timestamp) values (?, ?, ?, ?, ?)"); + "insert or replace into Cache(input, info, path, immutable, timestamp) values (?, ?, ?, false, ?)"); state->lookup.create(state->db, "select info, path, immutable, timestamp from Cache where input = ?"); @@ -59,7 +59,6 @@ struct CacheImpl : Cache (attrsToJSON(inAttrs).dump()) (attrsToJSON(infoAttrs).dump()) ("") // no path - (false) (time(0)).exec(); } @@ -109,14 +108,12 @@ struct CacheImpl : Cache Store & store, const Attrs & inAttrs, const Attrs & infoAttrs, - const StorePath & storePath, - bool locked) override + const StorePath & storePath) override { _state.lock()->add.use() (attrsToJSON(inAttrs).dump()) (attrsToJSON(infoAttrs).dump()) (store.printStorePath(storePath)) - (locked) (time(0)).exec(); } diff --git a/src/libfetchers/cache.hh b/src/libfetchers/cache.hh index 791d77025..5e05d7af8 100644 --- a/src/libfetchers/cache.hh +++ b/src/libfetchers/cache.hh @@ -53,8 +53,7 @@ struct Cache Store & store, const Attrs & inAttrs, const Attrs & infoAttrs, - const StorePath & storePath, - bool locked) = 0; + const StorePath & storePath) = 0; virtual std::optional> lookup( Store & store, diff --git a/src/libfetchers/fetch-to-store.cc b/src/libfetchers/fetch-to-store.cc index 398286065..4156302c4 100644 --- a/src/libfetchers/fetch-to-store.cc +++ b/src/libfetchers/fetch-to-store.cc @@ -47,10 +47,9 @@ StorePath fetchToStore( name, *path.accessor, path.path, method, HashAlgorithm::SHA256, {}, filter2, repair); if (cacheKey && mode == FetchMode::Copy) - fetchers::getCache()->add(store, *cacheKey, {}, storePath, true); + fetchers::getCache()->add(store, *cacheKey, {}, storePath); return storePath; } - } diff --git a/src/libfetchers/tarball.cc b/src/libfetchers/tarball.cc index a1f934c35..fd59c1132 100644 --- a/src/libfetchers/tarball.cc +++ b/src/libfetchers/tarball.cc @@ -99,8 +99,7 @@ DownloadFileResult downloadFile( *store, inAttrs, infoAttrs, - *storePath, - false); + *storePath); } return { diff --git a/src/libfetchers/unix/mercurial.cc b/src/libfetchers/unix/mercurial.cc index a2702338f..783e338bf 100644 --- a/src/libfetchers/unix/mercurial.cc +++ b/src/libfetchers/unix/mercurial.cc @@ -224,22 +224,17 @@ struct MercurialInputScheme : InputScheme if (!input.getRef()) input.attrs.insert_or_assign("ref", "default"); - auto checkHashAlgorithm = [&](const std::optional & hash) + auto revInfoCacheKey = [&](const Hash & rev) { - if (hash.has_value() && hash->algo != HashAlgorithm::SHA1) - throw Error("Hash '%s' is not supported by Mercurial. Only sha1 is supported.", hash->to_string(HashFormat::Base16, true)); - }; + if (rev.algo != HashAlgorithm::SHA1) + throw Error("Hash '%s' is not supported by Mercurial. Only sha1 is supported.", rev.to_string(HashFormat::Base16, true)); - - auto getLockedAttrs = [&]() - { - checkHashAlgorithm(input.getRev()); - - return Attrs({ - {"type", "hg"}, + return Attrs{ + {"_what", "hgRev"}, + {"store", store->storeDir}, {"name", name}, - {"rev", input.getRev()->gitRev()}, - }); + {"rev", input.getRev()->gitRev()} + }; }; auto makeResult = [&](const Attrs & infoAttrs, const StorePath & storePath) -> StorePath @@ -250,26 +245,22 @@ struct MercurialInputScheme : InputScheme return storePath; }; - if (input.getRev()) { - if (auto res = getCache()->lookup(*store, getLockedAttrs())) - return makeResult(res->first, std::move(res->second)); + /* Check the cache for the most recent rev for this URL/ref. */ + Attrs refToRevCacheKey{ + {"_what", "hgRefToRev"}, + {"url", actualUrl}, + {"ref", *input.getRef()} + }; + + if (!input.getRev()) { + if (auto res = getCache()->lookupWithTTL(refToRevCacheKey)) + input.attrs.insert_or_assign("rev", getRevAttr(*res, "rev").gitRev()); } - auto revOrRef = input.getRev() ? input.getRev()->gitRev() : *input.getRef(); - - Attrs unlockedAttrs({ - {"type", "hg"}, - {"name", name}, - {"url", actualUrl}, - {"ref", *input.getRef()}, - }); - - if (auto res = getCache()->lookup(*store, unlockedAttrs)) { - auto rev2 = Hash::parseAny(getStrAttr(res->first, "rev"), HashAlgorithm::SHA1); - if (!input.getRev() || input.getRev() == rev2) { - input.attrs.insert_or_assign("rev", rev2.gitRev()); - return makeResult(res->first, std::move(res->second)); - } + /* If we have a rev, check if we have a cached store path. */ + if (auto rev = input.getRev()) { + if (auto res = getCache()->lookupExpired(*store, revInfoCacheKey(*rev))) + return makeResult(res->infoAttrs, res->storePath); } Path cacheDir = fmt("%s/nix/hg/%s", getCacheDir(), hashString(HashAlgorithm::SHA256, actualUrl).to_string(HashFormat::Nix32, false)); @@ -302,21 +293,29 @@ struct MercurialInputScheme : InputScheme } } + /* Fetch the remote rev or ref. */ auto tokens = tokenizeString>( - runHg({ "log", "-R", cacheDir, "-r", revOrRef, "--template", "{node} {rev} {branch}" })); + runHg({ + "log", "-R", cacheDir, + "-r", input.getRev() ? input.getRev()->gitRev() : *input.getRef(), + "--template", "{node} {rev} {branch}" + })); assert(tokens.size() == 3); - input.attrs.insert_or_assign("rev", Hash::parseAny(tokens[0], HashAlgorithm::SHA1).gitRev()); + auto rev = Hash::parseAny(tokens[0], HashAlgorithm::SHA1); + input.attrs.insert_or_assign("rev", rev.gitRev()); auto revCount = std::stoull(tokens[1]); input.attrs.insert_or_assign("ref", tokens[2]); - if (auto res = getCache()->lookup(*store, getLockedAttrs())) - return makeResult(res->first, std::move(res->second)); + /* Now that we have the rev, check the cache again for a + cached store path. */ + if (auto res = getCache()->lookupExpired(*store, revInfoCacheKey(rev))) + return makeResult(res->infoAttrs, res->storePath); Path tmpDir = createTempDir(); AutoDelete delTmpDir(tmpDir, true); - runHg({ "archive", "-R", cacheDir, "-r", input.getRev()->gitRev(), tmpDir }); + runHg({ "archive", "-R", cacheDir, "-r", rev.gitRev(), tmpDir }); deletePath(tmpDir + "/.hg_archival.txt"); @@ -324,24 +323,17 @@ struct MercurialInputScheme : InputScheme auto storePath = store->addToStore(name, accessor, CanonPath { tmpDir }); Attrs infoAttrs({ - {"rev", input.getRev()->gitRev()}, {"revCount", (uint64_t) revCount}, }); if (!origRev) - getCache()->add( - *store, - unlockedAttrs, - infoAttrs, - storePath, - false); + getCache()->upsert(refToRevCacheKey, {{"rev", rev.gitRev()}}); getCache()->add( *store, - getLockedAttrs(), + revInfoCacheKey(rev), infoAttrs, - storePath, - true); + storePath); return makeResult(infoAttrs, std::move(storePath)); } diff --git a/tests/functional/fetchMercurial.sh b/tests/functional/fetchMercurial.sh index e133df1f8..9f7cef7b2 100644 --- a/tests/functional/fetchMercurial.sh +++ b/tests/functional/fetchMercurial.sh @@ -101,6 +101,7 @@ path4=$(nix eval --impure --refresh --raw --expr "(builtins.fetchMercurial file: [[ $path2 = $path4 ]] echo paris > $repo/hello + # Passing a `name` argument should be reflected in the output path path5=$(nix eval -vvvvv --impure --refresh --raw --expr "(builtins.fetchMercurial { url = \"file://$repo\"; name = \"foo\"; } ).outPath") [[ $path5 =~ -foo$ ]]