From fac0c2d54a6b04175b40009506f2720d2594ed4e Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 29 May 2020 16:19:48 -0400 Subject: [PATCH] Remove addToStore variant as requested by `FIXME` The idea is it's always more flexible to consumer a `Source` than a plain string, and it might even reduce memory consumption. I also looked at `addToStoreFromDump` with its `// FIXME: remove?`, but the worked needed for that is far more up for interpretation, so I punted for now. --- src/libfetchers/tarball.cc | 3 ++- src/libstore/binary-cache-store.cc | 11 ++++++++--- src/libstore/binary-cache-store.hh | 2 +- src/libstore/export-import.cc | 5 ++++- src/libstore/store-api.cc | 15 --------------- src/libstore/store-api.hh | 7 +------ src/nix/add-to-store.cc | 6 ++++-- 7 files changed, 20 insertions(+), 29 deletions(-) diff --git a/src/libfetchers/tarball.cc b/src/libfetchers/tarball.cc index bf2b2a5ff..b6e57379b 100644 --- a/src/libfetchers/tarball.cc +++ b/src/libfetchers/tarball.cc @@ -71,7 +71,8 @@ DownloadFileResult downloadFile( info.narHash = hashString(htSHA256, *sink.s); info.narSize = sink.s->size(); info.ca = makeFixedOutputCA(FileIngestionMethod::Flat, hash); - store->addToStore(info, sink.s, NoRepair, NoCheckSigs); + auto source = StringSource { *sink.s }; + store->addToStore(info, source, NoRepair, NoCheckSigs); storePath = std::move(info.path); } diff --git a/src/libstore/binary-cache-store.cc b/src/libstore/binary-cache-store.cc index f13736c58..357962007 100644 --- a/src/libstore/binary-cache-store.cc +++ b/src/libstore/binary-cache-store.cc @@ -113,9 +113,12 @@ void BinaryCacheStore::writeNarInfo(ref narInfo) diskCache->upsertNarInfo(getUri(), hashPart, std::shared_ptr(narInfo)); } -void BinaryCacheStore::addToStore(const ValidPathInfo & info, const ref & nar, +void BinaryCacheStore::addToStore(const ValidPathInfo & info, Source & narSource, RepairFlag repair, CheckSigsFlag checkSigs, std::shared_ptr accessor) { + // FIXME: See if we can use the original source to reduce memory usage. + auto nar = make_ref(narSource.drain()); + if (!repair && isValidPath(info.path)) return; /* Verify that all references are valid. This may do some .narinfo @@ -347,7 +350,8 @@ StorePath BinaryCacheStore::addToStore(const string & name, const Path & srcPath ValidPathInfo info(makeFixedOutputPath(method, h, name)); - addToStore(info, sink.s, repair, CheckSigs, nullptr); + auto source = StringSource { *sink.s }; + addToStore(info, source, repair, CheckSigs, nullptr); return std::move(info.path); } @@ -361,7 +365,8 @@ StorePath BinaryCacheStore::addTextToStore(const string & name, const string & s if (repair || !isValidPath(info.path)) { StringSink sink; dumpString(s, sink); - addToStore(info, sink.s, repair, CheckSigs, nullptr); + auto source = StringSource { *sink.s }; + addToStore(info, source, repair, CheckSigs, nullptr); } return std::move(info.path); diff --git a/src/libstore/binary-cache-store.hh b/src/libstore/binary-cache-store.hh index 4ff5609ed..52ef8aa7a 100644 --- a/src/libstore/binary-cache-store.hh +++ b/src/libstore/binary-cache-store.hh @@ -74,7 +74,7 @@ public: std::optional queryPathFromHashPart(const std::string & hashPart) override { unsupported("queryPathFromHashPart"); } - void addToStore(const ValidPathInfo & info, const ref & nar, + void addToStore(const ValidPathInfo & info, Source & narSource, RepairFlag repair, CheckSigsFlag checkSigs, std::shared_ptr accessor) override; diff --git a/src/libstore/export-import.cc b/src/libstore/export-import.cc index 4692d1a7b..f0d01a240 100644 --- a/src/libstore/export-import.cc +++ b/src/libstore/export-import.cc @@ -1,3 +1,4 @@ +#include "serialise.hh" #include "store-api.hh" #include "archive.hh" #include "worker-protocol.hh" @@ -100,7 +101,9 @@ StorePaths Store::importPaths(Source & source, std::shared_ptr acces if (readInt(source) == 1) readString(source); - addToStore(info, tee.source.data, NoRepair, checkSigs, accessor); + // Can't use underlying source, which would have been exhausted + auto source = StringSource { *tee.source.data }; + addToStore(info, source, NoRepair, checkSigs, accessor); res.push_back(info.path.clone()); } diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index d479b86cb..095363d0c 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -840,21 +840,6 @@ std::string makeFixedOutputCA(FileIngestionMethod recursive, const Hash & hash) } -void Store::addToStore(const ValidPathInfo & info, Source & narSource, - RepairFlag repair, CheckSigsFlag checkSigs, - std::shared_ptr accessor) -{ - addToStore(info, make_ref(narSource.drain()), repair, checkSigs, accessor); -} - -void Store::addToStore(const ValidPathInfo & info, const ref & nar, - RepairFlag repair, CheckSigsFlag checkSigs, - std::shared_ptr accessor) -{ - StringSource source(*nar); - addToStore(info, source, repair, checkSigs, accessor); -} - } diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 3def209a6..b1e25fc7d 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -450,12 +450,7 @@ public: /* Import a path into the store. */ virtual void addToStore(const ValidPathInfo & info, Source & narSource, RepairFlag repair = NoRepair, CheckSigsFlag checkSigs = CheckSigs, - std::shared_ptr accessor = 0); - - // FIXME: remove - virtual void addToStore(const ValidPathInfo & info, const ref & nar, - RepairFlag repair = NoRepair, CheckSigsFlag checkSigs = CheckSigs, - std::shared_ptr accessor = 0); + std::shared_ptr accessor = 0) = 0; /* Copy the contents of a path to the store and register the validity the resulting path. The resulting path is returned. diff --git a/src/nix/add-to-store.cc b/src/nix/add-to-store.cc index 4b4ba81cb..f43f774c1 100644 --- a/src/nix/add-to-store.cc +++ b/src/nix/add-to-store.cc @@ -50,8 +50,10 @@ struct CmdAddToStore : MixDryRun, StoreCommand info.narSize = sink.s->size(); info.ca = makeFixedOutputCA(FileIngestionMethod::Recursive, info.narHash); - if (!dryRun) - store->addToStore(info, sink.s); + if (!dryRun) { + auto source = StringSource { *sink.s }; + store->addToStore(info, source); + } logger->stdout("%s", store->printStorePath(info.path)); }