From aa00a5a8c9ba7b74c2102b8dc7fc4c3ed58606d8 Mon Sep 17 00:00:00 2001 From: Alois Wohlschlager Date: Wed, 12 Jun 2024 19:42:38 +0200 Subject: [PATCH 01/47] 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/47] 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/47] 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/47] 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/47] 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/47] 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/47] 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 50472aa5bea95b9e800df833f4f7ec7691e63807 Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Mon, 17 Jun 2024 22:13:24 -0700 Subject: [PATCH 08/47] libstore: remove operations that are never called by supported clients I did a whole bunch of `git log -S` to find out exactly when all these things were obsoleted and found the commit in which their usage was removed, which I have added in either the error message or a comment. I've also made *some* of the version checks into static asserts for when we update the minimum supported protocol version. In the end this is not a lot of code we are deleting, but it's code that we will never have to support into the future when we build a protocol bridge, which is why I did it. It is not in the support baseline. Change-Id: Iea3c80795c75ea74f328cf7ede7cbedf8c41926b --- src/libstore/daemon.cc | 115 ++++++++++++++++---------------- src/libstore/remote-store.cc | 4 ++ src/libstore/serve-protocol.hh | 5 ++ src/libstore/worker-protocol.hh | 28 ++++---- 4 files changed, 81 insertions(+), 71 deletions(-) diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc index c89b5f5b3..6d64644d1 100644 --- a/src/libstore/daemon.cc +++ b/src/libstore/daemon.cc @@ -296,17 +296,6 @@ static void performOp(TunnelLogger * logger, ref store, break; } - case WorkerProto::Op::HasSubstitutes: { - auto path = store->parseStorePath(readString(from)); - logger->startWork(); - StorePathSet paths; // FIXME - paths.insert(path); - auto res = store->querySubstitutablePaths(paths); - logger->stopWork(); - to << (res.count(path) != 0); - break; - } - case WorkerProto::Op::QuerySubstitutablePaths: { auto paths = WorkerProto::Serialise::read(*store, rconn); logger->startWork(); @@ -316,36 +305,74 @@ static void performOp(TunnelLogger * logger, ref store, break; } - case WorkerProto::Op::QueryPathHash: { - auto path = store->parseStorePath(readString(from)); - logger->startWork(); - auto hash = store->queryPathInfo(path)->narHash; - logger->stopWork(); - to << hash.to_string(Base16, false); + case WorkerProto::Op::HasSubstitutes: { + throw UnimplementedError("HasSubstitutes is not supported in Lix. This is not used if the declared server protocol is > 1.12 (Nix 1.0, 2012)"); + break; + } + + case WorkerProto::Op::QueryPathHash: { + throw UnimplementedError("QueryPathHash is not supported in Lix, client usages were removed in 2016 in e0204f8d462041387651af388074491fd0bf36d6"); + break; + } + + case WorkerProto::Op::QueryReferences: { + throw UnimplementedError("QueryReferences is not supported in Lix, client usages were removed in 2016 in e0204f8d462041387651af388074491fd0bf36d6"); + break; + } + + case WorkerProto::Op::QueryDeriver: { + throw UnimplementedError("QueryDeriver is not supported in Lix, client usages were removed in 2016 in e0204f8d462041387651af388074491fd0bf36d6"); + break; + } + + case WorkerProto::Op::ExportPath: { + throw UnimplementedError("ExportPath is not supported in Lix, client usage were removed in 2017 in 27dc76c1a5dbe654465245ff5f6bc22e2c8902da"); + break; + } + + case WorkerProto::Op::ImportPaths: { + throw UnimplementedError("ImportPaths is not supported in Lix. This is not used if the declared server protocol is >= 1.18 (Nix 2.0, 2016)"); break; } - case WorkerProto::Op::QueryReferences: case WorkerProto::Op::QueryReferrers: case WorkerProto::Op::QueryValidDerivers: case WorkerProto::Op::QueryDerivationOutputs: { auto path = store->parseStorePath(readString(from)); logger->startWork(); StorePathSet paths; - if (op == WorkerProto::Op::QueryReferences) - for (auto & i : store->queryPathInfo(path)->references) - paths.insert(i); - else if (op == WorkerProto::Op::QueryReferrers) - store->queryReferrers(path, paths); - else if (op == WorkerProto::Op::QueryValidDerivers) - paths = store->queryValidDerivers(path); - else paths = store->queryDerivationOutputs(path); + + #pragma GCC diagnostic push + #pragma GCC diagnostic ignored "-Wswitch-enum" + switch (op) { + case WorkerProto::Op::QueryReferrers: { + store->queryReferrers(path, paths); + break; + } + case WorkerProto::Op::QueryValidDerivers: { + paths = store->queryValidDerivers(path); + break; + } + case WorkerProto::Op::QueryDerivationOutputs: { + // Only sent if server presents proto version <= 1.21 + REMOVE_AFTER_DROPPING_PROTO_MINOR(21); + paths = store->queryDerivationOutputs(path); + break; + } + default: + abort(); + break; + } + #pragma GCC diagnostic pop + logger->stopWork(); WorkerProto::write(*store, wconn, paths); break; } case WorkerProto::Op::QueryDerivationOutputNames: { + // Unused in CppNix >= 2.4 (removed in 045b07200c77bf1fe19c0a986aafb531e7e1ba54) + REMOVE_AFTER_DROPPING_PROTO_MINOR(31); auto path = store->parseStorePath(readString(from)); logger->startWork(); auto names = store->readDerivation(path).outputNames(); @@ -363,15 +390,6 @@ static void performOp(TunnelLogger * logger, ref store, break; } - case WorkerProto::Op::QueryDeriver: { - auto path = store->parseStorePath(readString(from)); - logger->startWork(); - auto info = store->queryPathInfo(path); - logger->stopWork(); - to << (info->deriver ? store->printStorePath(*info->deriver) : ""); - break; - } - case WorkerProto::Op::QueryPathFromHashPart: { auto hashPart = readString(from); logger->startWork(); @@ -493,29 +511,6 @@ static void performOp(TunnelLogger * logger, ref store, break; } - case WorkerProto::Op::ExportPath: { - auto path = store->parseStorePath(readString(from)); - readInt(from); // obsolete - logger->startWork(); - TunnelSink sink(to); - store->exportPath(path, sink); - logger->stopWork(); - to << 1; - break; - } - - case WorkerProto::Op::ImportPaths: { - logger->startWork(); - TunnelSource source(from, to); - auto paths = store->importPaths(source, - trusted ? NoCheckSigs : CheckSigs); - logger->stopWork(); - Strings paths2; - for (auto & i : paths) paths2.push_back(store->printStorePath(i)); - to << paths2; - break; - } - case WorkerProto::Op::BuildPaths: { auto drvs = WorkerProto::Serialise::read(*store, rconn); BuildMode mode = buildModeFromInteger(readInt(from)); @@ -666,8 +661,10 @@ static void performOp(TunnelLogger * logger, ref store, break; } - // Obsolete. + // Obsolete since 9947f1646a26b339fff2e02b77798e9841fac7f0 (included in CppNix 2.5.0). case WorkerProto::Op::SyncWithGC: { + // CppNix 2.5.0 is 32 + REMOVE_AFTER_DROPPING_PROTO_MINOR(31); logger->startWork(); logger->stopWork(); to << 1; diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 575078289..b2f8a285d 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -307,6 +307,7 @@ StorePathSet RemoteStore::queryDerivationOutputs(const StorePath & path) if (GET_PROTOCOL_MINOR(getProtocol()) >= 22) { return Store::queryDerivationOutputs(path); } + REMOVE_AFTER_DROPPING_PROTO_MINOR(21); auto conn(getConnection()); conn->to << WorkerProto::Op::QueryDerivationOutputs << printStorePath(path); conn.processStderr(); @@ -336,6 +337,7 @@ std::map> RemoteStore::queryPartialDerivat return outputs; } } else { + REMOVE_AFTER_DROPPING_PROTO_MINOR(21); auto & evalStore = evalStore_ ? *evalStore_ : *this; // Fallback for old daemon versions. // For floating-CA derivations (and their co-dependencies) this is an @@ -530,6 +532,7 @@ void RemoteStore::registerDrvOutput(const Realisation & info) auto conn(getConnection()); conn->to << WorkerProto::Op::RegisterDrvOutput; if (GET_PROTOCOL_MINOR(conn->daemonVersion) < 31) { + REMOVE_AFTER_DROPPING_PROTO_MINOR(30); conn->to << info.id.to_string(); conn->to << std::string(info.outPath.to_string()); } else { @@ -617,6 +620,7 @@ std::vector RemoteStore::buildPathsWithResults( conn.processStderr(); return WorkerProto::Serialise>::read(*this, *conn); } else { + REMOVE_AFTER_DROPPING_PROTO_MINOR(33); // Avoid deadlock. conn_.reset(); diff --git a/src/libstore/serve-protocol.hh b/src/libstore/serve-protocol.hh index 3f82c8177..742320933 100644 --- a/src/libstore/serve-protocol.hh +++ b/src/libstore/serve-protocol.hh @@ -8,6 +8,11 @@ namespace nix { #define SERVE_MAGIC_1 0x390c9deb #define SERVE_MAGIC_2 0x5452eecb +// This must remain at 2.7 (Nix 2.18) forever in Lix, since the protocol +// versioning is monotonic, so if we ever change it in the future, it will +// break compatibility with any potential CppNix-originated protocol changes. +// +// Lix intends to replace this protocol entirely. #define SERVE_PROTOCOL_VERSION (2 << 8 | 7) #define GET_PROTOCOL_MAJOR(x) ((x) & 0xff00) #define GET_PROTOCOL_MINOR(x) ((x) & 0x00ff) diff --git a/src/libstore/worker-protocol.hh b/src/libstore/worker-protocol.hh index 5f24c189f..36acf2a8d 100644 --- a/src/libstore/worker-protocol.hh +++ b/src/libstore/worker-protocol.hh @@ -23,6 +23,10 @@ namespace nix { #define GET_PROTOCOL_MINOR(x) ((x) & 0x00ff) +#define REMOVE_AFTER_DROPPING_PROTO_MINOR(protoMinor) \ + static_assert(MIN_SUPPORTED_MINOR_WORKER_PROTO_VERSION <= (protoMinor)) + + #define STDERR_NEXT 0x6f6c6d67 #define STDERR_READ 0x64617461 // data needed from source #define STDERR_WRITE 0x64617416 // data for sink @@ -136,30 +140,30 @@ struct WorkerProto enum struct WorkerProto::Op : uint64_t { IsValidPath = 1, - HasSubstitutes = 3, - QueryPathHash = 4, // obsolete - QueryReferences = 5, // obsolete + HasSubstitutes = 3, // obsolete since 2012, stubbed to error + QueryPathHash = 4, // obsolete since 2016, stubbed to error + QueryReferences = 5, // obsolete since 2016, stubbed to error QueryReferrers = 6, AddToStore = 7, - AddTextToStore = 8, // obsolete since 1.25, Nix 3.0. Use WorkerProto::Op::AddToStore + AddTextToStore = 8, // obsolete since protocol 1.25, CppNix 2.4. Use WorkerProto::Op::AddToStore BuildPaths = 9, EnsurePath = 10, AddTempRoot = 11, AddIndirectRoot = 12, - SyncWithGC = 13, + SyncWithGC = 13, // obsolete since CppNix 2.5.0 FindRoots = 14, - ExportPath = 16, // obsolete - QueryDeriver = 18, // obsolete + ExportPath = 16, // obsolete since 2017, stubbed to error + QueryDeriver = 18, // obsolete since 2016, stubbed to error SetOptions = 19, CollectGarbage = 20, QuerySubstitutablePathInfo = 21, - QueryDerivationOutputs = 22, // obsolete + QueryDerivationOutputs = 22, // obsolete since protocol 1.21, CppNix 2.4 QueryAllValidPaths = 23, - QueryFailedPaths = 24, - ClearFailedPaths = 25, + QueryFailedPaths = 24, // obsolete, removed + ClearFailedPaths = 25, // obsolete, removed QueryPathInfo = 26, - ImportPaths = 27, // obsolete - QueryDerivationOutputNames = 28, // obsolete + ImportPaths = 27, // obsolete since 2016 + QueryDerivationOutputNames = 28, // obsolete since CppNix 2.4 QueryPathFromHashPart = 29, QuerySubstitutablePathInfos = 30, QueryValidPaths = 31, From 7d52d74bbe39376f331887759020cff097dace55 Mon Sep 17 00:00:00 2001 From: K900 Date: Thu, 20 Jun 2024 08:54:43 +0300 Subject: [PATCH 09/47] 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 10/47] 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 11/47] 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 12/47] 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 13/47] 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 14/47] 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 15/47] 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 16/47] 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 17/47] 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 18/47] 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 19/47] 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 da4e46dd1fc04067b5ba4bc16dd68134fa7efad2 Mon Sep 17 00:00:00 2001 From: Finn Behrens Date: Sat, 1 Jun 2024 16:06:26 +0200 Subject: [PATCH 20/47] libmain: add progress bar with multiple status lines Add the log-formats `multiline` and `multiline-with-logs` which offer multiple current active building status lines. Change-Id: Idd8afe62f8591b5d8b70e258c5cefa09be4cab03 --- doc/manual/change-authors.yml | 4 + doc/manual/rl-next/multiline-log-format.md | 14 +++ doc/manual/src/command-ref/opt-common.md | 10 ++ src/libmain/common-args.cc | 2 +- src/libmain/loggers.cc | 15 +++ src/libmain/loggers.hh | 2 + src/libmain/progress-bar.cc | 112 ++++++++++++++------- src/libmain/progress-bar.hh | 9 +- src/libutil/logging.hh | 3 + 9 files changed, 132 insertions(+), 39 deletions(-) create mode 100644 doc/manual/rl-next/multiline-log-format.md diff --git a/doc/manual/change-authors.yml b/doc/manual/change-authors.yml index c56f588ca..5a7b8117e 100644 --- a/doc/manual/change-authors.yml +++ b/doc/manual/change-authors.yml @@ -60,6 +60,10 @@ jade: forgejo: jade github: lf- +kloenk: + forgejo: kloenk + github: kloenk + lovesegfault: github: lovesegfault diff --git a/doc/manual/rl-next/multiline-log-format.md b/doc/manual/rl-next/multiline-log-format.md new file mode 100644 index 000000000..62bb9f1e7 --- /dev/null +++ b/doc/manual/rl-next/multiline-log-format.md @@ -0,0 +1,14 @@ +--- +synopsis: Add log formats `multiline` and `multiline-with-logs` +cls: [1369] +credits: [kloenk] +category: Improvements +--- + +Added two new log formats (`multiline` and `multiline-with-logs`) that display +current activities below each other for better visibility. + +These formats attempt to use the maximum available lines +(defaulting to 25 if unable to determine) and print up to that many lines. +The status bar is displayed as the first line, with each subsequent +activity on its own line. diff --git a/doc/manual/src/command-ref/opt-common.md b/doc/manual/src/command-ref/opt-common.md index cd289b7ea..7740da953 100644 --- a/doc/manual/src/command-ref/opt-common.md +++ b/doc/manual/src/command-ref/opt-common.md @@ -78,6 +78,16 @@ Most commands in Lix accept the following command-line options: Display the raw logs, with the progress bar at the bottom. + - `multiline` + + Display a progress bar during the builds and in the lines below that one line per activity. + + + - `multiline-with-logs` + + Displayes the raw logs, with a progress bar and activities each in a new line at the bottom. + + - [`--no-build-output`](#opt-no-build-output) / `-Q` By default, output written by builders to standard output and standard error is echoed to the Lix command's standard error. diff --git a/src/libmain/common-args.cc b/src/libmain/common-args.cc index 8fcb10325..0b756301c 100644 --- a/src/libmain/common-args.cc +++ b/src/libmain/common-args.cc @@ -58,7 +58,7 @@ MixCommonArgs::MixCommonArgs(const std::string & programName) addFlag({ .longName = "log-format", - .description = "Set the format of log output; one of `raw`, `internal-json`, `bar` or `bar-with-logs`.", + .description = "Set the format of log output; one of `raw`, `internal-json`, `bar`, `bar-with-logs`, `multiline` or `multiline-with-logs`.", .category = loggingCategory, .labels = {"format"}, .handler = {[](std::string format) { setLogFormat(format); }}, diff --git a/src/libmain/loggers.cc b/src/libmain/loggers.cc index 80080d616..8c3c4e355 100644 --- a/src/libmain/loggers.cc +++ b/src/libmain/loggers.cc @@ -17,6 +17,10 @@ LogFormat parseLogFormat(const std::string & logFormatStr) { return LogFormat::bar; else if (logFormatStr == "bar-with-logs") return LogFormat::barWithLogs; + else if (logFormatStr == "multiline") + return LogFormat::multiline; + else if (logFormatStr == "multiline-with-logs") + return LogFormat::multilineWithLogs; throw Error("option 'log-format' has an invalid value '%s'", logFormatStr); } @@ -35,6 +39,17 @@ Logger * makeDefaultLogger() { logger->setPrintBuildLogs(true); return logger; } + case LogFormat::multiline: { + auto logger = makeProgressBar(); + logger->setPrintMultiline(true); + return logger; + } + case LogFormat::multilineWithLogs: { + auto logger = makeProgressBar(); + logger->setPrintMultiline(true); + logger->setPrintBuildLogs(true); + return logger; + } default: abort(); } diff --git a/src/libmain/loggers.hh b/src/libmain/loggers.hh index e5721420c..6064b6160 100644 --- a/src/libmain/loggers.hh +++ b/src/libmain/loggers.hh @@ -11,6 +11,8 @@ enum class LogFormat { internalJSON, bar, barWithLogs, + multiline, + multilineWithLogs, }; void setLogFormat(const std::string & logFormatStr); diff --git a/src/libmain/progress-bar.cc b/src/libmain/progress-bar.cc index b3466a860..b3b46fc21 100644 --- a/src/libmain/progress-bar.cc +++ b/src/libmain/progress-bar.cc @@ -95,8 +95,7 @@ void ProgressBar::logEI(const ErrorInfo & ei) 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); + draw(state, s); } else { auto s2 = s + ANSI_NORMAL "\n"; if (!isTTY) s2 = filterANSIEscapes(s2, true); @@ -234,10 +233,14 @@ void ProgressBar::result(ActivityId act, ResultType type, const std::vectoractivities.erase(i->second); - info.lastLine = lastLine; - state->activities.emplace_back(info); - i->second = std::prev(state->activities.end()); + if (!printMultiline) { + state->activities.erase(i->second); + info.lastLine = lastLine; + state->activities.emplace_back(info); + i->second = std::prev(state->activities.end()); + } else { + i->second->lastLine = lastLine; + } update(*state); } } @@ -290,60 +293,93 @@ void ProgressBar::update(State & state) updateCV.notify_one(); } -std::chrono::milliseconds ProgressBar::draw(State & state) +std::chrono::milliseconds ProgressBar::draw(State & state, const std::optional & s) { auto nextWakeup = A_LONG_TIME; state.haveUpdate = false; if (state.paused || !state.active) return nextWakeup; - std::string line; + auto windowSize = getWindowSize(); + auto width = windowSize.second; + if (width <= 0) { + width = std::numeric_limits::max(); + } + if (printMultiline && (state.lastLines >= 1)) { + // FIXME: make sure this works on windows + writeToStderr(fmt("\e[G\e[%dF\e[J", state.lastLines)); + } + + state.lastLines = 0; + + if (s != std::nullopt) + writeToStderr("\r\e[K" + filterANSIEscapes(s.value(), !isTTY) + ANSI_NORMAL "\n"); + + std::string line; std::string status = getStatus(state); if (!status.empty()) { line += '['; line += status; line += "]"; } + if (printMultiline && !line.empty()) { + writeToStderr(filterANSIEscapes(line, false, width) + "\n"); + state.lastLines++; + } + auto height = windowSize.first > 0 ? windowSize.first : 25; + auto moreActivities = 0; auto now = std::chrono::steady_clock::now(); + std::string activity_line; 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))); + for (auto i = state.activities.begin(); i != state.activities.end(); ++i) { + if (!(i->visible && (!i->s.empty() || !i->lastLine.empty()))) { + continue; + } + /* 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) { + nextWakeup = std::min( + nextWakeup, + std::chrono::duration_cast( + delay - (now - i->startTime) + ) + ); } - ++i; - } - if (i != state.activities.rend()) { - line += i->s; + activity_line = i->s; + if (!i->phase.empty()) { - line += " ("; - line += i->phase; - line += ")"; + activity_line += " ("; + activity_line += i->phase; + activity_line += ")"; } if (!i->lastLine.empty()) { - if (!i->s.empty()) line += ": "; - line += i->lastLine; + if (!i->s.empty()) + activity_line += ": "; + activity_line += i->lastLine; + } + + if (printMultiline) { + if (state.lastLines < (height -1)) { + writeToStderr(filterANSIEscapes(activity_line, false, width) + "\n"); + state.lastLines++; + } else moreActivities++; } } } - auto width = getWindowSize().second; - if (width <= 0) width = std::numeric_limits::max(); + if (printMultiline && moreActivities) + writeToStderr(fmt("And %d more...", moreActivities)); - writeToStderr("\r" + filterANSIEscapes(line, false, width) + ANSI_NORMAL + "\e[K"); + if (!printMultiline) { + line += " " + activity_line; + writeToStderr("\r" + filterANSIEscapes(line, false, width) + ANSI_NORMAL + "\e[K"); + } return nextWakeup; } @@ -442,9 +478,8 @@ void ProgressBar::writeToStdout(std::string_view s) { auto state(state_.lock()); if (state->active) { - std::cerr << "\r\e[K"; Logger::writeToStdout(s); - draw(*state); + draw(*state, {}); } else { Logger::writeToStdout(s); } @@ -457,7 +492,7 @@ std::optional ProgressBar::ask(std::string_view msg) std::cerr << fmt("\r\e[K%s ", msg); auto s = trim(readLine(STDIN_FILENO)); if (s.size() != 1) return {}; - draw(*state); + draw(*state, {}); return s[0]; } @@ -466,6 +501,11 @@ void ProgressBar::setPrintBuildLogs(bool printBuildLogs) this->printBuildLogs = printBuildLogs; } +void ProgressBar::setPrintMultiline(bool printMultiline) +{ + this->printMultiline = printMultiline; +} + Logger * makeProgressBar() { return new ProgressBar(shouldANSI()); diff --git a/src/libmain/progress-bar.hh b/src/libmain/progress-bar.hh index 76e2ed4ff..176e941e8 100644 --- a/src/libmain/progress-bar.hh +++ b/src/libmain/progress-bar.hh @@ -47,6 +47,8 @@ struct ProgressBar : public Logger std::map activitiesByType; + int lastLines = 0; + uint64_t filesLinked = 0, bytesLinked = 0; uint64_t corruptedPaths = 0, untrustedPaths = 0; @@ -63,6 +65,7 @@ struct ProgressBar : public Logger std::condition_variable quitCV, updateCV; bool printBuildLogs = false; + bool printMultiline = false; bool isTTY; ProgressBar(bool isTTY) @@ -75,7 +78,7 @@ struct ProgressBar : public Logger while (state->active) { if (!state->haveUpdate) state.wait_for(updateCV, nextWakeup); - nextWakeup = draw(*state); + nextWakeup = draw(*state, {}); state.wait_for(quitCV, std::chrono::milliseconds(50)); } }); @@ -114,7 +117,7 @@ struct ProgressBar : public Logger void update(State & state); - std::chrono::milliseconds draw(State & state); + std::chrono::milliseconds draw(State & state, const std::optional & s); std::string getStatus(State & state); @@ -123,6 +126,8 @@ struct ProgressBar : public Logger std::optional ask(std::string_view msg) override; void setPrintBuildLogs(bool printBuildLogs) override; + + void setPrintMultiline(bool printMultiline) override; }; Logger * makeProgressBar(); diff --git a/src/libutil/logging.hh b/src/libutil/logging.hh index 64be8bc62..115b979f8 100644 --- a/src/libutil/logging.hh +++ b/src/libutil/logging.hh @@ -114,6 +114,9 @@ public: virtual void setPrintBuildLogs(bool printBuildLogs) { } + + virtual void setPrintMultiline(bool printMultiline) + { } }; /** From 35eec921af1043fc6322edc0ad88c872d41623b8 Mon Sep 17 00:00:00 2001 From: Maximilian Bosch Date: Sat, 4 May 2024 12:55:10 +0200 Subject: [PATCH 21/47] libfetchers: make attribute / URL query handling consistent The original idea was to fix lix#174, but for a user friendly solution, I figured that we'd need more consistency: * Invalid query params will cause an error, just like invalid attributes. This has the following two consequences: * The `?dir=`-param from flakes will be removed before the URL to be fetched is passed to libfetchers. * The tarball fetcher doesn't allow URLs with custom query params anymore. I think this was questionable anyways given that an arbitrary set of query params was silently removed from the URL you wanted to fetch. The correct way is to use an attribute-set with a key `url` that contains the tarball URL to fetch. * Same for the git & mercurial fetchers: in that case it doesn't even matter though: both fetchers added unused query params to the URL that's passed from the input scheme to the fetcher (`url2` in the code). It turns out that this was never used since the query parameters were erased again in `getActualUrl`. * Validation happens for both attributes and URLs. Previously, a lot of fetchers validated e.g. refs/revs only when specified in a URL and the validity of attribute names only in `inputFromAttrs`. Now, all the validation is done in `inputFromAttrs` and `inputFromURL` constructs attributes that will be passed to `inputFromAttrs`. * Accept all attributes as URL query parameters. That also includes lesser used ones such as `narHash`. And "output" attributes like `lastModified`: these could be declared already when declaring inputs as attribute rather than URL. Now the behavior is at least consistent. Personally, I think we should differentiate in the future between "fetched input" (basically the attr-set that ends up in the lock-file) and "unfetched input" earlier: both inputFrom{Attrs,URL} entrypoints are probably OK for unfetched inputs, but for locked/fetched inputs a custom entrypoint should be used. Then, the current entrypoints wouldn't have to allow these attributes anymore. Change-Id: I1be1992249f7af8287cfc37891ab505ddaa2e8cd --- src/libexpr/flake/flakeref.cc | 8 ++- src/libfetchers/fetchers.hh | 31 +++++++++ src/libfetchers/git.cc | 17 ++--- src/libfetchers/github.cc | 114 ++++++++++++++++++++------------- src/libfetchers/indirect.cc | 34 ++++++---- src/libfetchers/mercurial.cc | 7 +- src/libfetchers/tarball.cc | 26 ++------ tests/functional/fetchers.sh | 91 ++++++++++++++++++++++++++ tests/functional/meson.build | 1 + tests/nixos/tarball-flakes.nix | 2 +- 10 files changed, 235 insertions(+), 96 deletions(-) create mode 100644 tests/functional/fetchers.sh diff --git a/src/libexpr/flake/flakeref.cc b/src/libexpr/flake/flakeref.cc index 1c90bfc43..8668961fe 100644 --- a/src/libexpr/flake/flakeref.cc +++ b/src/libexpr/flake/flakeref.cc @@ -204,7 +204,13 @@ std::pair parseFlakeRefWithFragment( std::string fragment; std::swap(fragment, parsedURL.fragment); - auto input = Input::fromURL(parsedURL, isFlake); + // This has a special meaning for flakes and must not be passed to libfetchers. + // Of course this means that libfetchers cannot have fetchers + // expecting an argument `dir` 🫠 + ParsedURL urlForFetchers(parsedURL); + urlForFetchers.query.erase("dir"); + + auto input = Input::fromURL(urlForFetchers, isFlake); input.parent = baseDir; return std::make_pair( diff --git a/src/libfetchers/fetchers.hh b/src/libfetchers/fetchers.hh index 2bb4248be..40f2b6294 100644 --- a/src/libfetchers/fetchers.hh +++ b/src/libfetchers/fetchers.hh @@ -159,6 +159,37 @@ struct InputScheme std::optional commitMsg) const; virtual std::pair fetch(ref store, const Input & input) = 0; + +protected: + void emplaceURLQueryIntoAttrs( + const ParsedURL & parsedURL, + Attrs & attrs, + const StringSet & numericParams, + const StringSet & booleanParams) const + { + for (auto &[name, value] : parsedURL.query) { + if (name == "url") { + throw BadURL( + "URL '%s' must not override url via query param!", + parsedURL.to_string() + ); + } else if (numericParams.count(name) != 0) { + if (auto n = string2Int(value)) { + attrs.insert_or_assign(name, *n); + } else { + throw BadURL( + "URL '%s' has non-numeric parameter '%s'", + parsedURL.to_string(), + name + ); + } + } else if (booleanParams.count(name) != 0) { + attrs.emplace(name, Explicit { value == "1" }); + } else { + attrs.emplace(name, value); + } + } + } }; void registerInputScheme(std::shared_ptr && fetcher); diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc index 2817fde23..0cb826075 100644 --- a/src/libfetchers/git.cc +++ b/src/libfetchers/git.cc @@ -273,18 +273,15 @@ struct GitInputScheme : InputScheme Attrs attrs; attrs.emplace("type", "git"); - - for (auto & [name, value] : url.query) { - if (name == "rev" || name == "ref") - attrs.emplace(name, value); - else if (name == "shallow" || name == "submodules" || name == "allRefs") - attrs.emplace(name, Explicit { value == "1" }); - else - url2.query.emplace(name, value); - } - attrs.emplace("url", url2.to_string()); + emplaceURLQueryIntoAttrs( + url, + attrs, + {"lastModified", "revCount"}, + {"shallow", "submodules", "allRefs"} + ); + return inputFromAttrs(attrs); } diff --git a/src/libfetchers/github.cc b/src/libfetchers/github.cc index 60fefd1f3..b971781ae 100644 --- a/src/libfetchers/github.cc +++ b/src/libfetchers/github.cc @@ -1,3 +1,4 @@ +#include "attrs.hh" #include "filetransfer.hh" #include "cache.hh" #include "globals.hh" @@ -36,18 +37,11 @@ struct GitArchiveInputScheme : InputScheme auto path = tokenizeString>(url.path, "/"); - std::optional rev; - std::optional ref; - std::optional host_url; + std::optional refOrRev; auto size = path.size(); if (size == 3) { - if (std::regex_match(path[2], revRegex)) - rev = Hash::parseAny(path[2], htSHA1); - else if (std::regex_match(path[2], refRegex)) - ref = path[2]; - else - throw BadURL("in URL '%s', '%s' is not a commit hash or branch/tag name", url.url, path[2]); + refOrRev = path[2]; } else if (size > 3) { std::string rs; for (auto i = std::next(path.begin(), 2); i != path.end(); i++) { @@ -58,61 +52,91 @@ struct GitArchiveInputScheme : InputScheme } if (std::regex_match(rs, refRegex)) { - ref = rs; + refOrRev = rs; } else { throw BadURL("in URL '%s', '%s' is not a branch/tag name", url.url, rs); } } else if (size < 2) throw BadURL("URL '%s' is invalid", url.url); + Attrs attrs; + attrs.emplace("type", type()); + attrs.emplace("owner", path[0]); + attrs.emplace("repo", path[1]); + for (auto &[name, value] : url.query) { - if (name == "rev") { - if (rev) - throw BadURL("URL '%s' contains multiple commit hashes", url.url); - rev = Hash::parseAny(value, htSHA1); + if (name == "rev" || name == "ref") { + if (refOrRev) { + throw BadURL("URL '%s' already contains a ref or rev", url.url); + } else { + refOrRev = value; + } + } else if (name == "lastModified") { + if (auto n = string2Int(value)) { + attrs.emplace(name, *n); + } else { + throw Error( + "Attribute 'lastModified' in URL '%s' must be an integer", + url.to_string() + ); + } + } else { + attrs.emplace(name, value); } - else if (name == "ref") { - if (!std::regex_match(value, refRegex)) - throw BadURL("URL '%s' contains an invalid branch/tag name", url.url); - if (ref) - throw BadURL("URL '%s' contains multiple branch/tag names", url.url); - ref = value; - } - else if (name == "host") { - if (!std::regex_match(value, hostRegex)) - throw BadURL("URL '%s' contains an invalid instance host", url.url); - host_url = value; - } - // FIXME: barf on unsupported attributes } - if (ref && rev) - throw BadURL("URL '%s' contains both a commit hash and a branch/tag name %s %s", url.url, *ref, rev->gitRev()); + if (refOrRev) attrs.emplace("refOrRev", *refOrRev); - Input input; - input.attrs.insert_or_assign("type", type()); - input.attrs.insert_or_assign("owner", path[0]); - input.attrs.insert_or_assign("repo", path[1]); - if (rev) input.attrs.insert_or_assign("rev", rev->gitRev()); - if (ref) input.attrs.insert_or_assign("ref", *ref); - if (host_url) input.attrs.insert_or_assign("host", *host_url); - - return input; + return inputFromAttrs(attrs); } std::optional inputFromAttrs(const Attrs & attrs) const override { - if (maybeGetStrAttr(attrs, "type") != type()) return {}; + // Attributes can contain refOrRev and it needs to be figured out + // which one it is (see inputFromURL for when that may happen). + // The correct one (ref or rev) will be written into finalAttrs and + // it needs to be mutable for that. + Attrs finalAttrs(attrs); + auto type_ = maybeGetStrAttr(finalAttrs, "type"); + if (type_ != type()) return {}; - for (auto & [name, value] : attrs) - if (name != "type" && name != "owner" && name != "repo" && name != "ref" && name != "rev" && name != "narHash" && name != "lastModified" && name != "host") + auto owner = getStrAttr(finalAttrs, "owner"); + auto repo = getStrAttr(finalAttrs, "repo"); + + auto url = fmt("%s:%s/%s", *type_, owner, repo); + if (auto host = maybeGetStrAttr(finalAttrs, "host")) { + if (!std::regex_match(*host, hostRegex)) { + throw BadURL("URL '%s' contains an invalid instance host", url); + } + } + + if (auto refOrRev = maybeGetStrAttr(finalAttrs, "refOrRev")) { + finalAttrs.erase("refOrRev"); + if (std::regex_match(*refOrRev, revRegex)) { + finalAttrs.emplace("rev", *refOrRev); + } else if (std::regex_match(*refOrRev, refRegex)) { + finalAttrs.emplace("ref", *refOrRev); + } else { + throw Error( + "in URL '%s', '%s' is not a commit hash or a branch/tag name", + url, + *refOrRev + ); + } + } else if (auto ref = maybeGetStrAttr(finalAttrs, "ref")) { + if (!std::regex_match(*ref, refRegex)) { + throw BadURL("URL '%s' contains an invalid branch/tag name", url); + } + } + + for (auto & [name, value] : finalAttrs) { + if (name != "type" && name != "owner" && name != "repo" && name != "ref" && name != "rev" && name != "narHash" && name != "lastModified" && name != "host") { throw Error("unsupported input attribute '%s'", name); - - getStrAttr(attrs, "owner"); - getStrAttr(attrs, "repo"); + } + } Input input; - input.attrs = attrs; + input.attrs = finalAttrs; return input; } diff --git a/src/libfetchers/indirect.cc b/src/libfetchers/indirect.cc index c73505b31..8c0176e84 100644 --- a/src/libfetchers/indirect.cc +++ b/src/libfetchers/indirect.cc @@ -17,6 +17,8 @@ struct IndirectInputScheme : InputScheme std::optional rev; std::optional ref; + Attrs attrs; + if (path.size() == 1) { } else if (path.size() == 2) { if (std::regex_match(path[1], revRegex)) @@ -26,29 +28,21 @@ struct IndirectInputScheme : InputScheme else throw BadURL("in flake URL '%s', '%s' is not a commit hash or branch/tag name", url.url, path[1]); } else if (path.size() == 3) { - if (!std::regex_match(path[1], refRegex)) - throw BadURL("in flake URL '%s', '%s' is not a branch/tag name", url.url, path[1]); ref = path[1]; - if (!std::regex_match(path[2], revRegex)) - throw BadURL("in flake URL '%s', '%s' is not a commit hash", url.url, path[2]); rev = Hash::parseAny(path[2], htSHA1); } else throw BadURL("GitHub URL '%s' is invalid", url.url); std::string id = path[0]; - if (!std::regex_match(id, flakeRegex)) - throw BadURL("'%s' is not a valid flake ID", id); - // FIXME: forbid query params? + attrs.emplace("type", "indirect"); + attrs.emplace("id", id); + if (rev) attrs.emplace("rev", rev->gitRev()); + if (ref) attrs.emplace("ref", *ref); - Input input; - input.direct = false; - input.attrs.insert_or_assign("type", "indirect"); - input.attrs.insert_or_assign("id", id); - if (rev) input.attrs.insert_or_assign("rev", rev->gitRev()); - if (ref) input.attrs.insert_or_assign("ref", *ref); + emplaceURLQueryIntoAttrs(url, attrs, {}, {}); - return input; + return inputFromAttrs(attrs); } std::optional inputFromAttrs(const Attrs & attrs) const override @@ -63,6 +57,18 @@ struct IndirectInputScheme : InputScheme if (!std::regex_match(id, flakeRegex)) throw BadURL("'%s' is not a valid flake ID", id); + // TODO come up with a nicer error message for those two. + if (auto rev = maybeGetStrAttr(attrs, "rev")) { + if (!std::regex_match(*rev, revRegex)) { + throw BadURL("in flake '%s', '%s' is not a commit hash", id, *rev); + } + } + if (auto ref = maybeGetStrAttr(attrs, "ref")) { + if (!std::regex_match(*ref, refRegex)) { + throw BadURL("in flake '%s', '%s' is not a valid branch/tag name", id, *ref); + } + } + Input input; input.direct = false; input.attrs = attrs; diff --git a/src/libfetchers/mercurial.cc b/src/libfetchers/mercurial.cc index 4fffa71d3..6015f8ec7 100644 --- a/src/libfetchers/mercurial.cc +++ b/src/libfetchers/mercurial.cc @@ -57,12 +57,7 @@ struct MercurialInputScheme : InputScheme Attrs attrs; attrs.emplace("type", "hg"); - for (auto &[name, value] : url.query) { - if (name == "rev" || name == "ref") - attrs.emplace(name, value); - else - url2.query.emplace(name, value); - } + emplaceURLQueryIntoAttrs(url, attrs, {"revCount"}, {}); attrs.emplace("url", url2.to_string()); diff --git a/src/libfetchers/tarball.cc b/src/libfetchers/tarball.cc index c903895e2..8dfdecda8 100644 --- a/src/libfetchers/tarball.cc +++ b/src/libfetchers/tarball.cc @@ -201,29 +201,17 @@ struct CurlInputScheme : InputScheme if (!isValidURL(_url, requireTree)) return std::nullopt; - Input input; - auto url = _url; + Attrs attrs; + attrs.emplace("type", inputType()); + url.scheme = parseUrlScheme(url.scheme).transport; - auto narHash = url.query.find("narHash"); - if (narHash != url.query.end()) - input.attrs.insert_or_assign("narHash", narHash->second); + emplaceURLQueryIntoAttrs(url, attrs, {"revCount"}, {}); - if (auto i = get(url.query, "rev")) - input.attrs.insert_or_assign("rev", *i); - - if (auto i = get(url.query, "revCount")) - if (auto n = string2Int(*i)) - input.attrs.insert_or_assign("revCount", *n); - - url.query.erase("rev"); - url.query.erase("revCount"); - - input.attrs.insert_or_assign("type", inputType()); - input.attrs.insert_or_assign("url", url.to_string()); - return input; + attrs.emplace("url", url.to_string()); + return inputFromAttrs(attrs); } std::optional inputFromAttrs(const Attrs & attrs) const override @@ -235,7 +223,7 @@ struct CurlInputScheme : InputScheme std::set allowedNames = {"type", "url", "narHash", "name", "unpack", "rev", "revCount", "lastModified"}; for (auto & [name, value] : attrs) if (!allowedNames.count(name)) - throw Error("unsupported %s input attribute '%s'", *type, name); + throw Error("unsupported %s input attribute '%s'. If you wanted to fetch a tarball with a query parameter, please use '{ type = \"tarball\"; url = \"...\"; }'", *type, name); Input input; input.attrs = attrs; diff --git a/tests/functional/fetchers.sh b/tests/functional/fetchers.sh new file mode 100644 index 000000000..0f888dc33 --- /dev/null +++ b/tests/functional/fetchers.sh @@ -0,0 +1,91 @@ +source common.sh + +requireGit + +clearStore + +testFetchTreeError() { + rawFetchTreeArg="${1?fetchTree arg missing}" + messageSubstring="${2?messageSubstring missing}" + + output="$(nix eval --impure --raw --expr "(builtins.fetchTree $rawFetchTreeArg).outPath" 2>&1)" && status=0 || status=$? + grepQuiet "$messageSubstring" <<<"$output" + test "$status" -ne 0 +} + +# github/gitlab/sourcehut fetcher input validation +for provider in github gitlab sourcehut; do + # ref/rev validation + testFetchTreeError \ + "{ type = \"$provider\"; owner = \"foo\"; repo = \"bar\"; ref = \",\"; }" \ + "URL '$provider:foo/bar' contains an invalid branch/tag name" + + testFetchTreeError \ + "\"$provider://host/foo/bar/,\"" \ + "URL '$provider:foo/bar', ',' is not a commit hash or a branch/tag name" + + testFetchTreeError \ + "\"$provider://host/foo/bar/f16d8f43dd0998cdb315a2cccf2e4d10027e7ca4?rev=abc\"" \ + "URL '$provider://host/foo/bar/f16d8f43dd0998cdb315a2cccf2e4d10027e7ca4?rev=abc' already contains a ref or rev" + + testFetchTreeError \ + "\"$provider://host/foo/bar/ref?ref=ref2\"" \ + "URL '$provider://host/foo/bar/ref?ref=ref2' already contains a ref or rev" + + # host validation + testFetchTreeError \ + "{ type = \"$provider\"; owner = \"foo\"; repo = \"bar\"; host = \"git_hub.com\"; }" \ + "URL '$provider:foo/bar' contains an invalid instance host" + + testFetchTreeError \ + "\"$provider://host/foo/bar/ref?host=git_hub.com\"" \ + "URL '$provider:foo/bar' contains an invalid instance host" + + # invalid attributes + testFetchTreeError \ + "{ type = \"$provider\"; owner = \"foo\"; repo = \"bar\"; wrong = true; }" \ + "unsupported input attribute 'wrong'" + + testFetchTreeError \ + "\"$provider://host/foo/bar/ref?wrong=1\"" \ + "unsupported input attribute 'wrong'" +done + +# unsupported attributes w/ tarball fetcher +testFetchTreeError \ + "\"https://host/foo?wrong=1\"" \ + "unsupported tarball input attribute 'wrong'. If you wanted to fetch a tarball with a query parameter, please use '{ type = \"tarball\"; url = \"...\"; }" + +# test for unsupported attributes / validation in git fetcher +testFetchTreeError \ + "\"git+https://github.com/owner/repo?invalid=1\"" \ + "unsupported Git input attribute 'invalid'" + +testFetchTreeError \ + "\"git+https://github.com/owner/repo?url=foo\"" \ + "URL 'git+https://github.com/owner/repo?url=foo' must not override url via query param!" + +testFetchTreeError \ + "\"git+https://github.com/owner/repo?ref=foo.lock\"" \ + "invalid Git branch/tag name 'foo.lock'" + +testFetchTreeError \ + "{ type = \"git\"; url =\"https://github.com/owner/repo\"; ref = \"foo.lock\"; }" \ + "invalid Git branch/tag name 'foo.lock'" + +# same for mercurial +testFetchTreeError \ + "\"hg+https://forge.tld/owner/repo?invalid=1\"" \ + "unsupported Mercurial input attribute 'invalid'" + +testFetchTreeError \ + "{ type = \"hg\"; url = \"https://forge.tld/owner/repo\"; invalid = 1; }" \ + "unsupported Mercurial input attribute 'invalid'" + +testFetchTreeError \ + "\"hg+https://forge.tld/owner/repo?ref=,\"" \ + "invalid Mercurial branch/tag name ','" + +testFetchTreeError \ + "{ type = \"hg\"; url = \"https://forge.tld/owner/repo\"; ref = \",\"; }" \ + "invalid Mercurial branch/tag name ','" diff --git a/tests/functional/meson.build b/tests/functional/meson.build index a13dee001..d99f9bbd3 100644 --- a/tests/functional/meson.build +++ b/tests/functional/meson.build @@ -93,6 +93,7 @@ functional_tests_scripts = [ 'fetchGitRefs.sh', 'gc-runtime.sh', 'tarball.sh', + 'fetchers.sh', 'fetchGit.sh', 'fetchurl.sh', 'fetchPath.sh', diff --git a/tests/nixos/tarball-flakes.nix b/tests/nixos/tarball-flakes.nix index ca7627bd1..5deba4a12 100644 --- a/tests/nixos/tarball-flakes.nix +++ b/tests/nixos/tarball-flakes.nix @@ -69,7 +69,7 @@ in # Check that we got redirected to the immutable URL. locked_url = info["locked"]["url"] - assert locked_url == "http://localhost/stable/${nixpkgs.rev}.tar.gz", f"{locked_url=} != http://localhost/stable/${nixpkgs.rev}.tar.gz" + assert locked_url == "http://localhost/stable/${nixpkgs.rev}.tar.gz?rev=${nixpkgs.rev}&revCount=1234", f"{locked_url=} != http://localhost/stable/${nixpkgs.rev}.tar.gz" # Check that we got the rev and revCount attributes. revision = info["revision"] From fc6a1451af673cd332cc3210d018c98be5265e2f Mon Sep 17 00:00:00 2001 From: Maximilian Bosch Date: Sat, 22 Jun 2024 17:38:25 +0200 Subject: [PATCH 22/47] libstore: reduce loglevel of `waiting for a machine to build` This comes quite often when the available job slots on all remote builders are exhausted and this is pretty spammy. This isn't really an issue, but expected behavior. A better way to display this is a nom-like approach where all scheduled builds are shown in a tree and pending builds are being marked as such IMHO. Change-Id: I6bc14e6054f84e3eb0768127b490e263d8cdcf89 --- src/libstore/build/derivation-goal.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 46399b0a8..c0cd523e6 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -760,7 +760,7 @@ void DerivationGoal::tryToBuild() /* Not now; wait until at least one child finishes or the wake-up timeout expires. */ if (!actLock) - actLock = std::make_unique(*logger, lvlWarn, actBuildWaiting, + actLock = std::make_unique(*logger, lvlTalkative, actBuildWaiting, fmt("waiting for a machine to build '%s'", Magenta(worker.store.printStorePath(drvPath)))); worker.waitForAWhile(shared_from_this()); outputLocks.unlock(); From e09cc60df9698f268fa06ed1d9879101687b9eff Mon Sep 17 00:00:00 2001 From: Qyriad Date: Sat, 22 Jun 2024 18:52:28 -0600 Subject: [PATCH 23/47] doc-comment ExprSelect's fields Change-Id: I63e79991a4bab93421266785e9258e0f5bb89b8f --- src/libexpr/nixexpr.hh | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh index 45d44d1d1..418f888b3 100644 --- a/src/libexpr/nixexpr.hh +++ b/src/libexpr/nixexpr.hh @@ -157,8 +157,18 @@ struct ExprInheritFrom : ExprVar struct ExprSelect : Expr { PosIdx pos; - std::unique_ptr e, def; + + /** The expression attributes are being selected on. e.g. `foo` in `foo.bar.baz`. */ + std::unique_ptr e; + + /** A default value specified with `or`, if the selected attr doesn't exist. + * e.g. `bix` in `foo.bar.baz or bix` + */ + std::unique_ptr def; + + /** The path of attributes being selected. e.g. `bar.baz` in `foo.bar.baz.` */ AttrPath attrPath; + ExprSelect(const PosIdx & pos, std::unique_ptr e, AttrPath attrPath, std::unique_ptr def) : pos(pos), e(std::move(e)), def(std::move(def)), attrPath(std::move(attrPath)) { }; ExprSelect(const PosIdx & pos, std::unique_ptr e, Symbol name) : pos(pos), e(std::move(e)) { attrPath.push_back(AttrName(name)); }; PosIdx getPos() const override { return pos; } From fb8553f63c423e58931faead70b429cbc86159c4 Mon Sep 17 00:00:00 2001 From: Qyriad Date: Sat, 22 Jun 2024 18:51:10 -0600 Subject: [PATCH 24/47] mildly cleanup ExprSelect::eval Better variable names, some comments, and a slight logic rearrange. Change-Id: I9685ae252f83217aa85f06432234159c9ad19d1c --- src/libexpr/eval.cc | 92 ++++++++++++++++++++++++++++++--------------- 1 file changed, 61 insertions(+), 31 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 9bd27e22d..afee89420 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -4,6 +4,7 @@ #include "primops.hh" #include "print-options.hh" #include "shared.hh" +#include "suggestions.hh" #include "types.hh" #include "store-api.hh" #include "derivations.hh" @@ -1426,11 +1427,13 @@ static std::string showAttrPath(EvalState & state, Env & env, const AttrPath & a void ExprSelect::eval(EvalState & state, Env & env, Value & v) { - Value vTmp; - PosIdx pos2; - Value * vAttrs = &vTmp; + Value vFirst; - e->eval(state, env, vTmp); + // Pointer to the current attrset Value in this select chain. + Value * vCurrent = &vFirst; + // Position for the current attrset Value in this select chain. + PosIdx posCurrent; + e->eval(state, env, vFirst); try { auto dts = state.debugRepl @@ -1443,48 +1446,75 @@ void ExprSelect::eval(EvalState & state, Env & env, Value & v) showAttrPath(state, env, attrPath)) : nullptr; - for (auto & i : attrPath) { + for (auto const & currentAttrName : attrPath) { state.nrLookups++; - Bindings::iterator j; - auto name = getName(i, state, env); - if (def) { - state.forceValue(*vAttrs, pos); - if (vAttrs->type() != nAttrs || - (j = vAttrs->attrs->find(name)) == vAttrs->attrs->end()) - { - def->eval(state, env, v); + + Symbol const name = getName(currentAttrName, state, env); + + state.forceValue(*vCurrent, pos); + + if (vCurrent->type() != nAttrs) { + + // If we have an `or` provided default, + // then this is allowed to not be an attrset. + if (def != nullptr) { + this->def->eval(state, env, v); return; } - } else { - state.forceAttrs(*vAttrs, pos, "while selecting an attribute"); - if ((j = vAttrs->attrs->find(name)) == vAttrs->attrs->end()) { - std::set allAttrNames; - for (auto & attr : *vAttrs->attrs) - allAttrNames.insert(state.symbols[attr.name]); - auto suggestions = Suggestions::bestMatches(allAttrNames, state.symbols[name]); - state.error("attribute '%1%' missing", state.symbols[name]) - .atPos(pos).withSuggestions(suggestions).withFrame(env, *this).debugThrow(); - } + + // Otherwise, we must type error. + state.error( + "expected a set but found %s: %s", + showType(*vCurrent), + ValuePrinter(state, *vCurrent, errorPrintOptions) + ).withTrace(pos, "while selecting an attribute").debugThrow(); } - vAttrs = j->value; - pos2 = j->pos; - if (state.countCalls) state.attrSelects[pos2]++; + + // Now that we know this is actually an attrset, try to find an attr + // with the selected name. + Bindings::iterator attrIt = vCurrent->attrs->find(name); + if (attrIt == vCurrent->attrs->end()) { + + // If we have an `or` provided default, then we'll use that. + if (def != nullptr) { + this->def->eval(state, env, v); + return; + } + + // Otherwise, missing attr error. + std::set allAttrNames; + for (auto const & attr : *vCurrent->attrs) { + allAttrNames.insert(state.symbols[attr.name]); + } + auto suggestions = Suggestions::bestMatches(allAttrNames, state.symbols[name]); + state.error("attribute '%s' missing", state.symbols[name]) + .atPos(pos) + .withSuggestions(suggestions) + .withFrame(env, *this) + .debugThrow(); + } + + // If we're here, then we successfully found the attribute. + // Set our currently operated-on attrset to this one, and keep going. + vCurrent = attrIt->value; + posCurrent = attrIt->pos; + if (state.countCalls) state.attrSelects[posCurrent]++; } - state.forceValue(*vAttrs, (pos2 ? pos2 : this->pos ) ); + state.forceValue(*vCurrent, (posCurrent ? posCurrent : this->pos)); } catch (Error & e) { - if (pos2) { - auto pos2r = state.positions[pos2]; + if (posCurrent) { + auto pos2r = state.positions[posCurrent]; auto origin = std::get_if(&pos2r.origin); if (!(origin && *origin == state.derivationInternal)) - state.addErrorTrace(e, pos2, "while evaluating the attribute '%1%'", + state.addErrorTrace(e, posCurrent, "while evaluating the attribute '%1%'", showAttrPath(state, env, attrPath)); } throw; } - v = *vAttrs; + v = *vCurrent; } From 12f5d27363316df8e04af1e2e376c39588e12057 Mon Sep 17 00:00:00 2001 From: Artemis Tosini Date: Sun, 12 May 2024 21:09:26 +0000 Subject: [PATCH 25/47] libstore: Start creating LocalDerivationGoal subclasses LocalDerivationGoal includes a large number of low-level sandboxing primitives for Darwin and Linux, intermingled with ifdefs. Start creating platform-specific classes to make it easier to add new platforms and review platform-specific code. This change only creates support infrastructure and moves two function, more functions will be moved in future changes. Change-Id: I9fc29fa2a7345107d4fc96c46fa90b4eabf6bb89 --- src/libstore/build/local-derivation-goal.cc | 49 ++++----------------- src/libstore/build/local-derivation-goal.hh | 35 ++++++++++++++- src/libstore/build/worker.cc | 8 ++-- src/libstore/platform.cc | 40 +++++++++++++++++ src/libstore/platform/darwin.cc | 26 +++++++++++ src/libstore/platform/darwin.hh | 16 +++++++ src/libstore/platform/fallback.hh | 11 +++++ src/libstore/platform/linux.cc | 14 ++++++ src/libstore/platform/linux.hh | 13 ++++++ 9 files changed, 165 insertions(+), 47 deletions(-) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index bae821266..4e616c838 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -51,9 +51,6 @@ #endif #if __APPLE__ -#include -#include - /* This definition is undocumented but depended upon by all major browsers. */ extern "C" int sandbox_init_with_parameters(const char *profile, uint64_t flags, const char *const parameters[], char **errorbuf); #endif @@ -161,19 +158,7 @@ void LocalDerivationGoal::killChild() void LocalDerivationGoal::killSandbox(bool getStats) { - if (cgroup) { - #if __linux__ - auto stats = destroyCgroup(*cgroup); - if (getStats) { - buildResult.cpuUser = stats.cpuUser; - buildResult.cpuSystem = stats.cpuSystem; - } - #else - abort(); - #endif - } - - else if (buildUser) { + if (buildUser) { auto uid = buildUser->getUID(); assert(uid != 0); killUser(uid); @@ -2177,31 +2162,8 @@ void LocalDerivationGoal::runChild() } } -#if __APPLE__ - posix_spawnattr_t attrp; - - if (posix_spawnattr_init(&attrp)) - throw SysError("failed to initialize builder"); - - if (posix_spawnattr_setflags(&attrp, POSIX_SPAWN_SETEXEC)) - throw SysError("failed to initialize builder"); - - if (drv->platform == "aarch64-darwin") { - // Unset kern.curproc_arch_affinity so we can escape Rosetta - int affinity = 0; - sysctlbyname("kern.curproc_arch_affinity", NULL, NULL, &affinity, sizeof(affinity)); - - cpu_type_t cpu = CPU_TYPE_ARM64; - posix_spawnattr_setbinpref_np(&attrp, 1, &cpu, NULL); - } else if (drv->platform == "x86_64-darwin") { - cpu_type_t cpu = CPU_TYPE_X86_64; - posix_spawnattr_setbinpref_np(&attrp, 1, &cpu, NULL); - } - - posix_spawn(NULL, drv->builder.c_str(), NULL, &attrp, stringsToCharPtrs(args).data(), stringsToCharPtrs(envStrs).data()); -#else - execve(drv->builder.c_str(), stringsToCharPtrs(args).data(), stringsToCharPtrs(envStrs).data()); -#endif + execBuilder(drv->builder, args, envStrs); + // execBuilder should not return throw SysError("executing '%1%'", drv->builder); @@ -2217,6 +2179,11 @@ void LocalDerivationGoal::runChild() } } +void LocalDerivationGoal::execBuilder(std::string builder, Strings args, Strings envStrs) +{ + execve(builder.c_str(), stringsToCharPtrs(args).data(), stringsToCharPtrs(envStrs).data()); +} + SingleDrvOutputs LocalDerivationGoal::registerOutputs() { diff --git a/src/libstore/build/local-derivation-goal.hh b/src/libstore/build/local-derivation-goal.hh index f3a83d42f..91329ca35 100644 --- a/src/libstore/build/local-derivation-goal.hh +++ b/src/libstore/build/local-derivation-goal.hh @@ -178,7 +178,28 @@ struct LocalDerivationGoal : public DerivationGoal friend struct RestrictedStore; - using DerivationGoal::DerivationGoal; + /** + * Create a LocalDerivationGoal without an on-disk .drv file, + * possibly a platform-specific subclass + */ + static std::shared_ptr makeLocalDerivationGoal( + const StorePath & drvPath, + const OutputsSpec & wantedOutputs, + Worker & worker, + BuildMode buildMode + ); + + /** + * Create a LocalDerivationGoal for an on-disk .drv file, + * possibly a platform-specific subclass + */ + static std::shared_ptr makeLocalDerivationGoal( + const StorePath & drvPath, + const BasicDerivation & drv, + const OutputsSpec & wantedOutputs, + Worker & worker, + BuildMode buildMode + ); virtual ~LocalDerivationGoal() noexcept(false) override; @@ -282,7 +303,7 @@ struct LocalDerivationGoal : public DerivationGoal * Kill any processes running under the build user UID or in the * cgroup of the build. */ - void killSandbox(bool getStats); + virtual void killSandbox(bool getStats); /** * Create alternative path calculated from but distinct from the @@ -299,6 +320,16 @@ struct LocalDerivationGoal : public DerivationGoal * rewrites caught everything */ StorePath makeFallbackPath(OutputNameView outputName); + +protected: + using DerivationGoal::DerivationGoal; + + /** + * Execute the builder, replacing the current process. + * Generally this means an `execve` call. + */ + virtual void execBuilder(std::string builder, Strings args, Strings envStrs); + }; } diff --git a/src/libstore/build/worker.cc b/src/libstore/build/worker.cc index 04be0da99..a7a298c34 100644 --- a/src/libstore/build/worker.cc +++ b/src/libstore/build/worker.cc @@ -65,8 +65,8 @@ std::shared_ptr Worker::makeDerivationGoal(const StorePath & drv { return makeDerivationGoalCommon(drvPath, wantedOutputs, [&]() -> std::shared_ptr { return !dynamic_cast(&store) - ? std::make_shared(drvPath, wantedOutputs, *this, buildMode) - : std::make_shared(drvPath, wantedOutputs, *this, buildMode); + ? std::make_shared(drvPath, wantedOutputs, *this, buildMode) + : LocalDerivationGoal::makeLocalDerivationGoal(drvPath, wantedOutputs, *this, buildMode); }); } @@ -76,8 +76,8 @@ std::shared_ptr Worker::makeBasicDerivationGoal(const StorePath { return makeDerivationGoalCommon(drvPath, wantedOutputs, [&]() -> std::shared_ptr { return !dynamic_cast(&store) - ? std::make_shared(drvPath, drv, wantedOutputs, *this, buildMode) - : std::make_shared(drvPath, drv, wantedOutputs, *this, buildMode); + ? std::make_shared(drvPath, drv, wantedOutputs, *this, buildMode) + : LocalDerivationGoal::makeLocalDerivationGoal(drvPath, drv, wantedOutputs, *this, buildMode); }); } diff --git a/src/libstore/platform.cc b/src/libstore/platform.cc index acdedab99..d10d33f0e 100644 --- a/src/libstore/platform.cc +++ b/src/libstore/platform.cc @@ -1,4 +1,5 @@ #include "local-store.hh" +#include "build/local-derivation-goal.hh" #if __linux__ #include "platform/linux.hh" @@ -19,4 +20,43 @@ std::shared_ptr LocalStore::makeLocalStore(const Params & params) return std::shared_ptr(new FallbackLocalStore(params)); #endif } + +std::shared_ptr LocalDerivationGoal::makeLocalDerivationGoal( + const StorePath & drvPath, + const OutputsSpec & wantedOutputs, + Worker & worker, + BuildMode buildMode +) +{ +#if __linux__ + return std::make_shared(drvPath, wantedOutputs, worker, buildMode); +#elif __APPLE__ + return std::make_shared(drvPath, wantedOutputs, worker, buildMode); +#else + return std::make_shared(drvPath, wantedOutputs, worker, buildMode); +#endif +} + +std::shared_ptr LocalDerivationGoal::makeLocalDerivationGoal( + const StorePath & drvPath, + const BasicDerivation & drv, + const OutputsSpec & wantedOutputs, + Worker & worker, + BuildMode buildMode +) +{ +#if __linux__ + return std::make_shared( + drvPath, drv, wantedOutputs, worker, buildMode + ); +#elif __APPLE__ + return std::make_shared( + drvPath, drv, wantedOutputs, worker, buildMode + ); +#else + return std::make_shared( + drvPath, drv, wantedOutputs, worker, buildMode + ); +#endif +} } diff --git a/src/libstore/platform/darwin.cc b/src/libstore/platform/darwin.cc index bbb81784c..83b4b4183 100644 --- a/src/libstore/platform/darwin.cc +++ b/src/libstore/platform/darwin.cc @@ -6,6 +6,7 @@ #include #include #include +#include #include @@ -220,4 +221,29 @@ void DarwinLocalStore::findPlatformRoots(UncheckedRoots & unchecked) } } } + +void DarwinLocalDerivationGoal::execBuilder(std::string builder, Strings args, Strings envStrs) +{ + posix_spawnattr_t attrp; + + if (posix_spawnattr_init(&attrp)) + throw SysError("failed to initialize builder"); + + if (posix_spawnattr_setflags(&attrp, POSIX_SPAWN_SETEXEC)) + throw SysError("failed to initialize builder"); + + if (drv->platform == "aarch64-darwin") { + // Unset kern.curproc_arch_affinity so we can escape Rosetta + int affinity = 0; + sysctlbyname("kern.curproc_arch_affinity", NULL, NULL, &affinity, sizeof(affinity)); + + cpu_type_t cpu = CPU_TYPE_ARM64; + posix_spawnattr_setbinpref_np(&attrp, 1, &cpu, NULL); + } else if (drv->platform == "x86_64-darwin") { + cpu_type_t cpu = CPU_TYPE_X86_64; + posix_spawnattr_setbinpref_np(&attrp, 1, &cpu, NULL); + } + + posix_spawn(NULL, builder.c_str(), NULL, &attrp, stringsToCharPtrs(args).data(), stringsToCharPtrs(envStrs).data()); +} } diff --git a/src/libstore/platform/darwin.hh b/src/libstore/platform/darwin.hh index b7170aa05..0ac7077fb 100644 --- a/src/libstore/platform/darwin.hh +++ b/src/libstore/platform/darwin.hh @@ -1,6 +1,7 @@ #pragma once ///@file +#include "build/local-derivation-goal.hh" #include "gc-store.hh" #include "local-store.hh" @@ -32,4 +33,19 @@ private: void findPlatformRoots(UncheckedRoots & unchecked) override; }; +/** + * Darwin-specific implementation of LocalDerivationGoal + */ +class DarwinLocalDerivationGoal : public LocalDerivationGoal +{ +public: + using LocalDerivationGoal::LocalDerivationGoal; + +private: + /** + * Set process flags to enter or leave rosetta, then execute the builder + */ + void execBuilder(std::string builder, Strings args, Strings envStrs) override; +}; + } diff --git a/src/libstore/platform/fallback.hh b/src/libstore/platform/fallback.hh index fd27edbe6..3b77536bb 100644 --- a/src/libstore/platform/fallback.hh +++ b/src/libstore/platform/fallback.hh @@ -1,6 +1,7 @@ #pragma once ///@file +#include "build/local-derivation-goal.hh" #include "local-store.hh" namespace nix { @@ -28,4 +29,14 @@ public: } }; +/** + * Fallback platform implementation of LocalDerivationGoal + * Exists so we can make LocalDerivationGoal constructor protected + */ +class FallbackLocalDerivationGoal : public LocalDerivationGoal +{ +public: + using LocalDerivationGoal::LocalDerivationGoal; +}; + } diff --git a/src/libstore/platform/linux.cc b/src/libstore/platform/linux.cc index a34608894..6b94c01cc 100644 --- a/src/libstore/platform/linux.cc +++ b/src/libstore/platform/linux.cc @@ -1,3 +1,4 @@ +#include "cgroup.hh" #include "gc-store.hh" #include "signals.hh" #include "platform/linux.hh" @@ -114,4 +115,17 @@ void LinuxLocalStore::findPlatformRoots(UncheckedRoots & unchecked) readFileRoots("/proc/sys/kernel/fbsplash", unchecked); readFileRoots("/proc/sys/kernel/poweroff_cmd", unchecked); } + +void LinuxLocalDerivationGoal::killSandbox(bool getStats) +{ + if (cgroup) { + auto stats = destroyCgroup(*cgroup); + if (getStats) { + buildResult.cpuUser = stats.cpuUser; + buildResult.cpuSystem = stats.cpuSystem; + } + } else { + LocalDerivationGoal::killSandbox(getStats); + } +} } diff --git a/src/libstore/platform/linux.hh b/src/libstore/platform/linux.hh index 8b97e17c5..2cad001ea 100644 --- a/src/libstore/platform/linux.hh +++ b/src/libstore/platform/linux.hh @@ -1,6 +1,7 @@ #pragma once ///@file +#include "build/local-derivation-goal.hh" #include "gc-store.hh" #include "local-store.hh" @@ -32,4 +33,16 @@ private: void findPlatformRoots(UncheckedRoots & unchecked) override; }; +/** + * Linux-specific implementation of LocalDerivationGoal + */ +class LinuxLocalDerivationGoal : public LocalDerivationGoal +{ +public: + using LocalDerivationGoal::LocalDerivationGoal; + +private: + void killSandbox(bool getStatus) override; +}; + } From 39a1e248c9e7adea312c5b7e1743a8e55da63e6c Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Sat, 6 Apr 2024 22:08:58 +0200 Subject: [PATCH 26/47] libutil: remove sinkToSource eof callback this is only used in one place, and only to set a nicer error message on EndOfFile. the only caller that actually *catches* this exception should provide an error message in that catch block rather than forcing support for setting error message so deep into the stack. copyStorePath is never called outside of PathSubstitutionGoal anyway, which catches everything. Change-Id: Ifbae8706d781c388737706faf4c8a8b7917ca278 --- src/libstore/build/substitution-goal.cc | 12 ++++++-- src/libstore/store-api.cc | 2 -- src/libutil/serialise.cc | 15 +++++----- src/libutil/serialise.hh | 6 +--- tests/functional/meson.build | 1 + tests/functional/substitute-truncated-nar.sh | 29 ++++++++++++++++++++ 6 files changed, 48 insertions(+), 17 deletions(-) create mode 100644 tests/functional/substitute-truncated-nar.sh diff --git a/src/libstore/build/substitution-goal.cc b/src/libstore/build/substitution-goal.cc index d8d9ba283..cc4cb3c8c 100644 --- a/src/libstore/build/substitution-goal.cc +++ b/src/libstore/build/substitution-goal.cc @@ -217,6 +217,7 @@ void PathSubstitutionGoal::tryToRun() promise = std::promise(); thr = std::thread([this]() { + auto & fetchPath = subPath ? *subPath : storePath; try { ReceiveInterrupts receiveInterrupts; @@ -226,10 +227,17 @@ void PathSubstitutionGoal::tryToRun() Activity act(*logger, actSubstitute, Logger::Fields{worker.store.printStorePath(storePath), sub->getUri()}); PushActivity pact(act.id); - copyStorePath(*sub, worker.store, - subPath ? *subPath : storePath, repair, sub->isTrusted ? NoCheckSigs : CheckSigs); + copyStorePath( + *sub, worker.store, fetchPath, repair, sub->isTrusted ? NoCheckSigs : CheckSigs + ); promise.set_value(); + } catch (const EndOfFile &) { + promise.set_exception(std::make_exception_ptr(EndOfFile( + "NAR for '%s' fetched from '%s' is incomplete", + sub->printStorePath(fetchPath), + sub->getUri() + ))); } catch (...) { promise.set_exception(std::current_exception()); } diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 244ecf256..7c0902978 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -1072,8 +1072,6 @@ void copyStorePath( }); TeeSink tee { sink, progressSink }; srcStore.narFromPath(storePath, tee); - }, [&]() { - throw EndOfFile("NAR for '%s' fetched from '%s' is incomplete", srcStore.printStorePath(storePath), srcStore.getUri()); }); dstStore.addToStore(*info, *source, repair, checkSigs); diff --git a/src/libutil/serialise.cc b/src/libutil/serialise.cc index 3a8a01f16..80b111f08 100644 --- a/src/libutil/serialise.cc +++ b/src/libutil/serialise.cc @@ -266,20 +266,17 @@ std::unique_ptr sourceToSink(std::function fun) } -std::unique_ptr sinkToSource( - std::function fun, - std::function eof) +std::unique_ptr sinkToSource(std::function fun) { struct SinkToSource : Source { typedef boost::coroutines2::coroutine coro_t; std::function fun; - std::function eof; std::optional coro; - SinkToSource(std::function fun, std::function eof) - : fun(fun), eof(eof) + SinkToSource(std::function fun) + : fun(fun) { } @@ -298,7 +295,9 @@ std::unique_ptr sinkToSource( }); } - if (!*coro) { eof(); abort(); } + if (!*coro) { + throw EndOfFile("coroutine has finished"); + } if (pos == cur.size()) { if (!cur.empty()) { @@ -317,7 +316,7 @@ std::unique_ptr sinkToSource( } }; - return std::make_unique(fun, eof); + return std::make_unique(fun); } diff --git a/src/libutil/serialise.hh b/src/libutil/serialise.hh index c9294ba2d..0632e3109 100644 --- a/src/libutil/serialise.hh +++ b/src/libutil/serialise.hh @@ -338,11 +338,7 @@ std::unique_ptr sourceToSink(std::function fun); * Convert a function that feeds data into a Sink into a Source. The * Source executes the function as a coroutine. */ -std::unique_ptr sinkToSource( - std::function fun, - std::function eof = []() { - throw EndOfFile("coroutine has finished"); - }); +std::unique_ptr sinkToSource(std::function fun); void writePadding(size_t len, Sink & sink); diff --git a/tests/functional/meson.build b/tests/functional/meson.build index a13dee001..eede1834c 100644 --- a/tests/functional/meson.build +++ b/tests/functional/meson.build @@ -182,6 +182,7 @@ functional_tests_scripts = [ 'debugger.sh', 'test-libstoreconsumer.sh', 'extra-sandbox-profile.sh', + 'substitute-truncated-nar.sh', ] # Plugin tests require shared libraries support. diff --git a/tests/functional/substitute-truncated-nar.sh b/tests/functional/substitute-truncated-nar.sh new file mode 100644 index 000000000..1ac7efaf6 --- /dev/null +++ b/tests/functional/substitute-truncated-nar.sh @@ -0,0 +1,29 @@ +source common.sh + +BINARY_CACHE=file://$cacheDir + +build() { + nix-build --no-out-link "$@" --expr 'derivation { + name = "text"; + system = builtins.currentSystem; + builder = "/bin/sh"; + args = [ "-c" "echo some text to make the nar less empty > $out" ]; + }' +} + +path=$(build) +nix copy --to "$BINARY_CACHE" "$path" +nix-collect-garbage >/dev/null 2>&1 + +nar=0bylmx35yjy2b1b4k7gjsl7i4vc03cpmryb41grfb1mp40n3hifl.nar.xz + +[ -e $cacheDir/nar/$nar ] || fail "long nar missing?" + +xzcat $cacheDir/nar/$nar > $TEST_HOME/tmp +truncate -s $(( $(stat -c %s $TEST_HOME/tmp) - 10 )) $TEST_HOME/tmp +xz < $TEST_HOME/tmp > $cacheDir/nar/$nar + +# Copying back '$path' from the binary cache. This should fail as it is truncated +if build --option substituters "$BINARY_CACHE" --option require-sigs false -j0; then + fail "Importing a truncated nar should fail" +fi From a9949f4760fea60ee790c27629e9620bf3ab5e3f Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Thu, 21 Mar 2024 15:54:41 +0100 Subject: [PATCH 27/47] libutil: add some serialize.hh serializer tests Change-Id: I0116265a18bc44bba16c07bf419af70d5195f07d --- tests/unit/libutil/serialise.cc | 166 ++++++++++++++++++++++++++++++++ tests/unit/meson.build | 1 + 2 files changed, 167 insertions(+) create mode 100644 tests/unit/libutil/serialise.cc diff --git a/tests/unit/libutil/serialise.cc b/tests/unit/libutil/serialise.cc new file mode 100644 index 000000000..95ae43115 --- /dev/null +++ b/tests/unit/libutil/serialise.cc @@ -0,0 +1,166 @@ +#include "serialise.hh" +#include "error.hh" +#include "fmt.hh" +#include "pos-table.hh" +#include "ref.hh" +#include "types.hh" + +#include +#include + +#include + +namespace nix { + +TEST(Sink, uint64_t) +{ + StringSink s; + s << 42; + ASSERT_EQ(s.s, std::string({42, 0, 0, 0, 0, 0, 0, 0})); +} + +TEST(Sink, string_view) +{ + StringSink s; + s << ""; + // clang-format off + ASSERT_EQ( + s.s, + std::string({ + // length + 0, 0, 0, 0, 0, 0, 0, 0, + // data (omitted) + }) + ); + // clang-format on + + s = {}; + s << "test"; + // clang-format off + ASSERT_EQ( + s.s, + std::string({ + // length + 4, 0, 0, 0, 0, 0, 0, 0, + // data + 't', 'e', 's', 't', + // padding + 0, 0, 0, 0, + }) + ); + // clang-format on + + s = {}; + s << "longer string"; + // clang-format off + ASSERT_EQ( + s.s, + std::string({ + // length + 13, 0, 0, 0, 0, 0, 0, 0, + // data + 'l', 'o', 'n', 'g', 'e', 'r', ' ', 's', 't', 'r', 'i', 'n', 'g', + // padding + 0, 0, 0, + }) + ); + // clang-format on +} + +TEST(Sink, StringSet) +{ + StringSink s; + s << StringSet{}; + // clang-format off + ASSERT_EQ( + s.s, + std::string({ + // length + 0, 0, 0, 0, 0, 0, 0, 0, + // data (omitted) + }) + ); + // clang-format on + + s = {}; + s << StringSet{"a", ""}; + // clang-format off + ASSERT_EQ( + s.s, + std::string({ + // length + 2, 0, 0, 0, 0, 0, 0, 0, + // data "" + 0, 0, 0, 0, 0, 0, 0, 0, + // data "a" + 1, 0, 0, 0, 0, 0, 0, 0, 'a', 0, 0, 0, 0, 0, 0, 0, + }) + ); + // clang-format on +} + +TEST(Sink, Strings) +{ + StringSink s; + s << Strings{}; + // clang-format off + ASSERT_EQ( + s.s, + std::string({ + // length + 0, 0, 0, 0, 0, 0, 0, 0, + // data (omitted) + }) + ); + // clang-format on + + s = {}; + s << Strings{"a", ""}; + // clang-format off + ASSERT_EQ( + s.s, + std::string({ + // length + 2, 0, 0, 0, 0, 0, 0, 0, + // data "a" + 1, 0, 0, 0, 0, 0, 0, 0, 'a', 0, 0, 0, 0, 0, 0, 0, + // data "" + 0, 0, 0, 0, 0, 0, 0, 0, + }) + ); + // clang-format on +} + +TEST(Sink, Error) +{ + PosTable pt; + auto o = pt.addOrigin(Pos::String{make_ref("test")}, 4); + + StringSink s; + s << Error{ErrorInfo{ + .level = lvlInfo, + .msg = HintFmt("foo"), + .pos = pt[pt.add(o, 1)], + .traces = {{.pos = pt[pt.add(o, 2)], .hint = HintFmt("b %1%", "foo")}}, + }}; + // NOTE position of the error and all traces are ignored + // by the wire format + // clang-format off + ASSERT_EQ( + s.s, + std::string({ + 5, 0, 0, 0, 0, 0, 0, 0, 'E', 'r', 'r', 'o', 'r', 0, 0, 0, + 3, 0, 0, 0, 0, 0, 0, 0, + 5, 0, 0, 0, 0, 0, 0, 0, 'E', 'r', 'r', 'o', 'r', 0, 0, 0, + 3, 0, 0, 0, 0, 0, 0, 0, 'f', 'o', 'o', 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, + 1, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, + 16, 0, 0, 0, 0, 0, 0, 0, + 'b', ' ', '\x1b', '[', '3', '5', ';', '1', 'm', 'f', 'o', 'o', '\x1b', '[', '0', 'm', + }) + ); + // clang-format on +} + +} diff --git a/tests/unit/meson.build b/tests/unit/meson.build index 9f19e5fd9..2b5471526 100644 --- a/tests/unit/meson.build +++ b/tests/unit/meson.build @@ -49,6 +49,7 @@ libutil_tests_sources = files( 'libutil/paths-setting.cc', 'libutil/pool.cc', 'libutil/references.cc', + 'libutil/serialise.cc', 'libutil/suggestions.cc', 'libutil/tests.cc', 'libutil/url.cc', From b43a2e84c4b2fa7cb1167693652702e6dac95f53 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Fri, 5 Apr 2024 20:35:13 +0200 Subject: [PATCH 28/47] libutil: make Pid -> pid_t operations explicit Change-Id: I3137cc140590001fe7ba542844e735944a0a9255 --- src/libmain/shared.cc | 2 +- src/libstore/build/hook-instance.cc | 2 +- src/libstore/build/local-derivation-goal.cc | 16 ++++++++-------- src/libstore/ssh.cc | 2 +- src/libutil/processes.cc | 6 ------ src/libutil/processes.hh | 3 ++- 6 files changed, 13 insertions(+), 18 deletions(-) diff --git a/src/libmain/shared.cc b/src/libmain/shared.cc index f99777a20..29538a9ca 100644 --- a/src/libmain/shared.cc +++ b/src/libmain/shared.cc @@ -378,7 +378,7 @@ RunPager::RunPager() RunPager::~RunPager() { try { - if (pid != -1) { + if (pid) { std::cout.flush(); dup2(std_out, STDOUT_FILENO); pid.wait(); diff --git a/src/libstore/build/hook-instance.cc b/src/libstore/build/hook-instance.cc index 86f72486e..9a93646f4 100644 --- a/src/libstore/build/hook-instance.cc +++ b/src/libstore/build/hook-instance.cc @@ -79,7 +79,7 @@ HookInstance::~HookInstance() { try { toHook.writeSide.reset(); - if (pid != -1) pid.kill(); + if (pid) pid.kill(); } catch (...) { ignoreException(); } diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 4e616c838..eb6229c56 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -137,7 +137,7 @@ LocalStore & LocalDerivationGoal::getLocalStore() void LocalDerivationGoal::killChild() { - if (pid != -1) { + if (pid) { worker.childTerminated(this); /* If we're using a build user, then there is a tricky race @@ -145,7 +145,7 @@ void LocalDerivationGoal::killChild() done its setuid() to the build user uid, then it won't be killed, and we'll potentially lock up in pid.wait(). So also send a conventional kill to the child. */ - ::kill(-pid, SIGKILL); /* ignore the result */ + ::kill(-pid.get(), SIGKILL); /* ignore the result */ killSandbox(true); @@ -961,13 +961,13 @@ void LocalDerivationGoal::startBuilder() uid_t hostGid = buildUser ? buildUser->getGID() : getgid(); uid_t nrIds = buildUser ? buildUser->getUIDCount() : 1; - writeFile("/proc/" + std::to_string(pid) + "/uid_map", + writeFile("/proc/" + std::to_string(pid.get()) + "/uid_map", fmt("%d %d %d", sandboxUid(), hostUid, nrIds)); if (!buildUser || buildUser->getUIDCount() == 1) - writeFile("/proc/" + std::to_string(pid) + "/setgroups", "deny"); + writeFile("/proc/" + std::to_string(pid.get()) + "/setgroups", "deny"); - writeFile("/proc/" + std::to_string(pid) + "/gid_map", + writeFile("/proc/" + std::to_string(pid.get()) + "/gid_map", fmt("%d %d %d", sandboxGid(), hostGid, nrIds)); } else { debug("note: not using a user namespace"); @@ -990,19 +990,19 @@ void LocalDerivationGoal::startBuilder() /* Save the mount- and user namespace of the child. We have to do this *before* the child does a chroot. */ - sandboxMountNamespace = AutoCloseFD{open(fmt("/proc/%d/ns/mnt", (pid_t) pid).c_str(), O_RDONLY)}; + sandboxMountNamespace = AutoCloseFD{open(fmt("/proc/%d/ns/mnt", pid.get()).c_str(), O_RDONLY)}; if (sandboxMountNamespace.get() == -1) throw SysError("getting sandbox mount namespace"); if (usingUserNamespace) { - sandboxUserNamespace = AutoCloseFD{open(fmt("/proc/%d/ns/user", (pid_t) pid).c_str(), O_RDONLY)}; + sandboxUserNamespace = AutoCloseFD{open(fmt("/proc/%d/ns/user", pid.get()).c_str(), O_RDONLY)}; if (sandboxUserNamespace.get() == -1) throw SysError("getting sandbox user namespace"); } /* Move the child into its own cgroup. */ if (cgroup) - writeFile(*cgroup + "/cgroup.procs", fmt("%d", (pid_t) pid)); + writeFile(*cgroup + "/cgroup.procs", fmt("%d", pid.get())); /* Signal the builder that we've updated its user namespace. */ writeFull(userNamespaceSync.writeSide.get(), "1"); diff --git a/src/libstore/ssh.cc b/src/libstore/ssh.cc index 932ebaa52..0d7bfa01d 100644 --- a/src/libstore/ssh.cc +++ b/src/libstore/ssh.cc @@ -131,7 +131,7 @@ Path SSHMaster::startMaster() auto state(state_.lock()); - if (state->sshMaster != -1) return state->socketPath; + if (state->sshMaster) return state->socketPath; state->socketPath = (Path) *state->tmpDir + "/ssh.sock"; diff --git a/src/libutil/processes.cc b/src/libutil/processes.cc index 8639a77f8..3bb419e45 100644 --- a/src/libutil/processes.cc +++ b/src/libutil/processes.cc @@ -55,12 +55,6 @@ void Pid::operator =(pid_t pid) } -Pid::operator pid_t() -{ - return pid; -} - - int Pid::kill() { assert(pid != -1); diff --git a/src/libutil/processes.hh b/src/libutil/processes.hh index b84fc7c4b..2079e1b36 100644 --- a/src/libutil/processes.hh +++ b/src/libutil/processes.hh @@ -29,13 +29,14 @@ public: Pid(pid_t pid); ~Pid() noexcept(false); void operator =(pid_t pid); - operator pid_t(); + explicit operator bool() const { return pid != -1; } int kill(); int wait(); void setSeparatePG(bool separatePG); void setKillSignal(int signal); pid_t release(); + pid_t get() const { return pid; } }; /** From 3d155fc509e19354ba3798b1cc1b9cbcdb789c85 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Fri, 5 Apr 2024 21:02:04 +0200 Subject: [PATCH 29/47] libutil: give Pid proper resource semantics copy-constructing or assigning from pid_t can easily lead to duplicate Pid instances for the same process if a pid_t was used carelessly, and Pid itself was copy-constructible. both could cause surprising results such as killing processes twice (which could become very problemantic, but luckily modern systems don't reuse PIDs all that quickly), or more than one piece of the code believing it owns a process when neither do Change-Id: Ifea7445f84200b34c1a1d0acc2cdffe0f01e20c6 --- src/libmain/shared.cc | 4 +-- src/libstore/build/hook-instance.cc | 4 +-- src/libstore/build/local-derivation-goal.cc | 10 +++---- src/libstore/ssh.cc | 8 +++--- src/libutil/namespaces.cc | 12 +++------ src/libutil/processes.cc | 30 +++++++++++---------- src/libutil/processes.hh | 5 ++-- src/libutil/unix-domain-socket.cc | 4 +-- 8 files changed, 37 insertions(+), 40 deletions(-) diff --git a/src/libmain/shared.cc b/src/libmain/shared.cc index 29538a9ca..5dabd0c8e 100644 --- a/src/libmain/shared.cc +++ b/src/libmain/shared.cc @@ -354,7 +354,7 @@ RunPager::RunPager() Pipe toPager; toPager.create(); - pid = startProcess([&]() { + pid = Pid{startProcess([&]() { if (dup2(toPager.readSide.get(), STDIN_FILENO) == -1) throw SysError("dupping stdin"); if (!getenv("LESS")) @@ -366,7 +366,7 @@ RunPager::RunPager() execlp("less", "less", nullptr); execlp("more", "more", nullptr); throw SysError("executing '%1%'", pager); - }); + })}; pid.setKillSignal(SIGINT); std_out = fcntl(STDOUT_FILENO, F_DUPFD_CLOEXEC, 0); diff --git a/src/libstore/build/hook-instance.cc b/src/libstore/build/hook-instance.cc index 9a93646f4..b8d8fe7a4 100644 --- a/src/libstore/build/hook-instance.cc +++ b/src/libstore/build/hook-instance.cc @@ -35,7 +35,7 @@ HookInstance::HookInstance() builderOut.create(); /* Fork the hook. */ - pid = startProcess([&]() { + pid = Pid{startProcess([&]() { if (dup2(fromHook.writeSide.get(), STDERR_FILENO) == -1) throw SysError("cannot pipe standard error into log file"); @@ -60,7 +60,7 @@ HookInstance::HookInstance() execv(buildHook.c_str(), stringsToCharPtrs(args).data()); throw SysError("executing '%s'", buildHook); - }); + })}; pid.setSeparatePG(true); fromHook.writeSide.reset(); diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index eb6229c56..a2ff495bf 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -906,7 +906,7 @@ void LocalDerivationGoal::startBuilder() Pipe sendPid; sendPid.create(); - Pid helper = startProcess([&]() { + Pid helper{startProcess([&]() { sendPid.readSide.close(); /* We need to open the slave early, before @@ -934,7 +934,7 @@ void LocalDerivationGoal::startBuilder() writeFull(sendPid.writeSide.get(), fmt("%d\n", child)); _exit(0); - }); + })}; sendPid.writeSide.close(); @@ -951,7 +951,7 @@ void LocalDerivationGoal::startBuilder() auto ss = tokenizeString>(readLine(sendPid.readSide.get())); assert(ss.size() == 1); - pid = string2Int(ss[0]).value(); + pid = Pid{string2Int(ss[0]).value()}; if (usingUserNamespace) { /* Set the UID/GID mapping of the builder's user namespace @@ -1010,10 +1010,10 @@ void LocalDerivationGoal::startBuilder() } else #endif { - pid = startProcess([&]() { + pid = Pid{startProcess([&]() { openSlave(); runChild(); - }); + })}; } /* parent */ diff --git a/src/libstore/ssh.cc b/src/libstore/ssh.cc index 0d7bfa01d..c40eba894 100644 --- a/src/libstore/ssh.cc +++ b/src/libstore/ssh.cc @@ -70,7 +70,7 @@ std::unique_ptr SSHMaster::startCommand(const std::string } Finally cleanup = [&]() { logger->resume(); }; - conn->sshPid = startProcess([&]() { + conn->sshPid = Pid{startProcess([&]() { restoreProcessContext(); close(in.writeSide.get()); @@ -99,7 +99,7 @@ std::unique_ptr SSHMaster::startCommand(const std::string // could not exec ssh/bash throw SysError("unable to execute '%s'", args.front()); - }, options); + }, options)}; in.readSide.reset(); @@ -147,7 +147,7 @@ Path SSHMaster::startMaster() if (isMasterRunning()) return state->socketPath; - state->sshMaster = startProcess([&]() { + state->sshMaster = Pid{startProcess([&]() { restoreProcessContext(); close(out.readSide.get()); @@ -160,7 +160,7 @@ Path SSHMaster::startMaster() execvp(args.begin()->c_str(), stringsToCharPtrs(args).data()); throw SysError("unable to execute '%s'", args.front()); - }, options); + }, options)}; out.writeSide.reset(); diff --git a/src/libutil/namespaces.cc b/src/libutil/namespaces.cc index 98d3cd306..4b7a1b577 100644 --- a/src/libutil/namespaces.cc +++ b/src/libutil/namespaces.cc @@ -94,12 +94,7 @@ bool userNamespacesSupported() static auto res = [&]() -> bool { try { - Pid pid = startProcess([&]() - { - _exit(0); - }, { - .cloneFlags = CLONE_NEWUSER - }); + Pid pid{startProcess([&]() { _exit(0); }, {.cloneFlags = CLONE_NEWUSER})}; auto r = pid.wait(); assert(!r); @@ -120,8 +115,7 @@ bool mountAndPidNamespacesSupported() { try { - Pid pid = startProcess([&]() - { + Pid pid{startProcess([&]() { /* Make sure we don't remount the parent's /proc. */ if (mount(0, "/", 0, MS_PRIVATE | MS_REC, 0) == -1) _exit(1); @@ -136,7 +130,7 @@ bool mountAndPidNamespacesSupported() _exit(0); }, { .cloneFlags = CLONE_NEWNS | CLONE_NEWPID | (userNamespacesSupported() ? CLONE_NEWUSER : 0) - }); + })}; if (pid.wait()) { debug("PID namespaces do not work on this system: cannot remount /proc"); diff --git a/src/libutil/processes.cc b/src/libutil/processes.cc index 3bb419e45..9c99f047c 100644 --- a/src/libutil/processes.cc +++ b/src/libutil/processes.cc @@ -35,9 +35,19 @@ Pid::Pid() } -Pid::Pid(pid_t pid) - : pid(pid) +Pid::Pid(Pid && other) : pid(other.pid), separatePG(other.separatePG), killSignal(other.killSignal) { + other.pid = -1; +} + + +Pid & Pid::operator=(Pid && other) +{ + Pid tmp(std::move(other)); + std::swap(pid, tmp.pid); + std::swap(separatePG, tmp.separatePG); + std::swap(killSignal, tmp.killSignal); + return *this; } @@ -47,14 +57,6 @@ Pid::~Pid() noexcept(false) } -void Pid::operator =(pid_t pid) -{ - if (this->pid != -1 && this->pid != pid) kill(); - this->pid = pid; - killSignal = SIGKILL; // reset signal to default -} - - int Pid::kill() { assert(pid != -1); @@ -125,7 +127,7 @@ void killUser(uid_t uid) users to which the current process can send signals. So we fork a process, switch to uid, and send a mass kill. */ - Pid pid = startProcess([&]() { + Pid pid{startProcess([&]() { if (setuid(uid) == -1) throw SysError("setting uid"); @@ -147,7 +149,7 @@ void killUser(uid_t uid) } _exit(0); - }); + })}; int status = pid.wait(); if (status != 0) @@ -288,7 +290,7 @@ void runProgram2(const RunOptions & options) } /* Fork. */ - Pid pid = startProcess([&]() { + Pid pid{startProcess([&]() { if (options.environment) replaceEnv(*options.environment); if (options.standardOut && dup2(out.writeSide.get(), STDOUT_FILENO) == -1) @@ -322,7 +324,7 @@ void runProgram2(const RunOptions & options) execv(options.program.c_str(), stringsToCharPtrs(args_).data()); throw SysError("executing '%1%'", options.program); - }, processOptions); + }, processOptions)}; out.writeSide.close(); diff --git a/src/libutil/processes.hh b/src/libutil/processes.hh index 2079e1b36..8fc241ab1 100644 --- a/src/libutil/processes.hh +++ b/src/libutil/processes.hh @@ -26,9 +26,10 @@ class Pid int killSignal = SIGKILL; public: Pid(); - Pid(pid_t pid); + explicit Pid(pid_t pid): pid(pid) {} + Pid(Pid && other); + Pid & operator=(Pid && other); ~Pid() noexcept(false); - void operator =(pid_t pid); explicit operator bool() const { return pid != -1; } int kill(); int wait(); diff --git a/src/libutil/unix-domain-socket.cc b/src/libutil/unix-domain-socket.cc index a6e46ca50..d4fc37fab 100644 --- a/src/libutil/unix-domain-socket.cc +++ b/src/libutil/unix-domain-socket.cc @@ -65,7 +65,7 @@ static void bindConnectProcHelper( if (path.size() + 1 >= sizeof(addr.sun_path)) { Pipe pipe; pipe.create(); - Pid pid = startProcess([&] { + Pid pid{startProcess([&] { try { pipe.readSide.close(); Path dir = dirOf(path); @@ -83,7 +83,7 @@ static void bindConnectProcHelper( } catch (...) { writeFull(pipe.writeSide.get(), "-1\n"); } - }); + })}; pipe.writeSide.close(); auto errNo = string2Int(chomp(drainFD(pipe.readSide.get()))); if (!errNo || *errNo == -1) From ce6cb14995e869cfea395570ccb300b0369c72dc Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Fri, 5 Apr 2024 21:15:22 +0200 Subject: [PATCH 30/47] libutil: return Pid from startProcess, not pid_t Change-Id: Icc8a15090c77f54ea7d9220aadedcd4a19922814 --- src/libmain/shared.cc | 4 ++-- src/libstore/build/hook-instance.cc | 4 ++-- src/libstore/build/local-derivation-goal.cc | 14 +++++++------- src/libstore/ssh.cc | 8 ++++---- src/libutil/namespaces.cc | 6 +++--- src/libutil/processes.cc | 4 ++-- src/libutil/processes.hh | 3 ++- src/nix/daemon.cc | 2 +- .../repl_characterization/test-session.cc | 4 ++-- .../repl_characterization/test-session.hh | 3 ++- 10 files changed, 27 insertions(+), 25 deletions(-) diff --git a/src/libmain/shared.cc b/src/libmain/shared.cc index 5dabd0c8e..29538a9ca 100644 --- a/src/libmain/shared.cc +++ b/src/libmain/shared.cc @@ -354,7 +354,7 @@ RunPager::RunPager() Pipe toPager; toPager.create(); - pid = Pid{startProcess([&]() { + pid = startProcess([&]() { if (dup2(toPager.readSide.get(), STDIN_FILENO) == -1) throw SysError("dupping stdin"); if (!getenv("LESS")) @@ -366,7 +366,7 @@ RunPager::RunPager() execlp("less", "less", nullptr); execlp("more", "more", nullptr); throw SysError("executing '%1%'", pager); - })}; + }); pid.setKillSignal(SIGINT); std_out = fcntl(STDOUT_FILENO, F_DUPFD_CLOEXEC, 0); diff --git a/src/libstore/build/hook-instance.cc b/src/libstore/build/hook-instance.cc index b8d8fe7a4..9a93646f4 100644 --- a/src/libstore/build/hook-instance.cc +++ b/src/libstore/build/hook-instance.cc @@ -35,7 +35,7 @@ HookInstance::HookInstance() builderOut.create(); /* Fork the hook. */ - pid = Pid{startProcess([&]() { + pid = startProcess([&]() { if (dup2(fromHook.writeSide.get(), STDERR_FILENO) == -1) throw SysError("cannot pipe standard error into log file"); @@ -60,7 +60,7 @@ HookInstance::HookInstance() execv(buildHook.c_str(), stringsToCharPtrs(args).data()); throw SysError("executing '%s'", buildHook); - })}; + }); pid.setSeparatePG(true); fromHook.writeSide.reset(); diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index a2ff495bf..efba648a4 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -906,7 +906,7 @@ void LocalDerivationGoal::startBuilder() Pipe sendPid; sendPid.create(); - Pid helper{startProcess([&]() { + Pid helper = startProcess([&]() { sendPid.readSide.close(); /* We need to open the slave early, before @@ -930,11 +930,11 @@ void LocalDerivationGoal::startBuilder() if (usingUserNamespace) options.cloneFlags |= CLONE_NEWUSER; - pid_t child = startProcess([&]() { runChild(); }, options); + pid_t child = startProcess([&]() { runChild(); }, options).release(); writeFull(sendPid.writeSide.get(), fmt("%d\n", child)); _exit(0); - })}; + }); sendPid.writeSide.close(); @@ -1010,10 +1010,10 @@ void LocalDerivationGoal::startBuilder() } else #endif { - pid = Pid{startProcess([&]() { + pid = startProcess([&]() { openSlave(); runChild(); - })}; + }); } /* parent */ @@ -1570,7 +1570,7 @@ void LocalDerivationGoal::addDependency(const StorePath & path) entering its mount namespace, which is not possible in multithreaded programs. So we do this in a child process.*/ - Pid child(startProcess([&]() { + Pid child = startProcess([&]() { if (usingUserNamespace && (setns(sandboxUserNamespace.get(), 0) == -1)) throw SysError("entering sandbox user namespace"); @@ -1581,7 +1581,7 @@ void LocalDerivationGoal::addDependency(const StorePath & path) doBind(source, target); _exit(0); - })); + }); int status = child.wait(); if (status != 0) diff --git a/src/libstore/ssh.cc b/src/libstore/ssh.cc index c40eba894..0d7bfa01d 100644 --- a/src/libstore/ssh.cc +++ b/src/libstore/ssh.cc @@ -70,7 +70,7 @@ std::unique_ptr SSHMaster::startCommand(const std::string } Finally cleanup = [&]() { logger->resume(); }; - conn->sshPid = Pid{startProcess([&]() { + conn->sshPid = startProcess([&]() { restoreProcessContext(); close(in.writeSide.get()); @@ -99,7 +99,7 @@ std::unique_ptr SSHMaster::startCommand(const std::string // could not exec ssh/bash throw SysError("unable to execute '%s'", args.front()); - }, options)}; + }, options); in.readSide.reset(); @@ -147,7 +147,7 @@ Path SSHMaster::startMaster() if (isMasterRunning()) return state->socketPath; - state->sshMaster = Pid{startProcess([&]() { + state->sshMaster = startProcess([&]() { restoreProcessContext(); close(out.readSide.get()); @@ -160,7 +160,7 @@ Path SSHMaster::startMaster() execvp(args.begin()->c_str(), stringsToCharPtrs(args).data()); throw SysError("unable to execute '%s'", args.front()); - }, options)}; + }, options); out.writeSide.reset(); diff --git a/src/libutil/namespaces.cc b/src/libutil/namespaces.cc index 4b7a1b577..247fba2c4 100644 --- a/src/libutil/namespaces.cc +++ b/src/libutil/namespaces.cc @@ -94,7 +94,7 @@ bool userNamespacesSupported() static auto res = [&]() -> bool { try { - Pid pid{startProcess([&]() { _exit(0); }, {.cloneFlags = CLONE_NEWUSER})}; + Pid pid = startProcess([&]() { _exit(0); }, {.cloneFlags = CLONE_NEWUSER}); auto r = pid.wait(); assert(!r); @@ -115,7 +115,7 @@ bool mountAndPidNamespacesSupported() { try { - Pid pid{startProcess([&]() { + Pid pid = startProcess([&]() { /* Make sure we don't remount the parent's /proc. */ if (mount(0, "/", 0, MS_PRIVATE | MS_REC, 0) == -1) _exit(1); @@ -130,7 +130,7 @@ bool mountAndPidNamespacesSupported() _exit(0); }, { .cloneFlags = CLONE_NEWNS | CLONE_NEWPID | (userNamespacesSupported() ? CLONE_NEWUSER : 0) - })}; + }); if (pid.wait()) { debug("PID namespaces do not work on this system: cannot remount /proc"); diff --git a/src/libutil/processes.cc b/src/libutil/processes.cc index 9c99f047c..e8af12fbd 100644 --- a/src/libutil/processes.cc +++ b/src/libutil/processes.cc @@ -183,7 +183,7 @@ static int childEntry(void * arg) #endif -pid_t startProcess(std::function fun, const ProcessOptions & options) +Pid startProcess(std::function fun, const ProcessOptions & options) { std::function wrapper = [&]() { logger = makeSimpleLogger(); @@ -227,7 +227,7 @@ pid_t startProcess(std::function fun, const ProcessOptions & options) if (pid == -1) throw SysError("unable to fork"); - return pid; + return Pid{pid}; } std::string runProgram(Path program, bool searchPath, const Strings & args, diff --git a/src/libutil/processes.hh b/src/libutil/processes.hh index 8fc241ab1..001e1fa12 100644 --- a/src/libutil/processes.hh +++ b/src/libutil/processes.hh @@ -62,7 +62,8 @@ struct ProcessOptions int cloneFlags = 0; }; -pid_t startProcess(std::function fun, const ProcessOptions & options = ProcessOptions()); +[[nodiscard]] +Pid startProcess(std::function fun, const ProcessOptions & options = ProcessOptions()); /** diff --git a/src/nix/daemon.cc b/src/nix/daemon.cc index f1cc1ee9c..a9211d64a 100644 --- a/src/nix/daemon.cc +++ b/src/nix/daemon.cc @@ -369,7 +369,7 @@ static void daemonLoop(std::optional forceTrustClientOpt) processConnection(openUncachedStore(), from, to, trusted, NotRecursive); exit(0); - }, options); + }, options).release(); } catch (Interrupted & e) { return; diff --git a/tests/functional/repl_characterization/test-session.cc b/tests/functional/repl_characterization/test-session.cc index e59064fc5..96f101cdd 100644 --- a/tests/functional/repl_characterization/test-session.cc +++ b/tests/functional/repl_characterization/test-session.cc @@ -22,7 +22,7 @@ RunningProcess RunningProcess::start(std::string executable, Strings args) procStdout.create(); // This is separate from runProgram2 because we have different IO requirements - pid_t pid = startProcess([&]() { + auto pid = startProcess([&]() { if (dup2(procStdout.writeSide.get(), STDOUT_FILENO) == -1) { throw SysError("dupping stdout"); } @@ -42,7 +42,7 @@ RunningProcess RunningProcess::start(std::string executable, Strings args) procStdin.readSide.close(); return RunningProcess{ - .pid = pid, + .pid = std::move(pid), .procStdin = std::move(procStdin), .procStdout = std::move(procStdout), }; diff --git a/tests/functional/repl_characterization/test-session.hh b/tests/functional/repl_characterization/test-session.hh index c77cce6d5..a9ba9cf0c 100644 --- a/tests/functional/repl_characterization/test-session.hh +++ b/tests/functional/repl_characterization/test-session.hh @@ -7,13 +7,14 @@ #include #include "file-descriptor.hh" +#include "processes.hh" #include "tests/terminal-code-eater.hh" namespace nix { struct RunningProcess { - pid_t pid; + Pid pid; Pipe procStdin; Pipe procStdout; From ce4dee0fa55b15d89e443bdce247a204bf3abeb1 Mon Sep 17 00:00:00 2001 From: Qyriad Date: Thu, 20 Jun 2024 13:50:31 -0600 Subject: [PATCH 31/47] 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 b3b46fc21..28bb14863 100644 --- a/src/libmain/progress-bar.cc +++ b/src/libmain/progress-bar.cc @@ -402,31 +402,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 701eb332bd1a3a4196050aa78ee90521279349cb Mon Sep 17 00:00:00 2001 From: Qyriad Date: Thu, 20 Jun 2024 18:00:25 -0600 Subject: [PATCH 32/47] 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 115b979f8..3cead4296 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 2fe9157808ab26e7ca5ac2b7e800c44067d1ef8e Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Sun, 23 Jun 2024 15:03:37 +0200 Subject: [PATCH 33/47] flakes: add --commit-lock-file message test we had no test to ensure that we generated a commit message at all? Change-Id: Ic9aa8fde92b83e1ea6f61cd2a21867aa73d4e885 --- tests/functional/flakes/flakes.sh | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/functional/flakes/flakes.sh b/tests/functional/flakes/flakes.sh index 3ef518b23..68a2fd2ce 100644 --- a/tests/functional/flakes/flakes.sh +++ b/tests/functional/flakes/flakes.sh @@ -278,6 +278,19 @@ git -C $flake3Dir commit -m 'Add nonFlakeInputs' # Check whether `nix build` works with a lockfile which is missing a # nonFlakeInputs. nix build -o $TEST_ROOT/result $flake3Dir#sth --commit-lock-file +# check that the commit message is broadly correct. we can't check for +# exact contents of the message becase the build dirs change too much. +[[ "$(git -C $flake3Dir show -s --format=format:%B)" = \ +"flake.lock: Update + +Flake lock file updates: + +"?" Added input 'nonFlake': + 'git+file://"*"/flakes/flakes/nonFlake?ref=refs/heads/master&rev="*"' "*" +"?" Added input 'nonFlakeFile': + 'path:"*"/flakes/flakes/nonFlake/README.md?lastModified="*"&narHash=sha256-cPh6hp48IOdRxVV3xGd0PDgSxgzj5N/2cK0rMPNaR4o%3D' "*" +"?" Added input 'nonFlakeFile2': + 'path:"*"/flakes/flakes/nonFlake/README.md?lastModified="*"&narHash=sha256-cPh6hp48IOdRxVV3xGd0PDgSxgzj5N/2cK0rMPNaR4o%3D' "* ]] nix build -o $TEST_ROOT/result flake3#fnord [[ $(cat $TEST_ROOT/result) = FNORD ]] From 2bbdaf0b19066ea1764e8d5810c2b250dbf0a850 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Sun, 23 Jun 2024 15:10:31 +0200 Subject: [PATCH 34/47] libfetchers: write git commit message to tempfile we want to remove runProgram's ability to provide stdin to a process because the concurrency issues of handling both stdin and stdout are much more pronounced once runProgram returns not is collected output but a source. this is possible in the current c++ framework, however it isn't necessary in almost all cases (as demonstrated by only this single user existing) and in much better handled with a proper async concurrency model that lets the caller handle both at the same time. Change-Id: I29da1e1ad898d45e2e33a7320b246d5003e7712b --- src/libfetchers/git.cc | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc index 0cb826075..f2d577914 100644 --- a/src/libfetchers/git.cc +++ b/src/libfetchers/git.cc @@ -396,12 +396,15 @@ struct GitInputScheme : InputScheme if (commitMsg) { + auto [_fd, msgPath] = createTempFile("nix-msg"); + AutoDelete const _delete{msgPath}; + writeFile(msgPath, *commitMsg); + // Pause the logger to allow for user input (such as a gpg passphrase) in `git commit` logger->pause(); Finally restoreLogger([]() { logger->resume(); }); runProgram("git", true, - { "-C", *root, "--git-dir", gitDir, "commit", std::string(path.rel()), "-F", "-" }, - *commitMsg); + { "-C", *root, "--git-dir", gitDir, "commit", std::string(path.rel()), "-F", msgPath }); } } } From d477b34d1df3b471f8132525b0a008bbd03ddb6d Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Sun, 23 Jun 2024 15:19:47 +0200 Subject: [PATCH 35/47] libutil: remove runProgram2 stdin functionality this was only used in one place, and that place has been rewritten to use a temporary file instead. keeping this around is not very helpful at this time, and in any case we'd be better off rewriting subprocess handling in rust where we not only have a much safer library for such things but also async frameworks necessary for this easily available. Change-Id: I6f8641b756857c84ae2602cdf41f74ee7a1fda02 --- src/libcmd/repl.cc | 4 +-- src/libfetchers/git.cc | 8 +++--- src/libfetchers/mercurial.cc | 3 +- src/libutil/processes.cc | 55 ++---------------------------------- src/libutil/processes.hh | 5 +--- 5 files changed, 10 insertions(+), 65 deletions(-) diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index 99abbe44b..32cbf8e33 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -242,8 +242,7 @@ NixRepl::NixRepl(const SearchPath & searchPath, nix::ref store, ref & input = {}) +void runNix(Path program, const Strings & args) { auto subprocessEnv = getEnv(); subprocessEnv["NIX_CONFIG"] = globalConfig.toKeyValue(); @@ -252,7 +251,6 @@ void runNix(Path program, const Strings & args, .program = settings.nixBinDir+ "/" + program, .args = args, .environment = subprocessEnv, - .input = input, }); return; diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc index f2d577914..a06bc86b2 100644 --- a/src/libfetchers/git.cc +++ b/src/libfetchers/git.cc @@ -358,7 +358,7 @@ struct GitInputScheme : InputScheme args.push_back(destDir); - runProgram("git", true, args, {}, true); + runProgram("git", true, args, true); } std::optional getSourcePath(const Input & input) const override @@ -589,7 +589,7 @@ struct GitInputScheme : InputScheme : ref == "HEAD" ? *ref : "refs/heads/" + *ref; - runProgram("git", true, { "-C", repoDir, "--git-dir", gitDir, "fetch", "--quiet", "--force", "--", actualUrl, fmt("%s:%s", fetchRef, fetchRef) }, {}, true); + runProgram("git", true, { "-C", repoDir, "--git-dir", gitDir, "fetch", "--quiet", "--force", "--", actualUrl, fmt("%s:%s", fetchRef, fetchRef) }, true); } catch (Error & e) { if (!pathExists(localRefFile)) throw; warn("could not update local clone of Git repository '%s'; continuing with the most recent version", actualUrl); @@ -656,7 +656,7 @@ struct GitInputScheme : InputScheme // everything to ensure we get the rev. Activity act(*logger, lvlTalkative, actUnknown, fmt("making temporary clone of '%s'", repoDir)); runProgram("git", true, { "-C", tmpDir, "fetch", "--quiet", "--force", - "--update-head-ok", "--", repoDir, "refs/*:refs/*" }, {}, true); + "--update-head-ok", "--", repoDir, "refs/*:refs/*" }, true); } runProgram("git", true, { "-C", tmpDir, "checkout", "--quiet", input.getRev()->gitRev() }); @@ -683,7 +683,7 @@ struct GitInputScheme : InputScheme { Activity act(*logger, lvlTalkative, actUnknown, fmt("fetching submodules of '%s'", actualUrl)); - runProgram("git", true, { "-C", tmpDir, "submodule", "--quiet", "update", "--init", "--recursive" }, {}, true); + runProgram("git", true, { "-C", tmpDir, "submodule", "--quiet", "update", "--init", "--recursive" }, true); } filter = isNotDotGitDirectory; diff --git a/src/libfetchers/mercurial.cc b/src/libfetchers/mercurial.cc index 6015f8ec7..b4150e9df 100644 --- a/src/libfetchers/mercurial.cc +++ b/src/libfetchers/mercurial.cc @@ -28,10 +28,9 @@ static RunOptions hgOptions(const Strings & args) } // runProgram wrapper that uses hgOptions instead of stock RunOptions. -static std::string runHg(const Strings & args, const std::optional & input = {}) +static std::string runHg(const Strings & args) { RunOptions opts = hgOptions(args); - opts.input = input; auto res = runProgram(std::move(opts)); diff --git a/src/libutil/processes.cc b/src/libutil/processes.cc index e8af12fbd..250092393 100644 --- a/src/libutil/processes.cc +++ b/src/libutil/processes.cc @@ -230,10 +230,9 @@ Pid startProcess(std::function fun, const ProcessOptions & options) return Pid{pid}; } -std::string runProgram(Path program, bool searchPath, const Strings & args, - const std::optional & input, bool isInteractive) +std::string runProgram(Path program, bool searchPath, const Strings & args, bool isInteractive) { - auto res = runProgram(RunOptions {.program = program, .searchPath = searchPath, .args = args, .input = input, .isInteractive = isInteractive}); + auto res = runProgram(RunOptions {.program = program, .searchPath = searchPath, .args = args, .isInteractive = isInteractive}); if (!statusOk(res.first)) throw ExecError(res.first, "program '%1%' %2%", program, statusToString(res.first)); @@ -262,20 +261,9 @@ void runProgram2(const RunOptions & options) { checkInterrupt(); - assert(!(options.standardIn && options.input)); - - std::unique_ptr source_; - Source * source = options.standardIn; - - if (options.input) { - source_ = std::make_unique(*options.input); - source = source_.get(); - } - /* Create a pipe. */ - Pipe out, in; + Pipe out; if (options.standardOut) out.create(); - if (source) in.create(); ProcessOptions processOptions; @@ -298,8 +286,6 @@ void runProgram2(const RunOptions & options) if (options.mergeStderrToStdout) if (dup2(STDOUT_FILENO, STDERR_FILENO) == -1) throw SysError("cannot dup stdout into stderr"); - if (source && dup2(in.readSide.get(), STDIN_FILENO) == -1) - throw SysError("dupping stdin"); if (options.chdir && chdir((*options.chdir).c_str()) == -1) throw SysError("chdir failed"); @@ -328,47 +314,12 @@ void runProgram2(const RunOptions & options) out.writeSide.close(); - std::thread writerThread; - - std::promise promise; - - Finally doJoin([&]() { - if (writerThread.joinable()) - writerThread.join(); - }); - - - if (source) { - in.readSide.close(); - writerThread = std::thread([&]() { - try { - std::vector buf(8 * 1024); - while (true) { - size_t n; - try { - n = source->read(buf.data(), buf.size()); - } catch (EndOfFile &) { - break; - } - writeFull(in.writeSide.get(), {buf.data(), n}); - } - promise.set_value(); - } catch (...) { - promise.set_exception(std::current_exception()); - } - in.writeSide.close(); - }); - } - if (options.standardOut) drainFD(out.readSide.get(), *options.standardOut); /* Wait for the child to finish. */ int status = pid.wait(); - /* Wait for the writer thread to finish. */ - if (source) promise.get_future().get(); - if (status) throw ExecError(status, "program '%1%' %2%", options.program, statusToString(status)); } diff --git a/src/libutil/processes.hh b/src/libutil/processes.hh index 001e1fa12..48a195481 100644 --- a/src/libutil/processes.hh +++ b/src/libutil/processes.hh @@ -71,8 +71,7 @@ Pid startProcess(std::function fun, const ProcessOptions & options = Pro * shell backtick operator). */ std::string runProgram(Path program, bool searchPath = false, - const Strings & args = Strings(), - const std::optional & input = {}, bool isInteractive = false); + const Strings & args = Strings(), bool isInteractive = false); struct RunOptions { @@ -83,8 +82,6 @@ struct RunOptions std::optional gid; std::optional chdir; std::optional> environment; - std::optional input; - Source * standardIn = nullptr; Sink * standardOut = nullptr; bool mergeStderrToStdout = false; bool isInteractive = false; From 3baffbdcc5718815d4e2958531dcb1d906ae0061 Mon Sep 17 00:00:00 2001 From: Delan Azabani Date: Mon, 24 Jun 2024 12:44:35 +0800 Subject: [PATCH 36/47] Fix compile error under gcc with -Denable-pch-std=false Following the latest hacking.md currently fails because of a missing include in upstream editline. This patch fixes the build by adding that missing include. Fixes #410. Change-Id: Iefd4cb687ed3da71ccda9fe9624f38e6ef4623e5 --- src/libcmd/repl.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index 99abbe44b..3db2401e4 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -1,3 +1,4 @@ +#include #include #include #include From 5d33e4cd59093b29446d63ac70cf95cc9a25d81b Mon Sep 17 00:00:00 2001 From: Delan Azabani Date: Mon, 24 Jun 2024 12:55:00 +0800 Subject: [PATCH 37/47] Fix build instructions in hacking.md and justfile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The stdenv phases don’t actually do anything (at least not anymore), and our justfile doesn’t behave the same as our docs. This patch removes the stdenv phases from the docs, documents our usage of just, and makes `just setup` heed `$mesonFlags`. Fixes #413. Fixes #414. Change-Id: Ieb0b2a8ae420526238b5f9a73d7849ec6919995d --- doc/manual/src/contributing/hacking.md | 20 ++++++++++---------- justfile | 2 +- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/doc/manual/src/contributing/hacking.md b/doc/manual/src/contributing/hacking.md index 781aa75c1..cd4f6d5d5 100644 --- a/doc/manual/src/contributing/hacking.md +++ b/doc/manual/src/contributing/hacking.md @@ -39,17 +39,19 @@ $ nix-shell -A native-clangStdenvPackages ### Building from the development shell -As always you may run [stdenv's phases by name](https://nixos.org/manual/nixpkgs/unstable/#sec-building-stdenv-package-in-nix-shell), e.g.: +You can build and test Lix with just: ```bash -$ configurePhase -$ buildPhase -$ checkPhase -$ installPhase -$ installCheckPhase +$ just setup +$ just build +$ just test --suite=check +$ just install +$ just test --suite=installcheck ``` -To build manually, however, use the following: +(Check and installcheck may both be done after install, allowing you to omit the --suite argument entirely, but this is the order package.nix runs them in.) + +You can also build Lix manually: ```bash $ meson setup ./build "--prefix=$out" $mesonFlags @@ -64,9 +66,7 @@ $ meson install -C build $ meson test -C build --suite=installcheck ``` -(Check and installcheck may both be done after install, allowing you to omit the --suite argument entirely, but this is the order package.nix runs them in.) - -This will install Lix to `$PWD/outputs`, the `/bin` of which is prepended to PATH in the development shells. +In both cases, Lix will be installed to `$PWD/outputs`, the `/bin` of which is prepended to PATH in the development shells. If the tests fail and Meson helpfully has no output for why, use the `--print-error-logs` option to `meson test`. diff --git a/justfile b/justfile index dfa58bdf2..d63c54990 100644 --- a/justfile +++ b/justfile @@ -10,7 +10,7 @@ clean: # Prepare meson for building setup: - meson setup build --prefix="$PWD/outputs/out" + meson setup build --prefix="$PWD/outputs/out" $mesonFlags # Build lix build *OPTIONS: From d86009bd76ae85e56e1d26aab26d38f90ecb6439 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 22 Mar 2024 22:41:50 +0100 Subject: [PATCH 38/47] Add build-dir setting, clean up default TMPDIR handling This is a squash of upstream PRs #10303, #10312 and #10883. fix: Treat empty TMPDIR as unset Fixes an instance of nix: src/libutil/util.cc:139: nix::Path nix::canonPath(PathView, bool): Assertion `path != ""' failed. ... which I've been getting in one of my shells for some reason. I have yet to find out why TMPDIR was empty, but it's no reason for Nix to break. (cherry picked from commit c3fb2aa1f9d1fa756dac38d3588c836c5a5395dc) fix: Treat empty XDG_RUNTIME_DIR as unset See preceding commit. Not observed in the wild, but is sensible and consistent with TMPDIR behavior. (cherry picked from commit b9e7f5aa2df3f0e223f5c44b8089cbf9b81be691) local-derivation-goal.cc: Reuse defaultTempDir() (cherry picked from commit fd31945742710984de22805ee8d97fbd83c3f8eb) fix: remove usage of XDG_RUNTIME_DIR for TMP (cherry picked from commit 1363f51bcb24ab9948b7b5093490a009947f7453) tests/functional: Add count() (cherry picked from commit 6221770c9de4d28137206bdcd1a67eea12e1e499) Remove uncalled for message (cherry picked from commit b1fe388d33530f0157dcf9f461348b61eda13228) Add build-dir setting (cherry picked from commit 8b16cced18925aa612049d08d5e78eccbf0530e4) Change-Id: Ic7b75ff0b6a3b19e50a4ac8ff2d70f15c683c16a --- doc/manual/rl-next/build-dir.md | 12 ++++++++ src/libstore/build/local-derivation-goal.cc | 6 ++-- src/libstore/globals.cc | 2 +- src/libstore/globals.hh | 30 +++++++++++++++---- src/libutil/file-system.cc | 8 +++-- src/libutil/file-system.hh | 5 ++++ src/nix-build/nix-build.cc | 5 ++-- tests/functional/check.sh | 15 ++++++++++ .../common/vars-and-functions.sh.in | 5 ++++ 9 files changed, 74 insertions(+), 14 deletions(-) create mode 100644 doc/manual/rl-next/build-dir.md diff --git a/doc/manual/rl-next/build-dir.md b/doc/manual/rl-next/build-dir.md new file mode 100644 index 000000000..88bcd8da4 --- /dev/null +++ b/doc/manual/rl-next/build-dir.md @@ -0,0 +1,12 @@ +--- +synopsis: "Add a `build-dir` setting to set the backing directory for builds" +cls: 1514 +credits: [roberth, tomberek] +category: Improvements +--- + +`build-dir` can now be set in the Nix configuration to choose the backing directory for the build sandbox. +This can be useful on systems with `/tmp` on tmpfs, or simply to relocate large builds to another disk. + +Also, `XDG_RUNTIME_DIR` is no longer considered when selecting the default temporary directory, +as it's not intended to be used for large amounts of data. diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index efba648a4..968f669ec 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -495,7 +495,7 @@ void LocalDerivationGoal::startBuilder() /* Create a temporary directory where the build will take place. */ - tmpDir = createTempDir("", "nix-build-" + std::string(drvPath.name()), false, false, 0700); + tmpDir = createTempDir(settings.buildDir.get().value_or(""), "nix-build-" + std::string(drvPath.name()), false, false, 0700); chownToBuilder(tmpDir); @@ -2107,8 +2107,8 @@ void LocalDerivationGoal::runChild() bool allowLocalNetworking = parsedDrv->getBoolAttr("__darwinAllowLocalNetworking"); /* The tmpDir in scope points at the temporary build directory for our derivation. Some packages try different mechanisms - to find temporary directories, so we want to open up a broader place for them to dump their files, if needed. */ - Path globalTmpDir = canonPath(getEnvNonEmpty("TMPDIR").value_or("/tmp"), true); + to find temporary directories, so we want to open up a broader place for them to put their files, if needed. */ + Path globalTmpDir = canonPath(defaultTempDir(), true); /* They don't like trailing slashes on subpath directives */ if (globalTmpDir.back() == '/') globalTmpDir.pop_back(); diff --git a/src/libstore/globals.cc b/src/libstore/globals.cc index 763dc9158..02d26e034 100644 --- a/src/libstore/globals.cc +++ b/src/libstore/globals.cc @@ -429,7 +429,7 @@ void initLibStore() { /* On macOS, don't use the per-session TMPDIR (as set e.g. by sshd). This breaks build users because they don't have access to the TMPDIR, in particular in ‘nix-store --serve’. */ - if (getEnv("TMPDIR").value_or("/tmp").starts_with("/var/folders/")) + if (defaultTempDir().starts_with("/var/folders/")) unsetenv("TMPDIR"); #endif diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index 947a2fbf0..d360e5c5e 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -582,16 +582,36 @@ public: Setting sandboxShmSize{ this, "50%", "sandbox-dev-shm-size", R"( - This option determines the maximum size of the `tmpfs` filesystem - mounted on `/dev/shm` in Linux sandboxes. For the format, see the - description of the `size` option of `tmpfs` in mount(8). The default - is `50%`. + *Linux only* + + This option determines the maximum size of the `tmpfs` filesystem + mounted on `/dev/shm` in Linux sandboxes. For the format, see the + description of the `size` option of `tmpfs` in mount(8). The default + is `50%`. )"}; Setting sandboxBuildDir{this, "/build", "sandbox-build-dir", - "The build directory inside the sandbox."}; + R"( + *Linux only* + + The build directory inside the sandbox. + + This directory is backed by [`build-dir`](#conf-build-dir) on the host. + )"}; #endif + Setting> buildDir{this, std::nullopt, "build-dir", + R"( + The directory on the host, in which derivations' temporary build directories are created. + + If not set, Nix will use the system temporary directory indicated by the `TMPDIR` environment variable. + Note that builds are often performed by the Nix daemon, so its `TMPDIR` is used, and not that of the Nix command line interface. + + This is also the location where [`--keep-failed`](@docroot@/command-ref/opt-common.md#opt-keep-failed) leaves its files. + + If Nix runs without sandbox, or if the platform does not support sandboxing with bind mounts (e.g. macOS), then the [`builder`](@docroot@/language/derivations.md#attr-builder)'s environment will contain this directory, instead of the virtual location [`sandbox-build-dir`](#conf-sandbox-build-dir). + )"}; + Setting allowedImpureHostPrefixes{this, {}, "allowed-impure-host-deps", "Which prefixes to allow derivations to ask for access to (primarily for Darwin)."}; diff --git a/src/libutil/file-system.cc b/src/libutil/file-system.cc index f51f3c092..871707468 100644 --- a/src/libutil/file-system.cc +++ b/src/libutil/file-system.cc @@ -511,10 +511,14 @@ void AutoDelete::reset(const Path & p, bool recursive) { ////////////////////////////////////////////////////////////////////// +std::string defaultTempDir() { + return getEnvNonEmpty("TMPDIR").value_or("/tmp"); +} + static Path tempName(Path tmpRoot, const Path & prefix, bool includePid, std::atomic & counter) { - tmpRoot = canonPath(tmpRoot.empty() ? getEnv("TMPDIR").value_or("/tmp") : tmpRoot, true); + tmpRoot = canonPath(tmpRoot.empty() ? defaultTempDir() : tmpRoot, true); if (includePid) return fmt("%1%/%2%-%3%-%4%", tmpRoot, prefix, getpid(), counter++); else @@ -554,7 +558,7 @@ Path createTempDir(const Path & tmpRoot, const Path & prefix, std::pair createTempFile(const Path & prefix) { - Path tmpl(getEnv("TMPDIR").value_or("/tmp") + "/" + prefix + ".XXXXXX"); + Path tmpl(defaultTempDir() + "/" + prefix + ".XXXXXX"); // Strictly speaking, this is UB, but who cares... // FIXME: use O_TMPFILE. AutoCloseFD fd(mkstemp((char *) tmpl.c_str())); diff --git a/src/libutil/file-system.hh b/src/libutil/file-system.hh index 64d884227..17f5da062 100644 --- a/src/libutil/file-system.hh +++ b/src/libutil/file-system.hh @@ -258,6 +258,11 @@ Path createTempDir(const Path & tmpRoot = "", const Path & prefix = "nix", */ std::pair createTempFile(const Path & prefix = "nix"); +/** + * Return `TMPDIR`, or the default temporary directory if unset or empty. + */ +Path defaultTempDir(); + /** * Used in various places. */ diff --git a/src/nix-build/nix-build.cc b/src/nix-build/nix-build.cc index 4cce33eb1..37b553dbb 100644 --- a/src/nix-build/nix-build.cc +++ b/src/nix-build/nix-build.cc @@ -414,8 +414,7 @@ static void main_nix_build(int argc, char * * argv) // Set the environment. auto env = getEnv(); - auto tmp = getEnv("TMPDIR"); - if (!tmp) tmp = getEnv("XDG_RUNTIME_DIR").value_or("/tmp"); + auto tmp = defaultTempDir(); if (pure) { decltype(env) newEnv; @@ -427,7 +426,7 @@ static void main_nix_build(int argc, char * * argv) env["__ETC_PROFILE_SOURCED"] = "1"; } - env["NIX_BUILD_TOP"] = env["TMPDIR"] = env["TEMPDIR"] = env["TMP"] = env["TEMP"] = *tmp; + env["NIX_BUILD_TOP"] = env["TMPDIR"] = env["TEMPDIR"] = env["TMP"] = env["TEMP"] = tmp; env["NIX_STORE"] = store->storeDir; env["NIX_BUILD_CORES"] = std::to_string(settings.buildCores); diff --git a/tests/functional/check.sh b/tests/functional/check.sh index e13abf747..38883c5d7 100644 --- a/tests/functional/check.sh +++ b/tests/functional/check.sh @@ -34,6 +34,21 @@ nix-build check.nix -A failed --argstr checkBuildId $checkBuildId \ [ "$status" = "100" ] if checkBuildTempDirRemoved $TEST_ROOT/log; then false; fi +test_custom_build_dir() { + local customBuildDir="$TEST_ROOT/custom-build-dir" + + # Nix does not create the parent directories, and perhaps it shouldn't try to + # decide the permissions of build-dir. + mkdir "$customBuildDir" + nix-build check.nix -A failed --argstr checkBuildId $checkBuildId \ + --no-out-link --keep-failed --option build-dir "$TEST_ROOT/custom-build-dir" 2> $TEST_ROOT/log || status=$? + [ "$status" = "100" ] + [[ 1 == "$(count "$customBuildDir/nix-build-"*)" ]] + local buildDir="$customBuildDir/nix-build-"* + grep $checkBuildId $buildDir/checkBuildId +} +test_custom_build_dir + nix-build check.nix -A deterministic --argstr checkBuildId $checkBuildId \ --no-out-link 2> $TEST_ROOT/log checkBuildTempDirRemoved $TEST_ROOT/log diff --git a/tests/functional/common/vars-and-functions.sh.in b/tests/functional/common/vars-and-functions.sh.in index bfc4fbc44..eda15308d 100644 --- a/tests/functional/common/vars-and-functions.sh.in +++ b/tests/functional/common/vars-and-functions.sh.in @@ -274,6 +274,11 @@ grepQuietInverse() { ! grep "$@" > /dev/null } +# Return the number of arguments +count() { + echo $# +} + trap onError ERR fi # COMMON_VARS_AND_FUNCTIONS_SH_SOURCED From eb5de71adc7d376036dbf5c526300e8d20d2794e Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Mon, 24 Jun 2024 13:37:01 -0700 Subject: [PATCH 39/47] justfile: accept extra options to just setup and pass them to meson This lets you get the default options and still be able to add more. Change-Id: Ife32c348b1498ff2ccdddf051a5bba520cfa36f0 --- justfile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/justfile b/justfile index d63c54990..6a93fa63f 100644 --- a/justfile +++ b/justfile @@ -9,8 +9,8 @@ clean: rm -rf build # Prepare meson for building -setup: - meson setup build --prefix="$PWD/outputs/out" $mesonFlags +setup *OPTIONS: + meson setup build --prefix="$PWD/outputs/out" $mesonFlags {{ OPTIONS }} # Build lix build *OPTIONS: From d5637ee790799ffa00bcaa3067bb6940b6cbad16 Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Mon, 24 Jun 2024 13:37:41 -0700 Subject: [PATCH 40/47] devShell: guard against running from another directory I was working on nix-eval-jobs with a dev shell with some shenanigans to run against a locally built Lix and it was getting really annoying when `nix develop ../lix#` was messing up my other git repo's hooks. This is a fix via blunt force, but it is at least obvious how it works. Change-Id: Ia29eeb5be57ab6a2c88451c00ea18a51e4dfe65e --- .this-is-lix | 1 + package.nix | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-) create mode 100644 .this-is-lix diff --git a/.this-is-lix b/.this-is-lix new file mode 100644 index 000000000..b7ad8ef98 --- /dev/null +++ b/.this-is-lix @@ -0,0 +1 @@ +This is a file used by the dev shell shellHook in package.nix to check that this is actually a Lix repo before installing git hooks. Its contents have no meaning. diff --git a/package.nix b/package.nix index ef6f317e8..988379618 100644 --- a/package.nix +++ b/package.nix @@ -476,7 +476,7 @@ stdenv.mkDerivation (finalAttrs: { # https://git.lix.systems/lix-project/lix/src/commit/7575db522e9008685c4009423398f6900a16bcce/src/nix/develop.cc#L240-L241 # this is, of course, absurd. if [[ $name != lix-shell-env && $name != lix-shell-env-env ]]; then - return; + return fi PATH=$prefix/bin:$PATH @@ -486,6 +486,11 @@ stdenv.mkDerivation (finalAttrs: { # Make bash completion work. XDG_DATA_DIRS+=:$out/share + if [[ ! -f ./.this-is-lix ]]; then + echo "Dev shell not started from inside a Lix repo, skipping repo setup" >&2 + return + fi + ${lib.optionalString (pre-commit-checks ? shellHook) pre-commit-checks.shellHook} # Allow `touch .nocontribmsg` to turn this notice off. if ! [[ -f .nocontribmsg ]]; then From 3e151d4d77b5296b9da8c3ad209932d1dfa44c68 Mon Sep 17 00:00:00 2001 From: jade Date: Mon, 24 Jun 2024 22:49:17 +0000 Subject: [PATCH 41/47] Revert "libfetchers: make attribute / URL query handling consistent" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 35eec921af1043fc6322edc0ad88c872d41623b8. Reason for revert: Regressed nix-eval-jobs, and it appears to be this change is buggy/missing a case. It just needs another pass. Code causing the problem in n-e-j, when invoked with `nix-eval-jobs --flake '.#hydraJobs'`: ``` n-e-j/tests/assets » ../../build/src/nix-eval-jobs --meta --workers 1 --flake .#hydraJobs warning: unknown setting 'trusted-users' warning: `--gc-roots-dir' not specified error: unsupported Git input attribute 'dir' error: worker error: error: unsupported Git input attribute 'dir' ``` ``` nix::Value *vRoot = [&]() { if (args.flake) { auto [flakeRef, fragment, outputSpec] = nix::parseFlakeRefWithFragmentAndExtendedOutputsSpec( args.releaseExpr, nix::absPath(".")); nix::InstallableFlake flake{ {}, state, std::move(flakeRef), fragment, outputSpec, {}, {}, args.lockFlags}; return flake.toValue(*state).first; } else { return releaseExprTopLevelValue(*state, autoArgs, args); } }(); ``` Inspecting the program behaviour reveals that `dir` was in fact set in the URL going into the fetcher. This is in turn because unlike in the case changed in this commit, it was not erased before handing it to libfetchers, which is probably just a mistake. ``` (rr) up 3 0x00007ffff60262ae in nix::fetchers::Input::fromURL (url=..., requireTree=requireTree@entry=true) at src/libfetchers/fetchers.cc:39 warning: Source file is more recent than executable. 39 auto res = inputScheme->inputFromURL(url, requireTree); (rr) p url $1 = (const nix::ParsedURL &) @0x7fffdc874190: {url = "git+file:///home/jade/lix/nix-eval-jobs", base = "git+file:///home/jade/lix/nix-eval-jobs", scheme = "git+file", authority = std::optional = {[contained value] = ""}, path = "/home/jade/lix/nix-eval-jobs", query = std::map with 1 element = {["dir"] = "tests/assets"}, fragment = ""} (rr) up 4 0x00007ffff789d904 in nix::parseFlakeRefWithFragment (url=".#hydraJobs", baseDir=std::optional = {...}, allowMissing=allowMissing@entry=false, isFlake=isFlake@entry=true) at src/libexpr/flake/flakeref.cc:179 warning: Source file is more recent than executable. 179 FlakeRef(Input::fromURL(parsedURL, isFlake), getOr(parsedURL.query, "dir", "")), (rr) p parsedURL $2 = {url = "git+file:///home/jade/lix/nix-eval-jobs", base = "git+file:///home/jade/lix/nix-eval-jobs", scheme = "git+file", authority = std::optional = {[contained value] = ""}, path = "/home/jade/lix/nix-eval-jobs", query = std::map with 1 element = { ["dir"] = "tests/assets"}, fragment = ""} (rr) list 174 175 if (pathExists(flakeRoot + "/.git/shallow")) 176 parsedURL.query.insert_or_assign("shallow", "1"); 177 178 return std::make_pair( 179 FlakeRef(Input::fromURL(parsedURL, isFlake), getOr(parsedURL.query, "dir", "")), 180 fragment); 181 } ``` Change-Id: Ib55a882eaeb3e59228857761dc1e3b2e366b0f5e --- src/libexpr/flake/flakeref.cc | 8 +-- src/libfetchers/fetchers.hh | 31 --------- src/libfetchers/git.cc | 17 +++-- src/libfetchers/github.cc | 114 +++++++++++++-------------------- src/libfetchers/indirect.cc | 34 ++++------ src/libfetchers/mercurial.cc | 7 +- src/libfetchers/tarball.cc | 26 ++++++-- tests/functional/fetchers.sh | 91 -------------------------- tests/functional/meson.build | 1 - tests/nixos/tarball-flakes.nix | 2 +- 10 files changed, 96 insertions(+), 235 deletions(-) delete mode 100644 tests/functional/fetchers.sh diff --git a/src/libexpr/flake/flakeref.cc b/src/libexpr/flake/flakeref.cc index 8668961fe..1c90bfc43 100644 --- a/src/libexpr/flake/flakeref.cc +++ b/src/libexpr/flake/flakeref.cc @@ -204,13 +204,7 @@ std::pair parseFlakeRefWithFragment( std::string fragment; std::swap(fragment, parsedURL.fragment); - // This has a special meaning for flakes and must not be passed to libfetchers. - // Of course this means that libfetchers cannot have fetchers - // expecting an argument `dir` 🫠 - ParsedURL urlForFetchers(parsedURL); - urlForFetchers.query.erase("dir"); - - auto input = Input::fromURL(urlForFetchers, isFlake); + auto input = Input::fromURL(parsedURL, isFlake); input.parent = baseDir; return std::make_pair( diff --git a/src/libfetchers/fetchers.hh b/src/libfetchers/fetchers.hh index 40f2b6294..2bb4248be 100644 --- a/src/libfetchers/fetchers.hh +++ b/src/libfetchers/fetchers.hh @@ -159,37 +159,6 @@ struct InputScheme std::optional commitMsg) const; virtual std::pair fetch(ref store, const Input & input) = 0; - -protected: - void emplaceURLQueryIntoAttrs( - const ParsedURL & parsedURL, - Attrs & attrs, - const StringSet & numericParams, - const StringSet & booleanParams) const - { - for (auto &[name, value] : parsedURL.query) { - if (name == "url") { - throw BadURL( - "URL '%s' must not override url via query param!", - parsedURL.to_string() - ); - } else if (numericParams.count(name) != 0) { - if (auto n = string2Int(value)) { - attrs.insert_or_assign(name, *n); - } else { - throw BadURL( - "URL '%s' has non-numeric parameter '%s'", - parsedURL.to_string(), - name - ); - } - } else if (booleanParams.count(name) != 0) { - attrs.emplace(name, Explicit { value == "1" }); - } else { - attrs.emplace(name, value); - } - } - } }; void registerInputScheme(std::shared_ptr && fetcher); diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc index 0cb826075..2817fde23 100644 --- a/src/libfetchers/git.cc +++ b/src/libfetchers/git.cc @@ -273,14 +273,17 @@ struct GitInputScheme : InputScheme Attrs attrs; attrs.emplace("type", "git"); - attrs.emplace("url", url2.to_string()); - emplaceURLQueryIntoAttrs( - url, - attrs, - {"lastModified", "revCount"}, - {"shallow", "submodules", "allRefs"} - ); + for (auto & [name, value] : url.query) { + if (name == "rev" || name == "ref") + attrs.emplace(name, value); + else if (name == "shallow" || name == "submodules" || name == "allRefs") + attrs.emplace(name, Explicit { value == "1" }); + else + url2.query.emplace(name, value); + } + + attrs.emplace("url", url2.to_string()); return inputFromAttrs(attrs); } diff --git a/src/libfetchers/github.cc b/src/libfetchers/github.cc index b971781ae..60fefd1f3 100644 --- a/src/libfetchers/github.cc +++ b/src/libfetchers/github.cc @@ -1,4 +1,3 @@ -#include "attrs.hh" #include "filetransfer.hh" #include "cache.hh" #include "globals.hh" @@ -37,11 +36,18 @@ struct GitArchiveInputScheme : InputScheme auto path = tokenizeString>(url.path, "/"); - std::optional refOrRev; + std::optional rev; + std::optional ref; + std::optional host_url; auto size = path.size(); if (size == 3) { - refOrRev = path[2]; + if (std::regex_match(path[2], revRegex)) + rev = Hash::parseAny(path[2], htSHA1); + else if (std::regex_match(path[2], refRegex)) + ref = path[2]; + else + throw BadURL("in URL '%s', '%s' is not a commit hash or branch/tag name", url.url, path[2]); } else if (size > 3) { std::string rs; for (auto i = std::next(path.begin(), 2); i != path.end(); i++) { @@ -52,91 +58,61 @@ struct GitArchiveInputScheme : InputScheme } if (std::regex_match(rs, refRegex)) { - refOrRev = rs; + ref = rs; } else { throw BadURL("in URL '%s', '%s' is not a branch/tag name", url.url, rs); } } else if (size < 2) throw BadURL("URL '%s' is invalid", url.url); - Attrs attrs; - attrs.emplace("type", type()); - attrs.emplace("owner", path[0]); - attrs.emplace("repo", path[1]); - for (auto &[name, value] : url.query) { - if (name == "rev" || name == "ref") { - if (refOrRev) { - throw BadURL("URL '%s' already contains a ref or rev", url.url); - } else { - refOrRev = value; - } - } else if (name == "lastModified") { - if (auto n = string2Int(value)) { - attrs.emplace(name, *n); - } else { - throw Error( - "Attribute 'lastModified' in URL '%s' must be an integer", - url.to_string() - ); - } - } else { - attrs.emplace(name, value); + if (name == "rev") { + if (rev) + throw BadURL("URL '%s' contains multiple commit hashes", url.url); + rev = Hash::parseAny(value, htSHA1); } + else if (name == "ref") { + if (!std::regex_match(value, refRegex)) + throw BadURL("URL '%s' contains an invalid branch/tag name", url.url); + if (ref) + throw BadURL("URL '%s' contains multiple branch/tag names", url.url); + ref = value; + } + else if (name == "host") { + if (!std::regex_match(value, hostRegex)) + throw BadURL("URL '%s' contains an invalid instance host", url.url); + host_url = value; + } + // FIXME: barf on unsupported attributes } - if (refOrRev) attrs.emplace("refOrRev", *refOrRev); + if (ref && rev) + throw BadURL("URL '%s' contains both a commit hash and a branch/tag name %s %s", url.url, *ref, rev->gitRev()); - return inputFromAttrs(attrs); + Input input; + input.attrs.insert_or_assign("type", type()); + input.attrs.insert_or_assign("owner", path[0]); + input.attrs.insert_or_assign("repo", path[1]); + if (rev) input.attrs.insert_or_assign("rev", rev->gitRev()); + if (ref) input.attrs.insert_or_assign("ref", *ref); + if (host_url) input.attrs.insert_or_assign("host", *host_url); + + return input; } std::optional inputFromAttrs(const Attrs & attrs) const override { - // Attributes can contain refOrRev and it needs to be figured out - // which one it is (see inputFromURL for when that may happen). - // The correct one (ref or rev) will be written into finalAttrs and - // it needs to be mutable for that. - Attrs finalAttrs(attrs); - auto type_ = maybeGetStrAttr(finalAttrs, "type"); - if (type_ != type()) return {}; + if (maybeGetStrAttr(attrs, "type") != type()) return {}; - auto owner = getStrAttr(finalAttrs, "owner"); - auto repo = getStrAttr(finalAttrs, "repo"); - - auto url = fmt("%s:%s/%s", *type_, owner, repo); - if (auto host = maybeGetStrAttr(finalAttrs, "host")) { - if (!std::regex_match(*host, hostRegex)) { - throw BadURL("URL '%s' contains an invalid instance host", url); - } - } - - if (auto refOrRev = maybeGetStrAttr(finalAttrs, "refOrRev")) { - finalAttrs.erase("refOrRev"); - if (std::regex_match(*refOrRev, revRegex)) { - finalAttrs.emplace("rev", *refOrRev); - } else if (std::regex_match(*refOrRev, refRegex)) { - finalAttrs.emplace("ref", *refOrRev); - } else { - throw Error( - "in URL '%s', '%s' is not a commit hash or a branch/tag name", - url, - *refOrRev - ); - } - } else if (auto ref = maybeGetStrAttr(finalAttrs, "ref")) { - if (!std::regex_match(*ref, refRegex)) { - throw BadURL("URL '%s' contains an invalid branch/tag name", url); - } - } - - for (auto & [name, value] : finalAttrs) { - if (name != "type" && name != "owner" && name != "repo" && name != "ref" && name != "rev" && name != "narHash" && name != "lastModified" && name != "host") { + for (auto & [name, value] : attrs) + if (name != "type" && name != "owner" && name != "repo" && name != "ref" && name != "rev" && name != "narHash" && name != "lastModified" && name != "host") throw Error("unsupported input attribute '%s'", name); - } - } + + getStrAttr(attrs, "owner"); + getStrAttr(attrs, "repo"); Input input; - input.attrs = finalAttrs; + input.attrs = attrs; return input; } diff --git a/src/libfetchers/indirect.cc b/src/libfetchers/indirect.cc index 8c0176e84..c73505b31 100644 --- a/src/libfetchers/indirect.cc +++ b/src/libfetchers/indirect.cc @@ -17,8 +17,6 @@ struct IndirectInputScheme : InputScheme std::optional rev; std::optional ref; - Attrs attrs; - if (path.size() == 1) { } else if (path.size() == 2) { if (std::regex_match(path[1], revRegex)) @@ -28,21 +26,29 @@ struct IndirectInputScheme : InputScheme else throw BadURL("in flake URL '%s', '%s' is not a commit hash or branch/tag name", url.url, path[1]); } else if (path.size() == 3) { + if (!std::regex_match(path[1], refRegex)) + throw BadURL("in flake URL '%s', '%s' is not a branch/tag name", url.url, path[1]); ref = path[1]; + if (!std::regex_match(path[2], revRegex)) + throw BadURL("in flake URL '%s', '%s' is not a commit hash", url.url, path[2]); rev = Hash::parseAny(path[2], htSHA1); } else throw BadURL("GitHub URL '%s' is invalid", url.url); std::string id = path[0]; + if (!std::regex_match(id, flakeRegex)) + throw BadURL("'%s' is not a valid flake ID", id); - attrs.emplace("type", "indirect"); - attrs.emplace("id", id); - if (rev) attrs.emplace("rev", rev->gitRev()); - if (ref) attrs.emplace("ref", *ref); + // FIXME: forbid query params? - emplaceURLQueryIntoAttrs(url, attrs, {}, {}); + Input input; + input.direct = false; + input.attrs.insert_or_assign("type", "indirect"); + input.attrs.insert_or_assign("id", id); + if (rev) input.attrs.insert_or_assign("rev", rev->gitRev()); + if (ref) input.attrs.insert_or_assign("ref", *ref); - return inputFromAttrs(attrs); + return input; } std::optional inputFromAttrs(const Attrs & attrs) const override @@ -57,18 +63,6 @@ struct IndirectInputScheme : InputScheme if (!std::regex_match(id, flakeRegex)) throw BadURL("'%s' is not a valid flake ID", id); - // TODO come up with a nicer error message for those two. - if (auto rev = maybeGetStrAttr(attrs, "rev")) { - if (!std::regex_match(*rev, revRegex)) { - throw BadURL("in flake '%s', '%s' is not a commit hash", id, *rev); - } - } - if (auto ref = maybeGetStrAttr(attrs, "ref")) { - if (!std::regex_match(*ref, refRegex)) { - throw BadURL("in flake '%s', '%s' is not a valid branch/tag name", id, *ref); - } - } - Input input; input.direct = false; input.attrs = attrs; diff --git a/src/libfetchers/mercurial.cc b/src/libfetchers/mercurial.cc index 6015f8ec7..4fffa71d3 100644 --- a/src/libfetchers/mercurial.cc +++ b/src/libfetchers/mercurial.cc @@ -57,7 +57,12 @@ struct MercurialInputScheme : InputScheme Attrs attrs; attrs.emplace("type", "hg"); - emplaceURLQueryIntoAttrs(url, attrs, {"revCount"}, {}); + for (auto &[name, value] : url.query) { + if (name == "rev" || name == "ref") + attrs.emplace(name, value); + else + url2.query.emplace(name, value); + } attrs.emplace("url", url2.to_string()); diff --git a/src/libfetchers/tarball.cc b/src/libfetchers/tarball.cc index 8dfdecda8..c903895e2 100644 --- a/src/libfetchers/tarball.cc +++ b/src/libfetchers/tarball.cc @@ -201,17 +201,29 @@ struct CurlInputScheme : InputScheme if (!isValidURL(_url, requireTree)) return std::nullopt; - auto url = _url; + Input input; - Attrs attrs; - attrs.emplace("type", inputType()); + auto url = _url; url.scheme = parseUrlScheme(url.scheme).transport; - emplaceURLQueryIntoAttrs(url, attrs, {"revCount"}, {}); + auto narHash = url.query.find("narHash"); + if (narHash != url.query.end()) + input.attrs.insert_or_assign("narHash", narHash->second); - attrs.emplace("url", url.to_string()); - return inputFromAttrs(attrs); + if (auto i = get(url.query, "rev")) + input.attrs.insert_or_assign("rev", *i); + + if (auto i = get(url.query, "revCount")) + if (auto n = string2Int(*i)) + input.attrs.insert_or_assign("revCount", *n); + + url.query.erase("rev"); + url.query.erase("revCount"); + + input.attrs.insert_or_assign("type", inputType()); + input.attrs.insert_or_assign("url", url.to_string()); + return input; } std::optional inputFromAttrs(const Attrs & attrs) const override @@ -223,7 +235,7 @@ struct CurlInputScheme : InputScheme std::set allowedNames = {"type", "url", "narHash", "name", "unpack", "rev", "revCount", "lastModified"}; for (auto & [name, value] : attrs) if (!allowedNames.count(name)) - throw Error("unsupported %s input attribute '%s'. If you wanted to fetch a tarball with a query parameter, please use '{ type = \"tarball\"; url = \"...\"; }'", *type, name); + throw Error("unsupported %s input attribute '%s'", *type, name); Input input; input.attrs = attrs; diff --git a/tests/functional/fetchers.sh b/tests/functional/fetchers.sh deleted file mode 100644 index 0f888dc33..000000000 --- a/tests/functional/fetchers.sh +++ /dev/null @@ -1,91 +0,0 @@ -source common.sh - -requireGit - -clearStore - -testFetchTreeError() { - rawFetchTreeArg="${1?fetchTree arg missing}" - messageSubstring="${2?messageSubstring missing}" - - output="$(nix eval --impure --raw --expr "(builtins.fetchTree $rawFetchTreeArg).outPath" 2>&1)" && status=0 || status=$? - grepQuiet "$messageSubstring" <<<"$output" - test "$status" -ne 0 -} - -# github/gitlab/sourcehut fetcher input validation -for provider in github gitlab sourcehut; do - # ref/rev validation - testFetchTreeError \ - "{ type = \"$provider\"; owner = \"foo\"; repo = \"bar\"; ref = \",\"; }" \ - "URL '$provider:foo/bar' contains an invalid branch/tag name" - - testFetchTreeError \ - "\"$provider://host/foo/bar/,\"" \ - "URL '$provider:foo/bar', ',' is not a commit hash or a branch/tag name" - - testFetchTreeError \ - "\"$provider://host/foo/bar/f16d8f43dd0998cdb315a2cccf2e4d10027e7ca4?rev=abc\"" \ - "URL '$provider://host/foo/bar/f16d8f43dd0998cdb315a2cccf2e4d10027e7ca4?rev=abc' already contains a ref or rev" - - testFetchTreeError \ - "\"$provider://host/foo/bar/ref?ref=ref2\"" \ - "URL '$provider://host/foo/bar/ref?ref=ref2' already contains a ref or rev" - - # host validation - testFetchTreeError \ - "{ type = \"$provider\"; owner = \"foo\"; repo = \"bar\"; host = \"git_hub.com\"; }" \ - "URL '$provider:foo/bar' contains an invalid instance host" - - testFetchTreeError \ - "\"$provider://host/foo/bar/ref?host=git_hub.com\"" \ - "URL '$provider:foo/bar' contains an invalid instance host" - - # invalid attributes - testFetchTreeError \ - "{ type = \"$provider\"; owner = \"foo\"; repo = \"bar\"; wrong = true; }" \ - "unsupported input attribute 'wrong'" - - testFetchTreeError \ - "\"$provider://host/foo/bar/ref?wrong=1\"" \ - "unsupported input attribute 'wrong'" -done - -# unsupported attributes w/ tarball fetcher -testFetchTreeError \ - "\"https://host/foo?wrong=1\"" \ - "unsupported tarball input attribute 'wrong'. If you wanted to fetch a tarball with a query parameter, please use '{ type = \"tarball\"; url = \"...\"; }" - -# test for unsupported attributes / validation in git fetcher -testFetchTreeError \ - "\"git+https://github.com/owner/repo?invalid=1\"" \ - "unsupported Git input attribute 'invalid'" - -testFetchTreeError \ - "\"git+https://github.com/owner/repo?url=foo\"" \ - "URL 'git+https://github.com/owner/repo?url=foo' must not override url via query param!" - -testFetchTreeError \ - "\"git+https://github.com/owner/repo?ref=foo.lock\"" \ - "invalid Git branch/tag name 'foo.lock'" - -testFetchTreeError \ - "{ type = \"git\"; url =\"https://github.com/owner/repo\"; ref = \"foo.lock\"; }" \ - "invalid Git branch/tag name 'foo.lock'" - -# same for mercurial -testFetchTreeError \ - "\"hg+https://forge.tld/owner/repo?invalid=1\"" \ - "unsupported Mercurial input attribute 'invalid'" - -testFetchTreeError \ - "{ type = \"hg\"; url = \"https://forge.tld/owner/repo\"; invalid = 1; }" \ - "unsupported Mercurial input attribute 'invalid'" - -testFetchTreeError \ - "\"hg+https://forge.tld/owner/repo?ref=,\"" \ - "invalid Mercurial branch/tag name ','" - -testFetchTreeError \ - "{ type = \"hg\"; url = \"https://forge.tld/owner/repo\"; ref = \",\"; }" \ - "invalid Mercurial branch/tag name ','" diff --git a/tests/functional/meson.build b/tests/functional/meson.build index d99f9bbd3..a13dee001 100644 --- a/tests/functional/meson.build +++ b/tests/functional/meson.build @@ -93,7 +93,6 @@ functional_tests_scripts = [ 'fetchGitRefs.sh', 'gc-runtime.sh', 'tarball.sh', - 'fetchers.sh', 'fetchGit.sh', 'fetchurl.sh', 'fetchPath.sh', diff --git a/tests/nixos/tarball-flakes.nix b/tests/nixos/tarball-flakes.nix index 5deba4a12..ca7627bd1 100644 --- a/tests/nixos/tarball-flakes.nix +++ b/tests/nixos/tarball-flakes.nix @@ -69,7 +69,7 @@ in # Check that we got redirected to the immutable URL. locked_url = info["locked"]["url"] - assert locked_url == "http://localhost/stable/${nixpkgs.rev}.tar.gz?rev=${nixpkgs.rev}&revCount=1234", f"{locked_url=} != http://localhost/stable/${nixpkgs.rev}.tar.gz" + assert locked_url == "http://localhost/stable/${nixpkgs.rev}.tar.gz", f"{locked_url=} != http://localhost/stable/${nixpkgs.rev}.tar.gz" # Check that we got the rev and revCount attributes. revision = info["revision"] From 80298aab02231377adc33e5344a9b2c480c05b17 Mon Sep 17 00:00:00 2001 From: Qyriad Date: Mon, 24 Jun 2024 14:33:12 -0600 Subject: [PATCH 42/47] add an impl of Expr::show for ExprInheritFrom that doesn't crash ExprVar::show() assumes it has a name. dynamic inherits do not necessarily (ever?) have a name. Change-Id: If10893188e307431da17f0c1bd0787adc74f7141 --- src/libexpr/nixexpr.cc | 10 ++++++++++ src/libexpr/nixexpr.hh | 3 ++- tests/unit/libexpr/expr-print.cc | 32 ++++++++++++++++++++++++++++++++ tests/unit/meson.build | 1 + 4 files changed, 45 insertions(+), 1 deletion(-) create mode 100644 tests/unit/libexpr/expr-print.cc diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index bc53ca053..2d2fe1fcc 100644 --- a/src/libexpr/nixexpr.cc +++ b/src/libexpr/nixexpr.cc @@ -50,6 +50,16 @@ void ExprVar::show(const SymbolTable & symbols, std::ostream & str) const str << symbols[name]; } +void ExprInheritFrom::show(SymbolTable const & symbols, std::ostream & str) const +{ + if (name) { + str << symbols[name]; + } else { + // We can't get at the actual dynamic expression from here. + str << "(dynamic inherit)"; + } +} + void ExprSelect::show(const SymbolTable & symbols, std::ostream & str) const { str << "("; diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh index 418f888b3..ef4e55d99 100644 --- a/src/libexpr/nixexpr.hh +++ b/src/libexpr/nixexpr.hh @@ -151,7 +151,8 @@ struct ExprInheritFrom : ExprVar this->fromWith = nullptr; } - void bindVars(EvalState & es, const std::shared_ptr & env); + void show(SymbolTable const & symbols, std::ostream & str) const override; + void bindVars(EvalState & es, const std::shared_ptr & env) override; }; struct ExprSelect : Expr diff --git a/tests/unit/libexpr/expr-print.cc b/tests/unit/libexpr/expr-print.cc new file mode 100644 index 000000000..dc0ee27ed --- /dev/null +++ b/tests/unit/libexpr/expr-print.cc @@ -0,0 +1,32 @@ +#include +#include + +#include + +#include "tests/libexpr.hh" + +#include "nixexpr.hh" + +namespace nix +{ + +using namespace testing; +struct ExprPrintingTests : LibExprTest +{ + void test(Expr const & expr, std::string_view expected) + { + std::stringstream out; + expr.show(state.symbols, out); + ASSERT_EQ(out.str(), expected); + } +}; + +TEST_F(ExprPrintingTests, ExprInheritFrom) +{ + // ExprInheritFrom has its own show() impl. + // If it uses its parent class's impl it will crash. + ExprInheritFrom const eInheritFrom(noPos, 0); + test(eInheritFrom, "(dynamic inherit)"); +} + +} diff --git a/tests/unit/meson.build b/tests/unit/meson.build index 2b5471526..e2ca95629 100644 --- a/tests/unit/meson.build +++ b/tests/unit/meson.build @@ -185,6 +185,7 @@ libexpr_tests_sources = files( 'libexpr/primops.cc', 'libexpr/search-path.cc', 'libexpr/trivial.cc', + 'libexpr/expr-print.cc', 'libexpr/value/context.cc', 'libexpr/value/print.cc', ) From 3625147e77e8d37bbf6baaa7dc48955faddb314b Mon Sep 17 00:00:00 2001 From: Qyriad Date: Sat, 22 Jun 2024 21:17:41 -0600 Subject: [PATCH 43/47] add an ExprPrinter class, like ValuePrinter To be used Shortly Change-Id: I9def7975aa55f251eb8486391677771f7352d7ce --- src/libexpr/print.cc | 6 ++++++ src/libexpr/print.hh | 30 ++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/src/libexpr/print.cc b/src/libexpr/print.cc index e387a09fb..87db004b2 100644 --- a/src/libexpr/print.cc +++ b/src/libexpr/print.cc @@ -574,4 +574,10 @@ fmt_internal::HintFmt & fmt_internal::HintFmt::operator%(const ValuePrinter & va return *this; } +std::ostream & operator<<(std::ostream & output, ExprPrinter const & printer) +{ + printer.expr.show(printer.state.symbols, output); + return output; +} + } diff --git a/src/libexpr/print.hh b/src/libexpr/print.hh index 42826d94d..3deaa33d4 100644 --- a/src/libexpr/print.hh +++ b/src/libexpr/print.hh @@ -15,6 +15,8 @@ namespace nix { +struct Expr; + class EvalState; struct Value; @@ -50,6 +52,12 @@ void printValue(EvalState & state, std::ostream & str, Value & v, PrintOptions o /** * A partially-applied form of `printValue` which can be formatted using `<<` * without allocating an intermediate string. + * This class should not outlive the eval state or it will UAF. + * FIXME: This should take `nix::ref`s, to avoid that, but our eval methods all have + * EvalState &, not ref, and constructing a new shared_ptr to data that + * already has a shared_ptr is a much bigger footgun. In the current architecture of + * libexpr, using a ValuePrinter after an EvalState has been destroyed would be + * pretty hard. */ class ValuePrinter { friend std::ostream & operator << (std::ostream & output, const ValuePrinter & printer); @@ -73,4 +81,26 @@ std::ostream & operator<<(std::ostream & output, const ValuePrinter & printer); template<> fmt_internal::HintFmt & fmt_internal::HintFmt::operator%(const ValuePrinter & value); +/** + * A partially-applied form of Expr::show(), which can be formatted using `<<` + * without allocating an intermediate string. + * This class should not outlive the eval state or it will UAF. + * FIXME: This should take `nix::ref`s, to avoid that, but our eval methods all have + * EvalState &, not ref, and constructing a new shared_ptr to data that + * already has a shared_ptr is a much bigger footgun. In the current architecture of + * libexpr, using an ExprPrinter after an EvalState has been destroyed would be + * pretty hard. + */ +class ExprPrinter +{ + /** The eval state used to get symbols. */ + EvalState const & state; + /** The expression to print. */ + Expr const & expr; + +public: + ExprPrinter(EvalState const & state, Expr const & expr) : state(state), expr(expr) { } + friend std::ostream & operator << (std::ostream & output, ExprPrinter const & printer); +}; + } From 6b531e25b94aef35e09ae8c1c9cea50da2ca949e Mon Sep 17 00:00:00 2001 From: Qyriad Date: Sat, 22 Jun 2024 21:22:29 -0600 Subject: [PATCH 44/47] trace when the `foo` part of `foo.bar.baz` errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Turns errors like: let errpkg = throw "invalid foobar"; in errpkg.meta error: … while calling the 'throw' builtin at «string»:2:12: 1| let 2| errpkg = throw "invalid foobar"; | ^ 3| in errpkg.meta error: invalid foobar into errors like: let errpkg = throw "invalid foobar"; in errpkg.meta error: … while evaluating 'errpkg' to select 'meta' on it at «string»:3:4: 2| errpkg = throw "invalid foobar"; 3| in errpkg.meta | ^ … while calling the 'throw' builtin at «string»:2:12: 1| let 2| errpkg = throw "invalid foobar"; | ^ 3| in errpkg.meta error: invalid foobar For the low price of one try/catch, you too can have the incorrect line of code actually show up in the trace! Change-Id: If8d6200ec1567706669d405c34adcd7e2d2cd29d --- src/libexpr/eval.cc | 15 ++++++++++++++- tests/functional/lang/eval-fail-recursion.err.exp | 6 ++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index afee89420..51bd4a8c5 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -1433,7 +1433,20 @@ void ExprSelect::eval(EvalState & state, Env & env, Value & v) Value * vCurrent = &vFirst; // Position for the current attrset Value in this select chain. PosIdx posCurrent; - e->eval(state, env, vFirst); + + try { + e->eval(state, env, vFirst); + } catch (Error & e) { + assert(this->e != nullptr); + state.addErrorTrace( + e, + getPos(), + "while evaluating '%s' to select '%s' on it", + ExprPrinter(state, *this->e), + showAttrPath(state.symbols, this->attrPath) + ); + throw; + } try { auto dts = state.debugRepl diff --git a/tests/functional/lang/eval-fail-recursion.err.exp b/tests/functional/lang/eval-fail-recursion.err.exp index 19380dc65..f0057b2d5 100644 --- a/tests/functional/lang/eval-fail-recursion.err.exp +++ b/tests/functional/lang/eval-fail-recursion.err.exp @@ -1,4 +1,10 @@ error: + … while evaluating 'a' to select 'foo' on it + at /pwd/lang/eval-fail-recursion.nix:1:21: + 1| let a = {} // a; in a.foo + | ^ + 2| + … in the right operand of the update (//) operator at /pwd/lang/eval-fail-recursion.nix:1:12: 1| let a = {} // a; in a.foo From a8e529c47c71d93565816a87eab4386d24cde124 Mon Sep 17 00:00:00 2001 From: Qyriad Date: Sat, 22 Jun 2024 21:59:47 -0600 Subject: [PATCH 45/47] trace which part of `foo.bar.baz` errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Turns errors like: let somepkg.src = throw "invalid foobar"; in somepkg.src.meta error: … while evaluating the attribute 'src.meta' at «string»:2:3: 1| let 2| somepkg.src = throw "invalid foobar"; | ^ 3| in somepkg.src.meta … while calling the 'throw' builtin at «string»:2:17: 1| let 2| somepkg.src = throw "invalid foobar"; | ^ 3| in somepkg.src.meta error: invalid foobar into errors like: let somepkg.src = throw "invalid foobar"; in somepkg.src.meta error: … while evaluating the attribute 'src.meta' at «string»:2:3: 1| let 2| somepkg.src = throw "invalid foobar"; | ^ 3| in somepkg.src.meta … while evaluating 'somepkg.src' to select 'meta' on it at «string»:3:4: 2| somepkg.src = throw "invalid foobar"; 3| in somepkg.src.meta | ^ … while calling the 'throw' builtin at «string»:2:17: 1| let 2| somepkg.src = throw "invalid foobar"; | ^ 3| in somepkg.src.meta error: invalid foobar And for type errors, from: let somepkg.src = "I'm not an attrset"; in somepkg.src.meta error: … while evaluating the attribute 'src.meta' at «string»:2:3: 1| let 2| somepkg.src = "I'm not an attrset"; | ^ 3| in somepkg.src.meta … while selecting an attribute at «string»:3:4: 2| somepkg.src = "I'm not an attrset"; 3| in somepkg.src.meta | ^ error: expected a set but found a string: "I'm not an attrset" into: let somepkg.src = "I'm not an attrset"; in somepkg.src.meta error: … while evaluating the attribute 'src.meta' at «string»:2:3: 1| let 2| somepkg.src = "I'm not an attrset"; | ^ 3| in somepkg.src.meta … while selecting 'meta' on 'somepkg.src' at «string»:3:4: 2| somepkg.src = "I'm not an attrset"; 3| in somepkg.src.meta | ^ error: expected a set but found a string: "I'm not an attrset" For the low price of an enumerate() and a lambda you too can have the incorrect line of code actually show up in the trace! Change-Id: Ic1491c86e33c167891bdac9adad6224784760bd6 --- src/libexpr/eval.cc | 43 ++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 40 insertions(+), 3 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 51bd4a8c5..b4213c7d9 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -1459,12 +1459,46 @@ void ExprSelect::eval(EvalState & state, Env & env, Value & v) showAttrPath(state, env, attrPath)) : nullptr; - for (auto const & currentAttrName : attrPath) { + for (auto const & [partIdx, currentAttrName] : enumerate(attrPath)) { state.nrLookups++; Symbol const name = getName(currentAttrName, state, env); - state.forceValue(*vCurrent, pos); + // For formatting errors, which should be done only when needed. + auto partsSoFar = [&]() -> std::string { + std::stringstream ss; + // We start with the base thing this ExprSelect is selecting on. + assert(this->e != nullptr); + this->e->show(state.symbols, ss); + + // Then grab each part of the attr path up to this one. + assert(partIdx < attrPath.size()); + std::span const parts( + attrPath.begin(), + attrPath.begin() + partIdx + ); + + // And convert them to strings and join them. + for (auto const & part : parts) { + auto const partName = getName(part, state, env); + ss << "." << state.symbols[partName]; + } + + return ss.str(); + }; + + try { + state.forceValue(*vCurrent, pos); + } catch (Error & e) { + state.addErrorTrace( + e, + getPos(), + "while evaluating '%s' to select '%s' on it", + partsSoFar(), + state.symbols[name] + ); + throw; + } if (vCurrent->type() != nAttrs) { @@ -1480,7 +1514,10 @@ void ExprSelect::eval(EvalState & state, Env & env, Value & v) "expected a set but found %s: %s", showType(*vCurrent), ValuePrinter(state, *vCurrent, errorPrintOptions) - ).withTrace(pos, "while selecting an attribute").debugThrow(); + ).addTrace( + pos, + HintFmt("while selecting '%s' on '%s'", state.symbols[name], partsSoFar()) + ).debugThrow(); } // Now that we know this is actually an attrset, try to find an attr From f40bc4b522c067f23b2b9db3961912f8bcf9900f Mon Sep 17 00:00:00 2001 From: Qyriad Date: Mon, 24 Jun 2024 17:26:21 -0600 Subject: [PATCH 46/47] distinguish between throws & errors during throw MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Turns errors like this: let throwMsg = a: throw (a + " invalid bar"); in throwMsg "bullshit" error: … from call site at «string»:3:4: 2| throwMsg = a: throw (a + " invalid bar"); 3| in throwMsg "bullshit" | ^ … while calling 'throwMsg' at «string»:2:14: 1| let 2| throwMsg = a: throw (a + " invalid bar"); | ^ 3| in throwMsg "bullshit" … while calling the 'throw' builtin at «string»:2:17: 1| let 2| throwMsg = a: throw (a + " invalid bar"); | ^ 3| in throwMsg "bullshit" error: bullshit invalid bar into errors like this: let throwMsg = a: throw (a + " invalid bar"); in throwMsg "bullshit" error: … from call site at «string»:3:4: 2| throwMsg = a: throw (a + " invalid bar"); 3| in throwMsg "bullshit" | ^ … while calling 'throwMsg' at «string»:2:14: 1| let 2| throwMsg = a: throw (a + " invalid bar"); | ^ 3| in throwMsg "bullshit" … caused by explicit throw at «string»:2:17: 1| let 2| throwMsg = a: throw (a + " invalid bar"); | ^ 3| in throwMsg "bullshit" error: bullshit invalid bar Change-Id: I593688928ece20f97999d1bf03b2b46d9ac338cb --- src/libexpr/eval.cc | 9 +++++++++ tests/functional/eval.sh | 2 +- tests/functional/lang/eval-fail-duplicate-traces.err.exp | 2 +- .../eval-fail-foldlStrict-strict-op-application.err.exp | 2 +- tests/functional/lang/eval-fail-mutual-recursion.err.exp | 2 +- tests/functional/lang/eval-fail-not-throws.err.exp | 2 +- tests/functional/lang/eval-fail-toJSON.err.exp | 2 +- 7 files changed, 15 insertions(+), 6 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index b4213c7d9..dd5113fd9 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -1808,6 +1808,15 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value & try { fn->fun(*this, vCur.determinePos(noPos), args, vCur); + } catch (ThrownError & e) { + // Distinguish between an error that simply happened while "throw" + // was being evaluated and an explicit thrown error. + if (fn->name == "throw") { + addErrorTrace(e, pos, "caused by explicit %s", "throw"); + } else { + addErrorTrace(e, pos, "while calling the '%s' builtin", fn->name); + } + throw; } catch (Error & e) { addErrorTrace(e, pos, "while calling the '%1%' builtin", fn->name); throw; diff --git a/tests/functional/eval.sh b/tests/functional/eval.sh index 9c125b569..ae6fcec63 100644 --- a/tests/functional/eval.sh +++ b/tests/functional/eval.sh @@ -25,7 +25,7 @@ nix eval -E 'assert 1 + 2 == 3; true' # Top-level eval errors should be printed to stderr with a traceback. topLevelThrow="$(expectStderr 1 nix eval --expr 'throw "a sample throw message"')" [[ "$topLevelThrow" =~ "a sample throw message" ]] -[[ "$topLevelThrow" =~ "while calling the 'throw' builtin" ]] +[[ "$topLevelThrow" =~ "caused by explicit throw" ]] # But errors inside something should print an elided version, and exit with 0. outputOfNestedThrow="$(nix eval --expr '{ throws = throw "a sample throw message"; }')" diff --git a/tests/functional/lang/eval-fail-duplicate-traces.err.exp b/tests/functional/lang/eval-fail-duplicate-traces.err.exp index cedaebd3b..d9e2ec945 100644 --- a/tests/functional/lang/eval-fail-duplicate-traces.err.exp +++ b/tests/functional/lang/eval-fail-duplicate-traces.err.exp @@ -41,7 +41,7 @@ error: | ^ 5| if n > 0 - … while calling the 'throw' builtin + … caused by explicit throw at /pwd/lang/eval-fail-duplicate-traces.nix:7:10: 6| then throwAfter (n - 1) 7| else throw "Uh oh!"; diff --git a/tests/functional/lang/eval-fail-foldlStrict-strict-op-application.err.exp b/tests/functional/lang/eval-fail-foldlStrict-strict-op-application.err.exp index 4903bc82d..6955fad13 100644 --- a/tests/functional/lang/eval-fail-foldlStrict-strict-op-application.err.exp +++ b/tests/functional/lang/eval-fail-foldlStrict-strict-op-application.err.exp @@ -27,7 +27,7 @@ error: | ^ 6| - … while calling the 'throw' builtin + … caused by explicit throw at /pwd/lang/eval-fail-foldlStrict-strict-op-application.nix:5:9: 4| null 5| [ (_: throw "Not the final value, but is still forced!") (_: 23) ] diff --git a/tests/functional/lang/eval-fail-mutual-recursion.err.exp b/tests/functional/lang/eval-fail-mutual-recursion.err.exp index c034afcd5..c03d2e840 100644 --- a/tests/functional/lang/eval-fail-mutual-recursion.err.exp +++ b/tests/functional/lang/eval-fail-mutual-recursion.err.exp @@ -54,7 +54,7 @@ error: (21 duplicate frames omitted) - … while calling the 'throw' builtin + … caused by explicit throw at /pwd/lang/eval-fail-mutual-recursion.nix:34:10: 33| then throwAfterB true 10 34| else throw "Uh oh!"; diff --git a/tests/functional/lang/eval-fail-not-throws.err.exp b/tests/functional/lang/eval-fail-not-throws.err.exp index fc81f7277..5882a260a 100644 --- a/tests/functional/lang/eval-fail-not-throws.err.exp +++ b/tests/functional/lang/eval-fail-not-throws.err.exp @@ -5,7 +5,7 @@ error: | ^ 2| - … while calling the 'throw' builtin + … caused by explicit throw at /pwd/lang/eval-fail-not-throws.nix:1:4: 1| ! (throw "uh oh!") | ^ diff --git a/tests/functional/lang/eval-fail-toJSON.err.exp b/tests/functional/lang/eval-fail-toJSON.err.exp index ad267711b..18c334923 100644 --- a/tests/functional/lang/eval-fail-toJSON.err.exp +++ b/tests/functional/lang/eval-fail-toJSON.err.exp @@ -40,7 +40,7 @@ error: | ^ 8| } - … while calling the 'throw' builtin + … caused by explicit throw at /pwd/lang/eval-fail-toJSON.nix:7:13: 6| { 7| c.d = throw "hah no"; From ad4b6579a636c3ab29738a97a836e34f5250dc03 Mon Sep 17 00:00:00 2001 From: Qyriad Date: Mon, 24 Jun 2024 19:08:53 -0600 Subject: [PATCH 47/47] mildly cleanup libexpr/eval.hh Change-Id: I40d01a8f8b7fb101279c6f88ebdf1f0969d9d7f0 --- src/libexpr/eval.hh | 54 +++++++++++++++++++++++++-------------------- 1 file changed, 30 insertions(+), 24 deletions(-) diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 8e390e46d..2b52fc57f 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -9,14 +9,13 @@ #include "symbol-table.hh" #include "config.hh" #include "experimental-features.hh" -#include "input-accessor.hh" #include "search-path.hh" #include "repl-exit-status.hh" +#include #include #include #include -#include #include namespace nix { @@ -38,11 +37,29 @@ namespace eval_cache { class EvalCache; } +/** Alias for std::map which uses boehmgc's allocator conditional on us actually + * using boehmgc in this build. + */ +#if HAVE_BOEHMGC + template + using GcMap = std::map< + KeyT, + ValueT, + std::less, + traceable_allocator> + >; +#else + using GcMap = std::map +#endif + /** * Function that implements a primop. */ -typedef void (* PrimOpFun) (EvalState & state, const PosIdx pos, Value * * args, Value & v); +using PrimOpImpl = void(EvalState & state, PosIdx pos, Value ** args, Value & v); + +/** Pointer to a function that implements a primop. */ +using PrimOpFun = PrimOpImpl *; /** * Info about a primitive operation, and its implementation @@ -76,7 +93,7 @@ struct PrimOp /** * Implementation of the primop. */ - std::function::type> fun; + std::function fun; /** * Optional experimental for this to be gated on. @@ -115,11 +132,7 @@ struct Constant bool impureOnly = false; }; -#if HAVE_BOEHMGC - typedef std::map, traceable_allocator > > ValMap; -#else - typedef std::map ValMap; -#endif +using ValMap = GcMap; struct Env { @@ -214,7 +227,8 @@ public: /** * Debugger */ - ReplExitStatus (* debugRepl)(ref es, const ValMap & extraEnv); + using ReplCallbackImpl = ReplExitStatus(ref es, ValMap const & extraEnv); + ReplCallbackImpl * debugRepl; bool debugStop; bool inDebugger = false; int trylevel; @@ -252,21 +266,13 @@ private: /** * A cache from path names to parse trees. */ -#if HAVE_BOEHMGC - typedef std::map, traceable_allocator>> FileParseCache; -#else - typedef std::map FileParseCache; -#endif + using FileParseCache = GcMap; FileParseCache fileParseCache; /** * A cache from path names to values. */ -#if HAVE_BOEHMGC - typedef std::map, traceable_allocator>> FileEvalCache; -#else - typedef std::map FileEvalCache; -#endif + using FileEvalCache = GcMap; FileEvalCache fileEvalCache; SearchPath searchPath; @@ -737,15 +743,15 @@ private: bool countCalls; - typedef std::map PrimOpCalls; + using PrimOpCalls = std::map; PrimOpCalls primOpCalls; - typedef std::map FunctionCalls; + using FunctionCalls = std::map; FunctionCalls functionCalls; void incrFunctionCall(ExprLambda * fun); - typedef std::map AttrSelects; + using AttrSelects = std::map; AttrSelects attrSelects; friend struct ExprOpUpdate; @@ -787,7 +793,7 @@ std::string showType(const Value & v); */ SourcePath resolveExprPath(SourcePath path); -static const std::string corepkgsPrefix{"/__corepkgs__/"}; +static constexpr std::string_view corepkgsPrefix{"/__corepkgs__/"}; }