From 68dfb8c6aef7afebf0312c48bb5010653fc464b3 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 16 Jul 2020 05:09:41 +0000 Subject: [PATCH 1/9] 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 52c8be38e0563c964857491afef01eb2f543d0de Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 17 Jul 2020 12:36:12 +0200 Subject: [PATCH 2/9] 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 3/9] 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 4/9] 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 5/9] 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 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 6/9] 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 7/9] 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 ac2fc7ba1fe6b64ec535e4ce63d13fcadf7fdba7 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 20 Jul 2020 11:29:46 -0400 Subject: [PATCH 8/9] 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 9aae179f34ec2f38167585c07f18a8ab8acefafb Mon Sep 17 00:00:00 2001 From: Carlo Nucera Date: Mon, 20 Jul 2020 20:18:12 -0400 Subject: [PATCH 9/9] 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); }