From a76b8b546753bc14aa2e8f8fdc74f70407aebd31 Mon Sep 17 00:00:00 2001 From: Cole Helbling Date: Tue, 1 Sep 2020 12:05:49 -0700 Subject: [PATCH 01/14] README: update link to Hacking section --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 03c5deb7b..3cf4e44fa 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](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 +See our [Hacking guide](https://hydra.nixos.org/job/nix/master/build.x86_64-linux/latest/download-by-type/doc/manual/hacking.html) 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 a72ed3e8a1028c66c36b7dd66dc9dd50a0a6ecc1 Mon Sep 17 00:00:00 2001 From: Cole Helbling Date: Tue, 1 Sep 2020 12:06:02 -0700 Subject: [PATCH 02/14] hacking.md: add --prefix flag to configure Otherwise, the steps advertised in this document won't actually work (e.g. `make install` will fail, trying to access /usr, and `./inst/bin/nix` won't exist). --- doc/manual/src/hacking.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/manual/src/hacking.md b/doc/manual/src/hacking.md index 9049e42bb..5bd884ce8 100644 --- a/doc/manual/src/hacking.md +++ b/doc/manual/src/hacking.md @@ -39,7 +39,7 @@ To build Nix itself in this shell: ```console [nix-shell]$ ./bootstrap.sh -[nix-shell]$ ./configure $configureFlags +[nix-shell]$ ./configure $configureFlags --prefix=$(pwd)/inst [nix-shell]$ make -j $NIX_BUILD_CORES ``` From ee5906243ae37b7fe007db30aa575f46ae292a35 Mon Sep 17 00:00:00 2001 From: Gabriel Gonzalez Date: Fri, 4 Sep 2020 20:05:43 -0700 Subject: [PATCH 03/14] Add `nix-shell` support for preserving PS1 Fixes https://github.com/NixOS/nix/issues/1268 `nix-shell` will now preserve `PS1` if the `NIX_SHELL_PRESERVE_PROMPT` environment variable is set. --- src/nix-build/nix-build.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/nix-build/nix-build.cc b/src/nix-build/nix-build.cc index 471bcc10d..0400f94f5 100755 --- a/src/nix-build/nix-build.cc +++ b/src/nix-build/nix-build.cc @@ -98,8 +98,8 @@ static void _main(int argc, char * * argv) // List of environment variables kept for --pure std::set keepVars{ - "HOME", "USER", "LOGNAME", "DISPLAY", "PATH", "TERM", - "IN_NIX_SHELL", "TZ", "PAGER", "NIX_BUILD_SHELL", "SHLVL", + "HOME", "USER", "LOGNAME", "DISPLAY", "PATH", "TERM", "IN_NIX_SHELL", + "NIX_SHELL_PRESERVE_PROMPT", "TZ", "PAGER", "NIX_BUILD_SHELL", "SHLVL", "http_proxy", "https_proxy", "ftp_proxy", "all_proxy", "no_proxy" }; @@ -446,7 +446,7 @@ static void _main(int argc, char * * argv) "PATH=%4%:\"$PATH\"; " "SHELL=%5%; " "set +e; " - R"s([ -n "$PS1" ] && PS1='\n\[\033[1;32m\][nix-shell:\w]\$\[\033[0m\] '; )s" + R"s([ -n "$PS1" -a -z "$NIX_SHELL_PRESERVE_PROMPT" ] && PS1='\n\[\033[1;32m\][nix-shell:\w]\$\[\033[0m\] '; )s" "if [ \"$(type -t runHook)\" = function ]; then runHook shellHook; fi; " "unset NIX_ENFORCE_PURITY; " "shopt -u nullglob; " From 8dbd57a6a5fe497bde9e647a3249c1ce0ea121ab Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Fri, 11 Sep 2020 19:21:40 +0200 Subject: [PATCH 04/14] Fix auto argument passing for more auto arguments than formals The change in 626200713bb3cc844a9feb6af583c9b6b42c6dbc didn't account for when the number of auto arguments is bigger than the number of formal arguments. This causes the following: $ nix-instantiate --eval -E '{ ... }@args: args.foo' --argstr foo foo nix-instantiate: src/libexpr/attr-set.hh:55: void nix::Bindings::push_back(const nix::Attr&): Assertion `size_ < capacity_' failed. Aborted (core dumped) --- src/libexpr/eval.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 8c97b3760..dab11ce45 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -1348,7 +1348,7 @@ void EvalState::autoCallFunction(Bindings & args, Value & fun, Value & res) } Value * actualArgs = allocValue(); - mkAttrs(*actualArgs, fun.lambda.fun->formals->formals.size()); + mkAttrs(*actualArgs, std::max(static_cast(fun.lambda.fun->formals->formals.size()), args.size())); if (fun.lambda.fun->formals->ellipsis) { // If the formals have an ellipsis (eg the function accepts extra args) pass From 7cb5f643a6ee5b8ffee2e0bb9159a677ff76d17c Mon Sep 17 00:00:00 2001 From: Jade Date: Sat, 12 Sep 2020 13:08:40 -0700 Subject: [PATCH 05/14] docs+test: fix remaining installer downloads without -L (#4006) Co-authored-by: lf- --- doc/manual/src/release-notes/rl-1.7.md | 2 +- doc/manual/src/release-notes/rl-2.1.md | 2 +- tests/install-darwin.sh | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/manual/src/release-notes/rl-1.7.md b/doc/manual/src/release-notes/rl-1.7.md index 8d49ae54e..fb18e797d 100644 --- a/doc/manual/src/release-notes/rl-1.7.md +++ b/doc/manual/src/release-notes/rl-1.7.md @@ -117,7 +117,7 @@ features: - The binary tarball installer has been improved. You can now install Nix by running: - $ bash <(curl https://nixos.org/nix/install) + $ bash <(curl -L https://nixos.org/nix/install) - More evaluation errors include position information. For instance, selecting a missing attribute will print something like diff --git a/doc/manual/src/release-notes/rl-2.1.md b/doc/manual/src/release-notes/rl-2.1.md index b88834c83..a9592657b 100644 --- a/doc/manual/src/release-notes/rl-2.1.md +++ b/doc/manual/src/release-notes/rl-2.1.md @@ -10,7 +10,7 @@ in certain situations. In addition, it has the following new features: - The Nix installer now supports performing a Multi-User installation for Linux computers which are running systemd. You can select a Multi-User installation by passing the `--daemon` - flag to the installer: `sh <(curl https://nixos.org/nix/install) + flag to the installer: `sh <(curl -L https://nixos.org/nix/install) --daemon`. The multi-user installer cannot handle systems with SELinux. If diff --git a/tests/install-darwin.sh b/tests/install-darwin.sh index 9933eba94..7e44e54c4 100755 --- a/tests/install-darwin.sh +++ b/tests/install-darwin.sh @@ -53,7 +53,7 @@ trap finish EXIT # First setup Nix cleanup -curl -o install https://nixos.org/nix/install +curl -L -o install https://nixos.org/nix/install yes | bash ./install verify From 525b38eee8fac48eb2a82fb78fa0a933a9eee2a4 Mon Sep 17 00:00:00 2001 From: aszlig Date: Sun, 13 Sep 2020 02:40:51 +0200 Subject: [PATCH 06/14] Fix unspecified behaviour in readStorePathCAMap When deploying a Hydra instance with current Nix master, most builds would not run because of errors like this: queue monitor: error: --- Error --- hydra-queue-runner error: --- UsageError --- nix-daemon not a content address because it is not in the form ':': /nix/store/...-somedrv The last error message is from parseContentAddress, which expects a colon-separated string, however what we got here is a store path. Looking at the worker protocol, the following message sent to the Nix daemon caused the error above: 0x1E -> wopQuerySubstitutablePathInfos 0x01 -> Number of paths 0x16 -> Length of string "/nix/store/...-somedrv" 0x00 -> Length of string "" Looking at writeStorePathCAMap, the store path is indeed the first field that's transmitted. However, readStorePathCAMap expects it to be the *second* field *on my machine*, since expression evaluation order is a classic form of unspecified behaviour[1] in C++. This has been introduced in https://github.com/NixOS/nix/pull/3689, specifically in commit 66a62b3189c8c9b0965850e6b3c9b0fda0b50fd8. [1]: https://en.wikipedia.org/wiki/Unspecified_behavior#Order_of_evaluation_of_subexpressions Signed-off-by: aszlig --- src/libstore/remote-store.cc | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index e4a4ef5af..8bcf92301 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -43,8 +43,11 @@ StorePathCAMap readStorePathCAMap(const Store & store, Source & from) { StorePathCAMap paths; auto count = readNum(from); - while (count--) - paths.insert_or_assign(store.parseStorePath(readString(from)), parseContentAddressOpt(readString(from))); + while (count--) { + auto path = store.parseStorePath(readString(from)); + auto ca = parseContentAddressOpt(readString(from)); + paths.insert_or_assign(path, ca); + } return paths; } From c8b17212c8191d5a124c691957c05b971f6b4e5f Mon Sep 17 00:00:00 2001 From: Brian Leung Date: Sun, 13 Sep 2020 14:07:09 -0700 Subject: [PATCH 07/14] Add ccls files to .gitignore --- .gitignore | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.gitignore b/.gitignore index 0ea27c8c8..573fa0005 100644 --- a/.gitignore +++ b/.gitignore @@ -126,4 +126,10 @@ GRTAGS GSYMS GTAGS +# ccls +/.ccls-cache + +# auto-generated compilation database +compile_commands.json + nix-rust/target From a59e77d9e54e8e7bf0f3c3f40c22cd34b7a81225 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 14 Sep 2020 13:48:51 +0200 Subject: [PATCH 08/14] nix-daemon: Lower verbosity of restricted setting warning Fixes #3992. --- src/libstore/daemon.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc index f35ddb522..8adabf549 100644 --- a/src/libstore/daemon.cc +++ b/src/libstore/daemon.cc @@ -232,7 +232,7 @@ struct ClientSettings else if (setSubstituters(settings.extraSubstituters)) ; else - warn("ignoring the user-specified setting '%s', because it is a restricted setting and you are not a trusted user", name); + debug("ignoring the client-specified setting '%s', because it is a restricted setting and you are not a trusted user", name); } catch (UsageError & e) { warn(e.what()); } From 250f8a4bbae2aeafd3fa3e637b52fbbb59a7a8e0 Mon Sep 17 00:00:00 2001 From: regnat Date: Mon, 14 Sep 2020 17:19:25 +0200 Subject: [PATCH 09/14] Escape `${` in strings when printing Nix expressions Otherwise the result of the printing can't be parsed back correctly by Nix (because the unescaped `${` will be parsed as the begining of an anti-quotation). Fix #3989 --- src/libexpr/eval.cc | 1 + tests/lang/eval-okay-ind-string.exp | 2 +- tests/user-envs.nix | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index dab11ce45..d678231c6 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -87,6 +87,7 @@ static void printValue(std::ostream & str, std::set & active, con else if (*i == '\n') str << "\\n"; else if (*i == '\r') str << "\\r"; else if (*i == '\t') str << "\\t"; + else if (*i == '$' && *(i+1) == '{') str << "\\" << *i; else str << *i; str << "\""; break; diff --git a/tests/lang/eval-okay-ind-string.exp b/tests/lang/eval-okay-ind-string.exp index 9cf4bd2ee..7862331fa 100644 --- a/tests/lang/eval-okay-ind-string.exp +++ b/tests/lang/eval-okay-ind-string.exp @@ -1 +1 @@ -"This is an indented multi-line string\nliteral. An amount of whitespace at\nthe start of each line matching the minimum\nindentation of all lines in the string\nliteral together will be removed. Thus,\nin this case four spaces will be\nstripped from each line, even though\n THIS LINE is indented six spaces.\n\nAlso, empty lines don't count in the\ndetermination of the indentation level (the\nprevious empty line has indentation 0, but\nit doesn't matter).\nIf the string starts with whitespace\n followed by a newline, it's stripped, but\n that's not the case here. Two spaces are\n stripped because of the \" \" at the start. \nThis line is indented\na bit further.\nAnti-quotations, like so, are\nalso allowed.\n The \\ is not special here.\n' can be followed by any character except another ', e.g. 'x'.\nLikewise for $, e.g. $$ or $varName.\nBut ' followed by ' is special, as is $ followed by {.\nIf you want them, use anti-quotations: '', ${.\n Tabs are not interpreted as whitespace (since we can't guess\n what tab settings are intended), so don't use them.\n\tThis line starts with a space and a tab, so only one\n space will be stripped from each line.\nAlso note that if the last line (just before the closing ' ')\nconsists only of whitespace, it's ignored. But here there is\nsome non-whitespace stuff, so the line isn't removed. \nThis shows a hacky way to preserve an empty line after the start.\nBut there's no reason to do so: you could just repeat the empty\nline.\n Similarly you can force an indentation level,\n in this case to 2 spaces. This works because the anti-quote\n is significant (not whitespace).\nstart on network-interfaces\n\nstart script\n\n rm -f /var/run/opengl-driver\n ln -sf 123 /var/run/opengl-driver\n\n rm -f /var/log/slim.log\n \nend script\n\nenv SLIM_CFGFILE=abc\nenv SLIM_THEMESDIR=def\nenv FONTCONFIG_FILE=/etc/fonts/fonts.conf \t\t\t\t# !!! cleanup\nenv XKB_BINDIR=foo/bin \t\t\t\t# Needed for the Xkb extension.\nenv LD_LIBRARY_PATH=libX11/lib:libXext/lib:/usr/lib/ # related to xorg-sys-opengl - needed to load libglx for (AI)GLX support (for compiz)\n\nenv XORG_DRI_DRIVER_PATH=nvidiaDrivers/X11R6/lib/modules/drivers/ \n\nexec slim/bin/slim\nEscaping of ' followed by ': ''\nEscaping of $ followed by {: ${\nAnd finally to interpret \\n etc. as in a string: \n, \r, \t.\nfoo\n'bla'\nbar\ncut -d $'\\t' -f 1\nending dollar $$\n" +"This is an indented multi-line string\nliteral. An amount of whitespace at\nthe start of each line matching the minimum\nindentation of all lines in the string\nliteral together will be removed. Thus,\nin this case four spaces will be\nstripped from each line, even though\n THIS LINE is indented six spaces.\n\nAlso, empty lines don't count in the\ndetermination of the indentation level (the\nprevious empty line has indentation 0, but\nit doesn't matter).\nIf the string starts with whitespace\n followed by a newline, it's stripped, but\n that's not the case here. Two spaces are\n stripped because of the \" \" at the start. \nThis line is indented\na bit further.\nAnti-quotations, like so, are\nalso allowed.\n The \\ is not special here.\n' can be followed by any character except another ', e.g. 'x'.\nLikewise for $, e.g. $$ or $varName.\nBut ' followed by ' is special, as is $ followed by {.\nIf you want them, use anti-quotations: '', \${.\n Tabs are not interpreted as whitespace (since we can't guess\n what tab settings are intended), so don't use them.\n\tThis line starts with a space and a tab, so only one\n space will be stripped from each line.\nAlso note that if the last line (just before the closing ' ')\nconsists only of whitespace, it's ignored. But here there is\nsome non-whitespace stuff, so the line isn't removed. \nThis shows a hacky way to preserve an empty line after the start.\nBut there's no reason to do so: you could just repeat the empty\nline.\n Similarly you can force an indentation level,\n in this case to 2 spaces. This works because the anti-quote\n is significant (not whitespace).\nstart on network-interfaces\n\nstart script\n\n rm -f /var/run/opengl-driver\n ln -sf 123 /var/run/opengl-driver\n\n rm -f /var/log/slim.log\n \nend script\n\nenv SLIM_CFGFILE=abc\nenv SLIM_THEMESDIR=def\nenv FONTCONFIG_FILE=/etc/fonts/fonts.conf \t\t\t\t# !!! cleanup\nenv XKB_BINDIR=foo/bin \t\t\t\t# Needed for the Xkb extension.\nenv LD_LIBRARY_PATH=libX11/lib:libXext/lib:/usr/lib/ # related to xorg-sys-opengl - needed to load libglx for (AI)GLX support (for compiz)\n\nenv XORG_DRI_DRIVER_PATH=nvidiaDrivers/X11R6/lib/modules/drivers/ \n\nexec slim/bin/slim\nEscaping of ' followed by ': ''\nEscaping of $ followed by {: \${\nAnd finally to interpret \\n etc. as in a string: \n, \r, \t.\nfoo\n'bla'\nbar\ncut -d $'\\t' -f 1\nending dollar $$\n" diff --git a/tests/user-envs.nix b/tests/user-envs.nix index 1aa410cc9..43eff1a68 100644 --- a/tests/user-envs.nix +++ b/tests/user-envs.nix @@ -13,7 +13,7 @@ let builder = ./user-envs.builder.sh; } // { meta = { - description = "A silly test package"; + description = "A silly test package with some \${escaped anti-quotation} in it"; }; }); From 057c6203b5337cb4833958f4a4f29f6587f4eb2f Mon Sep 17 00:00:00 2001 From: regnat Date: Mon, 7 Sep 2020 11:26:09 +0200 Subject: [PATCH 10/14] gracefully handle old daemon versions Add a fallback path in `queryPartialDerivationOutputMap` for daemons that don't support it. Also upstreams a couple methods from `SSHStore` to `RemoteStore` as this is needed to handle the fallback path. --- src/libstore/remote-store.cc | 37 ++++++++++++++++++++++++++++++++---- src/libstore/remote-store.hh | 10 ++++++++++ src/libstore/ssh-store.cc | 17 ----------------- 3 files changed, 43 insertions(+), 21 deletions(-) diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 8bcf92301..dc61951d3 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -1,5 +1,6 @@ #include "serialise.hh" #include "util.hh" +#include "remote-fs-accessor.hh" #include "remote-store.hh" #include "worker-protocol.hh" #include "archive.hh" @@ -479,10 +480,26 @@ StorePathSet RemoteStore::queryDerivationOutputs(const StorePath & path) std::map> RemoteStore::queryPartialDerivationOutputMap(const StorePath & path) { - auto conn(getConnection()); - conn->to << wopQueryDerivationOutputMap << printStorePath(path); - conn.processStderr(); - return worker_proto::read(*this, conn->from, Phantom>> {}); + if (GET_PROTOCOL_MINOR(getProtocol()) >= 0x16) { + auto conn(getConnection()); + conn->to << wopQueryDerivationOutputMap << printStorePath(path); + conn.processStderr(); + return worker_proto::read(*this, conn->from, Phantom>> {}); + } else { + // Fallback for old daemon versions. + // For floating-CA derivations (and their co-dependencies) this is an + // under-approximation as it only returns the paths that can be inferred + // from the derivation itself (and not the ones that are known because + // the have been built), but as old stores don't handle floating-CA + // derivations this shouldn't matter + auto derivation = readDerivation(path); + auto outputsWithOptPaths = derivation.outputsAndOptPaths(*this); + std::map> ret; + for (auto & [outputName, outputAndPath] : outputsWithOptPaths) { + ret.emplace(outputName, outputAndPath.second); + } + return ret; + } } @@ -871,6 +888,18 @@ RemoteStore::Connection::~Connection() } } +void RemoteStore::narFromPath(const StorePath & path, Sink & sink) +{ + auto conn(connections->get()); + conn->to << wopNarFromPath << printStorePath(path); + conn->processStderr(); + copyNAR(conn->from, sink); +} + +ref RemoteStore::getFSAccessor() +{ + return make_ref(ref(shared_from_this())); +} static Logger::Fields readFields(Source & from) { diff --git a/src/libstore/remote-store.hh b/src/libstore/remote-store.hh index 7cf4c4d12..6f05f2197 100644 --- a/src/libstore/remote-store.hh +++ b/src/libstore/remote-store.hh @@ -131,6 +131,10 @@ protected: friend struct ConnectionHandle; + virtual ref getFSAccessor() override; + + virtual void narFromPath(const StorePath & path, Sink & sink) override; + private: std::atomic_bool failed{false}; @@ -149,6 +153,12 @@ public: bool sameMachine() override { return true; } + ref getFSAccessor() override + { return LocalFSStore::getFSAccessor(); } + + void narFromPath(const StorePath & path, Sink & sink) override + { LocalFSStore::narFromPath(path, sink); } + private: ref openConnection() override; diff --git a/src/libstore/ssh-store.cc b/src/libstore/ssh-store.cc index caae6b596..6cb97c1f1 100644 --- a/src/libstore/ssh-store.cc +++ b/src/libstore/ssh-store.cc @@ -40,10 +40,6 @@ public: bool sameMachine() override { return false; } - void narFromPath(const StorePath & path, Sink & sink) override; - - ref getFSAccessor() override; - private: struct Connection : RemoteStore::Connection @@ -68,19 +64,6 @@ private: }; }; -void SSHStore::narFromPath(const StorePath & path, Sink & sink) -{ - auto conn(connections->get()); - conn->to << wopNarFromPath << printStorePath(path); - conn->processStderr(); - copyNAR(conn->from, sink); -} - -ref SSHStore::getFSAccessor() -{ - return make_ref(ref(shared_from_this())); -} - ref SSHStore::openConnection() { auto conn = make_ref(); From 733d2e9402807e54d503c3113e854bfddb3d44e0 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 15 Sep 2020 13:48:23 +0200 Subject: [PATCH 11/14] .gitignore: inst -> outputs --- .gitignore | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index 573fa0005..3f81f8ef5 100644 --- a/.gitignore +++ b/.gitignore @@ -107,7 +107,7 @@ perl/Makefile.config /src/resolve-system-dependencies/resolve-system-dependencies -inst/ +outputs/ *.a *.o From c4bf219b55c76329988671d6b383d073373919f0 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 15 Sep 2020 14:26:56 +0000 Subject: [PATCH 12/14] Don't link deriver until after any delayed exception is thrown Otherwise, we will associate fixed-output derivations with outputs that they did indeed produce, but which had the wrong hash. That's no good. --- src/libstore/build.cc | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 9eff03f7b..d6a775ae9 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -4251,22 +4251,28 @@ void DerivationGoal::registerOutputs() /* Register each output path as valid, and register the sets of paths referenced by each of them. If there are cycles in the outputs, this will fail. */ - { - ValidPathInfos infos2; - for (auto & [outputName, newInfo] : infos) { - /* FIXME: we will want to track this mapping in the DB whether or - not we have a drv file. */ - if (useDerivation) - worker.store.linkDeriverToPath(drvPath, outputName, newInfo.path); - infos2.push_back(newInfo); - } - worker.store.registerValidPaths(infos2); + ValidPathInfos infos2; + for (auto & [outputName, newInfo] : infos) { + infos2.push_back(newInfo); } + worker.store.registerValidPaths(infos2); /* In case of a fixed-output derivation hash mismatch, throw an exception now that we have registered the output as valid. */ if (delayedException) std::rethrow_exception(delayedException); + + /* If we made it this far, we are sure the output matches the derivation + (since the delayedException would be a fixed output CA mismatch). That + means it's safe to link the derivation to the output hash. We must do + that for floating CA derivations, which otherwise couldn't be cached, + but it's fine to do in all cases. */ + for (auto & [outputName, newInfo] : infos) { + /* FIXME: we will want to track this mapping in the DB whether or + not we have a drv file. */ + if (useDerivation) + worker.store.linkDeriverToPath(drvPath, outputName, newInfo.path); + } } From 6387550d58884ac09204c339ed3a3cd700780a75 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 15 Sep 2020 15:19:45 +0000 Subject: [PATCH 13/14] Get rid of confusing `std::optional` for validity --- src/libstore/build.cc | 35 +++++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index d6a775ae9..b800a432f 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -718,16 +718,25 @@ typedef enum {rpAccept, rpDecline, rpPostpone} HookReply; class SubstitutionGoal; +/* Unless we are repairing, we don't both to test validity and just assume it, + so the choices are `Absent` or `Valid`. */ +enum struct PathStatus { + Corrupt, + Absent, + Valid, +}; + struct InitialOutputStatus { StorePath path; - /* The output optional indicates whether it's already valid; i.e. exists - and is registered. If we're repairing, inner bool indicates whether the - valid path is in fact not corrupt. Otherwise, the inner bool is always - true (assumed no corruption). */ - std::optional valid; + PathStatus status; /* Valid in the store, and additionally non-corrupt if we are repairing */ bool isValid() const { - return valid && *valid; + return status == PathStatus::Valid; + } + /* Merely present, allowed to be corrupt */ + bool isPresent() const { + return status == PathStatus::Corrupt + || status == PathStatus::Valid; } }; @@ -2148,10 +2157,10 @@ void DerivationGoal::startBuilder() : !needsHashRewrite() /* Can always use original path in sandbox */ ? status.known->path - : !status.known->valid + : !status.known->isPresent() /* If path doesn't yet exist can just use it */ ? status.known->path - : buildMode != bmRepair && !*status.known->valid + : buildMode != bmRepair && !status.known->isValid() /* If we aren't repairing we'll delete a corrupted path, so we can use original path */ ? status.known->path @@ -2161,7 +2170,7 @@ void DerivationGoal::startBuilder() scratchOutputs.insert_or_assign(outputName, scratchPath); /* A non-removed corrupted path needs to be stored here, too */ - if (buildMode == bmRepair && !*status.known->valid) + if (buildMode == bmRepair && !status.known->isValid()) redirectedBadOutputs.insert(status.known->path); /* Substitute output placeholders with the scratch output paths. @@ -4596,9 +4605,11 @@ void DerivationGoal::checkPathValidity() auto outputPath = *i.second; info.known = { .path = outputPath, - .valid = !worker.store.isValidPath(outputPath) - ? std::optional {} - : !checkHash || worker.pathContentsGood(outputPath), + .status = !worker.store.isValidPath(outputPath) + ? PathStatus::Absent + : !checkHash || worker.pathContentsGood(outputPath) + ? PathStatus::Valid + : PathStatus::Corrupt, }; } initialOutputs.insert_or_assign(i.first, info); From 3a5cdd737cf529a7f641c4347f002b821a456a5d Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 15 Sep 2020 15:21:39 +0000 Subject: [PATCH 14/14] Rename `Derivation::pathOpt` to `Derivation::path` We no longer need the `*Opt` to disambiguate. --- src/libexpr/get-drvs.cc | 2 +- src/libexpr/primops.cc | 2 +- src/libstore/build.cc | 4 ++-- src/libstore/derivations.cc | 4 ++-- src/libstore/derivations.hh | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/libexpr/get-drvs.cc b/src/libexpr/get-drvs.cc index e66832182..91916e8bf 100644 --- a/src/libexpr/get-drvs.cc +++ b/src/libexpr/get-drvs.cc @@ -40,7 +40,7 @@ DrvInfo::DrvInfo(EvalState & state, ref store, const std::string & drvPat throw Error("derivation '%s' does not have output '%s'", store->printStorePath(drvPath), outputName); auto & [outputName, output] = *i; - auto optStorePath = output.pathOpt(*store, drv.name, outputName); + auto optStorePath = output.path(*store, drv.name, outputName); if (optStorePath) outPath = store->printStorePath(*optStorePath); } diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 78dc314fa..4a0dd5544 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -91,7 +91,7 @@ static void mkOutputString(EvalState & state, Value & v, const StorePath & drvPath, const BasicDerivation & drv, std::pair o) { - auto optOutputPath = o.second.pathOpt(*state.store, drv.name, o.first); + auto optOutputPath = o.second.path(*state.store, drv.name, o.first); mkString( *state.allocAttr(v, state.symbols.create(o.first)), optOutputPath diff --git a/src/libstore/build.cc b/src/libstore/build.cc index b800a432f..ce973862d 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -4081,7 +4081,7 @@ void DerivationGoal::registerOutputs() floating CA derivations and hash-mismatching fixed-output derivations. */ PathLocks dynamicOutputLock; - auto optFixedPath = output.pathOpt(worker.store, drv->name, outputName); + auto optFixedPath = output.path(worker.store, drv->name, outputName); if (!optFixedPath || worker.store.printStorePath(*optFixedPath) != finalDestPath) { @@ -4574,7 +4574,7 @@ std::map> DerivationGoal::queryPartialDeri if (drv->type() != DerivationType::CAFloating) { std::map> res; for (auto & [name, output] : drv->outputs) - res.insert_or_assign(name, output.pathOpt(worker.store, drv->name, name)); + res.insert_or_assign(name, output.path(worker.store, drv->name, name)); return res; } else { return worker.store.queryPartialDerivationOutputMap(drvPath); diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index e5ec60811..9d8ce5e36 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -7,7 +7,7 @@ namespace nix { -std::optional DerivationOutput::pathOpt(const Store & store, std::string_view drvName, std::string_view outputName) const +std::optional DerivationOutput::path(const Store & store, std::string_view drvName, std::string_view outputName) const { return std::visit(overloaded { [](DerivationOutputInputAddressed doi) -> std::optional { @@ -557,7 +557,7 @@ DerivationOutputsAndOptPaths BasicDerivation::outputsAndOptPaths(const Store & s for (auto output : outputs) outsAndOptPaths.insert(std::make_pair( output.first, - std::make_pair(output.second, output.second.pathOpt(store, name, output.first)) + std::make_pair(output.second, output.second.path(store, name, output.first)) ) ); return outsAndOptPaths; diff --git a/src/libstore/derivations.hh b/src/libstore/derivations.hh index 2c0b1feab..0b5652685 100644 --- a/src/libstore/derivations.hh +++ b/src/libstore/derivations.hh @@ -51,7 +51,7 @@ struct DerivationOutput /* Note, when you use this function you should make sure that you're passing the right derivation name. When in doubt, you should use the safer interface provided by BasicDerivation::outputsAndOptPaths */ - std::optional pathOpt(const Store & store, std::string_view drvName, std::string_view outputName) const; + std::optional path(const Store & store, std::string_view drvName, std::string_view outputName) const; }; typedef std::map DerivationOutputs;