From aa00a5a8c9ba7b74c2102b8dc7fc4c3ed58606d8 Mon Sep 17 00:00:00 2001 From: Alois Wohlschlager Date: Wed, 12 Jun 2024 19:42:38 +0200 Subject: [PATCH 01/22] libfetchers: represent unfetched submodules consistently Unfetched submodules are included as empty directories in archives, so they end up as such in the store when fetched in clean mode. Make sure the same happens in dirty mode too. Fortunately, they are already correctly represented in the ls-files output, so we just need to make sure to include the empty directory in our filter. Fixes: https://github.com/NixOS/nix/issues/6247 Change-Id: I60d06ff360cfa305d081b920838c893c06da801c --- src/libfetchers/git.cc | 2 +- tests/functional/fetchGitSubmodules.sh | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc index 07cbc781c..2817fde23 100644 --- a/src/libfetchers/git.cc +++ b/src/libfetchers/git.cc @@ -232,7 +232,7 @@ std::pair fetchFromWorkdir(ref store, Input & input, co if (S_ISDIR(st.st_mode)) { auto prefix = file + "/"; auto i = files.lower_bound(prefix); - return i != files.end() && (*i).starts_with(prefix); + return (i != files.end() && (*i).starts_with(prefix)) || files.count(file); } return files.count(file); diff --git a/tests/functional/fetchGitSubmodules.sh b/tests/functional/fetchGitSubmodules.sh index df81232e5..3ff6697fd 100644 --- a/tests/functional/fetchGitSubmodules.sh +++ b/tests/functional/fetchGitSubmodules.sh @@ -40,6 +40,7 @@ initGitRepo $rootRepo git -C $rootRepo submodule init git -C $rootRepo submodule add $subRepo sub git -C $rootRepo add sub +r0=$(nix eval --impure --raw --expr "(builtins.fetchGit { url = file://$rootRepo; }).outPath") git -C $rootRepo commit -m "Add submodule" rev=$(git -C $rootRepo rev-parse HEAD) @@ -48,6 +49,7 @@ r1=$(nix eval --raw --expr "(builtins.fetchGit { url = file://$rootRepo; rev = \ r2=$(nix eval --raw --expr "(builtins.fetchGit { url = file://$rootRepo; rev = \"$rev\"; submodules = false; }).outPath") r3=$(nix eval --raw --expr "(builtins.fetchGit { url = file://$rootRepo; rev = \"$rev\"; submodules = true; }).outPath") +[[ $r0 == $r1 ]] # verify that unfetched submodules result in empty directories in dirty mode too [[ $r1 == $r2 ]] [[ $r2 != $r3 ]] From 66a9fbb7ffa781f46e246de08700739fa775650c Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Sat, 15 Jun 2024 13:10:03 -0700 Subject: [PATCH 02/22] libstore: fix queryValidPaths concurrency The lock usage was obviously wrong so it was entirely serialized. This has the predicted speedups, the only question is whether it is sound because it's exposing a bunch of new code to actual concurrency. I did audit all the stores' queryPathInfoUncached implementations and they all look *intended* to be thread safe, but whether that is actually sound or not: lol lmao. I am highly confident in the s3 one because it is calling s3 sdk methods that are thread safe and has no actual state. Others are using Pool and look to be *supposed* to be thread safe, but unsure if they actually are. Change-Id: I0369152a510e878b5ac56c9ac956a98d48cd5fef --- src/libstore/store-api.cc | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 55083e834..4e1fb4bd5 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -803,17 +803,31 @@ StorePathSet Store::queryValidPaths(const StorePathSet & paths, SubstituteFlag m auto doQuery = [&](const StorePath & path) { checkInterrupt(); - auto state(state_.lock()); + + bool exists = false; + std::exception_ptr newExc{}; + try { - auto info = queryPathInfo(path); - state->valid.insert(path); + queryPathInfo(path); + exists = true; } catch (InvalidPath &) { } catch (...) { - state->exc = std::current_exception(); + newExc = std::current_exception(); + } + + { + auto state(state_.lock()); + + if (exists) { + state->valid.insert(path); + } + if (newExc != nullptr) { + state->exc = newExc; + } + assert(state->left); + if (!--state->left) + wakeup.notify_one(); } - assert(state->left); - if (!--state->left) - wakeup.notify_one(); }; for (auto & path : paths) From 3425e90d76a311c6639b7c040f6a9e14856e18f4 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Fri, 3 May 2024 21:46:07 +0200 Subject: [PATCH 03/22] libstore: BinaryCacheStore::getFile{ -> Contents} if we want have getFile return a source instead of consuming a sink we'll have to disambiguate this overload another way, eg like this. Change-Id: Ia26de2020c309a37e7ccc3775c1ad1f32e0a778b --- src/libstore/binary-cache-store.cc | 12 ++++++------ src/libstore/binary-cache-store.hh | 2 +- src/libstore/http-binary-cache-store.cc | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/libstore/binary-cache-store.cc b/src/libstore/binary-cache-store.cc index 7df55a32d..07c722016 100644 --- a/src/libstore/binary-cache-store.cc +++ b/src/libstore/binary-cache-store.cc @@ -38,7 +38,7 @@ void BinaryCacheStore::init() { std::string cacheInfoFile = "nix-cache-info"; - auto cacheInfo = getFile(cacheInfoFile); + auto cacheInfo = getFileContents(cacheInfoFile); if (!cacheInfo) { upsertFile(cacheInfoFile, "StoreDir: " + storeDir + "\n", "text/x-nix-cache-info"); } else { @@ -69,10 +69,10 @@ void BinaryCacheStore::upsertFile(const std::string & path, void BinaryCacheStore::getFile(const std::string & path, Sink & sink) { - sink(*getFile(path)); + sink(*getFileContents(path)); } -std::optional BinaryCacheStore::getFile(const std::string & path) +std::optional BinaryCacheStore::getFileContents(const std::string & path) { StringSink sink; try { @@ -359,7 +359,7 @@ std::shared_ptr BinaryCacheStore::queryPathInfoUncached(con auto narInfoFile = narInfoFileFor(storePath); - auto data = getFile(narInfoFile); + auto data = getFileContents(narInfoFile); if (!data) return nullptr; @@ -446,7 +446,7 @@ std::shared_ptr BinaryCacheStore::queryRealisationUncached(co { auto outputInfoFilePath = realisationsPrefix + "/" + id.to_string() + ".doi"; - auto data = getFile(outputInfoFilePath); + auto data = getFileContents(outputInfoFilePath); if (!data) return {}; auto realisation = Realisation::fromJSON( @@ -486,7 +486,7 @@ std::optional BinaryCacheStore::getBuildLogExact(const StorePath & debug("fetching build log from binary cache '%s/%s'", getUri(), logPath); - return getFile(logPath); + return getFileContents(logPath); } void BinaryCacheStore::addBuildLog(const StorePath & drvPath, std::string_view log) diff --git a/src/libstore/binary-cache-store.hh b/src/libstore/binary-cache-store.hh index 510965d12..cd963fbf9 100644 --- a/src/libstore/binary-cache-store.hh +++ b/src/libstore/binary-cache-store.hh @@ -85,7 +85,7 @@ public: */ virtual void getFile(const std::string & path, Sink & sink); - virtual std::optional getFile(const std::string & path); + virtual std::optional getFileContents(const std::string & path); public: diff --git a/src/libstore/http-binary-cache-store.cc b/src/libstore/http-binary-cache-store.cc index 8cbb50ee9..f64da2569 100644 --- a/src/libstore/http-binary-cache-store.cc +++ b/src/libstore/http-binary-cache-store.cc @@ -164,7 +164,7 @@ protected: } } - std::optional getFile(const std::string & path) override + std::optional getFileContents(const std::string & path) override { checkEnabled(); From 67f778670c085615470b67eb3c54885b8c2d482e Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Fri, 3 May 2024 22:23:02 +0200 Subject: [PATCH 04/22] libutil: add makeDecompressionSource Change-Id: Iac7f24d79e24417436b9b5cbefd6af051aeea0a6 --- src/libutil/compression.cc | 21 ++++++++++++++++----- src/libutil/compression.hh | 1 + tests/unit/libutil/compression.cc | 11 ++++++----- 3 files changed, 23 insertions(+), 10 deletions(-) diff --git a/src/libutil/compression.cc b/src/libutil/compression.cc index e78d76500..d93a1b1d6 100644 --- a/src/libutil/compression.cc +++ b/src/libutil/compression.cc @@ -192,11 +192,9 @@ struct BrotliDecompressionSource : Source std::string decompress(const std::string & method, std::string_view in) { - StringSink ssink; - auto sink = makeDecompressionSink(method, ssink); - (*sink)(in); - sink->finish(); - return std::move(ssink.s); + StringSource src{in}; + auto filter = makeDecompressionSource(method, src); + return filter->drain(); } std::unique_ptr makeDecompressionSink(const std::string & method, Sink & nextSink) @@ -224,6 +222,19 @@ std::unique_ptr makeDecompressionSink(const std::string & method, Si }); } +std::unique_ptr makeDecompressionSource(const std::string & method, Source & inner) +{ + if (method == "none" || method == "") { + return std::make_unique([&](char * data, size_t len) { + return inner.read(data, len); + }); + } else if (method == "br") { + return std::make_unique(inner); + } else { + return std::make_unique(inner); + } +} + struct BrotliCompressionSink : ChunkedCompressionSink { Sink & nextSink; diff --git a/src/libutil/compression.hh b/src/libutil/compression.hh index 4e53a7b3c..8affdddd6 100644 --- a/src/libutil/compression.hh +++ b/src/libutil/compression.hh @@ -19,6 +19,7 @@ struct CompressionSink : BufferedSink, FinishSink std::string decompress(const std::string & method, std::string_view in); std::unique_ptr makeDecompressionSink(const std::string & method, Sink & nextSink); +std::unique_ptr makeDecompressionSource(const std::string & method, Source & inner); std::string compress(const std::string & method, std::string_view in, const bool parallel = false, int level = -1); diff --git a/tests/unit/libutil/compression.cc b/tests/unit/libutil/compression.cc index bbbf3500f..0542e7d33 100644 --- a/tests/unit/libutil/compression.cc +++ b/tests/unit/libutil/compression.cc @@ -81,16 +81,17 @@ namespace nix { } TEST(makeCompressionSink, compressAndDecompress) { - StringSink strSink; auto inputString = "slfja;sljfklsa;jfklsjfkl;sdjfkl;sadjfkl;sdjf;lsdfjsadlf"; - auto decompressionSink = makeDecompressionSink("bzip2", strSink); - auto sink = makeCompressionSink("bzip2", *decompressionSink); + StringSink strSink; + auto sink = makeCompressionSink("bzip2", strSink); (*sink)(inputString); sink->finish(); - decompressionSink->finish(); - ASSERT_STREQ(strSink.s.c_str(), inputString); + StringSource strSource{strSink.s}; + auto decompressionSource = makeDecompressionSource("bzip2", strSource); + + ASSERT_STREQ(decompressionSource->drain().c_str(), inputString); } } From 11f4a5bc7eca8a4cca2ae9f3d83b69cd497933f8 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Fri, 3 May 2024 22:53:24 +0200 Subject: [PATCH 05/22] libutil: return a source from readFile don't consume a sink, return a source instead. the only reason to not do this is a very slight reduction in dynamic allocations, but since we are going to *at least* do disk io that will not be a lot of overhead anyway Change-Id: Iae2f879ec64c3c3ac1d5310eeb6a85e696d4614a --- src/libstore/binary-cache-store.cc | 2 +- src/libstore/build/local-derivation-goal.cc | 4 ++-- src/libstore/local-binary-cache-store.cc | 2 +- src/libstore/local-store.cc | 4 ++-- src/libstore/store-api.cc | 2 +- src/libutil/file-system.cc | 9 +++++++-- src/libutil/file-system.hh | 3 ++- src/libutil/hash.cc | 2 +- src/nix/add-to-store.cc | 2 +- src/nix/hash.cc | 2 +- 10 files changed, 19 insertions(+), 13 deletions(-) diff --git a/src/libstore/binary-cache-store.cc b/src/libstore/binary-cache-store.cc index 07c722016..d6ded0a24 100644 --- a/src/libstore/binary-cache-store.cc +++ b/src/libstore/binary-cache-store.cc @@ -385,7 +385,7 @@ StorePath BinaryCacheStore::addToStore( if (method == FileIngestionMethod::Recursive) { dumpPath(srcPath, sink, filter); } else { - readFile(srcPath, sink); + readFileSource(srcPath)->drainInto(sink); } auto h = sink.finish().first; diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 7066f5c93..bae821266 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -2486,7 +2486,7 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs() HashModuloSink caSink { outputHash.hashType, oldHashPart }; std::visit(overloaded { [&](const TextIngestionMethod &) { - readFile(actualPath, caSink); + readFileSource(actualPath)->drainInto(caSink); }, [&](const FileIngestionMethod & m2) { switch (m2) { @@ -2494,7 +2494,7 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs() dumpPath(actualPath, caSink); break; case FileIngestionMethod::Flat: - readFile(actualPath, caSink); + readFileSource(actualPath)->drainInto(caSink); break; } }, diff --git a/src/libstore/local-binary-cache-store.cc b/src/libstore/local-binary-cache-store.cc index 5684dcd80..5f6730476 100644 --- a/src/libstore/local-binary-cache-store.cc +++ b/src/libstore/local-binary-cache-store.cc @@ -71,7 +71,7 @@ protected: void getFile(const std::string & path, Sink & sink) override { try { - readFile(binaryCacheDir + "/" + path, sink); + readFileSource(binaryCacheDir + "/" + path)->drainInto(sink); } catch (SysError & e) { if (e.errNo == ENOENT) throw NoSuchBinaryCacheFile("file '%s' does not exist in binary cache", path); diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 5e79630c6..6f441a0a1 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1890,7 +1890,7 @@ ContentAddress LocalStore::hashCAPath( HashModuloSink caSink ( hashType, std::string(pathHash) ); std::visit(overloaded { [&](const TextIngestionMethod &) { - readFile(path, caSink); + readFileSource(path)->drainInto(caSink); }, [&](const FileIngestionMethod & m2) { switch (m2) { @@ -1898,7 +1898,7 @@ ContentAddress LocalStore::hashCAPath( dumpPath(path, caSink); break; case FileIngestionMethod::Flat: - readFile(path, caSink); + readFileSource(path)->drainInto(caSink); break; } }, diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 55083e834..e954acff3 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -279,7 +279,7 @@ StorePath Store::addToStore( if (method == FileIngestionMethod::Recursive) dumpPath(srcPath, sink, filter); else - readFile(srcPath, sink); + readFileSource(srcPath)->drainInto(sink); }); return addToStoreFromDump(*source, name, method, hashAlgo, repair, references); } diff --git a/src/libutil/file-system.cc b/src/libutil/file-system.cc index d573b22b4..f51f3c092 100644 --- a/src/libutil/file-system.cc +++ b/src/libutil/file-system.cc @@ -289,12 +289,17 @@ std::string readFile(const Path & path) } -void readFile(const Path & path, Sink & sink) +box_ptr readFileSource(const Path & path) { AutoCloseFD fd{open(path.c_str(), O_RDONLY | O_CLOEXEC)}; if (!fd) throw SysError("opening file '%s'", path); - drainFD(fd.get(), sink); + + struct FileSource : FdSource { + AutoCloseFD fd; + explicit FileSource(AutoCloseFD fd) : FdSource(fd.get()), fd(std::move(fd)) {} + }; + return make_box_ptr(std::move(fd)); } diff --git a/src/libutil/file-system.hh b/src/libutil/file-system.hh index 6c1923d55..64d884227 100644 --- a/src/libutil/file-system.hh +++ b/src/libutil/file-system.hh @@ -5,6 +5,7 @@ * Utiltities for working with the file sytem and file paths. */ +#include "box_ptr.hh" #include "types.hh" #include "file-descriptor.hh" @@ -142,7 +143,7 @@ unsigned char getFileType(const Path & path); * Read the contents of a file into a string. */ std::string readFile(const Path & path); -void readFile(const Path & path, Sink & sink); +box_ptr readFileSource(const Path & path); /** * Write a string to a file. diff --git a/src/libutil/hash.cc b/src/libutil/hash.cc index 006b5000c..822fa150e 100644 --- a/src/libutil/hash.cc +++ b/src/libutil/hash.cc @@ -324,7 +324,7 @@ Hash hashString(HashType ht, std::string_view s) Hash hashFile(HashType ht, const Path & path) { HashSink sink(ht); - readFile(path, sink); + readFileSource(path)->drainInto(sink); return sink.finish().first; } diff --git a/src/nix/add-to-store.cc b/src/nix/add-to-store.cc index 39e5cc99d..7dbbbcc56 100644 --- a/src/nix/add-to-store.cc +++ b/src/nix/add-to-store.cc @@ -37,7 +37,7 @@ struct CmdAddToStore : MixDryRun, StoreCommand Hash hash = narHash; if (ingestionMethod == FileIngestionMethod::Flat) { HashSink hsink(htSHA256); - readFile(path, hsink); + readFileSource(path)->drainInto(hsink); hash = hsink.finish().first; } diff --git a/src/nix/hash.cc b/src/nix/hash.cc index 9feca9345..66c5516e7 100644 --- a/src/nix/hash.cc +++ b/src/nix/hash.cc @@ -85,7 +85,7 @@ struct CmdHashBase : Command switch (mode) { case FileIngestionMethod::Flat: - readFile(path, *hashSink); + readFileSource(path)->drainInto(*hashSink); break; case FileIngestionMethod::Recursive: dumpPath(path, *hashSink); From c55dcc6c13b864dc613a0a6ba51e0b897868f4b4 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Thu, 9 May 2024 21:22:48 +0200 Subject: [PATCH 06/22] filetransfer: return a Source from download() without this we will not be able to get rid of makeDecompressionSink, which in turn will be necessary to get rid of sourceToSink (since the libarchive archive wrapper *must* be a Source due to api limitations) Change-Id: Iccd3d333ba2cbcab49cb5a1d3125624de16bce27 --- src/libstore/builtins/fetchurl.cc | 2 +- src/libstore/filetransfer.cc | 140 +++++++++++++++--------- src/libstore/filetransfer.hh | 10 +- src/libstore/http-binary-cache-store.cc | 2 +- src/nix/prefetch.cc | 2 +- tests/unit/libstore/filetransfer.cc | 18 ++- 6 files changed, 109 insertions(+), 65 deletions(-) diff --git a/src/libstore/builtins/fetchurl.cc b/src/libstore/builtins/fetchurl.cc index 6bf46dad8..37d640fe4 100644 --- a/src/libstore/builtins/fetchurl.cc +++ b/src/libstore/builtins/fetchurl.cc @@ -41,7 +41,7 @@ void builtinFetchurl(const BasicDerivation & drv, const std::string & netrcData) auto decompressor = makeDecompressionSink( unpack && mainUrl.ends_with(".xz") ? "xz" : "none", sink); - fileTransfer->download(std::move(request), *decompressor); + fileTransfer->download(std::move(request))->drainInto(*decompressor); decompressor->finish(); }); diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index 065f38a0c..fcb947f96 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -686,16 +686,8 @@ struct curlFileTransfer : public FileTransfer ->callback.get_future(); } - void download(FileTransferRequest && request, Sink & sink) override + box_ptr download(FileTransferRequest && request) override { - /* Note: we can't call 'sink' via request.dataCallback, because - that would cause the sink to execute on the fileTransfer - thread. If 'sink' is a coroutine, this will fail. Also, if the - sink is expensive (e.g. one that does decompression and writing - to the Nix store), it would stall the download thread too much. - Therefore we use a buffer to communicate data between the - download thread and the calling thread. */ - struct State { bool done = false, failed = false; std::exception_ptr exc; @@ -705,13 +697,6 @@ struct curlFileTransfer : public FileTransfer auto _state = std::make_shared>(); - /* In case of an exception, wake up the download thread. */ - Finally finally([&]() { - auto state(_state->lock()); - state->failed |= std::uncaught_exceptions() != 0; - state->request.notify_one(); - }); - enqueueFileTransfer( request, [_state](std::exception_ptr ex) { @@ -750,50 +735,99 @@ struct curlFileTransfer : public FileTransfer } ); - std::unique_ptr decompressor; - - while (true) { - checkInterrupt(); - + struct InnerSource : Source + { + const std::shared_ptr> _state; std::string chunk; + std::string_view buffered; - /* Grab data if available, otherwise wait for the download - thread to wake us up. */ + explicit InnerSource(const std::shared_ptr> & state) : _state(state) {} + + ~InnerSource() { + // wake up the download thread if it's still going and have it abort auto state(_state->lock()); - - if (state->data.empty()) { - - if (state->done) { - if (state->exc) std::rethrow_exception(state->exc); - if (decompressor) { - decompressor->finish(); - } - return; - } - - state.wait(state->avail); - - if (state->data.empty()) continue; - } - - chunk = std::move(state->data); - /* Reset state->data after the move, since we check data.empty() */ - state->data = ""; - - if (!decompressor) { - decompressor = makeDecompressionSink(state->encoding, sink); - } - + state->failed |= !state->done; state->request.notify_one(); } - /* Flush the data to the sink and wake up the download thread - if it's blocked on a full buffer. We don't hold the state - lock while doing this to prevent blocking the download - thread if sink() takes a long time. */ - (*decompressor)(chunk); - } + void awaitData(Sync::Lock & state) + { + /* Grab data if available, otherwise wait for the download + thread to wake us up. */ + while (buffered.empty()) { + if (state->data.empty()) { + if (state->done) { + if (state->exc) { + std::rethrow_exception(state->exc); + } + return; + } + + state.wait(state->avail); + } + + chunk = std::move(state->data); + buffered = chunk; + state->request.notify_one(); + } + } + + size_t read(char * data, size_t len) override + { + auto readPartial = [this](char * data, size_t len) { + const auto available = std::min(len, buffered.size()); + memcpy(data, buffered.data(), available); + buffered.remove_prefix(available); + return available; + }; + size_t total = readPartial(data, len); + + while (total < len) { + { + auto state(_state->lock()); + awaitData(state); + } + const auto current = readPartial(data + total, len - total); + total += current; + if (total == 0 || current == 0) { + break; + } + } + + if (total == 0) { + throw EndOfFile("download finished"); + } + + return total; + } + }; + + struct DownloadSource : Source + { + InnerSource inner; + std::unique_ptr decompressor; + + explicit DownloadSource(const std::shared_ptr> & state) : inner(state) {} + + size_t read(char * data, size_t len) override + { + checkInterrupt(); + + if (!decompressor) { + auto state(inner._state->lock()); + inner.awaitData(state); + decompressor = makeDecompressionSource(state->encoding, inner); + } + + return decompressor->read(data, len); + } + }; + + auto source = make_box_ptr(_state); + auto lock(_state->lock()); + source->inner.awaitData(lock); + return source; } }; diff --git a/src/libstore/filetransfer.hh b/src/libstore/filetransfer.hh index 5d739112b..b2ee66312 100644 --- a/src/libstore/filetransfer.hh +++ b/src/libstore/filetransfer.hh @@ -1,6 +1,7 @@ #pragma once ///@file +#include "box_ptr.hh" #include "logging.hh" #include "serialise.hh" #include "types.hh" @@ -104,10 +105,13 @@ struct FileTransfer FileTransferResult transfer(const FileTransferRequest & request); /** - * Download a file, writing its data to a sink. The sink will be - * invoked on the thread of the caller. + * Download a file, returning its contents through a source. Will not return + * before the transfer has fully started, ensuring that any errors thrown by + * the setup phase (e.g. HTTP 404 or similar errors) are not postponed to be + * thrown by the returned source. The source will only throw errors detected + * during the transfer itself (decompression errors, connection drops, etc). */ - virtual void download(FileTransferRequest && request, Sink & sink) = 0; + virtual box_ptr download(FileTransferRequest && request) = 0; enum Error { NotFound, Forbidden, Misc, Transient, Interrupted }; }; diff --git a/src/libstore/http-binary-cache-store.cc b/src/libstore/http-binary-cache-store.cc index f64da2569..06297e2eb 100644 --- a/src/libstore/http-binary-cache-store.cc +++ b/src/libstore/http-binary-cache-store.cc @@ -155,7 +155,7 @@ protected: checkEnabled(); auto request(makeRequest(path)); try { - getFileTransfer()->download(std::move(request), sink); + getFileTransfer()->download(std::move(request))->drainInto(sink); } catch (FileTransferError & e) { if (e.error == FileTransfer::NotFound || e.error == FileTransfer::Forbidden) throw NoSuchBinaryCacheFile("file '%s' does not exist in binary cache '%s'", path, getUri()); diff --git a/src/nix/prefetch.cc b/src/nix/prefetch.cc index 2457e4cc8..cad70e726 100644 --- a/src/nix/prefetch.cc +++ b/src/nix/prefetch.cc @@ -98,7 +98,7 @@ std::tuple prefetchFile( FdSink sink(fd.get()); FileTransferRequest req(url); - getFileTransfer()->download(std::move(req), sink); + getFileTransfer()->download(std::move(req))->drainInto(sink); } /* Optionally unpack the file. */ diff --git a/tests/unit/libstore/filetransfer.cc b/tests/unit/libstore/filetransfer.cc index b60963a46..6e8cf3bbe 100644 --- a/tests/unit/libstore/filetransfer.cc +++ b/tests/unit/libstore/filetransfer.cc @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include @@ -136,7 +137,7 @@ TEST(FileTransfer, exceptionAbortsDownload) LambdaSink broken([](auto block) { throw Done(); }); - ASSERT_THROW(ft->download(FileTransferRequest("file:///dev/zero"), broken), Done); + ASSERT_THROW(ft->download(FileTransferRequest("file:///dev/zero"))->drainInto(broken), Done); // makeFileTransfer returns a ref<>, which cannot be cleared. since we also // can't default-construct it we'll have to overwrite it instead, but we'll @@ -159,16 +160,21 @@ TEST(FileTransfer, NOT_ON_DARWIN(reportsSetupErrors)) FileTransferError); } -TEST(FileTransfer, NOT_ON_DARWIN(reportsTransferError)) +TEST(FileTransfer, NOT_ON_DARWIN(defersFailures)) { - auto [port, srv] = serveHTTP("200 ok", "content-length: 100\r\n", [] { + auto [port, srv] = serveHTTP("200 ok", "content-length: 100000000\r\n", [] { std::this_thread::sleep_for(10ms); - return ""; + // just a bunch of data to fill the curl wrapper buffer, otherwise the + // initial wait for header data will also wait for the the response to + // complete (the source is only woken when curl returns data, and curl + // might only do so once its internal buffer has already been filled.) + return std::string(1024 * 1024, ' '); }); auto ft = makeFileTransfer(); FileTransferRequest req(fmt("http://[::1]:%d/index", port)); req.baseRetryTimeMs = 0; - ASSERT_THROW(ft->transfer(req), FileTransferError); + auto src = ft->download(std::move(req)); + ASSERT_THROW(src->drain(), FileTransferError); } TEST(FileTransfer, NOT_ON_DARWIN(handlesContentEncoding)) @@ -180,7 +186,7 @@ TEST(FileTransfer, NOT_ON_DARWIN(handlesContentEncoding)) auto ft = makeFileTransfer(); StringSink sink; - ft->download(FileTransferRequest(fmt("http://[::1]:%d/index", port)), sink); + ft->download(FileTransferRequest(fmt("http://[::1]:%d/index", port)))->drainInto(sink); EXPECT_EQ(sink.s, original); } From fb7d3154115cb5cf211c5ddbc124e17ee519bb64 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 26 Apr 2024 15:48:46 +0200 Subject: [PATCH 07/22] Merge pull request #10570 from layus/shared_caches Share evaluation caches across installables Before: $ rm -rf ~/.cache/nix && time -f '%E' nix build --dry-run \ 'nixpkgs#hello' \ 'nixpkgs#clang' \ 'nixpkgs#cargo' \ 'nixpkgs#rustup' \ 'nixpkgs#bear' \ 'nixpkgs#firefox' \ 'nixpkgs#git-revise' \ 'nixpkgs#hyperfine' \ 'nixpkgs#curlie' \ 'nixpkgs#xz' \ 'nixpkgs#ripgrep' 0:03.61 After: $ rm -rf ~/.cache/nix && time -f '%E' nix build --dry-run \ 'nixpkgs#hello' \ 'nixpkgs#clang' \ 'nixpkgs#cargo' \ 'nixpkgs#rustup' \ 'nixpkgs#bear' \ 'nixpkgs#firefox' \ 'nixpkgs#git-revise' \ 'nixpkgs#hyperfine' \ 'nixpkgs#curlie' \ 'nixpkgs#xz' \ 'nixpkgs#ripgrep' 0:01.46 This could probably use a more proper benchmark... Fixes #313 (cherry picked from commit de51e5c335865e3e0a8cccd283fec1a52cce243f) Change-Id: I9350bebd462b6af12c51db5bf432321abfe84a16 --- src/libcmd/installables.cc | 23 +++++++++++++++-------- src/libexpr/eval.hh | 9 +++++++++ 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index a10214561..711cf1b07 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -393,13 +393,10 @@ ref openEvalCache( EvalState & state, std::shared_ptr lockedFlake) { - auto fingerprint = lockedFlake->getFingerprint(); - return make_ref( - evalSettings.useEvalCache && evalSettings.pureEval - ? std::optional { std::cref(fingerprint) } - : std::nullopt, - state, - [&state, lockedFlake]() + auto fingerprint = evalSettings.useEvalCache && evalSettings.pureEval + ? std::make_optional(lockedFlake->getFingerprint()) + : std::nullopt; + auto rootLoader = [&state, lockedFlake]() { /* For testing whether the evaluation cache is complete. */ @@ -415,7 +412,17 @@ ref openEvalCache( assert(aOutputs); return aOutputs->value; - }); + }; + + if (fingerprint) { + auto search = state.evalCaches.find(fingerprint.value()); + if (search == state.evalCaches.end()) { + search = state.evalCaches.emplace(fingerprint.value(), make_ref(fingerprint, state, rootLoader)).first; + } + return search->second; + } else { + return make_ref(std::nullopt, state, rootLoader); + } } Installables SourceExprCommand::parseInstallables( diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index ec6e2bb5e..8e390e46d 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -33,6 +33,10 @@ class EvalState; class StorePath; struct SingleDerivedPath; enum RepairFlag : bool; +struct MemoryInputAccessor; +namespace eval_cache { + class EvalCache; +} /** @@ -234,6 +238,11 @@ public: return *new EvalErrorBuilder(*this, args...); } + /** + * A cache for evaluation caches, so as to reuse the same root value if possible + */ + std::map> evalCaches; + private: /* Cache for calls to addToStore(); maps source paths to the store From 7d52d74bbe39376f331887759020cff097dace55 Mon Sep 17 00:00:00 2001 From: K900 Date: Thu, 20 Jun 2024 08:54:43 +0300 Subject: [PATCH 08/22] BrotliDecompressionSource: don't bail out too early If we've consumed the entire input, that doesn't actually mean we're done decompressing - there might be more output left. This worked (?) in most cases because the input and output sizes are pretty comparable, but sometimes they're not and then things get very funny. Change-Id: I73435a654a911b8ce25119f713b80706c5783c1b --- src/libutil/compression.cc | 27 ++++++++++++++------------- tests/unit/libutil/compression.cc | 10 ++++++++++ 2 files changed, 24 insertions(+), 13 deletions(-) diff --git a/src/libutil/compression.cc b/src/libutil/compression.cc index d93a1b1d6..a66069e52 100644 --- a/src/libutil/compression.cc +++ b/src/libutil/compression.cc @@ -163,23 +163,24 @@ struct BrotliDecompressionSource : Source uint8_t * out = (uint8_t *) data; const auto * begin = out; - try { - while (len && !BrotliDecoderIsFinished(state.get())) { - checkInterrupt(); + while (len && !BrotliDecoderIsFinished(state.get())) { + checkInterrupt(); - while (avail_in == 0) { + while (avail_in == 0) { + try { avail_in = inner->read(buf.get(), BUF_SIZE); - next_in = (const uint8_t *) buf.get(); - } - - if (!BrotliDecoderDecompressStream( - state.get(), &avail_in, &next_in, &len, &out, nullptr - )) - { - throw CompressionError("error while decompressing brotli file"); + } catch (EndOfFile &) { + break; } + next_in = (const uint8_t *) buf.get(); + } + + if (!BrotliDecoderDecompressStream( + state.get(), &avail_in, &next_in, &len, &out, nullptr + )) + { + throw CompressionError("error while decompressing brotli file"); } - } catch (EndOfFile &) { } if (begin != out) { diff --git a/tests/unit/libutil/compression.cc b/tests/unit/libutil/compression.cc index 0542e7d33..3b40db0cd 100644 --- a/tests/unit/libutil/compression.cc +++ b/tests/unit/libutil/compression.cc @@ -66,6 +66,16 @@ namespace nix { ASSERT_THROW(decompress(method, str), CompressionError); } + TEST(decompress, veryLongBrotli) { + auto method = "br"; + auto str = std::string(65536, 'a'); + auto o = decompress(method, compress(method, str)); + + // This is just to not print 64k of "a" for most failures + ASSERT_EQ(o.length(), str.length()); + ASSERT_EQ(o, str); + } + /* ---------------------------------------------------------------------------- * compression sinks * --------------------------------------------------------------------------*/ From 3a4c21fc9e76a31d2a51b2ccce6c21c817e4c555 Mon Sep 17 00:00:00 2001 From: Qyriad Date: Wed, 19 Jun 2024 18:51:23 -0600 Subject: [PATCH 09/22] slight cleanup to ProgressBar::getStatus() Binaries were identical before and after this commit on our machine Change-Id: I6f8bfbe3298d6c5f43d5702c7a1e05cb180226cc --- src/libmain/progress-bar.cc | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/libmain/progress-bar.cc b/src/libmain/progress-bar.cc index d83b09cd4..b837eee8a 100644 --- a/src/libmain/progress-bar.cc +++ b/src/libmain/progress-bar.cc @@ -414,18 +414,18 @@ public: std::string getStatus(State & state) { - auto MiB = 1024.0 * 1024.0; + constexpr auto MiB = 1024.0 * 1024.0; std::string res; auto renderActivity = [&](ActivityType type, const std::string & itemFmt, const std::string & numberFmt = "%d", double unit = 1) { auto & act = state.activitiesByType[type]; uint64_t done = act.done, expected = act.done, running = 0, failed = act.failed; - for (auto & j : act.its) { - done += j.second->done; - expected += j.second->expected; - running += j.second->running; - failed += j.second->failed; + for (auto & [actId, infoIt] : act.its) { + done += infoIt->done; + expected += infoIt->expected; + running += infoIt->running; + failed += infoIt->failed; } expected = std::max(expected, act.expected); From f9594b592b520f6f13acf8b5e61ce676efde49af Mon Sep 17 00:00:00 2001 From: Qyriad Date: Wed, 19 Jun 2024 20:56:28 -0600 Subject: [PATCH 10/22] extract ProgressBar declaration into its header file Change-Id: Ica9e2ec41d99eaa196a0d535501edf45c589b2b6 --- src/libmain/progress-bar.cc | 829 +++++++++++++++++------------------- src/libmain/progress-bar.hh | 121 ++++++ 2 files changed, 503 insertions(+), 447 deletions(-) diff --git a/src/libmain/progress-bar.cc b/src/libmain/progress-bar.cc index b837eee8a..b3466a860 100644 --- a/src/libmain/progress-bar.cc +++ b/src/libmain/progress-bar.cc @@ -36,500 +36,435 @@ static std::string_view storePathToName(std::string_view path) return i == std::string::npos ? base.substr(0, 0) : base.substr(i + 1); } -// 100 years ought to be enough for anyone (yet sufficiently smaller than max() to not cause signed integer overflow). -constexpr const auto A_LONG_TIME = std::chrono::duration_cast(100 * 365 * 86400s); -class ProgressBar : public Logger +ProgressBar::~ProgressBar() { -private: + stop(); +} - struct ActInfo +/* Called by destructor, can't be overridden */ +void ProgressBar::stop() +{ { - std::string s, lastLine, phase; - ActivityType type = actUnknown; - uint64_t done = 0; - uint64_t expected = 0; - uint64_t running = 0; - uint64_t failed = 0; - std::map expectedByType; - bool visible = true; - ActivityId parent; - std::optional name; - std::chrono::time_point startTime; - }; - - struct ActivitiesByType - { - std::map::iterator> its; - uint64_t done = 0; - uint64_t expected = 0; - uint64_t failed = 0; - }; - - struct State - { - std::list activities; - std::map::iterator> its; - - std::map activitiesByType; - - uint64_t filesLinked = 0, bytesLinked = 0; - - uint64_t corruptedPaths = 0, untrustedPaths = 0; - - bool active = true; - bool paused = false; - bool haveUpdate = true; - }; - - Sync state_; - - std::thread updateThread; - - std::condition_variable quitCV, updateCV; - - bool printBuildLogs = false; - bool isTTY; - -public: - - ProgressBar(bool isTTY) - : isTTY(isTTY) - { - state_.lock()->active = isTTY; - updateThread = std::thread([&]() { - auto state(state_.lock()); - auto nextWakeup = A_LONG_TIME; - while (state->active) { - if (!state->haveUpdate) - state.wait_for(updateCV, nextWakeup); - nextWakeup = draw(*state); - state.wait_for(quitCV, std::chrono::milliseconds(50)); - } - }); - } - - ~ProgressBar() - { - stop(); - } - - /* Called by destructor, can't be overridden */ - void stop() override final - { - { - auto state(state_.lock()); - if (!state->active) return; - state->active = false; - writeToStderr("\r\e[K"); - updateCV.notify_one(); - quitCV.notify_one(); - } - updateThread.join(); - } - - void pause() override { - state_.lock()->paused = true; + auto state(state_.lock()); + if (!state->active) return; + state->active = false; writeToStderr("\r\e[K"); - } - - void resume() override { - state_.lock()->paused = false; - writeToStderr("\r\e[K"); - state_.lock()->haveUpdate = true; updateCV.notify_one(); + quitCV.notify_one(); + } + updateThread.join(); +} + +void ProgressBar::pause() +{ + state_.lock()->paused = true; + writeToStderr("\r\e[K"); +} + +void ProgressBar::resume() +{ + state_.lock()->paused = false; + writeToStderr("\r\e[K"); + state_.lock()->haveUpdate = true; + updateCV.notify_one(); +} + +bool ProgressBar::isVerbose() +{ + return printBuildLogs; +} + +void ProgressBar::log(Verbosity lvl, std::string_view s) +{ + if (lvl > verbosity) return; + auto state(state_.lock()); + log(*state, lvl, s); +} + +void ProgressBar::logEI(const ErrorInfo & ei) +{ + auto state(state_.lock()); + + std::stringstream oss; + showErrorInfo(oss, ei, loggerSettings.showTrace.get()); + + log(*state, ei.level, oss.str()); +} + +void ProgressBar::log(State & state, Verbosity lvl, std::string_view s) +{ + if (state.active) { + writeToStderr("\r\e[K" + filterANSIEscapes(s, !isTTY) + ANSI_NORMAL "\n"); + draw(state); + } else { + auto s2 = s + ANSI_NORMAL "\n"; + if (!isTTY) s2 = filterANSIEscapes(s2, true); + writeToStderr(s2); + } +} + +void ProgressBar::startActivity( + ActivityId act, + Verbosity lvl, + ActivityType type, + const std::string & s, + const Fields & fields, + ActivityId parent +) +{ + auto state(state_.lock()); + + if (lvl <= verbosity && !s.empty() && type != actBuildWaiting) + log(*state, lvl, s + "..."); + + state->activities.emplace_back(ActInfo { + .s = s, + .type = type, + .parent = parent, + .startTime = std::chrono::steady_clock::now() + }); + auto i = std::prev(state->activities.end()); + state->its.emplace(act, i); + state->activitiesByType[type].its.emplace(act, i); + + if (type == actBuild) { + std::string name(storePathToName(getS(fields, 0))); + if (name.ends_with(".drv")) + name = name.substr(0, name.size() - 4); + i->s = fmt("building " ANSI_BOLD "%s" ANSI_NORMAL, name); + auto machineName = getS(fields, 1); + if (machineName != "") + i->s += fmt(" on " ANSI_BOLD "%s" ANSI_NORMAL, machineName); + + // Used to be curRound and nrRounds, but the + // implementation was broken for a long time. + if (getI(fields, 2) != 1 || getI(fields, 3) != 1) { + throw Error("log message indicated repeating builds, but this is not currently implemented"); + } + i->name = DrvName(name).name; } - bool isVerbose() override - { - return printBuildLogs; + if (type == actSubstitute) { + auto name = storePathToName(getS(fields, 0)); + auto sub = getS(fields, 1); + i->s = fmt( + sub.starts_with("local") + ? "copying " ANSI_BOLD "%s" ANSI_NORMAL " from %s" + : "fetching " ANSI_BOLD "%s" ANSI_NORMAL " from %s", + name, sub); } - void log(Verbosity lvl, std::string_view s) override - { - if (lvl > verbosity) return; - auto state(state_.lock()); - log(*state, lvl, s); + if (type == actPostBuildHook) { + auto name = storePathToName(getS(fields, 0)); + if (name.ends_with(".drv")) + name = name.substr(0, name.size() - 4); + i->s = fmt("post-build " ANSI_BOLD "%s" ANSI_NORMAL, name); + i->name = DrvName(name).name; } - void logEI(const ErrorInfo & ei) override - { - auto state(state_.lock()); - - std::stringstream oss; - showErrorInfo(oss, ei, loggerSettings.showTrace.get()); - - log(*state, ei.level, oss.str()); + if (type == actQueryPathInfo) { + auto name = storePathToName(getS(fields, 0)); + i->s = fmt("querying " ANSI_BOLD "%s" ANSI_NORMAL " on %s", name, getS(fields, 1)); } - void log(State & state, Verbosity lvl, std::string_view s) - { - if (state.active) { - writeToStderr("\r\e[K" + filterANSIEscapes(s, !isTTY) + ANSI_NORMAL "\n"); - draw(state); - } else { - auto s2 = s + ANSI_NORMAL "\n"; - if (!isTTY) s2 = filterANSIEscapes(s2, true); - writeToStderr(s2); - } + if ((type == actFileTransfer && hasAncestor(*state, actCopyPath, parent)) + || (type == actFileTransfer && hasAncestor(*state, actQueryPathInfo, parent)) + || (type == actCopyPath && hasAncestor(*state, actSubstitute, parent))) + i->visible = false; + + update(*state); +} + +/* Check whether an activity has an ancestore with the specified + type. */ +bool ProgressBar::hasAncestor(State & state, ActivityType type, ActivityId act) +{ + while (act != 0) { + auto i = state.its.find(act); + if (i == state.its.end()) break; + if (i->second->type == type) return true; + act = i->second->parent; + } + return false; +} + +void ProgressBar::stopActivity(ActivityId act) +{ + auto state(state_.lock()); + + auto i = state->its.find(act); + if (i != state->its.end()) { + + auto & actByType = state->activitiesByType[i->second->type]; + actByType.done += i->second->done; + actByType.failed += i->second->failed; + + for (auto & j : i->second->expectedByType) + state->activitiesByType[j.first].expected -= j.second; + + actByType.its.erase(act); + state->activities.erase(i->second); + state->its.erase(i); } - void startActivity(ActivityId act, Verbosity lvl, ActivityType type, - const std::string & s, const Fields & fields, ActivityId parent) override - { - auto state(state_.lock()); + update(*state); +} - if (lvl <= verbosity && !s.empty() && type != actBuildWaiting) - log(*state, lvl, s + "..."); - - state->activities.emplace_back(ActInfo { - .s = s, - .type = type, - .parent = parent, - .startTime = std::chrono::steady_clock::now() - }); - auto i = std::prev(state->activities.end()); - state->its.emplace(act, i); - state->activitiesByType[type].its.emplace(act, i); - - if (type == actBuild) { - std::string name(storePathToName(getS(fields, 0))); - if (name.ends_with(".drv")) - name = name.substr(0, name.size() - 4); - i->s = fmt("building " ANSI_BOLD "%s" ANSI_NORMAL, name); - auto machineName = getS(fields, 1); - if (machineName != "") - i->s += fmt(" on " ANSI_BOLD "%s" ANSI_NORMAL, machineName); - - // Used to be curRound and nrRounds, but the - // implementation was broken for a long time. - if (getI(fields, 2) != 1 || getI(fields, 3) != 1) { - throw Error("log message indicated repeating builds, but this is not currently implemented"); - } - i->name = DrvName(name).name; - } - - if (type == actSubstitute) { - auto name = storePathToName(getS(fields, 0)); - auto sub = getS(fields, 1); - i->s = fmt( - sub.starts_with("local") - ? "copying " ANSI_BOLD "%s" ANSI_NORMAL " from %s" - : "fetching " ANSI_BOLD "%s" ANSI_NORMAL " from %s", - name, sub); - } - - if (type == actPostBuildHook) { - auto name = storePathToName(getS(fields, 0)); - if (name.ends_with(".drv")) - name = name.substr(0, name.size() - 4); - i->s = fmt("post-build " ANSI_BOLD "%s" ANSI_NORMAL, name); - i->name = DrvName(name).name; - } - - if (type == actQueryPathInfo) { - auto name = storePathToName(getS(fields, 0)); - i->s = fmt("querying " ANSI_BOLD "%s" ANSI_NORMAL " on %s", name, getS(fields, 1)); - } - - if ((type == actFileTransfer && hasAncestor(*state, actCopyPath, parent)) - || (type == actFileTransfer && hasAncestor(*state, actQueryPathInfo, parent)) - || (type == actCopyPath && hasAncestor(*state, actSubstitute, parent))) - i->visible = false; +void ProgressBar::result(ActivityId act, ResultType type, const std::vector & fields) +{ + auto state(state_.lock()); + if (type == resFileLinked) { + state->filesLinked++; + state->bytesLinked += getI(fields, 0); update(*state); } - /* Check whether an activity has an ancestore with the specified - type. */ - bool hasAncestor(State & state, ActivityType type, ActivityId act) - { - while (act != 0) { - auto i = state.its.find(act); - if (i == state.its.end()) break; - if (i->second->type == type) return true; - act = i->second->parent; + else if (type == resBuildLogLine || type == resPostBuildLogLine) { + auto lastLine = chomp(getS(fields, 0)); + if (!lastLine.empty()) { + auto i = state->its.find(act); + assert(i != state->its.end()); + ActInfo info = *i->second; + if (printBuildLogs) { + auto suffix = "> "; + if (type == resPostBuildLogLine) { + suffix = " (post)> "; + } + log(*state, lvlInfo, ANSI_FAINT + info.name.value_or("unnamed") + suffix + ANSI_NORMAL + lastLine); + } else { + state->activities.erase(i->second); + info.lastLine = lastLine; + state->activities.emplace_back(info); + i->second = std::prev(state->activities.end()); + update(*state); + } } - return false; } - void stopActivity(ActivityId act) override - { - auto state(state_.lock()); + else if (type == resUntrustedPath) { + state->untrustedPaths++; + update(*state); + } + else if (type == resCorruptedPath) { + state->corruptedPaths++; + update(*state); + } + + else if (type == resSetPhase) { auto i = state->its.find(act); - if (i != state->its.end()) { - - auto & actByType = state->activitiesByType[i->second->type]; - actByType.done += i->second->done; - actByType.failed += i->second->failed; - - for (auto & j : i->second->expectedByType) - state->activitiesByType[j.first].expected -= j.second; - - actByType.its.erase(act); - state->activities.erase(i->second); - state->its.erase(i); - } - + assert(i != state->its.end()); + i->second->phase = getS(fields, 0); update(*state); } - void result(ActivityId act, ResultType type, const std::vector & fields) override - { - auto state(state_.lock()); - - if (type == resFileLinked) { - state->filesLinked++; - state->bytesLinked += getI(fields, 0); - update(*state); - } - - else if (type == resBuildLogLine || type == resPostBuildLogLine) { - auto lastLine = chomp(getS(fields, 0)); - if (!lastLine.empty()) { - auto i = state->its.find(act); - assert(i != state->its.end()); - ActInfo info = *i->second; - if (printBuildLogs) { - auto suffix = "> "; - if (type == resPostBuildLogLine) { - suffix = " (post)> "; - } - log(*state, lvlInfo, ANSI_FAINT + info.name.value_or("unnamed") + suffix + ANSI_NORMAL + lastLine); - } else { - state->activities.erase(i->second); - info.lastLine = lastLine; - state->activities.emplace_back(info); - i->second = std::prev(state->activities.end()); - update(*state); - } - } - } - - else if (type == resUntrustedPath) { - state->untrustedPaths++; - update(*state); - } - - else if (type == resCorruptedPath) { - state->corruptedPaths++; - update(*state); - } - - else if (type == resSetPhase) { - auto i = state->its.find(act); - assert(i != state->its.end()); - i->second->phase = getS(fields, 0); - update(*state); - } - - else if (type == resProgress) { - auto i = state->its.find(act); - assert(i != state->its.end()); - ActInfo & actInfo = *i->second; - actInfo.done = getI(fields, 0); - actInfo.expected = getI(fields, 1); - actInfo.running = getI(fields, 2); - actInfo.failed = getI(fields, 3); - update(*state); - } - - else if (type == resSetExpected) { - auto i = state->its.find(act); - assert(i != state->its.end()); - ActInfo & actInfo = *i->second; - auto type = (ActivityType) getI(fields, 0); - auto & j = actInfo.expectedByType[type]; - state->activitiesByType[type].expected -= j; - j = getI(fields, 1); - state->activitiesByType[type].expected += j; - update(*state); - } + else if (type == resProgress) { + auto i = state->its.find(act); + assert(i != state->its.end()); + ActInfo & actInfo = *i->second; + actInfo.done = getI(fields, 0); + actInfo.expected = getI(fields, 1); + actInfo.running = getI(fields, 2); + actInfo.failed = getI(fields, 3); + update(*state); } - void update(State & state) - { - state.haveUpdate = true; - updateCV.notify_one(); + else if (type == resSetExpected) { + auto i = state->its.find(act); + assert(i != state->its.end()); + ActInfo & actInfo = *i->second; + auto type = (ActivityType) getI(fields, 0); + auto & j = actInfo.expectedByType[type]; + state->activitiesByType[type].expected -= j; + j = getI(fields, 1); + state->activitiesByType[type].expected += j; + update(*state); + } +} + +void ProgressBar::update(State & state) +{ + state.haveUpdate = true; + updateCV.notify_one(); +} + +std::chrono::milliseconds ProgressBar::draw(State & state) +{ + auto nextWakeup = A_LONG_TIME; + + state.haveUpdate = false; + if (state.paused || !state.active) return nextWakeup; + + std::string line; + + std::string status = getStatus(state); + if (!status.empty()) { + line += '['; + line += status; + line += "]"; } - std::chrono::milliseconds draw(State & state) - { - auto nextWakeup = A_LONG_TIME; + auto now = std::chrono::steady_clock::now(); - state.haveUpdate = false; - if (state.paused || !state.active) return nextWakeup; + if (!state.activities.empty()) { + if (!status.empty()) line += " "; + auto i = state.activities.rbegin(); - std::string line; - - std::string status = getStatus(state); - if (!status.empty()) { - line += '['; - line += status; - line += "]"; - } - - auto now = std::chrono::steady_clock::now(); - - if (!state.activities.empty()) { - if (!status.empty()) line += " "; - auto i = state.activities.rbegin(); - - while (i != state.activities.rend()) { - if (i->visible && (!i->s.empty() || !i->lastLine.empty())) { - /* Don't show activities until some time has - passed, to avoid displaying very short - activities. */ - auto delay = std::chrono::milliseconds(10); - if (i->startTime + delay < now) - break; - else - nextWakeup = std::min(nextWakeup, std::chrono::duration_cast(delay - (now - i->startTime))); - } - ++i; - } - - if (i != state.activities.rend()) { - line += i->s; - if (!i->phase.empty()) { - line += " ("; - line += i->phase; - line += ")"; - } - if (!i->lastLine.empty()) { - if (!i->s.empty()) line += ": "; - line += i->lastLine; - } - } - } - - auto width = getWindowSize().second; - if (width <= 0) width = std::numeric_limits::max(); - - writeToStderr("\r" + filterANSIEscapes(line, false, width) + ANSI_NORMAL + "\e[K"); - - return nextWakeup; - } - - std::string getStatus(State & state) - { - constexpr auto MiB = 1024.0 * 1024.0; - - std::string res; - - auto renderActivity = [&](ActivityType type, const std::string & itemFmt, const std::string & numberFmt = "%d", double unit = 1) { - auto & act = state.activitiesByType[type]; - uint64_t done = act.done, expected = act.done, running = 0, failed = act.failed; - for (auto & [actId, infoIt] : act.its) { - done += infoIt->done; - expected += infoIt->expected; - running += infoIt->running; - failed += infoIt->failed; - } - - expected = std::max(expected, act.expected); - - std::string s; - - if (running || done || expected || failed) { - if (running) - if (expected != 0) - s = fmt(ANSI_BLUE + numberFmt + ANSI_NORMAL "/" ANSI_GREEN + numberFmt + ANSI_NORMAL "/" + numberFmt, - running / unit, done / unit, expected / unit); - else - s = fmt(ANSI_BLUE + numberFmt + ANSI_NORMAL "/" ANSI_GREEN + numberFmt + ANSI_NORMAL, - running / unit, done / unit); - else if (expected != done) - if (expected != 0) - s = fmt(ANSI_GREEN + numberFmt + ANSI_NORMAL "/" + numberFmt, - done / unit, expected / unit); - else - s = fmt(ANSI_GREEN + numberFmt + ANSI_NORMAL, done / unit); + while (i != state.activities.rend()) { + if (i->visible && (!i->s.empty() || !i->lastLine.empty())) { + /* Don't show activities until some time has + passed, to avoid displaying very short + activities. */ + auto delay = std::chrono::milliseconds(10); + if (i->startTime + delay < now) + break; else - s = fmt(done ? ANSI_GREEN + numberFmt + ANSI_NORMAL : numberFmt, done / unit); - s = fmt(itemFmt, s); - - if (failed) - s += fmt(" (" ANSI_RED "%d failed" ANSI_NORMAL ")", failed / unit); + nextWakeup = std::min(nextWakeup, std::chrono::duration_cast(delay - (now - i->startTime))); } + ++i; + } - return s; - }; + if (i != state.activities.rend()) { + line += i->s; + if (!i->phase.empty()) { + line += " ("; + line += i->phase; + line += ")"; + } + if (!i->lastLine.empty()) { + if (!i->s.empty()) line += ": "; + line += i->lastLine; + } + } + } - auto showActivity = [&](ActivityType type, const std::string & itemFmt, const std::string & numberFmt = "%d", double unit = 1) { - auto s = renderActivity(type, itemFmt, numberFmt, unit); - if (s.empty()) return; + auto width = getWindowSize().second; + if (width <= 0) width = std::numeric_limits::max(); + + writeToStderr("\r" + filterANSIEscapes(line, false, width) + ANSI_NORMAL + "\e[K"); + + return nextWakeup; +} + +std::string ProgressBar::getStatus(State & state) +{ + constexpr auto MiB = 1024.0 * 1024.0; + + std::string res; + + auto renderActivity = [&](ActivityType type, const std::string & itemFmt, const std::string & numberFmt = "%d", double unit = 1) { + auto & act = state.activitiesByType[type]; + uint64_t done = act.done, expected = act.done, running = 0, failed = act.failed; + for (auto & [actId, infoIt] : act.its) { + done += infoIt->done; + expected += infoIt->expected; + running += infoIt->running; + failed += infoIt->failed; + } + + expected = std::max(expected, act.expected); + + std::string s; + + if (running || done || expected || failed) { + if (running) + if (expected != 0) + s = fmt(ANSI_BLUE + numberFmt + ANSI_NORMAL "/" ANSI_GREEN + numberFmt + ANSI_NORMAL "/" + numberFmt, + running / unit, done / unit, expected / unit); + else + s = fmt(ANSI_BLUE + numberFmt + ANSI_NORMAL "/" ANSI_GREEN + numberFmt + ANSI_NORMAL, + running / unit, done / unit); + else if (expected != done) + if (expected != 0) + s = fmt(ANSI_GREEN + numberFmt + ANSI_NORMAL "/" + numberFmt, + done / unit, expected / unit); + else + s = fmt(ANSI_GREEN + numberFmt + ANSI_NORMAL, done / unit); + else + s = fmt(done ? ANSI_GREEN + numberFmt + ANSI_NORMAL : numberFmt, done / unit); + s = fmt(itemFmt, s); + + if (failed) + s += fmt(" (" ANSI_RED "%d failed" ANSI_NORMAL ")", failed / unit); + } + + return s; + }; + + auto showActivity = [&](ActivityType type, const std::string & itemFmt, const std::string & numberFmt = "%d", double unit = 1) { + auto s = renderActivity(type, itemFmt, numberFmt, unit); + if (s.empty()) return; + if (!res.empty()) res += ", "; + res += s; + }; + + showActivity(actBuilds, "%s built"); + + auto s1 = renderActivity(actCopyPaths, "%s copied"); + auto s2 = renderActivity(actCopyPath, "%s MiB", "%.1f", MiB); + + if (!s1.empty() || !s2.empty()) { + if (!res.empty()) res += ", "; + if (s1.empty()) res += "0 copied"; else res += s1; + if (!s2.empty()) { res += " ("; res += s2; res += ')'; } + } + + showActivity(actFileTransfer, "%s MiB DL", "%.1f", MiB); + + { + auto s = renderActivity(actOptimiseStore, "%s paths optimised"); + if (s != "") { + s += fmt(", %.1f MiB / %d inodes freed", state.bytesLinked / MiB, state.filesLinked); if (!res.empty()) res += ", "; res += s; - }; - - showActivity(actBuilds, "%s built"); - - auto s1 = renderActivity(actCopyPaths, "%s copied"); - auto s2 = renderActivity(actCopyPath, "%s MiB", "%.1f", MiB); - - if (!s1.empty() || !s2.empty()) { - if (!res.empty()) res += ", "; - if (s1.empty()) res += "0 copied"; else res += s1; - if (!s2.empty()) { res += " ("; res += s2; res += ')'; } - } - - showActivity(actFileTransfer, "%s MiB DL", "%.1f", MiB); - - { - auto s = renderActivity(actOptimiseStore, "%s paths optimised"); - if (s != "") { - s += fmt(", %.1f MiB / %d inodes freed", state.bytesLinked / MiB, state.filesLinked); - if (!res.empty()) res += ", "; - res += s; - } - } - - // FIXME: don't show "done" paths in green. - showActivity(actVerifyPaths, "%s paths verified"); - - if (state.corruptedPaths) { - if (!res.empty()) res += ", "; - res += fmt(ANSI_RED "%d corrupted" ANSI_NORMAL, state.corruptedPaths); - } - - if (state.untrustedPaths) { - if (!res.empty()) res += ", "; - res += fmt(ANSI_RED "%d untrusted" ANSI_NORMAL, state.untrustedPaths); - } - - return res; - } - - void writeToStdout(std::string_view s) override - { - auto state(state_.lock()); - if (state->active) { - std::cerr << "\r\e[K"; - Logger::writeToStdout(s); - draw(*state); - } else { - Logger::writeToStdout(s); } } - std::optional ask(std::string_view msg) override - { - auto state(state_.lock()); - if (!state->active || !isatty(STDIN_FILENO)) return {}; - std::cerr << fmt("\r\e[K%s ", msg); - auto s = trim(readLine(STDIN_FILENO)); - if (s.size() != 1) return {}; + // FIXME: don't show "done" paths in green. + showActivity(actVerifyPaths, "%s paths verified"); + + if (state.corruptedPaths) { + if (!res.empty()) res += ", "; + res += fmt(ANSI_RED "%d corrupted" ANSI_NORMAL, state.corruptedPaths); + } + + if (state.untrustedPaths) { + if (!res.empty()) res += ", "; + res += fmt(ANSI_RED "%d untrusted" ANSI_NORMAL, state.untrustedPaths); + } + + return res; +} + +void ProgressBar::writeToStdout(std::string_view s) +{ + auto state(state_.lock()); + if (state->active) { + std::cerr << "\r\e[K"; + Logger::writeToStdout(s); draw(*state); - return s[0]; + } else { + Logger::writeToStdout(s); } +} - void setPrintBuildLogs(bool printBuildLogs) override - { - this->printBuildLogs = printBuildLogs; - } -}; +std::optional ProgressBar::ask(std::string_view msg) +{ + auto state(state_.lock()); + if (!state->active || !isatty(STDIN_FILENO)) return {}; + std::cerr << fmt("\r\e[K%s ", msg); + auto s = trim(readLine(STDIN_FILENO)); + if (s.size() != 1) return {}; + draw(*state); + return s[0]; +} + +void ProgressBar::setPrintBuildLogs(bool printBuildLogs) +{ + this->printBuildLogs = printBuildLogs; +} Logger * makeProgressBar() { diff --git a/src/libmain/progress-bar.hh b/src/libmain/progress-bar.hh index c3c6e3833..ae6e0d6da 100644 --- a/src/libmain/progress-bar.hh +++ b/src/libmain/progress-bar.hh @@ -1,10 +1,131 @@ #pragma once ///@file +#include + #include "logging.hh" +#include "sync.hh" namespace nix { +// 100 years ought to be enough for anyone (yet sufficiently smaller than max() to not cause signed integer overflow). +constexpr const auto A_LONG_TIME = std::chrono::duration_cast( + 100 * 365 * std::chrono::seconds(86400) +); + +class ProgressBar : public Logger +{ +private: + struct ActInfo + { + std::string s, lastLine, phase; + ActivityType type = actUnknown; + uint64_t done = 0; + uint64_t expected = 0; + uint64_t running = 0; + uint64_t failed = 0; + std::map expectedByType; + bool visible = true; + ActivityId parent; + std::optional name; + std::chrono::time_point startTime; + }; + + struct ActivitiesByType + { + std::map::iterator> its; + uint64_t done = 0; + uint64_t expected = 0; + uint64_t failed = 0; + }; + + struct State + { + std::list activities; + std::map::iterator> its; + + std::map activitiesByType; + + uint64_t filesLinked = 0, bytesLinked = 0; + + uint64_t corruptedPaths = 0, untrustedPaths = 0; + + bool active = true; + bool paused = false; + bool haveUpdate = true; + }; + + Sync state_; + + std::thread updateThread; + + std::condition_variable quitCV, updateCV; + + bool printBuildLogs = false; + bool isTTY; + +public: + + ProgressBar(bool isTTY) + : isTTY(isTTY) + { + state_.lock()->active = isTTY; + updateThread = std::thread([&]() { + auto state(state_.lock()); + auto nextWakeup = A_LONG_TIME; + while (state->active) { + if (!state->haveUpdate) + state.wait_for(updateCV, nextWakeup); + nextWakeup = draw(*state); + state.wait_for(quitCV, std::chrono::milliseconds(50)); + } + }); + } + + ~ProgressBar(); + + void stop() override final; + + void pause() override; + + void resume() override; + + bool isVerbose() override; + + void log(Verbosity lvl, std::string_view s) override; + + void logEI(const ErrorInfo & ei) override; + + void log(State & state, Verbosity lvl, std::string_view s); + + void startActivity( + ActivityId act, + Verbosity lvl, + ActivityType type, + const std::string & s, + const Fields & fields, + ActivityId parent + ) override; + + bool hasAncestor(State & state, ActivityType type, ActivityId act); + + void stopActivity(ActivityId act) override; + + void result(ActivityId act, ResultType type, const std::vector & fields) override; + + void update(State & state); + + std::chrono::milliseconds draw(State & state); + + std::string getStatus(State & state); + + void writeToStdout(std::string_view s) override; + + std::optional ask(std::string_view msg) override; + + void setPrintBuildLogs(bool printBuildLogs) override; +}; + Logger * makeProgressBar(); void startProgressBar(); From 8ba1939540a6f53166986446a5b8ee077591167a Mon Sep 17 00:00:00 2001 From: Qyriad Date: Wed, 19 Jun 2024 22:11:25 -0600 Subject: [PATCH 11/22] use a type alias for ProgressBar's chosen time point type Change-Id: I621a455b1daba806fc498958aee7931fbfc55445 --- src/libmain/progress-bar.hh | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libmain/progress-bar.hh b/src/libmain/progress-bar.hh index ae6e0d6da..1ac7ba97b 100644 --- a/src/libmain/progress-bar.hh +++ b/src/libmain/progress-bar.hh @@ -18,6 +18,8 @@ class ProgressBar : public Logger private: struct ActInfo { + using TimePoint = std::chrono::time_point; + std::string s, lastLine, phase; ActivityType type = actUnknown; uint64_t done = 0; @@ -28,7 +30,7 @@ private: bool visible = true; ActivityId parent; std::optional name; - std::chrono::time_point startTime; + TimePoint startTime; }; struct ActivitiesByType From 1e5f13456075722c33f3348de61847b1bdf6b60c Mon Sep 17 00:00:00 2001 From: Qyriad Date: Mon, 17 Jun 2024 14:33:28 -0600 Subject: [PATCH 12/22] de-inheritance CmdEval for InstallableValueCommand Change-Id: I08b1702310e863d15de26dc838eb0bcb62417c10 --- src/nix/eval.cc | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/nix/eval.cc b/src/nix/eval.cc index 469ff7391..9f265930b 100644 --- a/src/nix/eval.cc +++ b/src/nix/eval.cc @@ -1,4 +1,4 @@ -#include "command-installable-value.hh" +#include "command.hh" #include "common-args.hh" #include "print-options.hh" #include "shared.hh" @@ -12,13 +12,13 @@ using namespace nix; -struct CmdEval : MixJSON, InstallableValueCommand, MixReadOnlyOption +struct CmdEval : MixJSON, InstallableCommand, MixReadOnlyOption { bool raw = false; std::optional apply; std::optional writeTo; - CmdEval() : InstallableValueCommand() + CmdEval() : InstallableCommand() { addFlag({ .longName = "raw", @@ -55,14 +55,16 @@ struct CmdEval : MixJSON, InstallableValueCommand, MixReadOnlyOption Category category() override { return catSecondary; } - void run(ref store, ref installable) override + void run(ref store, ref installable) override { if (raw && json) throw UsageError("--raw and --json are mutually exclusive"); + auto const installableValue = InstallableValue::require(installable); + auto state = getEvalState(); - auto [v, pos] = installable->toValue(*state); + auto [v, pos] = installableValue->toValue(*state); NixStringContext context; if (apply) { From b9e9235ac0dcb647f9d2bf81be682ce27ffb113f Mon Sep 17 00:00:00 2001 From: Qyriad Date: Mon, 17 Jun 2024 14:35:42 -0600 Subject: [PATCH 13/22] de-inheritance CmdBundle for InstallableValueCommand Change-Id: Icbac4ef927ddcaf0d2a74b376e5a77299529cd34 --- src/nix/bundle.cc | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/nix/bundle.cc b/src/nix/bundle.cc index 504e35c81..8bf158d61 100644 --- a/src/nix/bundle.cc +++ b/src/nix/bundle.cc @@ -1,5 +1,5 @@ #include "installable-flake.hh" -#include "command-installable-value.hh" +#include "command.hh" #include "common-args.hh" #include "shared.hh" #include "store-api.hh" @@ -9,7 +9,7 @@ using namespace nix; -struct CmdBundle : InstallableValueCommand +struct CmdBundle : InstallableCommand { std::string bundler = "github:NixOS/bundlers"; std::optional outLink; @@ -71,11 +71,13 @@ struct CmdBundle : InstallableValueCommand return res; } - void run(ref store, ref installable) override + void run(ref store, ref installable) override { auto evalState = getEvalState(); - auto val = installable->toValue(*evalState).first; + auto const installableValue = InstallableValue::require(installable); + + auto val = installableValue->toValue(*evalState).first; auto [bundlerFlakeRef, bundlerName, extendedOutputsSpec] = parseFlakeRefWithFragmentAndExtendedOutputsSpec(bundler, absPath(".")); const flake::LockFlags lockFlags{ .writeLockFile = false }; From 079eeb1de72aeed6f9da16982c099d9936098ba6 Mon Sep 17 00:00:00 2001 From: Qyriad Date: Mon, 17 Jun 2024 14:37:57 -0600 Subject: [PATCH 14/22] de-inheritance CmdRun for InstallableValueCommand Change-Id: Ief858c1488197211e2ee8b70aa40ed6c65743558 --- src/nix/run.cc | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/nix/run.cc b/src/nix/run.cc index e1e896bb4..1e4406df5 100644 --- a/src/nix/run.cc +++ b/src/nix/run.cc @@ -1,6 +1,7 @@ #include "run.hh" -#include "command-installable-value.hh" +#include "command.hh" #include "common-args.hh" +#include "installables.hh" #include "shared.hh" #include "store-api.hh" #include "derivations.hh" @@ -145,7 +146,7 @@ struct CmdShell : InstallablesCommand, MixEnvironment static auto rCmdShell = registerCommand("shell"); -struct CmdRun : InstallableValueCommand +struct CmdRun : InstallableCommand { using InstallableCommand::run; @@ -191,12 +192,14 @@ struct CmdRun : InstallableValueCommand return res; } - void run(ref store, ref installable) override + void run(ref store, ref installable) override { auto state = getEvalState(); + auto installableValue = InstallableValue::require(installable); + lockFlags.applyNixConfig = true; - auto app = installable->toApp(*state).resolve(getEvalStore(), store); + auto app = installableValue->toApp(*state).resolve(getEvalStore(), store); Strings allArgs{app.program}; for (auto & i : args) allArgs.push_back(i); From 50be55ffca4d25e4e19175c18cd88f6046757652 Mon Sep 17 00:00:00 2001 From: Qyriad Date: Mon, 17 Jun 2024 14:39:19 -0600 Subject: [PATCH 15/22] de-inheritance CmdEdit for InstallableValueCommand Change-Id: If85ea78954a45470b0b25c08dc7d40bfebd53610 --- src/nix/edit.cc | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/nix/edit.cc b/src/nix/edit.cc index d1741a254..2f701f145 100644 --- a/src/nix/edit.cc +++ b/src/nix/edit.cc @@ -1,4 +1,4 @@ -#include "command-installable-value.hh" +#include "command.hh" #include "shared.hh" #include "eval.hh" #include "attr-path.hh" @@ -10,7 +10,7 @@ using namespace nix; -struct CmdEdit : InstallableValueCommand +struct CmdEdit : InstallableCommand { std::string description() override { @@ -26,17 +26,19 @@ struct CmdEdit : InstallableValueCommand Category category() override { return catSecondary; } - void run(ref store, ref installable) override + void run(ref store, ref installable) override { auto state = getEvalState(); + auto const installableValue = InstallableValue::require(installable); + const auto [file, line] = [&] { - auto [v, pos] = installable->toValue(*state); + auto [v, pos] = installableValue->toValue(*state); try { return findPackageFilename(*state, *v, installable->what()); } catch (NoPositionInfo &) { - throw Error("cannot find position information for '%s", installable->what()); + throw Error("cannot find position information for '%s", installableValue->what()); } }(); From 6515b1a4954131010a7b19acf98dc010dde647d0 Mon Sep 17 00:00:00 2001 From: Qyriad Date: Mon, 17 Jun 2024 14:41:22 -0600 Subject: [PATCH 16/22] de-inheritance CmdSearch for InstallableValueCommand Change-Id: I125c8cac05c8e924b55e4eb1060496e35ea4e941 --- src/nix/search.cc | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/nix/search.cc b/src/nix/search.cc index 97ef1375e..9f6b7a24e 100644 --- a/src/nix/search.cc +++ b/src/nix/search.cc @@ -1,4 +1,4 @@ -#include "command-installable-value.hh" +#include "command.hh" #include "globals.hh" #include "eval.hh" #include "eval-inline.hh" @@ -23,7 +23,7 @@ std::string wrap(std::string prefix, std::string s) return concatStrings(prefix, s, ANSI_NORMAL); } -struct CmdSearch : InstallableValueCommand, MixJSON +struct CmdSearch : InstallableCommand, MixJSON { std::vector res; std::vector excludeRes; @@ -62,8 +62,10 @@ struct CmdSearch : InstallableValueCommand, MixJSON }; } - void run(ref store, ref installable) override + void run(ref store, ref installable) override { + auto const installableValue = InstallableValue::require(installable); + settings.readOnlyMode = true; evalSettings.enableImportFromDerivation.setDefault(false); @@ -192,7 +194,7 @@ struct CmdSearch : InstallableValueCommand, MixJSON } }; - for (auto & cursor : installable->getCursors(*state)) + for (auto & cursor : installableValue->getCursors(*state)) visit(*cursor, cursor->getAttrPath(), true); if (json) From e44dcd63c4d96807536cdcf2afb688a537cce9be Mon Sep 17 00:00:00 2001 From: Qyriad Date: Mon, 17 Jun 2024 14:45:10 -0600 Subject: [PATCH 17/22] remove InstallableValueCommand class Change-Id: Id12383f4741cba07159712700ebcfbe37e61560c --- src/libcmd/command-installable-value.cc | 11 ----------- src/libcmd/command-installable-value.hh | 23 ----------------------- src/libcmd/meson.build | 2 -- src/nix/develop.cc | 2 +- 4 files changed, 1 insertion(+), 37 deletions(-) delete mode 100644 src/libcmd/command-installable-value.cc delete mode 100644 src/libcmd/command-installable-value.hh diff --git a/src/libcmd/command-installable-value.cc b/src/libcmd/command-installable-value.cc deleted file mode 100644 index 7e0c15eb8..000000000 --- a/src/libcmd/command-installable-value.cc +++ /dev/null @@ -1,11 +0,0 @@ -#include "command-installable-value.hh" - -namespace nix { - -void InstallableValueCommand::run(ref store, ref installable) -{ - auto installableValue = InstallableValue::require(installable); - run(store, installableValue); -} - -} diff --git a/src/libcmd/command-installable-value.hh b/src/libcmd/command-installable-value.hh deleted file mode 100644 index 7880d4119..000000000 --- a/src/libcmd/command-installable-value.hh +++ /dev/null @@ -1,23 +0,0 @@ -#pragma once -///@file - -#include "installable-value.hh" -#include "command.hh" - -namespace nix { - -/** - * An InstallableCommand where the single positional argument must be an - * InstallableValue in particular. - */ -struct InstallableValueCommand : InstallableCommand -{ - /** - * Entry point to this command - */ - virtual void run(ref store, ref installable) = 0; - - void run(ref store, ref installable) override; -}; - -} diff --git a/src/libcmd/meson.build b/src/libcmd/meson.build index 3d92f36a4..73deb035c 100644 --- a/src/libcmd/meson.build +++ b/src/libcmd/meson.build @@ -1,6 +1,5 @@ libcmd_sources = files( 'built-path.cc', - 'command-installable-value.cc', 'cmd-profiles.cc', 'command.cc', 'common-eval-args.cc', @@ -18,7 +17,6 @@ libcmd_sources = files( libcmd_headers = files( 'built-path.hh', - 'command-installable-value.hh', 'cmd-profiles.hh', 'command.hh', 'common-eval-args.hh', diff --git a/src/nix/develop.cc b/src/nix/develop.cc index 90bdf3bed..353bf0110 100644 --- a/src/nix/develop.cc +++ b/src/nix/develop.cc @@ -1,6 +1,6 @@ #include "eval.hh" #include "installable-flake.hh" -#include "command-installable-value.hh" +#include "command.hh" #include "common-args.hh" #include "shared.hh" #include "store-api.hh" From fd250c51ed14132d79eb1329a4e679d41174e5b8 Mon Sep 17 00:00:00 2001 From: Qyriad Date: Wed, 19 Jun 2024 22:12:24 -0600 Subject: [PATCH 18/22] add a basic libmain test for the progress bar rendering Hooray for leaky abstraction allowing us to test this particular part of the render pipeline. Change-Id: Ie0f251ff874f63324e6a9c6388b84ec6507eeae2 --- src/libmain/progress-bar.hh | 5 +--- tests/unit/libmain/progress-bar.cc | 43 ++++++++++++++++++++++++++++++ tests/unit/meson.build | 22 +++++++++++++++ 3 files changed, 66 insertions(+), 4 deletions(-) create mode 100644 tests/unit/libmain/progress-bar.cc diff --git a/src/libmain/progress-bar.hh b/src/libmain/progress-bar.hh index 1ac7ba97b..76e2ed4ff 100644 --- a/src/libmain/progress-bar.hh +++ b/src/libmain/progress-bar.hh @@ -13,9 +13,8 @@ constexpr const auto A_LONG_TIME = std::chrono::duration_cast; @@ -66,8 +65,6 @@ private: bool printBuildLogs = false; bool isTTY; -public: - ProgressBar(bool isTTY) : isTTY(isTTY) { diff --git a/tests/unit/libmain/progress-bar.cc b/tests/unit/libmain/progress-bar.cc new file mode 100644 index 000000000..e44a8b37e --- /dev/null +++ b/tests/unit/libmain/progress-bar.cc @@ -0,0 +1,43 @@ +#include + +#include "eval.hh" +#include "progress-bar.hh" +#include "logging.hh" +#include "shared.hh" + +constexpr std::string_view TEST_URL = "https://github.com/NixOS/nixpkgs/archive/master.tar.gz"; +// Arbitrary number. We picked the size of a Nixpkgs tarball that we downloaded. +constexpr uint64_t TEST_EXPECTED = 43'370'307; +// Arbitrary number. We picked the progress made on a Nixpkgs tarball download we interrupted. +constexpr uint64_t TEST_DONE = 1'787'251; + +constexpr std::string_view EXPECTED = ANSI_GREEN "1.7" ANSI_NORMAL "/41.4 MiB DL"; +// Mostly here for informational purposes, but also if we change the way the escape codes +// are defined this test might break in some annoying to debug way. +constexpr std::string_view EXPECTED_RAW = "\x1b[32;1m1.7\x1b[0m/41.4 MiB DL"; +static_assert(EXPECTED == EXPECTED_RAW, "Hey, hey, the ANSI escape code definitions prolly changed"); + +namespace nix +{ + TEST(ProgressBar, basicStatusRender) { + initNix(); + initGC(); + + startProgressBar(); + ASSERT_NE(dynamic_cast(logger), nullptr); + ProgressBar & progressBar = dynamic_cast(*logger); + + Activity act( + progressBar, + lvlDebug, + actFileTransfer, + fmt("downloading '%s'", TEST_URL), + { "https://github.com/NixOS/nixpkgs/archive/master.tar.gz" } + ); + act.progress(TEST_DONE, TEST_EXPECTED); + auto state = progressBar.state_.lock(); + std::string const renderedStatus = progressBar.getStatus(*state); + + ASSERT_EQ(renderedStatus, EXPECTED); + } +} diff --git a/tests/unit/meson.build b/tests/unit/meson.build index c3eefeede..9f19e5fd9 100644 --- a/tests/unit/meson.build +++ b/tests/unit/meson.build @@ -249,3 +249,25 @@ test( suite : 'check', protocol : 'gtest', ) + +libmain_tester = executable( + 'liblixmain-tests', + files('libmain/progress-bar.cc'), + dependencies : [ + liblixmain, + liblixexpr, + liblixutil, + liblixstore, + gtest, + boost, + ], + cpp_pch : cpp_pch, +) + +test( + 'libmain-unit-tests', + libmain_tester, + args : tests_args, + suite : 'check', + protocol : 'gtest', +) From 91d8d0e58eb43a63c9a349e633bf6aff945abb93 Mon Sep 17 00:00:00 2001 From: Qyriad Date: Thu, 20 Jun 2024 13:50:31 -0600 Subject: [PATCH 19/22] mildly refactor the renderActivity if hell-chain This is primarily for readability, but iwrc chaining std::string's operator+ is also pretty scuffed performance-wise, and this was doing a lot of operator+ chainging. Change-Id: I9f25235df153eb2bbb491f1ff7ebbeed9a8ec985 --- src/libmain/progress-bar.cc | 66 ++++++++++++++++++++++++++----------- 1 file changed, 47 insertions(+), 19 deletions(-) diff --git a/src/libmain/progress-bar.cc b/src/libmain/progress-bar.cc index b3466a860..d97b54f8b 100644 --- a/src/libmain/progress-bar.cc +++ b/src/libmain/progress-bar.cc @@ -366,31 +366,59 @@ std::string ProgressBar::getStatus(State & state) expected = std::max(expected, act.expected); - std::string s; + std::string rendered; if (running || done || expected || failed) { - if (running) - if (expected != 0) - s = fmt(ANSI_BLUE + numberFmt + ANSI_NORMAL "/" ANSI_GREEN + numberFmt + ANSI_NORMAL "/" + numberFmt, - running / unit, done / unit, expected / unit); - else - s = fmt(ANSI_BLUE + numberFmt + ANSI_NORMAL "/" ANSI_GREEN + numberFmt + ANSI_NORMAL, - running / unit, done / unit); - else if (expected != done) - if (expected != 0) - s = fmt(ANSI_GREEN + numberFmt + ANSI_NORMAL "/" + numberFmt, - done / unit, expected / unit); - else - s = fmt(ANSI_GREEN + numberFmt + ANSI_NORMAL, done / unit); - else - s = fmt(done ? ANSI_GREEN + numberFmt + ANSI_NORMAL : numberFmt, done / unit); - s = fmt(itemFmt, s); + if (running) { + if (expected != 0) { + auto const runningPart = fmt(numberFmt, running / unit); + auto const donePart = fmt(numberFmt, done / unit); + auto const expectedPart = fmt(numberFmt, expected / unit); + rendered = fmt( + ANSI_BLUE "%s" ANSI_NORMAL "/" ANSI_GREEN "%s" ANSI_NORMAL "/%s", + runningPart, + donePart, + expectedPart + ); + } else { + auto const runningPart = fmt(numberFmt, running / unit); + auto const donePart = fmt(numberFmt, done / unit); + rendered = fmt( + ANSI_BLUE "%s" ANSI_NORMAL "/" ANSI_GREEN "%s" ANSI_NORMAL, + runningPart, + donePart + ); + } + } else if (expected != done) { + if (expected != 0) { + auto const donePart = fmt(numberFmt, done / unit); + auto const expectedPart = fmt(numberFmt, expected / unit); + rendered = fmt( + ANSI_GREEN "%s" ANSI_NORMAL "/%s", + donePart, + expectedPart + ); + } else { + auto const donePart = fmt(numberFmt, done / unit); + rendered = fmt(ANSI_GREEN "%s" ANSI_NORMAL, donePart); + } + } else { + auto const donePart = fmt(numberFmt, done / unit); + + // We only color if `done` is non-zero. + if (done) { + rendered = concatStrings(ANSI_GREEN, donePart, ANSI_NORMAL); + } else { + rendered = donePart; + } + } + rendered = fmt(itemFmt, rendered); if (failed) - s += fmt(" (" ANSI_RED "%d failed" ANSI_NORMAL ")", failed / unit); + rendered += fmt(" (" ANSI_RED "%d failed" ANSI_NORMAL ")", failed / unit); } - return s; + return rendered; }; auto showActivity = [&](ActivityType type, const std::string & itemFmt, const std::string & numberFmt = "%d", double unit = 1) { From 5623e759ecdc3ec0ce904f33d5331a1d92cee966 Mon Sep 17 00:00:00 2001 From: Qyriad Date: Thu, 20 Jun 2024 18:00:25 -0600 Subject: [PATCH 20/22] doc-comment Fields for Activity and Result types The ones we were able to figure out, at least. Change-Id: I697f4f3942e35a7adf1b2a6cc28b3168d1de111c --- src/libutil/logging.hh | 48 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/src/libutil/logging.hh b/src/libutil/logging.hh index 64be8bc62..944a70786 100644 --- a/src/libutil/logging.hh +++ b/src/libutil/logging.hh @@ -14,23 +14,71 @@ typedef enum { actRealise = 102, actCopyPaths = 103, actBuilds = 104, + + /** Fields: + * 0: string: path to store derivation being built. + * 1: string: representing the machine this is being built on. Empty string if local machine. + * 2: int: curRound, not used anymore, always 1? + * 3: int: nrRounds, not used anymore always 1? + */ actBuild = 105, actOptimiseStore = 106, actVerifyPaths = 107, + + /** Fields: + * 0: string: store path + * 1: string: substituter + */ actSubstitute = 108, + + /** Fields: + * 0: string: store path + * 1: string: substituter + */ actQueryPathInfo = 109, + + /** Fields: + * 0: string: store path + */ actPostBuildHook = 110, actBuildWaiting = 111, } ActivityType; typedef enum { + /** Fields: + * 0: int: bytes linked + */ resFileLinked = 100, + + /** Fields: + * 0: string: last line + */ resBuildLogLine = 101, resUntrustedPath = 102, resCorruptedPath = 103, + + /** Fields: + * 0: string: phase name + */ resSetPhase = 104, + + /** Fields: + * 0: int: done + * 1: int: expected + * 2: int: running + * 3: int: failed + */ resProgress = 105, + + /** Fields: + * 0: int: ActivityType + * 1: int: expected + */ resSetExpected = 106, + + /** Fields: + * 0: string: last line + */ resPostBuildLogLine = 107, } ResultType; From e30b15ca93983ea45b7e9fd5cdabed5384128206 Mon Sep 17 00:00:00 2001 From: Qyriad Date: Thu, 20 Jun 2024 18:18:12 -0600 Subject: [PATCH 21/22] progress bar: factor out activity ancestor precedence logic Change-Id: I2bc736cd176f0147477dfb94dd48a4c8a1f8ec77 --- src/libmain/progress-bar.cc | 43 ++++++++++++++++++++++++++++--------- src/libmain/progress-bar.hh | 10 +++++++-- 2 files changed, 41 insertions(+), 12 deletions(-) diff --git a/src/libmain/progress-bar.cc b/src/libmain/progress-bar.cc index d97b54f8b..5e177cb8f 100644 --- a/src/libmain/progress-bar.cc +++ b/src/libmain/progress-bar.cc @@ -168,23 +168,29 @@ void ProgressBar::startActivity( i->s = fmt("querying " ANSI_BOLD "%s" ANSI_NORMAL " on %s", name, getS(fields, 1)); } - if ((type == actFileTransfer && hasAncestor(*state, actCopyPath, parent)) - || (type == actFileTransfer && hasAncestor(*state, actQueryPathInfo, parent)) - || (type == actCopyPath && hasAncestor(*state, actSubstitute, parent))) + if (ancestorTakesPrecedence(*state, type, parent)) { i->visible = false; + } update(*state); } -/* Check whether an activity has an ancestore with the specified - type. */ -bool ProgressBar::hasAncestor(State & state, ActivityType type, ActivityId act) +bool ProgressBar::hasAncestor(State & state, ActivityType type, ActivityId act) const { + // 0 is the sentinel value for a non-existent activity ID. while (act != 0) { - auto i = state.its.find(act); - if (i == state.its.end()) break; - if (i->second->type == type) return true; - act = i->second->parent; + auto const foundActIt = state.its.find(act); + if (foundActIt == state.its.end()) { + break; + } + + ActInfo const & foundActInfo = *foundActIt->second; + + if (foundActInfo.type == type) { + return true; + } + + act = foundActInfo.parent; } return false; } @@ -494,6 +500,23 @@ void ProgressBar::setPrintBuildLogs(bool printBuildLogs) this->printBuildLogs = printBuildLogs; } +bool ProgressBar::ancestorTakesPrecedence(State & state, ActivityType type, ActivityId parent) +{ + if (type == actFileTransfer && hasAncestor(state, actCopyPath, parent)) { + return true; + } + + if (type == actFileTransfer && hasAncestor(state, actQueryPathInfo, parent)) { + return true; + } + + if (type == actCopyPath && hasAncestor(state, actSubstitute, parent)) { + return true; + } + + return false; +} + Logger * makeProgressBar() { return new ProgressBar(shouldANSI()); diff --git a/src/libmain/progress-bar.hh b/src/libmain/progress-bar.hh index 76e2ed4ff..5750ff1bd 100644 --- a/src/libmain/progress-bar.hh +++ b/src/libmain/progress-bar.hh @@ -106,7 +106,8 @@ struct ProgressBar : public Logger ActivityId parent ) override; - bool hasAncestor(State & state, ActivityType type, ActivityId act); + /** Check whether an activity has an ancestor with the specified type. */ + bool hasAncestor(State & state, ActivityType type, ActivityId act) const; void stopActivity(ActivityId act) override; @@ -116,13 +117,18 @@ struct ProgressBar : public Logger std::chrono::milliseconds draw(State & state); - std::string getStatus(State & state); + std::string getStatus(State & state) const; void writeToStdout(std::string_view s) override; std::optional ask(std::string_view msg) override; void setPrintBuildLogs(bool printBuildLogs) override; + + /** Returns true if the given activity should not be rendered, + * in lieu of rendering its parent instead. + */ + bool ancestorTakesPrecedence(State & state, ActivityType type, ActivityId parent); }; Logger * makeProgressBar(); From cc005058a9b06c3363ad425d2770c3a52be17397 Mon Sep 17 00:00:00 2001 From: Qyriad Date: Fri, 21 Jun 2024 20:25:03 -0600 Subject: [PATCH 22/22] state commit Change-Id: I43a2bb6f771c67fe819cf38c272bc338fa4cb048 --- src/libmain/progress-bar.cc | 129 +++++++++++++++++------------ src/libutil/logging.hh | 13 +++ tests/unit/libmain/progress-bar.cc | 49 ++++++++++- 3 files changed, 132 insertions(+), 59 deletions(-) diff --git a/src/libmain/progress-bar.cc b/src/libmain/progress-bar.cc index 5e177cb8f..7a9dc1eeb 100644 --- a/src/libmain/progress-bar.cc +++ b/src/libmain/progress-bar.cc @@ -11,6 +11,8 @@ #include #include +#include + namespace nix { using namespace std::literals::chrono_literals; @@ -124,31 +126,32 @@ void ProgressBar::startActivity( .parent = parent, .startTime = std::chrono::steady_clock::now() }); - auto i = std::prev(state->activities.end()); - state->its.emplace(act, i); - state->activitiesByType[type].its.emplace(act, i); + + auto mostRecentAct = std::prev(state->activities.end()); + state->its.emplace(act, mostRecentAct); + state->activitiesByType[type].its.emplace(act, mostRecentAct); if (type == actBuild) { std::string name(storePathToName(getS(fields, 0))); if (name.ends_with(".drv")) name = name.substr(0, name.size() - 4); - i->s = fmt("building " ANSI_BOLD "%s" ANSI_NORMAL, name); + mostRecentAct->s = fmt("building " ANSI_BOLD "%s" ANSI_NORMAL, name); auto machineName = getS(fields, 1); if (machineName != "") - i->s += fmt(" on " ANSI_BOLD "%s" ANSI_NORMAL, machineName); + mostRecentAct->s += fmt(" on " ANSI_BOLD "%s" ANSI_NORMAL, machineName); // Used to be curRound and nrRounds, but the // implementation was broken for a long time. if (getI(fields, 2) != 1 || getI(fields, 3) != 1) { throw Error("log message indicated repeating builds, but this is not currently implemented"); } - i->name = DrvName(name).name; + mostRecentAct->name = DrvName(name).name; } if (type == actSubstitute) { auto name = storePathToName(getS(fields, 0)); auto sub = getS(fields, 1); - i->s = fmt( + mostRecentAct->s = fmt( sub.starts_with("local") ? "copying " ANSI_BOLD "%s" ANSI_NORMAL " from %s" : "fetching " ANSI_BOLD "%s" ANSI_NORMAL " from %s", @@ -159,17 +162,17 @@ void ProgressBar::startActivity( auto name = storePathToName(getS(fields, 0)); if (name.ends_with(".drv")) name = name.substr(0, name.size() - 4); - i->s = fmt("post-build " ANSI_BOLD "%s" ANSI_NORMAL, name); - i->name = DrvName(name).name; + mostRecentAct->s = fmt("post-build " ANSI_BOLD "%s" ANSI_NORMAL, name); + mostRecentAct->name = DrvName(name).name; } if (type == actQueryPathInfo) { auto name = storePathToName(getS(fields, 0)); - i->s = fmt("querying " ANSI_BOLD "%s" ANSI_NORMAL " on %s", name, getS(fields, 1)); + mostRecentAct->s = fmt("querying " ANSI_BOLD "%s" ANSI_NORMAL " on %s", name, getS(fields, 1)); } if (ancestorTakesPrecedence(*state, type, parent)) { - i->visible = false; + mostRecentAct->visible = false; } update(*state); @@ -199,19 +202,22 @@ void ProgressBar::stopActivity(ActivityId act) { auto state(state_.lock()); - auto i = state->its.find(act); - if (i != state->its.end()) { + auto const currentAct = state->its.find(act); + if (currentAct != state->its.end()) { - auto & actByType = state->activitiesByType[i->second->type]; - actByType.done += i->second->done; - actByType.failed += i->second->failed; + std::list::iterator const & actInfo = currentAct->second; - for (auto & j : i->second->expectedByType) - state->activitiesByType[j.first].expected -= j.second; + auto & actByType = state->activitiesByType[actInfo->type]; + actByType.done += actInfo->done; + actByType.failed += actInfo->failed; + + for (auto const & [type, expected] : actInfo->expectedByType) { + state->activitiesByType[type].expected -= expected; + } actByType.its.erase(act); - state->activities.erase(i->second); - state->its.erase(i); + state->activities.erase(actInfo); + state->its.erase(currentAct); } update(*state); @@ -230,9 +236,9 @@ void ProgressBar::result(ActivityId act, ResultType type, const std::vectorits.find(act); - assert(i != state->its.end()); - ActInfo info = *i->second; + auto found = state->its.find(act); + assert(found != state->its.end()); + ActInfo info = *found->second; if (printBuildLogs) { auto suffix = "> "; if (type == resPostBuildLogLine) { @@ -240,10 +246,10 @@ void ProgressBar::result(ActivityId act, ResultType type, const std::vectoractivities.erase(i->second); + state->activities.erase(found->second); info.lastLine = lastLine; state->activities.emplace_back(info); - i->second = std::prev(state->activities.end()); + found->second = std::prev(state->activities.end()); update(*state); } } @@ -281,11 +287,11 @@ void ProgressBar::result(ActivityId act, ResultType type, const std::vectorits.find(act); assert(i != state->its.end()); ActInfo & actInfo = *i->second; - auto type = (ActivityType) getI(fields, 0); - auto & j = actInfo.expectedByType[type]; - state->activitiesByType[type].expected -= j; - j = getI(fields, 1); - state->activitiesByType[type].expected += j; + auto const type = static_cast(getI(fields, 0)); + uint64_t & thisExpected = actInfo.expectedByType[type]; + state->activitiesByType[type].expected -= thisExpected; + thisExpected = getI(fields, 1); + state->activitiesByType[type].expected += thisExpected; update(*state); } } @@ -354,16 +360,24 @@ std::chrono::milliseconds ProgressBar::draw(State & state) return nextWakeup; } -std::string ProgressBar::getStatus(State & state) +std::string ProgressBar::getStatus(State & state) const { - constexpr auto MiB = 1024.0 * 1024.0; + constexpr static auto MiB = 1024.0 * 1024.0; - std::string res; + // Each time the `showActivity` lambda is called: + // actBuild, actFileTransfer, actVerifyPaths, + // + 3 permutations for actCopyPaths, actCopyPath, + // + corrupted and unstrusted paths. + constexpr static auto MAX_PARTS = 8; + + // Stack-allocated vector. + // No need to heap allocate here when we have such a low maxiumum. + boost::container::static_vector parts; auto renderActivity = [&](ActivityType type, const std::string & itemFmt, const std::string & numberFmt = "%d", double unit = 1) { - auto & act = state.activitiesByType[type]; + auto const & act = state.activitiesByType[type]; uint64_t done = act.done, expected = act.done, running = 0, failed = act.failed; - for (auto & [actId, infoIt] : act.its) { + for (auto const & [actId, infoIt] : act.its) { done += infoIt->done; expected += infoIt->expected; running += infoIt->running; @@ -428,31 +442,38 @@ std::string ProgressBar::getStatus(State & state) }; auto showActivity = [&](ActivityType type, const std::string & itemFmt, const std::string & numberFmt = "%d", double unit = 1) { - auto s = renderActivity(type, itemFmt, numberFmt, unit); - if (s.empty()) return; - if (!res.empty()) res += ", "; - res += s; + auto rendered = renderActivity(type, itemFmt, numberFmt, unit); + if (!rendered.empty()) { + parts.emplace_back(std::move(rendered)); + } }; showActivity(actBuilds, "%s built"); - auto s1 = renderActivity(actCopyPaths, "%s copied"); - auto s2 = renderActivity(actCopyPath, "%s MiB", "%.1f", MiB); + { + auto const renderedMultiCopy = renderActivity(actCopyPaths, "%s copied"); + auto const renderedSingleCopy = renderActivity(actCopyPath, "%s MiB", "%.1f", MiB); - if (!s1.empty() || !s2.empty()) { - if (!res.empty()) res += ", "; - if (s1.empty()) res += "0 copied"; else res += s1; - if (!s2.empty()) { res += " ("; res += s2; res += ')'; } + if (!renderedMultiCopy.empty() || !renderedSingleCopy.empty()) { + std::string part = concatStrings( + renderedMultiCopy.empty() ? "0 copied" : renderedMultiCopy, + !renderedSingleCopy.empty() ? fmt(" (%s)", renderedSingleCopy) : "" + ); + parts.emplace_back(std::move(part)); + } } showActivity(actFileTransfer, "%s MiB DL", "%.1f", MiB); { - auto s = renderActivity(actOptimiseStore, "%s paths optimised"); - if (s != "") { - s += fmt(", %.1f MiB / %d inodes freed", state.bytesLinked / MiB, state.filesLinked); - if (!res.empty()) res += ", "; - res += s; + auto renderedOptimise = renderActivity(actOptimiseStore, "%s paths optimised"); + if (!renderedOptimise.empty()) { + parts.emplace_back(std::move(renderedOptimise)); + parts.emplace_back(fmt( + "%.1f MiB / %d inodes freed", + state.bytesLinked / MiB, + state.filesLinked + )); } } @@ -460,16 +481,14 @@ std::string ProgressBar::getStatus(State & state) showActivity(actVerifyPaths, "%s paths verified"); if (state.corruptedPaths) { - if (!res.empty()) res += ", "; - res += fmt(ANSI_RED "%d corrupted" ANSI_NORMAL, state.corruptedPaths); + parts.emplace_back(fmt(ANSI_RED "%d corrupted" ANSI_NORMAL, state.corruptedPaths)); } if (state.untrustedPaths) { - if (!res.empty()) res += ", "; - res += fmt(ANSI_RED "%d untrusted" ANSI_NORMAL, state.untrustedPaths); + parts.emplace_back(fmt(ANSI_RED "%d untrusted" ANSI_NORMAL, state.untrustedPaths)); } - return res; + return concatStringsSep(", ", parts); } void ProgressBar::writeToStdout(std::string_view s) diff --git a/src/libutil/logging.hh b/src/libutil/logging.hh index 944a70786..fa1f39ffe 100644 --- a/src/libutil/logging.hh +++ b/src/libutil/logging.hh @@ -1,6 +1,7 @@ #pragma once ///@file +#include "escape-string.hh" #include "types.hh" #include "error.hh" #include "config.hh" @@ -111,6 +112,18 @@ public: Field(const std::string & s) : type(tString), s(s) { } Field(const char * s) : type(tString), s(s) { } Field(const uint64_t & i) : type(tInt), i(i) { } + + inline std::string to_string() const + { + switch (this->type) { + case tString: + return fmt("Field { s = \"%s\" }", escapeString(this->s, EscapeStringOptions{ + .escapeNonPrinting = true, + })); + case tInt: + return fmt("Field { i = %d }", this->i); + } + } }; typedef std::vector Fields; diff --git a/tests/unit/libmain/progress-bar.cc b/tests/unit/libmain/progress-bar.cc index e44a8b37e..859b90699 100644 --- a/tests/unit/libmain/progress-bar.cc +++ b/tests/unit/libmain/progress-bar.cc @@ -19,13 +19,18 @@ static_assert(EXPECTED == EXPECTED_RAW, "Hey, hey, the ANSI escape code definiti namespace nix { - TEST(ProgressBar, basicStatusRender) { + ProgressBar & init() + { initNix(); initGC(); - startProgressBar(); - ASSERT_NE(dynamic_cast(logger), nullptr); - ProgressBar & progressBar = dynamic_cast(*logger); + + assert(dynamic_cast(logger) != nullptr); + return dynamic_cast(*logger); + } + + TEST(ProgressBar, basicStatusRender) { + ProgressBar & progressBar = init(); Activity act( progressBar, @@ -40,4 +45,40 @@ namespace nix ASSERT_EQ(renderedStatus, EXPECTED); } + + //TEST(ProgressBar, resultPermute) { + // ProgressBar & progressBar = init(); + // + // Activity act( + // progressBar, + // lvlDebug, + // actBuild, + // "building '/nix/store/zdp9na4ci9s3g68xi9hnn5gbllf6ak03-lix-2.91.0-dev-lixpre20240616-0ba37dc.drv'", + // Logger::Fields{ + // "/nix/store/zdp9na4ci9s3g68xi9hnn5gbllf6ak03-lix-2.91.0-dev-lixpre20240616-0ba37dc.drv", + // "", + // 1, + // 1, + // } + // ); + // + // act.progress( + // /* done */ 13, + // /* expected */ 156, + // /* running */ 2, + // /* failed */ 0 + // ); + // + // act.progress( + // /* done */ 13, + // /* expected */ 156, + // /* running */ 2, + // /* failed */ 0 + // ); + // + // auto state = progressBar.state_.lock(); + // std::string const autoStatus = progressBar.getStatus(*state); + // + // ASSERT_EQ(autoStatus, "a"); + //} }