From fb05a6adcfe0362249bd16527a2e44ea2611e5f3 Mon Sep 17 00:00:00 2001 From: Carlo Nucera Date: Tue, 16 Jun 2020 12:45:36 -0400 Subject: [PATCH 01/42] Eliminate old TeeSink abstraction This was introduced in fa125b9b28bea25a4eeb4d39a71a481563127cb9, and then "reverted" in 1cf480110879ffc8aee94b4b75999da405b71d7c, except that revert left the struct around doing nothing useful. We're removing it all the way now because we want to make a new `TeeSink` complementing the already-exiting `TeeSource`, that is actually a completely different concept as far as the class hierarchy is concerned. --- src/libstore/daemon.cc | 7 ++++--- src/libstore/export-import.cc | 11 ++++++----- src/libutil/archive.hh | 7 ------- 3 files changed, 10 insertions(+), 15 deletions(-) diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc index e370e278c..b2e37c5b5 100644 --- a/src/libstore/daemon.cc +++ b/src/libstore/daemon.cc @@ -721,9 +721,10 @@ static void performOp(TunnelLogger * logger, ref store, if (GET_PROTOCOL_MINOR(clientVersion) >= 21) source = std::make_unique(from, to); else { - TeeSink tee(from); - parseDump(tee, tee.source); - saved = std::move(*tee.source.data); + TeeSource tee(from); + ParseSink sink; + parseDump(sink, tee); + saved = std::move(*tee.data); source = std::make_unique(saved); } diff --git a/src/libstore/export-import.cc b/src/libstore/export-import.cc index cb9da027d..0e33fb687 100644 --- a/src/libstore/export-import.cc +++ b/src/libstore/export-import.cc @@ -77,8 +77,9 @@ StorePaths Store::importPaths(Source & source, std::shared_ptr acces if (n != 1) throw Error("input doesn't look like something created by 'nix-store --export'"); /* Extract the NAR from the source. */ - TeeSink tee(source); - parseDump(tee, tee.source); + TeeSource tee(source); + ParseSink sink; + parseDump(sink, tee); uint32_t magic = readInt(source); if (magic != exportMagic) @@ -94,15 +95,15 @@ StorePaths Store::importPaths(Source & source, std::shared_ptr acces if (deriver != "") info.deriver = parseStorePath(deriver); - info.narHash = hashString(htSHA256, *tee.source.data); - info.narSize = tee.source.data->size(); + info.narHash = hashString(htSHA256, *tee.data); + info.narSize = tee.data->size(); // Ignore optional legacy signature. if (readInt(source) == 1) readString(source); // Can't use underlying source, which would have been exhausted - auto source = StringSource { *tee.source.data }; + auto source = StringSource { *tee.data }; addToStore(info, source, NoRepair, checkSigs, accessor); res.push_back(info.path); diff --git a/src/libutil/archive.hh b/src/libutil/archive.hh index 768fe2536..32d98a610 100644 --- a/src/libutil/archive.hh +++ b/src/libutil/archive.hh @@ -63,13 +63,6 @@ struct ParseSink virtual void createSymlink(const Path & path, const string & target) { }; }; -struct TeeSink : ParseSink -{ - TeeSource source; - - TeeSink(Source & source) : source(source) { } -}; - void parseDump(ParseSink & sink, Source & source); void restorePath(const Path & path, Source & source); From 289b9b8dcf8dc651c2d245d9328911f7addd1626 Mon Sep 17 00:00:00 2001 From: Carlo Nucera Date: Tue, 16 Jun 2020 15:14:11 -0400 Subject: [PATCH 02/42] Create a new TeeSink abstraction This is a bit complex because we want to expose extra functionality the wrapped class has. Perhaps there is some inheritancy trickery to do this nicer, but I don't know it, and this is the first thing we tried after a series of attempts that did build. This design is kind of like that of Rust's Writer, Reader, or Iter adapters, which impliment more traits based on what the inner type implements. --- src/libutil/compression.hh | 12 ++++++++++++ src/libutil/serialise.hh | 22 ++++++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/src/libutil/compression.hh b/src/libutil/compression.hh index dd666a4e1..1bd118b47 100644 --- a/src/libutil/compression.hh +++ b/src/libutil/compression.hh @@ -25,4 +25,16 @@ MakeError(UnknownCompressionMethod, Error); MakeError(CompressionError, Error); +template<> +struct TeeSink> : CompressionSink +{ + MAKE_TEE_SINK(ref); + void finish() override { + orig->finish(); + } + void write(const unsigned char * data, size_t len) override { + return orig->write(data, len); + } +}; + } diff --git a/src/libutil/serialise.hh b/src/libutil/serialise.hh index a04118512..88a6b7ffe 100644 --- a/src/libutil/serialise.hh +++ b/src/libutil/serialise.hh @@ -181,6 +181,28 @@ struct TeeSource : Source } }; +#define MAKE_TEE_SINK(T) \ + T orig; \ + ref data; \ + TeeSink(T && orig) \ + : orig(std::move(orig)), data(make_ref()) { } \ + void operator () (const unsigned char * data, size_t len) { \ + this->data->append((const char *) data, len); \ + (*this->orig)(data, len); \ + } \ + void operator () (const std::string & s) \ + { \ + *data += s; \ + (*this->orig)(s); \ + } + +template +struct TeeSink : Sink +{ + MAKE_TEE_SINK(T); +}; + + /* A reader that consumes the original Source until 'size'. */ struct SizedSource : Source { From a835c740ca67e063fe47e7c31444b7c1cac0fe81 Mon Sep 17 00:00:00 2001 From: Carlo Nucera Date: Tue, 16 Jun 2020 15:14:11 -0400 Subject: [PATCH 03/42] Replace `TransferItem::status` with a local variable Everywhere seems to use `getHTTPStatus` now. --- src/libstore/filetransfer.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index 531b85af8..9566e0ae9 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -56,7 +56,6 @@ struct curlFileTransfer : public FileTransfer Callback callback; CURL * req = 0; bool active = false; // whether the handle has been added to the multi object - std::string status; unsigned int attempt = 0; @@ -175,6 +174,7 @@ struct curlFileTransfer : public FileTransfer size_t realSize = size * nmemb; std::string line((char *) contents, realSize); printMsg(lvlVomit, format("got header for '%s': %s") % request.uri % trim(line)); + std::string status; if (line.compare(0, 5, "HTTP/") == 0) { // new response starts result.etag = ""; auto ss = tokenizeString>(line, " "); From 004570a377b2df355064d48afc54375690c3cdb0 Mon Sep 17 00:00:00 2001 From: Carlo Nucera Date: Tue, 16 Jun 2020 15:14:11 -0400 Subject: [PATCH 04/42] Add HTTP responses to FileTransferErrors --- src/libstore/filetransfer.cc | 26 ++++++++++++++++++++------ src/libstore/filetransfer.hh | 5 +++-- 2 files changed, 23 insertions(+), 8 deletions(-) diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index 9566e0ae9..d89f8388f 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -122,7 +122,7 @@ struct curlFileTransfer : public FileTransfer if (requestHeaders) curl_slist_free_all(requestHeaders); try { if (!done) - fail(FileTransferError(Interrupted, "download of '%s' was interrupted", request.uri)); + fail(FileTransferError(Interrupted, nullptr, "download of '%s' was interrupted", request.uri)); } catch (...) { ignoreException(); } @@ -152,8 +152,18 @@ struct curlFileTransfer : public FileTransfer size_t realSize = size * nmemb; result.bodySize += realSize; - if (!decompressionSink) + if (!decompressionSink) { decompressionSink = makeDecompressionSink(encoding, finalSink); + if (! successfulStatuses.count(getHTTPStatus())) { + // In this case we want to construct a TeeSink, to keep + // the response around (which we figure won't be big + // like an actual download should be) to improve error + // messages. + decompressionSink = std::make_shared>>( + ref{ decompressionSink } + ); + } + } (*decompressionSink)((unsigned char *) contents, realSize); @@ -408,16 +418,20 @@ struct curlFileTransfer : public FileTransfer attempt++; + std::shared_ptr response; + if (decompressionSink) + if (auto teeSink = std::dynamic_pointer_cast>>(decompressionSink)) + response = teeSink->data; auto exc = code == CURLE_ABORTED_BY_CALLBACK && _isInterrupted - ? FileTransferError(Interrupted, fmt("%s of '%s' was interrupted", request.verb(), request.uri)) + ? FileTransferError(Interrupted, response, fmt("%s of '%s' was interrupted", request.verb(), request.uri)) : httpStatus != 0 - ? FileTransferError(err, + ? FileTransferError(err, response, fmt("unable to %s '%s': HTTP error %d", request.verb(), request.uri, httpStatus) + (code == CURLE_OK ? "" : fmt(" (curl error: %s)", curl_easy_strerror(code))) ) - : FileTransferError(err, + : FileTransferError(err, response, fmt("unable to %s '%s': %s (%d)", request.verb(), request.uri, curl_easy_strerror(code), code)); @@ -675,7 +689,7 @@ struct curlFileTransfer : public FileTransfer auto s3Res = s3Helper.getObject(bucketName, key); FileTransferResult res; if (!s3Res.data) - throw FileTransferError(NotFound, fmt("S3 object '%s' does not exist", request.uri)); + throw FileTransferError(NotFound, nullptr, fmt("S3 object '%s' does not exist", request.uri)); res.data = s3Res.data; callback(std::move(res)); #else diff --git a/src/libstore/filetransfer.hh b/src/libstore/filetransfer.hh index 11dca2fe0..8e31a9e42 100644 --- a/src/libstore/filetransfer.hh +++ b/src/libstore/filetransfer.hh @@ -103,9 +103,10 @@ class FileTransferError : public Error { public: FileTransfer::Error error; + std::shared_ptr response; // intentionally optional template - FileTransferError(FileTransfer::Error error, const Args & ... args) - : Error(args...), error(error) + FileTransferError(FileTransfer::Error error, std::shared_ptr response, const Args & ... args) + : Error(args...), error(error), response(response) { } }; From 74b219ef6e5e171c56c8ad7385969e0d0df09ed8 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 18 Jun 2020 14:48:45 +0000 Subject: [PATCH 05/42] Adjust FileTransferError message to use opt response --- src/libstore/filetransfer.cc | 12 ++++++++++++ src/libstore/filetransfer.hh | 7 ++++--- src/libutil/error.hh | 3 +-- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index d89f8388f..fa8ad33f5 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -834,6 +834,18 @@ void FileTransfer::download(FileTransferRequest && request, Sink & sink) } } +template +FileTransferError::FileTransferError(FileTransfer::Error error, std::shared_ptr response, const Args & ... args) + : Error(args...), error(error), response(response) +{ + const auto hf = hintfmt(args...); + if (response) { + err.hint = hintfmt("%1%\n\nresponse body:\n\n%2%", normaltxt(hf.str()), response); + } else { + err.hint = hf; + } +} + bool isUri(const string & s) { if (s.compare(0, 8, "channel:") == 0) return true; diff --git a/src/libstore/filetransfer.hh b/src/libstore/filetransfer.hh index 8e31a9e42..25ade0add 100644 --- a/src/libstore/filetransfer.hh +++ b/src/libstore/filetransfer.hh @@ -104,10 +104,11 @@ class FileTransferError : public Error public: FileTransfer::Error error; std::shared_ptr response; // intentionally optional + template - FileTransferError(FileTransfer::Error error, std::shared_ptr response, const Args & ... args) - : Error(args...), error(error), response(response) - { } + FileTransferError(FileTransfer::Error error, std::shared_ptr response, const Args & ... args); + + virtual const char* sname() const override { return "FileTransferError"; } }; bool isUri(const string & s); diff --git a/src/libutil/error.hh b/src/libutil/error.hh index 1e6102ce1..ac9d2e494 100644 --- a/src/libutil/error.hh +++ b/src/libutil/error.hh @@ -173,9 +173,8 @@ public: template SysError(const Args & ... args) - :Error("") + : Error(""), errNo(errno) { - errNo = errno; auto hf = hintfmt(args...); err.hint = hintfmt("%1%: %2%", normaltxt(hf.str()), strerror(errNo)); } From 639e20dc3ed9c5b28138285653912de78fe0507f Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 18 Jun 2020 17:54:16 +0000 Subject: [PATCH 06/42] Prevent '%' in URL from causing crashes We have a larger problem that passsing computed strings to the first variable argument of many exception constructors is unsafe because that first variable argument is interpreted not as a plain string, but format string, and if it contains '%' boost::format will abort, since there are no arguments to the format string. In this particular instance '%' was used as part of an escape code in a URL, which, when the download failed, caused Nix to abort displaying the `FileTransferError`. --- src/libstore/filetransfer.cc | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index 531b85af8..e795da860 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -410,16 +410,13 @@ struct curlFileTransfer : public FileTransfer auto exc = code == CURLE_ABORTED_BY_CALLBACK && _isInterrupted - ? FileTransferError(Interrupted, fmt("%s of '%s' was interrupted", request.verb(), request.uri)) + ? FileTransferError(Interrupted, "%s of '%s' was interrupted", request.verb(), request.uri) : httpStatus != 0 - ? FileTransferError(err, - fmt("unable to %s '%s': HTTP error %d", - request.verb(), request.uri, httpStatus) - + (code == CURLE_OK ? "" : fmt(" (curl error: %s)", curl_easy_strerror(code))) - ) - : FileTransferError(err, - fmt("unable to %s '%s': %s (%d)", - request.verb(), request.uri, curl_easy_strerror(code), code)); + ? FileTransferError(err, "unable to %s '%s': HTTP error %d%s", + request.verb(), request.uri, httpStatus, + code == CURLE_OK ? "" : fmt(" (curl error: %s)", curl_easy_strerror(code))) + : FileTransferError(err, "unable to %s '%s': %s (%d)", + request.verb(), request.uri, curl_easy_strerror(code), code); /* If this is a transient error, then maybe retry the download after a while. If we're writing to a @@ -675,7 +672,7 @@ struct curlFileTransfer : public FileTransfer auto s3Res = s3Helper.getObject(bucketName, key); FileTransferResult res; if (!s3Res.data) - throw FileTransferError(NotFound, fmt("S3 object '%s' does not exist", request.uri)); + throw FileTransferError(NotFound, "S3 object '%s' does not exist", request.uri); res.data = s3Res.data; callback(std::move(res)); #else From 1b23fe4afb8ed2c41604a1ed19cf3d49c34f46d1 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 18 Jun 2020 19:03:10 +0000 Subject: [PATCH 07/42] Fix bugs - Bad dynamic cast target ...classic - std::shared_ptr need explicit deref --- src/libstore/filetransfer.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index 9ae682bbb..8b66cbdad 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -420,7 +420,7 @@ struct curlFileTransfer : public FileTransfer std::shared_ptr response; if (decompressionSink) - if (auto teeSink = std::dynamic_pointer_cast>>(decompressionSink)) + if (auto teeSink = std::dynamic_pointer_cast>>(decompressionSink)) response = teeSink->data; auto exc = code == CURLE_ABORTED_BY_CALLBACK && _isInterrupted @@ -837,7 +837,7 @@ FileTransferError::FileTransferError(FileTransfer::Error error, std::shared_ptr< { const auto hf = hintfmt(args...); if (response) { - err.hint = hintfmt("%1%\n\nresponse body:\n\n%2%", normaltxt(hf.str()), response); + err.hint = hintfmt("%1%\n\nresponse body:\n\n%2%", normaltxt(hf.str()), *response); } else { err.hint = hf; } From e197bc622948973fca192774b6cd8e0d3157aeb6 Mon Sep 17 00:00:00 2001 From: Carlo Nucera Date: Tue, 23 Jun 2020 11:12:01 -0400 Subject: [PATCH 08/42] Enable the --store option to take relative paths In nix commands which accept --store options, we can now specify a relative path, which will be canonicalized. --- src/libstore/store-api.cc | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index e4a4ae11e..42ffa3ddb 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -864,7 +864,7 @@ StoreType getStoreType(const std::string & uri, const std::string & stateDir) { if (uri == "daemon") { return tDaemon; - } else if (uri == "local" || hasPrefix(uri, "/")) { + } else if (uri == "local" || hasPrefix(uri, "/") || hasPrefix(uri, "./")) { return tLocal; } else if (uri == "" || uri == "auto") { if (access(stateDir.c_str(), R_OK | W_OK) == 0) @@ -888,8 +888,11 @@ static RegisterStoreImplementation regStore([]( return std::shared_ptr(std::make_shared(params)); case tLocal: { Store::Params params2 = params; - if (hasPrefix(uri, "/")) + if (hasPrefix(uri, "/")) { params2["root"] = uri; + } else if (hasPrefix(uri, "./")) { + params2["root"] = absPath(uri); + } return std::shared_ptr(std::make_shared(params2)); } default: From 9ec10046e04a509cc982102f587cf06840f02327 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sat, 11 Jul 2020 16:06:24 +0000 Subject: [PATCH 09/42] Narrow scope of temporary value --- src/libstore/daemon.cc | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc index db7139374..6e0b290ed 100644 --- a/src/libstore/daemon.cc +++ b/src/libstore/daemon.cc @@ -375,21 +375,24 @@ static void performOp(TunnelLogger * logger, ref store, } case wopAddToStore: { - std::string s, baseName; + HashType hashAlgo; + std::string baseName; FileIngestionMethod method; { - bool fixed; uint8_t recursive; - from >> baseName >> fixed /* obsolete */ >> recursive >> s; + bool fixed; + uint8_t recursive; + std::string hashAlgoRaw; + from >> baseName >> fixed /* obsolete */ >> recursive >> hashAlgoRaw; if (recursive > (uint8_t) FileIngestionMethod::Recursive) throw Error("unsupported FileIngestionMethod with value of %i; you may need to upgrade nix-daemon", recursive); method = FileIngestionMethod { recursive }; /* Compatibility hack. */ if (!fixed) { - s = "sha256"; + hashAlgoRaw = "sha256"; method = FileIngestionMethod::Recursive; } + hashAlgo = parseHashType(hashAlgoRaw); } - HashType hashAlgo = parseHashType(s); StringSink savedNAR; TeeSource savedNARSource(from, savedNAR); From c86fc3a9657096b74fe967f2f0bbd120e46908f6 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sat, 11 Jul 2020 15:55:04 +0000 Subject: [PATCH 10/42] Crudely make `addToStoreFromDump` take `Source` not string I just as little beyond the type as possible, so the implementation changes this enables can be reviewed separately. --- src/libstore/build.cc | 2 +- src/libstore/daemon.cc | 5 ++++- src/libstore/local-store.cc | 5 ++++- src/libstore/local-store.hh | 2 +- src/libstore/store-api.hh | 2 +- 5 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index ac2e67574..62294a08c 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -2774,7 +2774,7 @@ struct RestrictedStore : public LocalFSStore goal.addDependency(info.path); } - StorePath addToStoreFromDump(const string & dump, const string & name, + StorePath addToStoreFromDump(Source & dump, const string & name, FileIngestionMethod method = FileIngestionMethod::Recursive, HashType hashAlgo = htSHA256, RepairFlag repair = NoRepair) override { auto path = next->addToStoreFromDump(dump, name, method, hashAlgo, repair); diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc index 6e0b290ed..69d7ef511 100644 --- a/src/libstore/daemon.cc +++ b/src/libstore/daemon.cc @@ -410,8 +410,11 @@ static void performOp(TunnelLogger * logger, ref store, logger->startWork(); if (!savedRegular.regular) throw Error("regular file expected"); + StringSource dumpSource { + method == FileIngestionMethod::Recursive ? *savedNAR.s : savedRegular.s + }; auto path = store->addToStoreFromDump( - method == FileIngestionMethod::Recursive ? *savedNAR.s : savedRegular.s, + dumpSource, baseName, method, hashAlgo); diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 26b226fe8..603f36352 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1033,9 +1033,12 @@ void LocalStore::addToStore(const ValidPathInfo & info, Source & source, } -StorePath LocalStore::addToStoreFromDump(const string & dump, const string & name, +StorePath LocalStore::addToStoreFromDump(Source & dumpSource, const string & name, FileIngestionMethod method, HashType hashAlgo, RepairFlag repair) { + // FIXME: See if we can use the original source to reduce memory usage. + auto dump = dumpSource.drain(); + Hash h = hashString(hashAlgo, dump); auto dstPath = makeFixedOutputPath(method, h, name); diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index c0e5d0286..355c2814f 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -153,7 +153,7 @@ public: in `dump', which is either a NAR serialisation (if recursive == true) or simply the contents of a regular file (if recursive == false). */ - StorePath addToStoreFromDump(const string & dump, const string & name, + StorePath addToStoreFromDump(Source & dump, const string & name, FileIngestionMethod method = FileIngestionMethod::Recursive, HashType hashAlgo = htSHA256, RepairFlag repair = NoRepair) override; StorePath addTextToStore(const string & name, const string & s, diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index a4be0411e..d1cb2035f 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -460,7 +460,7 @@ public: std::optional expectedCAHash = {}); // FIXME: remove? - virtual StorePath addToStoreFromDump(const string & dump, const string & name, + virtual StorePath addToStoreFromDump(Source & dump, const string & name, FileIngestionMethod method = FileIngestionMethod::Recursive, HashType hashAlgo = htSHA256, RepairFlag repair = NoRepair) { throw Error("addToStoreFromDump() is not supported by this store"); From 9de96ef7d409fedea092045c4dbae7177f88962a Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sat, 11 Jul 2020 19:03:39 +0000 Subject: [PATCH 11/42] Dedup `LocalStore::addToStore*` The downsides is that the coroutine has byte-by-byte loop transfer. Will fix that next. --- src/libstore/local-store.cc | 83 ++++++++++--------------------------- src/libstore/local-store.hh | 4 ++ 2 files changed, 25 insertions(+), 62 deletions(-) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 603f36352..925ac25bf 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1033,65 +1033,16 @@ void LocalStore::addToStore(const ValidPathInfo & info, Source & source, } -StorePath LocalStore::addToStoreFromDump(Source & dumpSource, const string & name, +StorePath LocalStore::addToStoreFromDump(Source & dump, const string & name, FileIngestionMethod method, HashType hashAlgo, RepairFlag repair) { - // FIXME: See if we can use the original source to reduce memory usage. - auto dump = dumpSource.drain(); - - Hash h = hashString(hashAlgo, dump); - - auto dstPath = makeFixedOutputPath(method, h, name); - - addTempRoot(dstPath); - - if (repair || !isValidPath(dstPath)) { - - /* The first check above is an optimisation to prevent - unnecessary lock acquisition. */ - - auto realPath = Store::toRealPath(dstPath); - - PathLocks outputLock({realPath}); - - if (repair || !isValidPath(dstPath)) { - - deletePath(realPath); - - autoGC(); - - if (method == FileIngestionMethod::Recursive) { - StringSource source(dump); - restorePath(realPath, source); - } else - writeFile(realPath, dump); - - canonicalisePathMetaData(realPath, -1); - - /* Register the SHA-256 hash of the NAR serialisation of - the path in the database. We may just have computed it - above (if called with recursive == true and hashAlgo == - sha256); otherwise, compute it here. */ - HashResult hash; - if (method == FileIngestionMethod::Recursive) { - hash.first = hashAlgo == htSHA256 ? h : hashString(htSHA256, dump); - hash.second = dump.size(); - } else - hash = hashPath(htSHA256, realPath); - - optimisePath(realPath); // FIXME: combine with hashPath() - - ValidPathInfo info(dstPath); - info.narHash = hash.first; - info.narSize = hash.second; - info.ca = FixedOutputHash { .method = method, .hash = h }; - registerValidPath(info); + return addToStoreCommon(name, method, hashAlgo, repair, [&](auto & sink) { + while (1) { + uint8_t buf[1]; + auto n = dump.read(buf, 1); + sink(buf, n); } - - outputLock.setDeletion(true); - } - - return dstPath; + }); } @@ -1100,6 +1051,19 @@ StorePath LocalStore::addToStore(const string & name, const Path & _srcPath, { Path srcPath(absPath(_srcPath)); + return addToStoreCommon(name, method, hashAlgo, repair, [&](auto & sink) { + if (method == FileIngestionMethod::Recursive) + dumpPath(srcPath, sink, filter); + else + readFile(srcPath, sink); + }); +} + + +StorePath LocalStore::addToStoreCommon( + const string & name, FileIngestionMethod method, HashType hashAlgo, RepairFlag repair, + std::function demux) +{ /* For computing the NAR hash. */ auto sha256Sink = std::make_unique(htSHA256); @@ -1120,7 +1084,6 @@ StorePath LocalStore::addToStore(const string & name, const Path & _srcPath, std::string nar; auto source = sinkToSource([&](Sink & sink) { - LambdaSink sink2([&](const unsigned char * buf, size_t len) { (*sha256Sink)(buf, len); if (hashSink) (*hashSink)(buf, len); @@ -1138,11 +1101,7 @@ StorePath LocalStore::addToStore(const string & name, const Path & _srcPath, if (!inMemory) sink(buf, len); }); - - if (method == FileIngestionMethod::Recursive) - dumpPath(srcPath, sink2, filter); - else - readFile(srcPath, sink2); + demux(sink2); }); std::unique_ptr delTempDir; diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 355c2814f..215731f87 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -290,6 +290,10 @@ private: specified by the ‘secret-key-files’ option. */ void signPathInfo(ValidPathInfo & info); + StorePath addToStoreCommon( + const string & name, FileIngestionMethod method, HashType hashAlgo, RepairFlag repair, + std::function demux); + Path getRealStoreDir() override { return realStoreDir; } void createUser(const std::string & userName, uid_t userId) override; From 592851fb67cd15807109d6f65fb81f6af89af966 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sat, 11 Jul 2020 23:40:49 +0000 Subject: [PATCH 12/42] LocalStore::addToStoreFromDump copy in chunks Rather than copying byte-by-byte, we let the coroutine know how much data we would like it to send back to us. --- src/libstore/local-store.cc | 16 +++++++++------- src/libstore/local-store.hh | 2 +- src/libutil/serialise.cc | 33 ++++++++++++++++++++------------- src/libutil/serialise.hh | 11 ++++++++++- 4 files changed, 40 insertions(+), 22 deletions(-) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 925ac25bf..dac7a50c4 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1036,11 +1036,13 @@ void LocalStore::addToStore(const ValidPathInfo & info, Source & source, StorePath LocalStore::addToStoreFromDump(Source & dump, const string & name, FileIngestionMethod method, HashType hashAlgo, RepairFlag repair) { - return addToStoreCommon(name, method, hashAlgo, repair, [&](auto & sink) { + return addToStoreCommon(name, method, hashAlgo, repair, [&](auto & sink, size_t & wanted) { while (1) { - uint8_t buf[1]; - auto n = dump.read(buf, 1); + constexpr size_t bufSize = 1024; + uint8_t buf[bufSize]; + auto n = dump.read(buf, std::min(wanted, bufSize)); sink(buf, n); + // when control is yielded back to us wanted will be updated. } }); } @@ -1051,7 +1053,7 @@ StorePath LocalStore::addToStore(const string & name, const Path & _srcPath, { Path srcPath(absPath(_srcPath)); - return addToStoreCommon(name, method, hashAlgo, repair, [&](auto & sink) { + return addToStoreCommon(name, method, hashAlgo, repair, [&](auto & sink, size_t & _) { if (method == FileIngestionMethod::Recursive) dumpPath(srcPath, sink, filter); else @@ -1062,7 +1064,7 @@ StorePath LocalStore::addToStore(const string & name, const Path & _srcPath, StorePath LocalStore::addToStoreCommon( const string & name, FileIngestionMethod method, HashType hashAlgo, RepairFlag repair, - std::function demux) + std::function demux) { /* For computing the NAR hash. */ auto sha256Sink = std::make_unique(htSHA256); @@ -1083,7 +1085,7 @@ StorePath LocalStore::addToStoreCommon( bool inMemory = true; std::string nar; - auto source = sinkToSource([&](Sink & sink) { + auto source = sinkToSource([&](Sink & sink, size_t & wanted) { LambdaSink sink2([&](const unsigned char * buf, size_t len) { (*sha256Sink)(buf, len); if (hashSink) (*hashSink)(buf, len); @@ -1101,7 +1103,7 @@ StorePath LocalStore::addToStoreCommon( if (!inMemory) sink(buf, len); }); - demux(sink2); + demux(sink2, wanted); }); std::unique_ptr delTempDir; diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 215731f87..ae23004c4 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -292,7 +292,7 @@ private: StorePath addToStoreCommon( const string & name, FileIngestionMethod method, HashType hashAlgo, RepairFlag repair, - std::function demux); + std::function demux); Path getRealStoreDir() override { return realStoreDir; } diff --git a/src/libutil/serialise.cc b/src/libutil/serialise.cc index c8b71188f..141e9e976 100644 --- a/src/libutil/serialise.cc +++ b/src/libutil/serialise.cc @@ -165,35 +165,43 @@ size_t StringSource::read(unsigned char * data, size_t len) #endif std::unique_ptr sinkToSource( - std::function fun, + std::function fun, std::function eof) { struct SinkToSource : Source { - typedef boost::coroutines2::coroutine coro_t; + typedef boost::coroutines2::coroutine> coro_t; - std::function fun; + std::function fun; std::function eof; std::optional coro; bool started = false; - SinkToSource(std::function fun, std::function eof) + /* It would be nicer to have the co-routines have both args and a + return value, but unfortunately that was removed from Boost's + implementation for some reason, so we use some extra state instead. + */ + size_t wanted = 0; + + SinkToSource(std::function fun, std::function eof) : fun(fun), eof(eof) { } - std::string cur; + std::basic_string cur; size_t pos = 0; size_t read(unsigned char * data, size_t len) override { - if (!coro) + wanted = len < cur.size() ? 0 : len - cur.size(); + if (!coro) { coro = coro_t::pull_type([&](coro_t::push_type & yield) { - LambdaSink sink([&](const unsigned char * data, size_t len) { - if (len) yield(std::string((const char *) data, len)); + LambdaSink sink([&](const uint8_t * data, size_t len) { + if (len) yield(std::basic_string { data, len }); }); - fun(sink); + fun(sink, wanted); }); + } if (!*coro) { eof(); abort(); } @@ -203,11 +211,10 @@ std::unique_ptr sinkToSource( pos = 0; } - auto n = std::min(cur.size() - pos, len); - memcpy(data, (unsigned char *) cur.data() + pos, n); - pos += n; + auto numCopied = cur.copy(data, len, pos); + pos += numCopied; - return n; + return numCopied; } }; diff --git a/src/libutil/serialise.hh b/src/libutil/serialise.hh index 8386a4991..6cb9d1bf5 100644 --- a/src/libutil/serialise.hh +++ b/src/libutil/serialise.hh @@ -260,11 +260,20 @@ struct LambdaSource : Source /* 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 fun, std::function eof = []() { throw EndOfFile("coroutine has finished"); }); +static inline std::unique_ptr sinkToSource( + std::function fun, + std::function eof = []() { + throw EndOfFile("coroutine has finished"); + }) +{ + return sinkToSource([fun](Sink & s, size_t & _) { fun(s); }, eof); +} + void writePadding(size_t len, Sink & sink); void writeString(const unsigned char * buf, size_t len, Sink & sink); From 8173e7bfefc6a5771b2c9ec48bd6edd3b161dd90 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 14 Jul 2020 21:11:08 +0000 Subject: [PATCH 13/42] Fix localhost::addToStore(...Path...) We were calculating the nar hash wrong when the file ingestion method was flat. I don't think there's anything we can do in that case but dump the file again, so that's what I do. As an optomization, we again could reuse the original dump for just the recursive and non-sha256 case, but I rather do that after this fix, and after my other PRs which deduplicate this code. --- src/libstore/local-store.cc | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 26b226fe8..5827dfc58 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1097,15 +1097,8 @@ StorePath LocalStore::addToStore(const string & name, const Path & _srcPath, { Path srcPath(absPath(_srcPath)); - /* For computing the NAR hash. */ - auto sha256Sink = std::make_unique(htSHA256); - - /* For computing the store path. In recursive SHA-256 mode, this - is the same as the NAR hash, so no need to do it again. */ - std::unique_ptr hashSink = - method == FileIngestionMethod::Recursive && hashAlgo == htSHA256 - ? nullptr - : std::make_unique(hashAlgo); + /* For computing the store path. */ + auto hashSink = std::make_unique(hashAlgo); /* Read the source path into memory, but only if it's up to narBufferSize bytes. If it's larger, write it to a temporary @@ -1114,13 +1107,12 @@ StorePath LocalStore::addToStore(const string & name, const Path & _srcPath, temporary path. Otherwise, we move it to the destination store path. */ bool inMemory = true; - std::string nar; + std::string nar; // TODO rename from "nar" to "dump" auto source = sinkToSource([&](Sink & sink) { LambdaSink sink2([&](const unsigned char * buf, size_t len) { - (*sha256Sink)(buf, len); - if (hashSink) (*hashSink)(buf, len); + (*hashSink)(buf, len); if (inMemory) { if (nar.size() + len > settings.narBufferSize) { @@ -1165,9 +1157,7 @@ StorePath LocalStore::addToStore(const string & name, const Path & _srcPath, /* The NAR fits in memory, so we didn't do restorePath(). */ } - auto sha256 = sha256Sink->finish(); - - Hash hash = hashSink ? hashSink->finish().first : sha256.first; + auto [hash, size] = hashSink->finish(); auto dstPath = makeFixedOutputPath(method, hash, name); @@ -1201,13 +1191,22 @@ StorePath LocalStore::addToStore(const string & name, const Path & _srcPath, throw Error("renaming '%s' to '%s'", tempPath, realPath); } + /* For computing the nar hash. In recursive SHA-256 mode, this + is the same as the store hash, so no need to do it again. */ + auto narHash = std::pair { hash, size }; + if (method != FileIngestionMethod::Recursive || hashAlgo != htSHA256) { + HashSink narSink { htSHA256 }; + dumpPath(realPath, narSink); + narHash = narSink.finish(); + } + canonicalisePathMetaData(realPath, -1); // FIXME: merge into restorePath optimisePath(realPath); ValidPathInfo info(dstPath); - info.narHash = sha256.first; - info.narSize = sha256.second; + info.narHash = narHash.first; + info.narSize = narHash.second; info.ca = FixedOutputHash { .method = method, .hash = hash }; registerValidPath(info); } From 650c2c655810c375296b52997e2f85298c7c566a Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 14 Jul 2020 21:28:50 +0000 Subject: [PATCH 14/42] Rename variable `nar` -> `dump` according to TODO --- src/libstore/local-store.cc | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 5827dfc58..cd92f138c 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1107,7 +1107,7 @@ StorePath LocalStore::addToStore(const string & name, const Path & _srcPath, temporary path. Otherwise, we move it to the destination store path. */ bool inMemory = true; - std::string nar; // TODO rename from "nar" to "dump" + std::string dump; auto source = sinkToSource([&](Sink & sink) { @@ -1115,13 +1115,13 @@ StorePath LocalStore::addToStore(const string & name, const Path & _srcPath, (*hashSink)(buf, len); if (inMemory) { - if (nar.size() + len > settings.narBufferSize) { + if (dump.size() + len > settings.narBufferSize) { inMemory = false; sink << 1; - sink((const unsigned char *) nar.data(), nar.size()); - nar.clear(); + sink((const unsigned char *) dump.data(), dump.size()); + dump.clear(); } else { - nar.append((const char *) buf, len); + dump.append((const char *) buf, len); } } @@ -1180,7 +1180,7 @@ StorePath LocalStore::addToStore(const string & name, const Path & _srcPath, if (inMemory) { /* Restore from the NAR in memory. */ - StringSource source(nar); + StringSource source(dump); if (method == FileIngestionMethod::Recursive) restorePath(realPath, source); else From d087cf48552ee82e2bc78bb6c99854bab350ee00 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 15 Jul 2020 21:10:33 +0000 Subject: [PATCH 15/42] Revert "Revert "LocalStore::addToStore(srcPath): Handle the flat case"" This reverts commit cff2157185912025c24a1b9dc99056161634176c. --- src/libstore/local-store.cc | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index b3f4b3f7d..26b226fe8 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1097,16 +1097,13 @@ StorePath LocalStore::addToStore(const string & name, const Path & _srcPath, { Path srcPath(absPath(_srcPath)); - if (method != FileIngestionMethod::Recursive) - return addToStoreFromDump(readFile(srcPath), name, method, hashAlgo, repair); - /* For computing the NAR hash. */ auto sha256Sink = std::make_unique(htSHA256); /* For computing the store path. In recursive SHA-256 mode, this is the same as the NAR hash, so no need to do it again. */ std::unique_ptr hashSink = - hashAlgo == htSHA256 + method == FileIngestionMethod::Recursive && hashAlgo == htSHA256 ? nullptr : std::make_unique(hashAlgo); @@ -1139,7 +1136,10 @@ StorePath LocalStore::addToStore(const string & name, const Path & _srcPath, if (!inMemory) sink(buf, len); }); - dumpPath(srcPath, sink2, filter); + if (method == FileIngestionMethod::Recursive) + dumpPath(srcPath, sink2, filter); + else + readFile(srcPath, sink2); }); std::unique_ptr delTempDir; @@ -1155,7 +1155,10 @@ StorePath LocalStore::addToStore(const string & name, const Path & _srcPath, delTempDir = std::make_unique(tempDir); tempPath = tempDir + "/x"; - restorePath(tempPath, *source); + if (method == FileIngestionMethod::Recursive) + restorePath(tempPath, *source); + else + writeFile(tempPath, *source); } catch (EndOfFile &) { if (!inMemory) throw; @@ -1188,7 +1191,10 @@ StorePath LocalStore::addToStore(const string & name, const Path & _srcPath, if (inMemory) { /* Restore from the NAR in memory. */ StringSource source(nar); - restorePath(realPath, source); + if (method == FileIngestionMethod::Recursive) + restorePath(realPath, source); + else + writeFile(realPath, source); } else { /* Move the temporary path we restored above. */ if (rename(tempPath.c_str(), realPath.c_str())) From bc109648c41f8021707b55b815e68a890a09f2f6 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 15 Jul 2020 23:14:30 +0000 Subject: [PATCH 16/42] Get rid of `LocalStore::addToStoreCommon` I got it to just become `LocalStore::addToStoreFromDump`, cleanly taking a store and then doing nothing too fancy with it. `LocalStore::addToStore(...Path...)` is now just a simple wrapper with a bare-bones sinkToSource of the right dump command. --- src/libstore/local-store.cc | 93 ++++++++++++++++--------------------- src/libstore/local-store.hh | 4 -- src/libutil/serialise.cc | 13 ++++++ src/libutil/serialise.hh | 15 +++++- 4 files changed, 67 insertions(+), 58 deletions(-) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index b9fae6089..07e1679da 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1033,38 +1033,22 @@ void LocalStore::addToStore(const ValidPathInfo & info, Source & source, } -StorePath LocalStore::addToStoreFromDump(Source & dump, const string & name, - FileIngestionMethod method, HashType hashAlgo, RepairFlag repair) -{ - return addToStoreCommon(name, method, hashAlgo, repair, [&](auto & sink, size_t & wanted) { - while (1) { - constexpr size_t bufSize = 1024; - uint8_t buf[bufSize]; - auto n = dump.read(buf, std::min(wanted, bufSize)); - sink(buf, n); - // when control is yielded back to us wanted will be updated. - } - }); -} - - StorePath LocalStore::addToStore(const string & name, const Path & _srcPath, FileIngestionMethod method, HashType hashAlgo, PathFilter & filter, RepairFlag repair) { Path srcPath(absPath(_srcPath)); - - return addToStoreCommon(name, method, hashAlgo, repair, [&](auto & sink, size_t & _) { + auto source = sinkToSource([&](Sink & sink, size_t & wanted) { if (method == FileIngestionMethod::Recursive) dumpPath(srcPath, sink, filter); else readFile(srcPath, sink); }); + return addToStoreFromDump(*source, name, method, hashAlgo, repair); } -StorePath LocalStore::addToStoreCommon( - const string & name, FileIngestionMethod method, HashType hashAlgo, RepairFlag repair, - std::function demux) +StorePath LocalStore::addToStoreFromDump(Source & source, const string & name, + FileIngestionMethod method, HashType hashAlgo, RepairFlag repair) { /* For computing the store path. */ auto hashSink = std::make_unique(hashAlgo); @@ -1075,50 +1059,53 @@ StorePath LocalStore::addToStoreCommon( destination store path is already valid, we just delete the temporary path. Otherwise, we move it to the destination store path. */ - bool inMemory = true; + bool inMemory = false; + std::string dump; - auto source = sinkToSource([&](Sink & sink, size_t & wanted) { - LambdaSink sink2([&](const unsigned char * buf, size_t len) { - (*hashSink)(buf, len); - - if (inMemory) { - if (dump.size() + len > settings.narBufferSize) { - inMemory = false; - sink << 1; - sink((const unsigned char *) dump.data(), dump.size()); - dump.clear(); - } else { - dump.append((const char *) buf, len); - } - } - - if (!inMemory) sink(buf, len); - }); - demux(sink2, wanted); - }); + /* Fill out buffer, and decide whether we are working strictly in + memory based on whether we break out because the buffer is full + or the original source is empty */ + while (dump.size() < settings.narBufferSize) { + auto oldSize = dump.size(); + constexpr size_t chunkSize = 1024; + auto want = std::min(chunkSize, settings.narBufferSize - oldSize); + dump.resize(oldSize + want); + auto got = 0; + try { + got = source.read((uint8_t *) dump.data() + oldSize, want); + } catch (EndOfFile &) { + inMemory = true; + break; + } + /* Start hashing as we get data */ + (*hashSink)((const uint8_t *) dump.data() + oldSize, got); + dump.resize(oldSize + got); + } std::unique_ptr delTempDir; Path tempPath; - try { - /* Wait for the source coroutine to give us some dummy - data. This is so that we don't create the temporary - directory if the NAR fits in memory. */ - readInt(*source); + if (!inMemory) { + StringSource dumpSource { dump }; + TeeSource rest { source, *hashSink }; + ChainSource bothSource { + .source1 = dumpSource, + /* Continue hashing what's left, but don't rehash what we + already did. */ + .source2 = rest, + }; auto tempDir = createTempDir(realStoreDir, "add"); delTempDir = std::make_unique(tempDir); tempPath = tempDir + "/x"; if (method == FileIngestionMethod::Recursive) - restorePath(tempPath, *source); + restorePath(tempPath, bothSource); else - writeFile(tempPath, *source); + writeFile(tempPath, bothSource); - } catch (EndOfFile &) { - if (!inMemory) throw; - /* The NAR fits in memory, so we didn't do restorePath(). */ + dump.clear(); } auto [hash, size] = hashSink->finish(); @@ -1143,12 +1130,12 @@ StorePath LocalStore::addToStoreCommon( autoGC(); if (inMemory) { + StringSource dumpSource { dump }; /* Restore from the NAR in memory. */ - StringSource source(dump); if (method == FileIngestionMethod::Recursive) - restorePath(realPath, source); + restorePath(realPath, dumpSource); else - writeFile(realPath, source); + writeFile(realPath, dumpSource); } else { /* Move the temporary path we restored above. */ if (rename(tempPath.c_str(), realPath.c_str())) diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index ae23004c4..355c2814f 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -290,10 +290,6 @@ private: specified by the ‘secret-key-files’ option. */ void signPathInfo(ValidPathInfo & info); - StorePath addToStoreCommon( - const string & name, FileIngestionMethod method, HashType hashAlgo, RepairFlag repair, - std::function demux); - Path getRealStoreDir() override { return realStoreDir; } void createUser(const std::string & userName, uid_t userId) override; diff --git a/src/libutil/serialise.cc b/src/libutil/serialise.cc index 141e9e976..4c72dc9f2 100644 --- a/src/libutil/serialise.cc +++ b/src/libutil/serialise.cc @@ -329,5 +329,18 @@ void StringSink::operator () (const unsigned char * data, size_t len) s->append((const char *) data, len); } +size_t ChainSource::read(unsigned char * data, size_t len) +{ + if (useSecond) { + return source2.read(data, len); + } else { + try { + return source1.read(data, len); + } catch (EndOfFile &) { + useSecond = true; + return this->read(data, len); + } + } +} } diff --git a/src/libutil/serialise.hh b/src/libutil/serialise.hh index 6cb9d1bf5..3e3735ca5 100644 --- a/src/libutil/serialise.hh +++ b/src/libutil/serialise.hh @@ -256,6 +256,19 @@ struct LambdaSource : Source } }; +/* Chain two sources together so after the first is exhausted, the second is + used */ +struct ChainSource : Source +{ + Source & source1, & source2; + bool useSecond = false; + ChainSource(Source & s1, Source & s2) + : source1(s1), source2(s2) + { } + + size_t read(unsigned char * data, size_t len) override; +}; + /* Convert a function that feeds data into a Sink into a Source. The Source executes the function as a coroutine. */ @@ -271,7 +284,7 @@ static inline std::unique_ptr sinkToSource( throw EndOfFile("coroutine has finished"); }) { - return sinkToSource([fun](Sink & s, size_t & _) { fun(s); }, eof); + return sinkToSource([fun](Sink & s, size_t & _) { fun(s); }, eof); } From 5602637d9ea195784368e99a226718fc95e6b978 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 15 Jul 2020 23:19:41 +0000 Subject: [PATCH 17/42] Revert "LocalStore::addToStoreFromDump copy in chunks" This reverts commit 592851fb67cd15807109d6f65fb81f6af89af966. We don't need this extra feature anymore --- src/libstore/local-store.cc | 2 +- src/libutil/serialise.cc | 33 +++++++++++++-------------------- src/libutil/serialise.hh | 11 +---------- 3 files changed, 15 insertions(+), 31 deletions(-) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 07e1679da..b2b5afadd 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1037,7 +1037,7 @@ StorePath LocalStore::addToStore(const string & name, const Path & _srcPath, FileIngestionMethod method, HashType hashAlgo, PathFilter & filter, RepairFlag repair) { Path srcPath(absPath(_srcPath)); - auto source = sinkToSource([&](Sink & sink, size_t & wanted) { + auto source = sinkToSource([&](Sink & sink) { if (method == FileIngestionMethod::Recursive) dumpPath(srcPath, sink, filter); else diff --git a/src/libutil/serialise.cc b/src/libutil/serialise.cc index 4c72dc9f2..00c945113 100644 --- a/src/libutil/serialise.cc +++ b/src/libutil/serialise.cc @@ -165,43 +165,35 @@ size_t StringSource::read(unsigned char * data, size_t len) #endif std::unique_ptr sinkToSource( - std::function fun, + std::function fun, std::function eof) { struct SinkToSource : Source { - typedef boost::coroutines2::coroutine> coro_t; + typedef boost::coroutines2::coroutine coro_t; - std::function fun; + std::function fun; std::function eof; std::optional coro; bool started = false; - /* It would be nicer to have the co-routines have both args and a - return value, but unfortunately that was removed from Boost's - implementation for some reason, so we use some extra state instead. - */ - size_t wanted = 0; - - SinkToSource(std::function fun, std::function eof) + SinkToSource(std::function fun, std::function eof) : fun(fun), eof(eof) { } - std::basic_string cur; + std::string cur; size_t pos = 0; size_t read(unsigned char * data, size_t len) override { - wanted = len < cur.size() ? 0 : len - cur.size(); - if (!coro) { + if (!coro) coro = coro_t::pull_type([&](coro_t::push_type & yield) { - LambdaSink sink([&](const uint8_t * data, size_t len) { - if (len) yield(std::basic_string { data, len }); + LambdaSink sink([&](const unsigned char * data, size_t len) { + if (len) yield(std::string((const char *) data, len)); }); - fun(sink, wanted); + fun(sink); }); - } if (!*coro) { eof(); abort(); } @@ -211,10 +203,11 @@ std::unique_ptr sinkToSource( pos = 0; } - auto numCopied = cur.copy(data, len, pos); - pos += numCopied; + auto n = std::min(cur.size() - pos, len); + memcpy(data, (unsigned char *) cur.data() + pos, n); + pos += n; - return numCopied; + return n; } }; diff --git a/src/libutil/serialise.hh b/src/libutil/serialise.hh index 3e3735ca5..aa6b42597 100644 --- a/src/libutil/serialise.hh +++ b/src/libutil/serialise.hh @@ -273,19 +273,10 @@ struct ChainSource : Source /* 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"); - }); - -static inline std::unique_ptr sinkToSource( std::function fun, std::function eof = []() { throw EndOfFile("coroutine has finished"); - }) -{ - return sinkToSource([fun](Sink & s, size_t & _) { fun(s); }, eof); -} + }); void writePadding(size_t len, Sink & sink); From 68dfb8c6aef7afebf0312c48bb5010653fc464b3 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 16 Jul 2020 05:09:41 +0000 Subject: [PATCH 18/42] Optimize `addToStoreSlow` and remove `TeeParseSink` --- src/libstore/daemon.cc | 47 +++++++---------------------------- src/libstore/export-import.cc | 12 +++++---- src/libstore/store-api.cc | 35 ++++++++++++++++++++------ src/libutil/archive.hh | 25 ++++++++++++++++--- 4 files changed, 65 insertions(+), 54 deletions(-) diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc index db7139374..573836f7f 100644 --- a/src/libstore/daemon.cc +++ b/src/libstore/daemon.cc @@ -173,31 +173,6 @@ struct TunnelSource : BufferedSource } }; -/* If the NAR archive contains a single file at top-level, then save - the contents of the file to `s'. Otherwise barf. */ -struct RetrieveRegularNARSink : ParseSink -{ - bool regular; - string s; - - RetrieveRegularNARSink() : regular(true) { } - - void createDirectory(const Path & path) - { - regular = false; - } - - void receiveContents(unsigned char * data, unsigned int len) - { - s.append((const char *) data, len); - } - - void createSymlink(const Path & path, const string & target) - { - regular = false; - } -}; - struct ClientSettings { bool keepFailed; @@ -391,9 +366,9 @@ static void performOp(TunnelLogger * logger, ref store, } HashType hashAlgo = parseHashType(s); - StringSink savedNAR; - TeeSource savedNARSource(from, savedNAR); - RetrieveRegularNARSink savedRegular; + StringSink saved; + TeeSource savedNARSource(from, saved); + RetrieveRegularNARSink savedRegular { saved }; if (method == FileIngestionMethod::Recursive) { /* Get the entire NAR dump from the client and save it to @@ -407,11 +382,7 @@ static void performOp(TunnelLogger * logger, ref store, logger->startWork(); if (!savedRegular.regular) throw Error("regular file expected"); - auto path = store->addToStoreFromDump( - method == FileIngestionMethod::Recursive ? *savedNAR.s : savedRegular.s, - baseName, - method, - hashAlgo); + auto path = store->addToStoreFromDump(*saved.s, baseName, method, hashAlgo); logger->stopWork(); to << store->printStorePath(path); @@ -727,15 +698,15 @@ static void performOp(TunnelLogger * logger, ref store, if (!trusted) info.ultimate = false; - std::string saved; std::unique_ptr source; if (GET_PROTOCOL_MINOR(clientVersion) >= 21) source = std::make_unique(from, to); else { - TeeParseSink tee(from); - parseDump(tee, tee.source); - saved = std::move(*tee.saved.s); - source = std::make_unique(saved); + StringSink saved; + TeeSource tee { from, saved }; + ParseSink ether; + parseDump(ether, tee); + source = std::make_unique(std::move(*saved.s)); } logger->startWork(); diff --git a/src/libstore/export-import.cc b/src/libstore/export-import.cc index 082d0f1d1..b963d64d7 100644 --- a/src/libstore/export-import.cc +++ b/src/libstore/export-import.cc @@ -60,8 +60,10 @@ StorePaths Store::importPaths(Source & source, CheckSigsFlag checkSigs) if (n != 1) throw Error("input doesn't look like something created by 'nix-store --export'"); /* Extract the NAR from the source. */ - TeeParseSink tee(source); - parseDump(tee, tee.source); + StringSink saved; + TeeSource tee { source, saved }; + ParseSink ether; + parseDump(ether, tee); uint32_t magic = readInt(source); if (magic != exportMagic) @@ -77,15 +79,15 @@ StorePaths Store::importPaths(Source & source, CheckSigsFlag checkSigs) if (deriver != "") info.deriver = parseStorePath(deriver); - info.narHash = hashString(htSHA256, *tee.saved.s); - info.narSize = tee.saved.s->size(); + info.narHash = hashString(htSHA256, *saved.s); + info.narSize = saved.s->size(); // Ignore optional legacy signature. if (readInt(source) == 1) readString(source); // Can't use underlying source, which would have been exhausted - auto source = StringSource { *tee.saved.s }; + auto source = StringSource { *saved.s }; addToStore(info, source, NoRepair, checkSigs); res.push_back(info.path); diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 5b9f79049..5c8dddba5 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -226,16 +226,37 @@ ValidPathInfo Store::addToStoreSlow(std::string_view name, const Path & srcPath, FileIngestionMethod method, HashType hashAlgo, std::optional expectedCAHash) { - /* FIXME: inefficient: we're reading/hashing 'tmpFile' three + /* FIXME: inefficient: we're reading/hashing 'tmpFile' two times. */ + HashSink narHashSink { htSHA256 }; + HashSink caHashSink { hashAlgo }; + RetrieveRegularNARSink fileSink { caHashSink }; - auto [narHash, narSize] = hashPath(htSHA256, srcPath); + TeeSink sinkIfNar { narHashSink, caHashSink }; - auto hash = method == FileIngestionMethod::Recursive - ? hashAlgo == htSHA256 - ? narHash - : hashPath(hashAlgo, srcPath).first - : hashFile(hashAlgo, srcPath); + /* We use the tee sink if we need to hash he nar twice */ + auto & sink = method == FileIngestionMethod::Recursive && hashAlgo != htSHA256 + ? static_cast(sinkIfNar) + : narHashSink; + + auto fileSource = sinkToSource([&](Sink & sink) { + dumpPath(srcPath, sink); + }); + + TeeSource tapped { *fileSource, sink }; + + ParseSink blank; + auto & parseSink = method == FileIngestionMethod::Flat + ? fileSink + : blank; + + parseDump(parseSink, tapped); + + auto [narHash, narSize] = narHashSink.finish(); + + auto hash = method == FileIngestionMethod::Recursive && hashAlgo == htSHA256 + ? narHash + : caHashSink.finish().first; if (expectedCAHash && expectedCAHash != hash) throw Error("hash mismatch for '%s'", srcPath); diff --git a/src/libutil/archive.hh b/src/libutil/archive.hh index 302b1bb18..57780d16a 100644 --- a/src/libutil/archive.hh +++ b/src/libutil/archive.hh @@ -63,12 +63,29 @@ struct ParseSink virtual void createSymlink(const Path & path, const string & target) { }; }; -struct TeeParseSink : ParseSink +/* If the NAR archive contains a single file at top-level, then save + the contents of the file to `s'. Otherwise barf. */ +struct RetrieveRegularNARSink : ParseSink { - StringSink saved; - TeeSource source; + bool regular = true; + Sink & sink; - TeeParseSink(Source & source) : source(source, saved) { } + RetrieveRegularNARSink(Sink & sink) : sink(sink) { } + + void createDirectory(const Path & path) + { + regular = false; + } + + void receiveContents(unsigned char * data, unsigned int len) + { + sink(data, len); + } + + void createSymlink(const Path & path, const string & target) + { + regular = false; + } }; void parseDump(ParseSink & sink, Source & source); From 5517eee17e37565a1d5b7fb19f9e810068c9428d Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 16 Jul 2020 15:14:22 +0200 Subject: [PATCH 19/42] Generations API cleanup --- src/libstore/profiles.cc | 81 ++++++++++++++++++---------------------- src/libstore/profiles.hh | 25 +++++-------- src/nix-env/nix-env.cc | 28 ++++++-------- 3 files changed, 59 insertions(+), 75 deletions(-) diff --git a/src/libstore/profiles.cc b/src/libstore/profiles.cc index 6cfe393a4..6862b42f0 100644 --- a/src/libstore/profiles.cc +++ b/src/libstore/profiles.cc @@ -12,30 +12,24 @@ namespace nix { -static bool cmpGensByNumber(const Generation & a, const Generation & b) -{ - return a.number < b.number; -} - - /* Parse a generation name of the format `--link'. */ -static int parseName(const string & profileName, const string & name) +static std::optional parseName(const string & profileName, const string & name) { - if (string(name, 0, profileName.size() + 1) != profileName + "-") return -1; + if (string(name, 0, profileName.size() + 1) != profileName + "-") return {}; string s = string(name, profileName.size() + 1); string::size_type p = s.find("-link"); - if (p == string::npos) return -1; - int n; + if (p == string::npos) return {}; + unsigned int n; if (string2Int(string(s, 0, p), n) && n >= 0) return n; else - return -1; + return {}; } -Generations findGenerations(Path profile, int & curGen) +std::pair> findGenerations(Path profile) { Generations gens; @@ -43,30 +37,34 @@ Generations findGenerations(Path profile, int & curGen) auto profileName = std::string(baseNameOf(profile)); for (auto & i : readDirectory(profileDir)) { - int n; - if ((n = parseName(profileName, i.name)) != -1) { - Generation gen; - gen.path = profileDir + "/" + i.name; - gen.number = n; + if (auto n = parseName(profileName, i.name)) { + auto path = profileDir + "/" + i.name; struct stat st; - if (lstat(gen.path.c_str(), &st) != 0) - throw SysError("statting '%1%'", gen.path); - gen.creationTime = st.st_mtime; - gens.push_back(gen); + if (lstat(path.c_str(), &st) != 0) + throw SysError("statting '%1%'", path); + gens.push_back({ + .number = *n, + .path = path, + .creationTime = st.st_mtime + }); } } - gens.sort(cmpGensByNumber); + gens.sort([](const Generation & a, const Generation & b) + { + return a.number < b.number; + }); - curGen = pathExists(profile) + return { + gens, + pathExists(profile) ? parseName(profileName, readLink(profile)) - : -1; - - return gens; + : std::nullopt + }; } -static void makeName(const Path & profile, unsigned int num, +static void makeName(const Path & profile, GenerationNumber num, Path & outLink) { Path prefix = (format("%1%-%2%") % profile % num).str(); @@ -78,10 +76,9 @@ Path createGeneration(ref store, Path profile, Path outPath) { /* The new generation number should be higher than old the previous ones. */ - int dummy; - Generations gens = findGenerations(profile, dummy); + auto [gens, dummy] = findGenerations(profile); - unsigned int num; + GenerationNumber num; if (gens.size() > 0) { Generation last = gens.back(); @@ -121,7 +118,7 @@ static void removeFile(const Path & path) } -void deleteGeneration(const Path & profile, unsigned int gen) +void deleteGeneration(const Path & profile, GenerationNumber gen) { Path generation; makeName(profile, gen, generation); @@ -129,7 +126,7 @@ void deleteGeneration(const Path & profile, unsigned int gen) } -static void deleteGeneration2(const Path & profile, unsigned int gen, bool dryRun) +static void deleteGeneration2(const Path & profile, GenerationNumber gen, bool dryRun) { if (dryRun) printInfo(format("would remove generation %1%") % gen); @@ -140,31 +137,29 @@ static void deleteGeneration2(const Path & profile, unsigned int gen, bool dryRu } -void deleteGenerations(const Path & profile, const std::set & gensToDelete, bool dryRun) +void deleteGenerations(const Path & profile, const std::set & gensToDelete, bool dryRun) { PathLocks lock; lockProfile(lock, profile); - int curGen; - Generations gens = findGenerations(profile, curGen); + auto [gens, curGen] = findGenerations(profile); - if (gensToDelete.find(curGen) != gensToDelete.end()) + if (gensToDelete.count(*curGen)) throw Error("cannot delete current generation of profile %1%'", profile); for (auto & i : gens) { - if (gensToDelete.find(i.number) == gensToDelete.end()) continue; + if (!gensToDelete.count(i.number)) continue; deleteGeneration2(profile, i.number, dryRun); } } -void deleteGenerationsGreaterThan(const Path & profile, int max, bool dryRun) +void deleteGenerationsGreaterThan(const Path & profile, GenerationNumber max, bool dryRun) { PathLocks lock; lockProfile(lock, profile); - int curGen; bool fromCurGen = false; - Generations gens = findGenerations(profile, curGen); + auto [gens, curGen] = findGenerations(profile); for (auto i = gens.rbegin(); i != gens.rend(); ++i) { if (i->number == curGen) { fromCurGen = true; @@ -186,8 +181,7 @@ void deleteOldGenerations(const Path & profile, bool dryRun) PathLocks lock; lockProfile(lock, profile); - int curGen; - Generations gens = findGenerations(profile, curGen); + auto [gens, curGen] = findGenerations(profile); for (auto & i : gens) if (i.number != curGen) @@ -200,8 +194,7 @@ void deleteGenerationsOlderThan(const Path & profile, time_t t, bool dryRun) PathLocks lock; lockProfile(lock, profile); - int curGen; - Generations gens = findGenerations(profile, curGen); + auto [gens, curGen] = findGenerations(profile); bool canDelete = false; for (auto i = gens.rbegin(); i != gens.rend(); ++i) diff --git a/src/libstore/profiles.hh b/src/libstore/profiles.hh index 78645d8b6..abe507f0e 100644 --- a/src/libstore/profiles.hh +++ b/src/libstore/profiles.hh @@ -9,37 +9,32 @@ namespace nix { +typedef unsigned int GenerationNumber; + struct Generation { - int number; + GenerationNumber number; Path path; time_t creationTime; - Generation() - { - number = -1; - } - operator bool() const - { - return number != -1; - } }; -typedef list Generations; +typedef std::list Generations; /* Returns the list of currently present generations for the specified - profile, sorted by generation number. */ -Generations findGenerations(Path profile, int & curGen); + profile, sorted by generation number. Also returns the number of + the current generation. */ +std::pair> findGenerations(Path profile); class LocalFSStore; Path createGeneration(ref store, Path profile, Path outPath); -void deleteGeneration(const Path & profile, unsigned int gen); +void deleteGeneration(const Path & profile, GenerationNumber gen); -void deleteGenerations(const Path & profile, const std::set & gensToDelete, bool dryRun); +void deleteGenerations(const Path & profile, const std::set & gensToDelete, bool dryRun); -void deleteGenerationsGreaterThan(const Path & profile, const int max, bool dryRun); +void deleteGenerationsGreaterThan(const Path & profile, GenerationNumber max, bool dryRun); void deleteOldGenerations(const Path & profile, bool dryRun); diff --git a/src/nix-env/nix-env.cc b/src/nix-env/nix-env.cc index c992b7d74..5795c2c09 100644 --- a/src/nix-env/nix-env.cc +++ b/src/nix-env/nix-env.cc @@ -1208,18 +1208,17 @@ static void opSwitchProfile(Globals & globals, Strings opFlags, Strings opArgs) } -static const int prevGen = -2; +static constexpr GenerationNumber prevGen = std::numeric_limits::max(); -static void switchGeneration(Globals & globals, int dstGen) +static void switchGeneration(Globals & globals, GenerationNumber dstGen) { PathLocks lock; lockProfile(lock, globals.profile); - int curGen; - Generations gens = findGenerations(globals.profile, curGen); + auto [gens, curGen] = findGenerations(globals.profile); - Generation dst; + std::optional dst; for (auto & i : gens) if ((dstGen == prevGen && i.number < curGen) || (dstGen >= 0 && i.number == dstGen)) @@ -1227,18 +1226,16 @@ static void switchGeneration(Globals & globals, int dstGen) if (!dst) { if (dstGen == prevGen) - throw Error("no generation older than the current (%1%) exists", - curGen); + throw Error("no generation older than the current (%1%) exists", curGen.value_or(0)); else throw Error("generation %1% does not exist", dstGen); } - printInfo(format("switching from generation %1% to %2%") - % curGen % dst.number); + printInfo("switching from generation %1% to %2%", curGen.value_or(0), dst->number); if (globals.dryRun) return; - switchLink(globals.profile, dst.path); + switchLink(globals.profile, dst->path); } @@ -1249,7 +1246,7 @@ static void opSwitchGeneration(Globals & globals, Strings opFlags, Strings opArg if (opArgs.size() != 1) throw UsageError("exactly one argument expected"); - int dstGen; + GenerationNumber dstGen; if (!string2Int(opArgs.front(), dstGen)) throw UsageError("expected a generation number"); @@ -1278,8 +1275,7 @@ static void opListGenerations(Globals & globals, Strings opFlags, Strings opArgs PathLocks lock; lockProfile(lock, globals.profile); - int curGen; - Generations gens = findGenerations(globals.profile, curGen); + auto [gens, curGen] = findGenerations(globals.profile); RunPager pager; @@ -1308,14 +1304,14 @@ static void opDeleteGenerations(Globals & globals, Strings opFlags, Strings opAr if(opArgs.front().size() < 2) throw Error("invalid number of generations ‘%1%’", opArgs.front()); string str_max = string(opArgs.front(), 1, opArgs.front().size()); - int max; + GenerationNumber max; if (!string2Int(str_max, max) || max == 0) throw Error("invalid number of generations to keep ‘%1%’", opArgs.front()); deleteGenerationsGreaterThan(globals.profile, max, globals.dryRun); } else { - std::set gens; + std::set gens; for (auto & i : opArgs) { - unsigned int n; + GenerationNumber n; if (!string2Int(i, n)) throw UsageError("invalid generation number '%1%'", i); gens.insert(n); From 3dcca18c30cbc09652f5ac644a9f8750f9ced0c9 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 16 Jul 2020 13:39:03 +0000 Subject: [PATCH 20/42] Fix bug in TeeSource We use this to simplify `LocalStore::addToStoreFromDump`. Also, hope I fixed build error with old clang (used in Darwin CI). --- src/libstore/local-store.cc | 14 ++++---------- src/libutil/serialise.hh | 2 +- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index b2b5afadd..96d10d9ba 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1047,11 +1047,12 @@ StorePath LocalStore::addToStore(const string & name, const Path & _srcPath, } -StorePath LocalStore::addToStoreFromDump(Source & source, const string & name, +StorePath LocalStore::addToStoreFromDump(Source & source0, const string & name, FileIngestionMethod method, HashType hashAlgo, RepairFlag repair) { /* For computing the store path. */ auto hashSink = std::make_unique(hashAlgo); + TeeSource source { source0, *hashSink }; /* Read the source path into memory, but only if it's up to narBufferSize bytes. If it's larger, write it to a temporary @@ -1078,8 +1079,6 @@ StorePath LocalStore::addToStoreFromDump(Source & source, const string & name, inMemory = true; break; } - /* Start hashing as we get data */ - (*hashSink)((const uint8_t *) dump.data() + oldSize, got); dump.resize(oldSize + got); } @@ -1087,14 +1086,9 @@ StorePath LocalStore::addToStoreFromDump(Source & source, const string & name, Path tempPath; if (!inMemory) { + /* Drain what we pulled so far, and then keep on pulling */ StringSource dumpSource { dump }; - TeeSource rest { source, *hashSink }; - ChainSource bothSource { - .source1 = dumpSource, - /* Continue hashing what's left, but don't rehash what we - already did. */ - .source2 = rest, - }; + ChainSource bothSource { dumpSource, source }; auto tempDir = createTempDir(realStoreDir, "add"); delTempDir = std::make_unique(tempDir); diff --git a/src/libutil/serialise.hh b/src/libutil/serialise.hh index aa6b42597..5d9acf887 100644 --- a/src/libutil/serialise.hh +++ b/src/libutil/serialise.hh @@ -189,7 +189,7 @@ struct TeeSource : Source size_t read(unsigned char * data, size_t len) { size_t n = orig.read(data, len); - sink(data, len); + sink(data, n); return n; } }; From 16c9f6762d082155b967710a5fd3a095937d76ba Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 16 Jul 2020 17:00:42 +0200 Subject: [PATCH 21/42] Add command 'nix profile diff-closure' MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This shows all changes between generations of a profile. E.g. $ nix profile diff-closures --profile /nix/var/nix/profiles/system Generation 654 -> 655: nix: 2.4pre20200617_5d69bbf → 2.4pre20200701_6ff9aa8, +42.2 KiB Generation 655 -> 656: blender-bin: 2.83.0 → 2.83.1, -294.2 KiB Generation 656 -> 657: curl: 7.68.0 → 7.70.0, +19.1 KiB firmware-linux-nonfree: 2020-01-22 → 2020-05-19, +30827.7 KiB ibus: -21.8 KiB initrd-linux: 5.4.46 → 5.4.49 ... --- src/nix/command.hh | 6 +++ src/nix/diff-closures.cc | 100 ++++++++++++++++++++++----------------- src/nix/profile.cc | 43 ++++++++++++++++- 3 files changed, 104 insertions(+), 45 deletions(-) diff --git a/src/nix/command.hh b/src/nix/command.hh index 1c7413300..856721ebf 100644 --- a/src/nix/command.hh +++ b/src/nix/command.hh @@ -244,4 +244,10 @@ void completeFlakeRefWithFragment( const Strings & defaultFlakeAttrPaths, std::string_view prefix); +void printClosureDiff( + ref store, + const StorePath & beforePath, + const StorePath & afterPath, + std::string_view indent); + } diff --git a/src/nix/diff-closures.cc b/src/nix/diff-closures.cc index 56ddb575b..4199dae0f 100644 --- a/src/nix/diff-closures.cc +++ b/src/nix/diff-closures.cc @@ -6,7 +6,7 @@ #include -using namespace nix; +namespace nix { struct Info { @@ -52,6 +52,60 @@ std::string showVersions(const std::set & versions) return concatStringsSep(", ", versions2); } +void printClosureDiff( + ref store, + const StorePath & beforePath, + const StorePath & afterPath, + std::string_view indent) +{ + auto beforeClosure = getClosureInfo(store, beforePath); + auto afterClosure = getClosureInfo(store, afterPath); + + std::set allNames; + for (auto & [name, _] : beforeClosure) allNames.insert(name); + for (auto & [name, _] : afterClosure) allNames.insert(name); + + for (auto & name : allNames) { + auto & beforeVersions = beforeClosure[name]; + auto & afterVersions = afterClosure[name]; + + auto totalSize = [&](const std::map> & versions) + { + uint64_t sum = 0; + for (auto & [_, paths] : versions) + for (auto & [path, _] : paths) + sum += store->queryPathInfo(path)->narSize; + return sum; + }; + + auto beforeSize = totalSize(beforeVersions); + auto afterSize = totalSize(afterVersions); + auto sizeDelta = (int64_t) afterSize - (int64_t) beforeSize; + auto showDelta = abs(sizeDelta) >= 8 * 1024; + + std::set removed, unchanged; + for (auto & [version, _] : beforeVersions) + if (!afterVersions.count(version)) removed.insert(version); else unchanged.insert(version); + + std::set added; + for (auto & [version, _] : afterVersions) + if (!beforeVersions.count(version)) added.insert(version); + + if (showDelta || !removed.empty() || !added.empty()) { + std::vector items; + if (!removed.empty() || !added.empty()) + items.push_back(fmt("%s → %s", showVersions(removed), showVersions(added))); + if (showDelta) + items.push_back(fmt("%s%+.1f KiB" ANSI_NORMAL, sizeDelta > 0 ? ANSI_RED : ANSI_GREEN, sizeDelta / 1024.0)); + std::cout << fmt("%s%s: %s\n", indent, name, concatStringsSep(", ", items)); + } + } +} + +} + +using namespace nix; + struct CmdDiffClosures : SourceExprCommand { std::string _before, _after; @@ -85,49 +139,7 @@ struct CmdDiffClosures : SourceExprCommand auto beforePath = toStorePath(store, Realise::Outputs, operateOn, before); auto after = parseInstallable(store, _after); auto afterPath = toStorePath(store, Realise::Outputs, operateOn, after); - - auto beforeClosure = getClosureInfo(store, beforePath); - auto afterClosure = getClosureInfo(store, afterPath); - - std::set allNames; - for (auto & [name, _] : beforeClosure) allNames.insert(name); - for (auto & [name, _] : afterClosure) allNames.insert(name); - - for (auto & name : allNames) { - auto & beforeVersions = beforeClosure[name]; - auto & afterVersions = afterClosure[name]; - - auto totalSize = [&](const std::map> & versions) - { - uint64_t sum = 0; - for (auto & [_, paths] : versions) - for (auto & [path, _] : paths) - sum += store->queryPathInfo(path)->narSize; - return sum; - }; - - auto beforeSize = totalSize(beforeVersions); - auto afterSize = totalSize(afterVersions); - auto sizeDelta = (int64_t) afterSize - (int64_t) beforeSize; - auto showDelta = abs(sizeDelta) >= 8 * 1024; - - std::set removed, unchanged; - for (auto & [version, _] : beforeVersions) - if (!afterVersions.count(version)) removed.insert(version); else unchanged.insert(version); - - std::set added; - for (auto & [version, _] : afterVersions) - if (!beforeVersions.count(version)) added.insert(version); - - if (showDelta || !removed.empty() || !added.empty()) { - std::vector items; - if (!removed.empty() || !added.empty()) - items.push_back(fmt("%s → %s", showVersions(removed), showVersions(added))); - if (showDelta) - items.push_back(fmt("%s%+.1f KiB" ANSI_NORMAL, sizeDelta > 0 ? ANSI_RED : ANSI_GREEN, sizeDelta / 1024.0)); - std::cout << fmt("%s: %s\n", name, concatStringsSep(", ", items)); - } - } + printClosureDiff(store, beforePath, afterPath, ""); } }; diff --git a/src/nix/profile.cc b/src/nix/profile.cc index 307e236d8..729924e3a 100644 --- a/src/nix/profile.cc +++ b/src/nix/profile.cc @@ -7,6 +7,7 @@ #include "builtins/buildenv.hh" #include "flake/flakeref.hh" #include "../nix-env/user-env.hh" +#include "profiles.hh" #include #include @@ -394,6 +395,46 @@ struct CmdProfileInfo : virtual EvalCommand, virtual StoreCommand, MixDefaultPro } }; +struct CmdProfileDiffClosures : virtual EvalCommand, virtual StoreCommand, MixDefaultProfile +{ + std::string description() override + { + return "show the closure difference between each generation of a profile"; + } + + Examples examples() override + { + return { + Example{ + "To show what changed between each generation of the NixOS system profile:", + "nix profile diff-closure --profile /nix/var/nix/profiles/system" + }, + }; + } + + void run(ref store) override + { + auto [gens, curGen] = findGenerations(*profile); + + std::optional prevGen; + bool first = true; + + for (auto & gen : gens) { + if (prevGen) { + if (!first) std::cout << "\n"; + first = false; + std::cout << fmt("Generation %d -> %d:\n", prevGen->number, gen.number); + printClosureDiff(store, + store->followLinksToStorePath(prevGen->path), + store->followLinksToStorePath(gen.path), + " "); + } + + prevGen = gen; + } + } +}; + struct CmdProfile : virtual MultiCommand, virtual Command { CmdProfile() @@ -402,6 +443,7 @@ struct CmdProfile : virtual MultiCommand, virtual Command {"remove", []() { return make_ref(); }}, {"upgrade", []() { return make_ref(); }}, {"info", []() { return make_ref(); }}, + {"diff-closures", []() { return make_ref(); }}, }) { } @@ -425,4 +467,3 @@ struct CmdProfile : virtual MultiCommand, virtual Command }; static auto r1 = registerCommand("profile"); - From 52c8be38e0563c964857491afef01eb2f543d0de Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 17 Jul 2020 12:36:12 +0200 Subject: [PATCH 22/42] nix profile diff-closures: Don't inherit EvalCommand --- src/nix/profile.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nix/profile.cc b/src/nix/profile.cc index 729924e3a..c6cd88c49 100644 --- a/src/nix/profile.cc +++ b/src/nix/profile.cc @@ -395,7 +395,7 @@ struct CmdProfileInfo : virtual EvalCommand, virtual StoreCommand, MixDefaultPro } }; -struct CmdProfileDiffClosures : virtual EvalCommand, virtual StoreCommand, MixDefaultProfile +struct CmdProfileDiffClosures : virtual StoreCommand, MixDefaultProfile { std::string description() override { From 17f75f9cc4dd70e3e6de7e266ef2bd18a0da310b Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 17 Jul 2020 14:54:21 +0200 Subject: [PATCH 23/42] parseFlakeRef(): Only search for the top-level directory for CLI flakerefs --- src/libexpr/flake/flakeref.cc | 79 +++++++++++++++++++---------------- tests/flakes.sh | 9 ++-- 2 files changed, 45 insertions(+), 43 deletions(-) diff --git a/src/libexpr/flake/flakeref.cc b/src/libexpr/flake/flakeref.cc index 701546671..6363446f6 100644 --- a/src/libexpr/flake/flakeref.cc +++ b/src/libexpr/flake/flakeref.cc @@ -102,56 +102,61 @@ std::pair parseFlakeRefWithFragment( percentDecode(std::string(match[6]))); } - /* Check if 'url' is a path (either absolute or relative to - 'baseDir'). If so, search upward to the root of the repo - (i.e. the directory containing .git). */ - else if (std::regex_match(url, match, pathUrlRegex)) { std::string path = match[1]; - if (!baseDir && !hasPrefix(path, "/")) - throw BadURL("flake reference '%s' is not an absolute path", url); - path = absPath(path, baseDir, true); + std::string fragment = percentDecode(std::string(match[3])); - if (!S_ISDIR(lstat(path).st_mode)) - throw BadURL("path '%s' is not a flake (because it's not a directory)", path); + if (baseDir) { + /* Check if 'url' is a path (either absolute or relative + to 'baseDir'). If so, search upward to the root of the + repo (i.e. the directory containing .git). */ - if (!allowMissing && !pathExists(path + "/flake.nix")) - throw BadURL("path '%s' is not a flake (because it doesn't contain a 'flake.nix' file)", path); + path = absPath(path, baseDir, true); - auto fragment = percentDecode(std::string(match[3])); + if (!S_ISDIR(lstat(path).st_mode)) + throw BadURL("path '%s' is not a flake (because it's not a directory)", path); - auto flakeRoot = path; - std::string subdir; + if (!allowMissing && !pathExists(path + "/flake.nix")) + throw BadURL("path '%s' is not a flake (because it doesn't contain a 'flake.nix' file)", path); - while (flakeRoot != "/") { - if (pathExists(flakeRoot + "/.git")) { - auto base = std::string("git+file://") + flakeRoot; + auto flakeRoot = path; + std::string subdir; - auto parsedURL = ParsedURL{ - .url = base, // FIXME - .base = base, - .scheme = "git+file", - .authority = "", - .path = flakeRoot, - .query = decodeQuery(match[2]), - }; + while (flakeRoot != "/") { + if (pathExists(flakeRoot + "/.git")) { + auto base = std::string("git+file://") + flakeRoot; - if (subdir != "") { - if (parsedURL.query.count("dir")) - throw Error("flake URL '%s' has an inconsistent 'dir' parameter", url); - parsedURL.query.insert_or_assign("dir", subdir); + auto parsedURL = ParsedURL{ + .url = base, // FIXME + .base = base, + .scheme = "git+file", + .authority = "", + .path = flakeRoot, + .query = decodeQuery(match[2]), + }; + + if (subdir != "") { + if (parsedURL.query.count("dir")) + throw Error("flake URL '%s' has an inconsistent 'dir' parameter", url); + parsedURL.query.insert_or_assign("dir", subdir); + } + + if (pathExists(flakeRoot + "/.git/shallow")) + parsedURL.query.insert_or_assign("shallow", "1"); + + return std::make_pair( + FlakeRef(Input::fromURL(parsedURL), get(parsedURL.query, "dir").value_or("")), + fragment); } - if (pathExists(flakeRoot + "/.git/shallow")) - parsedURL.query.insert_or_assign("shallow", "1"); - - return std::make_pair( - FlakeRef(Input::fromURL(parsedURL), get(parsedURL.query, "dir").value_or("")), - fragment); + subdir = std::string(baseNameOf(flakeRoot)) + (subdir.empty() ? "" : "/" + subdir); + flakeRoot = dirOf(flakeRoot); } - subdir = std::string(baseNameOf(flakeRoot)) + (subdir.empty() ? "" : "/" + subdir); - flakeRoot = dirOf(flakeRoot); + } else { + if (!hasPrefix(path, "/")) + throw BadURL("flake reference '%s' is not an absolute path", url); + path = canonPath(path); } fetchers::Attrs attrs; diff --git a/tests/flakes.sh b/tests/flakes.sh index 25e1847e1..5aec563ac 100644 --- a/tests/flakes.sh +++ b/tests/flakes.sh @@ -18,7 +18,6 @@ registry=$TEST_ROOT/registry.json flake1Dir=$TEST_ROOT/flake1 flake2Dir=$TEST_ROOT/flake2 flake3Dir=$TEST_ROOT/flake3 -flake4Dir=$TEST_ROOT/flake4 flake5Dir=$TEST_ROOT/flake5 flake6Dir=$TEST_ROOT/flake6 flake7Dir=$TEST_ROOT/flake7 @@ -390,14 +389,12 @@ cat > $flake3Dir/flake.nix < Date: Fri, 17 Jul 2020 14:57:22 +0000 Subject: [PATCH 24/42] Add back flake-compat shell.nix This was removed in the merge commit adf2fbbdc2c94644b0d1023d844c7dc0e485a20f. I think this was a mistake that occurred when resolving a conflict. --- shell.nix | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 shell.nix diff --git a/shell.nix b/shell.nix new file mode 100644 index 000000000..330df0ab6 --- /dev/null +++ b/shell.nix @@ -0,0 +1,3 @@ +(import (fetchTarball https://github.com/edolstra/flake-compat/archive/master.tar.gz) { + src = ./.; +}).shellNix From bc73590151bff82b03077c34f0c5aa9f84c89e63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lars=20M=C3=BChmel?= Date: Fri, 17 Jul 2020 17:35:59 +0200 Subject: [PATCH 25/42] nix edit: call restoreSignals() before `execvp`-ing the $EDITOR Currently resizing of the terminal doesn't play nicely with nix edit when using kakoune as the editor, as it relies on the SIGWINCH signal which is trapped by nix. How this is not a problem with e.g. vim is beyond me. Virtually all other exec* calls are following a call to restoreSignals(). This commit adds this behavior to nix edit as well. --- src/nix/edit.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/nix/edit.cc b/src/nix/edit.cc index dc9775635..378a3739c 100644 --- a/src/nix/edit.cc +++ b/src/nix/edit.cc @@ -45,6 +45,7 @@ struct CmdEdit : InstallableCommand auto args = editorFor(pos); + restoreSignals(); execvp(args.front().c_str(), stringsToCharPtrs(args).data()); std::string command; From 4178f36a1d4005a78a024c60d1f024c6ecccf8e8 Mon Sep 17 00:00:00 2001 From: Carlo Nucera Date: Fri, 17 Jul 2020 15:50:53 -0400 Subject: [PATCH 26/42] Test relative store paths --- tests/local-store.sh | 20 ++++++++++++++++++++ tests/local.mk | 2 +- 2 files changed, 21 insertions(+), 1 deletion(-) create mode 100644 tests/local-store.sh diff --git a/tests/local-store.sh b/tests/local-store.sh new file mode 100644 index 000000000..4ec3d64b0 --- /dev/null +++ b/tests/local-store.sh @@ -0,0 +1,20 @@ +source common.sh + +cd $TEST_ROOT + +echo example > example.txt +mkdir -p ./x + +NIX_STORE_DIR=$TEST_ROOT/x + +CORRECT_PATH=$(nix-store --store ./x --add example.txt) + +PATH1=$(nix path-info --store ./x $CORRECT_PATH) +[ $CORRECT_PATH == $PATH1 ] + +PATH2=$(nix path-info --store "$PWD/x" $CORRECT_PATH) +[ $CORRECT_PATH == $PATH2 ] + +# FIXME we could also test the query parameter version: +# PATH3=$(nix path-info --store "local?store=$PWD/x" $CORRECT_PATH) +# [ $CORRECT_PATH == $PATH3 ] diff --git a/tests/local.mk b/tests/local.mk index 81366160b..0f3bfe606 100644 --- a/tests/local.mk +++ b/tests/local.mk @@ -6,7 +6,7 @@ nix_tests = \ gc-auto.sh \ referrers.sh user-envs.sh logging.sh nix-build.sh misc.sh fixed.sh \ gc-runtime.sh check-refs.sh filter-source.sh \ - remote-store.sh export.sh export-graph.sh \ + local-store.sh remote-store.sh export.sh export-graph.sh \ timeout.sh secure-drv-outputs.sh nix-channel.sh \ multiple-outputs.sh import-derivation.sh fetchurl.sh optimise-store.sh \ binary-cache.sh nix-profile.sh repair.sh dump-db.sh case-hack.sh \ From 5526683ad3e259f1c02461c48c7e109de185383d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Sat, 18 Jul 2020 07:58:37 +0100 Subject: [PATCH 27/42] fix make's impurity on /bin/sh This is important when using tooling like BEAR to generate compilation database since the used glibc version needs to match for LD_PRELOAD to work. It might be also beneficial when building on systems other than NixOS with nix develop since /bin/sh might be not bash (which is what all nix devs use for testing). This fix is not perfect because Makefile.config.in itself is also build with make but strictly better than the status quo. --- Makefile.config.in | 1 + 1 file changed, 1 insertion(+) diff --git a/Makefile.config.in b/Makefile.config.in index b632444e8..5c245b8e9 100644 --- a/Makefile.config.in +++ b/Makefile.config.in @@ -19,6 +19,7 @@ LIBLZMA_LIBS = @LIBLZMA_LIBS@ OPENSSL_LIBS = @OPENSSL_LIBS@ PACKAGE_NAME = @PACKAGE_NAME@ PACKAGE_VERSION = @PACKAGE_VERSION@ +SHELL = @bash@ SODIUM_LIBS = @SODIUM_LIBS@ SQLITE3_LIBS = @SQLITE3_LIBS@ bash = @bash@ From 3294b0a4b05b8bfa9b8aa9be587dd46a67705864 Mon Sep 17 00:00:00 2001 From: Alex Kovar Date: Sat, 18 Jul 2020 10:23:43 -0500 Subject: [PATCH 28/42] Add newline to profile sourcing line #3393 --- scripts/install-nix-from-closure.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/install-nix-from-closure.sh b/scripts/install-nix-from-closure.sh index 5824c2217..6fb0beb2b 100644 --- a/scripts/install-nix-from-closure.sh +++ b/scripts/install-nix-from-closure.sh @@ -207,7 +207,7 @@ if [ -z "$NIX_INSTALLER_NO_MODIFY_PROFILE" ]; then if [ -w "$fn" ]; then if ! grep -q "$p" "$fn"; then echo "modifying $fn..." >&2 - echo "if [ -e $p ]; then . $p; fi # added by Nix installer" >> "$fn" + echo -e "\nif [ -e $p ]; then . $p; fi # added by Nix installer" >> "$fn" fi added=1 break @@ -218,7 +218,7 @@ if [ -z "$NIX_INSTALLER_NO_MODIFY_PROFILE" ]; then if [ -w "$fn" ]; then if ! grep -q "$p" "$fn"; then echo "modifying $fn..." >&2 - echo "if [ -e $p ]; then . $p; fi # added by Nix installer" >> "$fn" + echo -e "\nif [ -e $p ]; then . $p; fi # added by Nix installer" >> "$fn" fi added=1 break From f0100f55909d00f15bfdbef89d6cdcf6c38b2f59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Sat, 18 Jul 2020 08:48:13 +0100 Subject: [PATCH 29/42] README: improve development docs --- README.md | 25 +++-------------------- doc/manual/hacking.xml | 45 ++++++++++++++++++++++++++++++++++++------ 2 files changed, 42 insertions(+), 28 deletions(-) diff --git a/README.md b/README.md index a1588284d..e5f7a694f 100644 --- a/README.md +++ b/README.md @@ -12,7 +12,7 @@ for more details. On Linux and macOS the easiest way to Install Nix is to run the following shell command (as a user other than root): -``` +```console $ curl -L https://nixos.org/nix/install | sh ``` @@ -20,27 +20,8 @@ Information on additional installation methods is available on the [Nix download ## Building And Developing -### Building Nix - -You can build Nix using one of the targets provided by [release.nix](./release.nix): - -``` -$ nix-build ./release.nix -A build.aarch64-linux -$ nix-build ./release.nix -A build.x86_64-darwin -$ nix-build ./release.nix -A build.i686-linux -$ nix-build ./release.nix -A build.x86_64-linux -``` - -### Development Environment - -You can use the provided `shell.nix` to get a working development environment: - -``` -$ nix-shell -$ ./bootstrap.sh -$ ./configure -$ make -``` +See our [Hacking guide](hydra.nixos.org/job/nix/master/build.x86_64-linux/latest/download-by-type/doc/manual#chap-hacking) in our manual for instruction on how to +build nix from source with nix-build or how to get a development environment. ## Additional Resources diff --git a/doc/manual/hacking.xml b/doc/manual/hacking.xml index b671811d3..c0ece76b6 100644 --- a/doc/manual/hacking.xml +++ b/doc/manual/hacking.xml @@ -4,18 +4,37 @@ Hacking -This section provides some notes on how to hack on Nix. To get +This section provides some notes on how to hack on Nix. To get the latest version of Nix from GitHub: -$ git clone git://github.com/NixOS/nix.git +$ git clone https://github.com/NixOS/nix.git $ cd nix -To build it and its dependencies: +To build Nix for the current operating system/architecture use + -$ nix-build release.nix -A build.x86_64-linux +$ nix-build + +or if you have a flakes-enabled nix: + + +$ nix build + + +This will build defaultPackage attribute defined in the flake.nix file. + +To build for other platforms add one of the following suffixes to it: aarch64-linux, +i686-linux, x86_64-darwin, x86_64-linux. + +i.e. + + +nix-build -A defaultPackage.x86_64-linux + + To build all dependencies and start a shell in which all @@ -27,13 +46,27 @@ $ nix-shell To build Nix itself in this shell: [nix-shell]$ ./bootstrap.sh -[nix-shell]$ configurePhase -[nix-shell]$ make +[nix-shell]$ ./configure $configureFlags +[nix-shell]$ make -j $NIX_BUILD_CORES To install it in $(pwd)/inst and test it: [nix-shell]$ make install [nix-shell]$ make installcheck +[nix-shell]$ ./inst/bin/nix --version +nix (Nix) 2.4 + + +If you have a flakes-enabled nix you can replace: + + +$ nix-shell + + +by: + + +$ nix develop From ac2fc7ba1fe6b64ec535e4ce63d13fcadf7fdba7 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 20 Jul 2020 11:29:46 -0400 Subject: [PATCH 30/42] Apply suggestions from code review Co-authored-by: Eelco Dolstra --- src/libstore/store-api.cc | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 5c8dddba5..6c0a61766 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -234,7 +234,7 @@ ValidPathInfo Store::addToStoreSlow(std::string_view name, const Path & srcPath, TeeSink sinkIfNar { narHashSink, caHashSink }; - /* We use the tee sink if we need to hash he nar twice */ + /* We use the tee sink if we need to hash the nar twice */ auto & sink = method == FileIngestionMethod::Recursive && hashAlgo != htSHA256 ? static_cast(sinkIfNar) : narHashSink; @@ -250,7 +250,11 @@ ValidPathInfo Store::addToStoreSlow(std::string_view name, const Path & srcPath, ? fileSink : blank; - parseDump(parseSink, tapped); + parseDump( + parseSink, + method == FileIngestionMethod::Recursive && hashAlgo == htSHA256 + ? *fileSource // don't need to hash twice if we just can use the `narHash` twice + : tapped); auto [narHash, narSize] = narHashSink.finish(); From 0ca9744694a5294e995fcddc11f5f195c84036a4 Mon Sep 17 00:00:00 2001 From: Carlo Nucera Date: Mon, 20 Jul 2020 15:56:52 -0400 Subject: [PATCH 31/42] Use heuristics to decide when to show the response Due to https://github.com/NixOS/nix/issues/3841 we don't know how print different messages for different verbosity levels. --- src/libstore/filetransfer.cc | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index 4d35bd2b5..4149f8155 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -845,8 +845,11 @@ FileTransferError::FileTransferError(FileTransfer::Error error, std::shared_ptr< : Error(args...), error(error), response(response) { const auto hf = hintfmt(args...); - if (response) { - err.hint = hintfmt("%1%\n\nresponse body:\n\n%2%", normaltxt(hf.str()), *response); + // FIXME: Due to https://github.com/NixOS/nix/issues/3841 we don't know how + // to print different messages for different verbosity levels. For now + // we add some heuristics for detecting when we want to show the response. + if (response && (response->size() < 1024 || response->find("") != string::npos)) { + err.hint = hintfmt("%1%\n\nresponse body:\n\n%2%", normaltxt(hf.str()), *response); } else { err.hint = hf; } From 6633605341c6e01bc72b8311e478ed9932719e7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Mon, 20 Jul 2020 22:30:39 +0100 Subject: [PATCH 32/42] Update doc/manual/hacking.xml Co-authored-by: Eelco Dolstra --- doc/manual/hacking.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/manual/hacking.xml b/doc/manual/hacking.xml index c0ece76b6..d25d4b84a 100644 --- a/doc/manual/hacking.xml +++ b/doc/manual/hacking.xml @@ -12,7 +12,7 @@ $ cd nix -To build Nix for the current operating system/architecture use +To build Nix for the current operating system/architecture use $ nix-build From 9aae179f34ec2f38167585c07f18a8ab8acefafb Mon Sep 17 00:00:00 2001 From: Carlo Nucera Date: Mon, 20 Jul 2020 20:18:12 -0400 Subject: [PATCH 33/42] Correct bug, thoroughly document addToStoreSlow --- src/libstore/store-api.cc | 62 ++++++++++++++++++++++++++++----------- 1 file changed, 45 insertions(+), 17 deletions(-) diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 6c0a61766..14661722d 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -222,40 +222,68 @@ StorePath Store::computeStorePathForText(const string & name, const string & s, } +/* +The aim of this function is to compute in one pass the correct ValidPathInfo for +the files that we are trying to add to the store. To accomplish that in one +pass, given the different kind of inputs that we can take (normal nar archives, +nar archives with non SHA-256 hashes, and flat files), we set up a net of sinks +and aliases. Also, since the dataflow is obfuscated by this, we include here a +graphviz diagram: + +digraph graphname { + node [shape=box] + fileSource -> narSink + narSink [style=dashed] + narSink -> unsualHashTee [style = dashed, label = "Recursive && !SHA-256"] + narSink -> narHashSink [style = dashed, label = "else"] + unsualHashTee -> narHashSink + unsualHashTee -> caHashSink + fileSource -> parseSink + parseSink [style=dashed] + parseSink-> fileSink [style = dashed, label = "Flat"] + parseSink -> blank [style = dashed, label = "Recursive"] + fileSink -> caHashSink +} +*/ ValidPathInfo Store::addToStoreSlow(std::string_view name, const Path & srcPath, FileIngestionMethod method, HashType hashAlgo, std::optional expectedCAHash) { - /* FIXME: inefficient: we're reading/hashing 'tmpFile' two - times. */ HashSink narHashSink { htSHA256 }; HashSink caHashSink { hashAlgo }; + + /* Note that fileSink and unusualHashTee must be mutually exclusive, since + they both write to caHashSink. Note that that requisite is currently true + because the former is only used in the flat case. */ RetrieveRegularNARSink fileSink { caHashSink }; + TeeSink unusualHashTee { narHashSink, caHashSink }; - TeeSink sinkIfNar { narHashSink, caHashSink }; - - /* We use the tee sink if we need to hash the nar twice */ - auto & sink = method == FileIngestionMethod::Recursive && hashAlgo != htSHA256 - ? static_cast(sinkIfNar) + auto & narSink = method == FileIngestionMethod::Recursive && hashAlgo != htSHA256 + ? static_cast(unusualHashTee) : narHashSink; - auto fileSource = sinkToSource([&](Sink & sink) { - dumpPath(srcPath, sink); + /* Functionally, this means that fileSource will yield the content of + srcPath. The fact that we use scratchpadSink as a temporary buffer here + is an implementation detail. */ + auto fileSource = sinkToSource([&](Sink & scratchpadSink) { + dumpPath(srcPath, scratchpadSink); }); - TeeSource tapped { *fileSource, sink }; + /* tapped provides the same data as fileSource, but we also write all the + information to narSink. */ + TeeSource tapped { *fileSource, narSink }; ParseSink blank; auto & parseSink = method == FileIngestionMethod::Flat ? fileSink : blank; - parseDump( - parseSink, - method == FileIngestionMethod::Recursive && hashAlgo == htSHA256 - ? *fileSource // don't need to hash twice if we just can use the `narHash` twice - : tapped); + /* The information that flows from tapped (besides being replicated in + narSink), is now put in parseSink. */ + parseDump(parseSink, tapped); + /* We extract the result of the computation from the sink by calling + finish. */ auto [narHash, narSize] = narHashSink.finish(); auto hash = method == FileIngestionMethod::Recursive && hashAlgo == htSHA256 @@ -271,8 +299,8 @@ ValidPathInfo Store::addToStoreSlow(std::string_view name, const Path & srcPath, info.ca = FixedOutputHash { .method = method, .hash = hash }; if (!isValidPath(info.path)) { - auto source = sinkToSource([&](Sink & sink) { - dumpPath(srcPath, sink); + auto source = sinkToSource([&](Sink & scratchpadSink) { + dumpPath(srcPath, scratchpadSink); }); addToStore(info, *source); } From 922a845ffc4eaa51797bc376d237c6216f0d8391 Mon Sep 17 00:00:00 2001 From: Carlo Nucera Date: Tue, 21 Jul 2020 10:24:19 -0400 Subject: [PATCH 34/42] Update chunkSize to the suggested value This was a suggested course of action in a review in one of our earlier commits, https://github.com/NixOS/nix/pull/3801#discussion_r457557079 --- src/libstore/local-store.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 340fb5306..9ea71170f 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1069,7 +1069,7 @@ StorePath LocalStore::addToStoreFromDump(Source & source0, const string & name, or the original source is empty */ while (dump.size() < settings.narBufferSize) { auto oldSize = dump.size(); - constexpr size_t chunkSize = 1024; + constexpr size_t chunkSize = 65536; auto want = std::min(chunkSize, settings.narBufferSize - oldSize); dump.resize(oldSize + want); auto got = 0; From 6cce32c8e8b9559f197f6d3e8814796cfc490582 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 21 Jul 2020 15:39:47 +0000 Subject: [PATCH 35/42] Change logic for deciding what is a relative path for the local store The was Eelco's prefered logic, and it looks good to me! --- src/libstore/store-api.cc | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index d37e970df..67ce6e66c 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -914,12 +914,20 @@ ref openStore(const std::string & uri_, throw Error("don't know how to open Nix store '%s'", uri); } +static bool isNonUriPath(const std::string & spec) { + return + // is not a URL + spec.find("://") == std::string::npos + // Has at least one path separator, and so isn't a single word that + // might be special like "auto" + && spec.find("/") != std::string::npos; +} StoreType getStoreType(const std::string & uri, const std::string & stateDir) { if (uri == "daemon") { return tDaemon; - } else if (uri == "local" || hasPrefix(uri, "/") || hasPrefix(uri, "./")) { + } else if (uri == "local" || isNonUriPath(uri)) { return tLocal; } else if (uri == "" || uri == "auto") { if (access(stateDir.c_str(), R_OK | W_OK) == 0) @@ -943,9 +951,7 @@ static RegisterStoreImplementation regStore([]( return std::shared_ptr(std::make_shared(params)); case tLocal: { Store::Params params2 = params; - if (hasPrefix(uri, "/")) { - params2["root"] = uri; - } else if (hasPrefix(uri, "./")) { + if (isNonUriPath(uri)) { params2["root"] = absPath(uri); } return std::shared_ptr(std::make_shared(params2)); From ae9e9753ce9100ac8ac98fb76d4ac9de45b5734c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Josef=20Kemetm=C3=BCller?= Date: Wed, 22 Jul 2020 13:45:15 +0200 Subject: [PATCH 36/42] README: Fix link to hacking guide The link was previously interpreted as if it were relative to the current file. --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index e5f7a694f..03c5deb7b 100644 --- a/README.md +++ b/README.md @@ -20,7 +20,7 @@ Information on additional installation methods is available on the [Nix download ## Building And Developing -See our [Hacking guide](hydra.nixos.org/job/nix/master/build.x86_64-linux/latest/download-by-type/doc/manual#chap-hacking) in our manual for instruction on how to +See our [Hacking guide](https://hydra.nixos.org/job/nix/master/build.x86_64-linux/latest/download-by-type/doc/manual#chap-hacking) in our manual for instruction on how to build nix from source with nix-build or how to get a development environment. ## Additional Resources From c56356baccbf006c7835afa9ce58012631e90b31 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 22 Jul 2020 21:37:54 +0000 Subject: [PATCH 37/42] Separate concerns in `scanForReferences` with TeeSink This also will make it easier to use a `HashModuloSink` instead for CA derivations. --- src/libstore/references.cc | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/libstore/references.cc b/src/libstore/references.cc index a10d536a3..968e2e425 100644 --- a/src/libstore/references.cc +++ b/src/libstore/references.cc @@ -48,13 +48,12 @@ static void search(const unsigned char * s, size_t len, struct RefScanSink : Sink { - HashSink hashSink; StringSet hashes; StringSet seen; string tail; - RefScanSink() : hashSink(htSHA256) { } + RefScanSink() { } void operator () (const unsigned char * data, size_t len); }; @@ -62,8 +61,6 @@ struct RefScanSink : Sink void RefScanSink::operator () (const unsigned char * data, size_t len) { - hashSink(data, len); - /* It's possible that a reference spans the previous and current fragment, so search in the concatenation of the tail of the previous fragment and the start of the current fragment. */ @@ -82,7 +79,9 @@ void RefScanSink::operator () (const unsigned char * data, size_t len) PathSet scanForReferences(const string & path, const PathSet & refs, HashResult & hash) { - RefScanSink sink; + RefScanSink refsSink; + HashSink hashSink { htSHA256 }; + TeeSink sink { refsSink, hashSink }; std::map backMap; /* For efficiency (and a higher hit rate), just search for the @@ -97,7 +96,7 @@ PathSet scanForReferences(const string & path, assert(s.size() == refLength); assert(backMap.find(s) == backMap.end()); // parseHash(htSHA256, s); - sink.hashes.insert(s); + refsSink.hashes.insert(s); backMap[s] = i; } @@ -106,13 +105,13 @@ PathSet scanForReferences(const string & path, /* Map the hashes found back to their store paths. */ PathSet found; - for (auto & i : sink.seen) { + for (auto & i : refsSink.seen) { std::map::iterator j; if ((j = backMap.find(i)) == backMap.end()) abort(); found.insert(j->second); } - hash = sink.hashSink.finish(); + hash = hashSink.finish(); return found; } From b9ead08ca8f3b8e693a4528d0c6f642dcee026fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabian=20M=C3=B6ller?= Date: Thu, 23 Jul 2020 14:21:27 +0200 Subject: [PATCH 38/42] Save changes made by "nix registry pin" to user registry --- src/nix/registry.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/nix/registry.cc b/src/nix/registry.cc index 16d7e511f..ebee4545c 100644 --- a/src/nix/registry.cc +++ b/src/nix/registry.cc @@ -111,6 +111,7 @@ struct CmdRegistryPin : virtual Args, EvalCommand fetchers::Attrs extraAttrs; if (ref.subdir != "") extraAttrs["dir"] = ref.subdir; userRegistry->add(ref.input, resolved, extraAttrs); + userRegistry->write(fetchers::getUserRegistryPath()); } }; From 2274f63453dc0b5c76a50ae34d6f7c8ef329f411 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 23 Jul 2020 14:34:20 +0000 Subject: [PATCH 39/42] Get rid of `basicDerivation::findOutput` It's a tiny function which is: - hardly worth abstrating over, and also only used once. - doesn't work once we get CA drvs I rewrote the one callsite to be forwards compatable with CA derivations, and also potentially more performant: instead of reading in the derivation it can ust consult the SQLite DB in the common case. --- src/libstore/derivations.cc | 8 -------- src/libstore/derivations.hh | 4 ---- src/nix-env/nix-env.cc | 3 ++- 3 files changed, 2 insertions(+), 13 deletions(-) diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index f325e511a..870901421 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -7,14 +7,6 @@ namespace nix { -const StorePath & BasicDerivation::findOutput(const string & id) const -{ - auto i = outputs.find(id); - if (i == outputs.end()) - throw Error("derivation has no output '%s'", id); - return i->second.path; -} - bool BasicDerivation::isBuiltin() const { diff --git a/src/libstore/derivations.hh b/src/libstore/derivations.hh index 68c53c1ff..00cf1bb08 100644 --- a/src/libstore/derivations.hh +++ b/src/libstore/derivations.hh @@ -39,10 +39,6 @@ struct BasicDerivation BasicDerivation() { } virtual ~BasicDerivation() { }; - /* Return the path corresponding to the output identifier `id' in - the given derivation. */ - const StorePath & findOutput(const std::string & id) const; - bool isBuiltin() const; /* Return true iff this is a fixed-output derivation. */ diff --git a/src/nix-env/nix-env.cc b/src/nix-env/nix-env.cc index 5795c2c09..ddd036070 100644 --- a/src/nix-env/nix-env.cc +++ b/src/nix-env/nix-env.cc @@ -381,7 +381,8 @@ static void queryInstSources(EvalState & state, if (path.isDerivation()) { elem.setDrvPath(state.store->printStorePath(path)); - elem.setOutPath(state.store->printStorePath(state.store->derivationFromPath(path).findOutput("out"))); + auto outputs = state.store->queryDerivationOutputMap(path); + elem.setOutPath(state.store->printStorePath(outputs.at("out"))); if (name.size() >= drvExtension.size() && string(name, name.size() - drvExtension.size()) == drvExtension) name = string(name, 0, name.size() - drvExtension.size()); From 26fcab53e05ce66f5533f0f07ed13d727a891c8d Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 23 Jul 2020 13:40:39 +0200 Subject: [PATCH 40/42] Remove unused file --- doc/manual/expressions/builder-syntax.xml | 119 ---------------------- 1 file changed, 119 deletions(-) delete mode 100644 doc/manual/expressions/builder-syntax.xml diff --git a/doc/manual/expressions/builder-syntax.xml b/doc/manual/expressions/builder-syntax.xml deleted file mode 100644 index e51bade44..000000000 --- a/doc/manual/expressions/builder-syntax.xml +++ /dev/null @@ -1,119 +0,0 @@ -
- -Builder Syntax - -Build script for GNU Hello -(<filename>builder.sh</filename>) - -source $stdenv/setup - -PATH=$perl/bin:$PATH - -tar xvfz $src -cd hello-* -./configure --prefix=$out -make -make install - - - shows the builder referenced -from Hello's Nix expression (stored in -pkgs/applications/misc/hello/ex-1/builder.sh). -The builder can actually be made a lot shorter by using the -generic builder functions provided by -stdenv, but here we write out the build steps to -elucidate what a builder does. It performs the following -steps: - - - - - - When Nix runs a builder, it initially completely clears the - environment (except for the attributes declared in the - derivation). For instance, the PATH variable is - emptyActually, it's initialised to - /path-not-set to prevent Bash from setting it - to a default value.. This is done to prevent - undeclared inputs from being used in the build process. If for - example the PATH contained - /usr/bin, then you might accidentally use - /usr/bin/gcc. - - So the first step is to set up the environment. This is - done by calling the setup script of the - standard environment. The environment variable - stdenv points to the location of the standard - environment being used. (It wasn't specified explicitly as an - attribute in , but - mkDerivation adds it automatically.) - - - - - - Since Hello needs Perl, we have to make sure that Perl is in - the PATH. The perl environment - variable points to the location of the Perl package (since it - was passed in as an attribute to the derivation), so - $perl/bin is the - directory containing the Perl interpreter. - - - - - - Now we have to unpack the sources. The - src attribute was bound to the result of - fetching the Hello source tarball from the network, so the - src environment variable points to the location in - the Nix store to which the tarball was downloaded. After - unpacking, we cd to the resulting source - directory. - - The whole build is performed in a temporary directory - created in /tmp, by the way. This directory is - removed after the builder finishes, so there is no need to clean - up the sources afterwards. Also, the temporary directory is - always newly created, so you don't have to worry about files from - previous builds interfering with the current build. - - - - - - GNU Hello is a typical Autoconf-based package, so we first - have to run its configure script. In Nix - every package is stored in a separate location in the Nix store, - for instance - /nix/store/9a54ba97fb71b65fda531012d0443ce2-hello-2.1.1. - Nix computes this path by cryptographically hashing all attributes - of the derivation. The path is passed to the builder through the - out environment variable. So here we give - configure the parameter - --prefix=$out to cause Hello to be installed in - the expected location. - - - - - - Finally we build Hello (make) and install - it into the location specified by out - (make install). - - - - - -If you are wondering about the absence of error checking on the -result of various commands called in the builder: this is because the -shell script is evaluated with Bash's option, -which causes the script to be aborted if any command fails without an -error check. - -
\ No newline at end of file From 2292814049256980c6e809ab364ebe0da3c9d76a Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 24 Jul 2020 11:19:17 +0200 Subject: [PATCH 41/42] createUnixDomainSocket(): Fix off-by-one error in copying the socket path Reported by Kane York. --- src/libutil/util.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 93798a765..a0a8ff4d3 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -1581,7 +1581,7 @@ AutoCloseFD createUnixDomainSocket(const Path & path, mode_t mode) struct sockaddr_un addr; addr.sun_family = AF_UNIX; - if (path.size() >= sizeof(addr.sun_path)) + if (path.size() + 1 >= sizeof(addr.sun_path)) throw Error("socket path '%1%' is too long", path); strcpy(addr.sun_path, path.c_str()); From 72f8771094d575e924846f16e5c60742eea9420b Mon Sep 17 00:00:00 2001 From: Matthew Bauer Date: Sat, 25 Jul 2020 18:02:42 -0500 Subject: [PATCH 42/42] Allow PRECOMPILE_HEADERS in cross-compilation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In cross, CXX will look like aarch64-unknown-linux-gnu-g++. We could run some command to check what kind of compiler it is, but for now we can just check if g++ is anywhere in the string. I couldn’t find any "ends with" for makefile, so it can be anywhere in CXX. --- mk/precompiled-headers.mk | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mk/precompiled-headers.mk b/mk/precompiled-headers.mk index 1c0452dc2..500c99e4a 100644 --- a/mk/precompiled-headers.mk +++ b/mk/precompiled-headers.mk @@ -21,13 +21,13 @@ clean-files += $(GCH) $(PCH) ifeq ($(PRECOMPILE_HEADERS), 1) - ifeq ($(CXX), g++) + ifeq ($(findstring g++,$(CXX)), g++) GLOBAL_CXXFLAGS_PCH += -include $(buildprefix)precompiled-headers.h -Winvalid-pch GLOBAL_ORDER_AFTER += $(GCH) - else ifeq ($(CXX), clang++) + else ifeq ($(findstring clang++,$(CXX)), clang++) GLOBAL_CXXFLAGS_PCH += -include-pch $(PCH) -Winvalid-pch