From 180538672a017488f9c23a4f72da5e5738a7570c Mon Sep 17 00:00:00 2001 From: Valentin Gagarin Date: Mon, 5 Dec 2022 11:01:46 +0100 Subject: [PATCH 01/40] disallow selecting shell prompt in code samples this is a quick half-fix for command line examples, as discussed discussed in [1]. [1]: https://github.com/NixOS/nix/pull/7389 examples which look like this $ foo bar baz are confusing for Unix shell beginners, because it's hard to discern what is supposed to be entered into the actual command line when the convention of prefixing `$` is not known, as barely any real-world shell looks that way any more. this change prevents selecting the prompt part with the mouse in the HTML representation of the Nix manual. it does not prevent selecting the output part of the shell example. it also does not address that the copy button provided by mdBook takes the entire sample, including the prompts, into the clipboard. --- doc/manual/custom.css | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/doc/manual/custom.css b/doc/manual/custom.css index 69d48d4a7..b90f5423f 100644 --- a/doc/manual/custom.css +++ b/doc/manual/custom.css @@ -5,3 +5,7 @@ h1:not(:first-of-type) { h2 { margin-top: 1em; } + +.hljs-meta { + user-select: none; +} From f20d3726dd9031a4ec311c77f30caa7cc493138b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Sun, 29 Jan 2023 15:57:15 +0100 Subject: [PATCH 02/40] advertise transport encoding in http transfers to MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit tl;dr: With this 1 line change I was able to get a speedup of 1.5x on 1Gbit/s wan connections by enabling zstd compression in nginx. Also nix already supported all common compression format for http transfer, webservers usually only enable them if they are advertised through the Accept-Encoding header. This pull requests makes nix advertises content compression support for zstd, br, gzip and deflate. It's particular useful to add transparent compression for binary caches that serve packages from the host nix store in particular nix-serve, nix-serve-ng and harmonia. I tried so far gzip, brotli and zstd, whereas only zstd was able to bring me performance improvements for 1Gbit/s WAN connections. The following nginx configuration was used in combination with the [zstd module](https://github.com/tokers/zstd-nginx-module) and [harmonia](https://github.com/nix-community/harmonia/) ```nix { services.nginx.virtualHosts."cache.yourhost.com" = { locations."/".extraConfig = '' proxy_pass http://127.0.0.1:5000; proxy_set_header Host $host; proxy_redirect http:// https://; proxy_http_version 1.1; proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; proxy_set_header Upgrade $http_upgrade; proxy_set_header Connection $connection_upgrade; zstd on; zstd_types application/x-nix-archive; ''; }; } ``` For testing I unpacked a linux kernel tarball to the nix store using this command `nix-prefetch-url --unpack https://cdn.kernel.org/pub/linux/kernel/v6.x/linux-6.1.8.tar.gz`. Before: ```console $ nix build && rm -rf /tmp/hello && time ./result/bin/nix copy --no-check-sigs --from https://cache.thalheim.io --to 'file:///tmp/hello?compression=none' '/nix/store/j42mahch5f0jvfmayhzwbb88sw36fvah-linux-6.1.8.tar.gz' warning: Git tree '/scratch/joerg/nix' is dirty real 0m18,375s user 0m2,889s sys 0m1,558s ``` After: ```console $ nix build && rm -rf /tmp/hello && time ./result/bin/nix copy --no-check-sigs --from https://cache.thalheim.io --to 'file:///tmp/hello?compression=none' '/nix/store/j42mahch5f0jvfmayhzwb b88sw36fvah-linux-6.1.8.tar.gz' real 0m11,884s user 0m4,130s sys 0m1,439s ``` Signed-off-by: Jörg Thalheim Update src/libstore/filetransfer.cc Co-authored-by: Théophane Hufschmitt <7226587+thufschmitt@users.noreply.github.com> --- src/libstore/filetransfer.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index 756bd4423..b25089ec3 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -101,6 +101,7 @@ struct curlFileTransfer : public FileTransfer this->result.data.append(data); }) { + requestHeaders = curl_slist_append(requestHeaders, "Accept-Encoding: zstd, br, gzip, deflate, bzip2, xz"); if (!request.expectedETag.empty()) requestHeaders = curl_slist_append(requestHeaders, ("If-None-Match: " + request.expectedETag).c_str()); if (!request.mimeType.empty()) From 45fa297e4052acef962d9d124241e7abd02f58af Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 3 Feb 2023 11:21:47 -0500 Subject: [PATCH 03/40] Factor out `InstallableStorePath` to its own file, dedup `nix app` had something called `InstallableDerivedPath` which is actually the same thing. We go with the later's name because it has become more correct. I originally did this change (more hurriedly) as part of #6225 --- a mini store-only Nix and a full Nix need to share this code. In the first RFC meeting for https://github.com/NixOS/rfcs/pull/134 we discussed how some splitting of the massive `installables.cc` could begin prior, as that is a good thing anyways. (@edolstra's words, not mine!) This would be one such PR. --- src/libcmd/installable-derived-path.cc | 70 ++++++++++++++++++++++++++ src/libcmd/installable-derived-path.hh | 28 +++++++++++ src/libcmd/installables.cc | 70 ++------------------------ src/nix/app.cc | 27 +--------- 4 files changed, 104 insertions(+), 91 deletions(-) create mode 100644 src/libcmd/installable-derived-path.cc create mode 100644 src/libcmd/installable-derived-path.hh diff --git a/src/libcmd/installable-derived-path.cc b/src/libcmd/installable-derived-path.cc new file mode 100644 index 000000000..a9921b901 --- /dev/null +++ b/src/libcmd/installable-derived-path.cc @@ -0,0 +1,70 @@ +#include "installable-derived-path.hh" +#include "derivations.hh" + +namespace nix { + +std::string InstallableDerivedPath::what() const +{ + return derivedPath.to_string(*store); +} + +DerivedPathsWithInfo InstallableDerivedPath::toDerivedPaths() +{ + return {{.path = derivedPath, .info = {} }}; +} + +std::optional InstallableDerivedPath::getStorePath() +{ + return std::visit(overloaded { + [&](const DerivedPath::Built & bfd) { + return bfd.drvPath; + }, + [&](const DerivedPath::Opaque & bo) { + return bo.path; + }, + }, derivedPath.raw()); +} + +InstallableDerivedPath InstallableDerivedPath::parse( + ref store, + std::string_view prefix, + ExtendedOutputsSpec extendedOutputsSpec) +{ + auto derivedPath = std::visit(overloaded { + // If the user did not use ^, we treat the output more liberally. + [&](const ExtendedOutputsSpec::Default &) -> DerivedPath { + // First, we accept a symlink chain or an actual store path. + auto storePath = store->followLinksToStorePath(prefix); + // Second, we see if the store path ends in `.drv` to decide what sort + // of derived path they want. + // + // This handling predates the `^` syntax. The `^*` in + // `/nix/store/hash-foo.drv^*` unambiguously means "do the + // `DerivedPath::Built` case", so plain `/nix/store/hash-foo.drv` could + // also unambiguously mean "do the DerivedPath::Opaque` case". + // + // Issue #7261 tracks reconsidering this `.drv` dispatching. + return storePath.isDerivation() + ? (DerivedPath) DerivedPath::Built { + .drvPath = std::move(storePath), + .outputs = OutputsSpec::All {}, + } + : (DerivedPath) DerivedPath::Opaque { + .path = std::move(storePath), + }; + }, + // If the user did use ^, we just do exactly what is written. + [&](const ExtendedOutputsSpec::Explicit & outputSpec) -> DerivedPath { + return DerivedPath::Built { + .drvPath = store->parseStorePath(prefix), + .outputs = outputSpec, + }; + }, + }, extendedOutputsSpec.raw()); + return InstallableDerivedPath { + store, + std::move(derivedPath), + }; +} + +} diff --git a/src/libcmd/installable-derived-path.hh b/src/libcmd/installable-derived-path.hh new file mode 100644 index 000000000..042878b91 --- /dev/null +++ b/src/libcmd/installable-derived-path.hh @@ -0,0 +1,28 @@ +#pragma once + +#include "installables.hh" + +namespace nix { + +struct InstallableDerivedPath : Installable +{ + ref store; + DerivedPath derivedPath; + + InstallableDerivedPath(ref store, DerivedPath && derivedPath) + : store(store), derivedPath(std::move(derivedPath)) + { } + + std::string what() const override; + + DerivedPathsWithInfo toDerivedPaths() override; + + std::optional getStorePath() override; + + static InstallableDerivedPath parse( + ref store, + std::string_view prefix, + ExtendedOutputsSpec extendedOutputsSpec); +}; + +} diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index 24f458f1a..5a75ed7af 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -1,5 +1,6 @@ #include "globals.hh" #include "installables.hh" +#include "installable-derived-path.hh" #include "outputs-spec.hh" #include "util.hh" #include "command.hh" @@ -396,38 +397,6 @@ static StorePath getDeriver( return *derivers.begin(); } -struct InstallableStorePath : Installable -{ - ref store; - DerivedPath req; - - InstallableStorePath(ref store, DerivedPath && req) - : store(store), req(std::move(req)) - { } - - std::string what() const override - { - return req.to_string(*store); - } - - DerivedPathsWithInfo toDerivedPaths() override - { - return {{.path = req, .info = {} }}; - } - - std::optional getStorePath() override - { - return std::visit(overloaded { - [&](const DerivedPath::Built & bfd) { - return bfd.drvPath; - }, - [&](const DerivedPath::Opaque & bo) { - return bo.path; - }, - }, req.raw()); - } -}; - struct InstallableAttrPath : InstallableValue { SourceExprCommand & cmd; @@ -792,41 +761,10 @@ std::vector> SourceExprCommand::parseInstallables( auto prefix = std::move(prefix_); auto extendedOutputsSpec = std::move(extendedOutputsSpec_); - auto found = prefix.find('/'); - if (found != std::string::npos) { + if (prefix.find('/') != std::string::npos) { try { - auto derivedPath = std::visit(overloaded { - // If the user did not use ^, we treat the output more liberally. - [&](const ExtendedOutputsSpec::Default &) -> DerivedPath { - // First, we accept a symlink chain or an actual store path. - auto storePath = store->followLinksToStorePath(prefix); - // Second, we see if the store path ends in `.drv` to decide what sort - // of derived path they want. - // - // This handling predates the `^` syntax. The `^*` in - // `/nix/store/hash-foo.drv^*` unambiguously means "do the - // `DerivedPath::Built` case", so plain `/nix/store/hash-foo.drv` could - // also unambiguously mean "do the DerivedPath::Opaque` case". - // - // Issue #7261 tracks reconsidering this `.drv` dispatching. - return storePath.isDerivation() - ? (DerivedPath) DerivedPath::Built { - .drvPath = std::move(storePath), - .outputs = OutputsSpec::All {}, - } - : (DerivedPath) DerivedPath::Opaque { - .path = std::move(storePath), - }; - }, - // If the user did use ^, we just do exactly what is written. - [&](const ExtendedOutputsSpec::Explicit & outputSpec) -> DerivedPath { - return DerivedPath::Built { - .drvPath = store->parseStorePath(prefix), - .outputs = outputSpec, - }; - }, - }, extendedOutputsSpec.raw()); - result.push_back(std::make_shared(store, std::move(derivedPath))); + result.push_back(std::make_shared( + InstallableDerivedPath::parse(store, prefix, extendedOutputsSpec))); continue; } catch (BadStorePath &) { } catch (...) { diff --git a/src/nix/app.cc b/src/nix/app.cc index 08cd0ccd4..5cd65136f 100644 --- a/src/nix/app.cc +++ b/src/nix/app.cc @@ -1,4 +1,5 @@ #include "installables.hh" +#include "installable-derived-path.hh" #include "store-api.hh" #include "eval-inline.hh" #include "eval-cache.hh" @@ -8,30 +9,6 @@ namespace nix { -struct InstallableDerivedPath : Installable -{ - ref store; - const DerivedPath derivedPath; - - InstallableDerivedPath(ref store, const DerivedPath & derivedPath) - : store(store) - , derivedPath(derivedPath) - { - } - - std::string what() const override { return derivedPath.to_string(*store); } - - DerivedPathsWithInfo toDerivedPaths() override - { - return {{derivedPath}}; - } - - std::optional getStorePath() override - { - return std::nullopt; - } -}; - /** * Return the rewrites that are needed to resolve a string whose context is * included in `dependencies`. @@ -146,7 +123,7 @@ App UnresolvedApp::resolve(ref evalStore, ref store) for (auto & ctxElt : unresolved.context) installableContext.push_back( - std::make_shared(store, ctxElt)); + std::make_shared(store, DerivedPath { ctxElt })); auto builtContext = Installable::build(evalStore, store, Realise::Outputs, installableContext); res.program = resolveString(*store, unresolved.program, builtContext); From 6352e20bc8dba9477e25eedbe7bea91cbe1614f9 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sat, 4 Feb 2023 18:26:43 -0500 Subject: [PATCH 04/40] Remove `--derivation` from test It doesn't do anything here, and in the next commit `show-derivation will no longer accept this flag. --- tests/ca/build.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/ca/build.sh b/tests/ca/build.sh index 92f8b429a..cc225c6c8 100644 --- a/tests/ca/build.sh +++ b/tests/ca/build.sh @@ -3,7 +3,7 @@ source common.sh drv=$(nix-instantiate --experimental-features ca-derivations ./content-addressed.nix -A rootCA --arg seed 1) -nix --experimental-features 'nix-command ca-derivations' show-derivation --derivation "$drv" --arg seed 1 +nix --experimental-features 'nix-command ca-derivations' show-derivation "$drv" --arg seed 1 buildAttr () { local derivationPath=$1 From 44bea52ae3ca9569250eb1f50100f2c3260cc688 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sat, 4 Feb 2023 18:27:17 -0500 Subject: [PATCH 05/40] Scope down `--derivation` to just the commands that use it Per the old FIXME, this flag was on too many commands, and mostly ignored. Now it is just on the commands where it actually has an effect. Per https://github.com/NixOS/nix/issues/7261, I would still like to get rid of it entirely, but that is a separate project. This change should be good with or without doing that. --- src/libcmd/command.cc | 10 ++++++++++ src/libcmd/command.hh | 12 ++++++++---- src/libcmd/installables.cc | 7 ------- src/nix/diff-closures.cc | 2 +- src/nix/why-depends.cc | 2 +- 5 files changed, 20 insertions(+), 13 deletions(-) diff --git a/src/libcmd/command.cc b/src/libcmd/command.cc index 0740ea960..517cdf617 100644 --- a/src/libcmd/command.cc +++ b/src/libcmd/command.cc @@ -127,6 +127,16 @@ ref EvalCommand::getEvalState() return ref(evalState); } +MixOperateOnOptions::MixOperateOnOptions() +{ + addFlag({ + .longName = "derivation", + .description = "Operate on the [store derivation](../../glossary.md#gloss-store-derivation) rather than its outputs.", + .category = installablesCategory, + .handler = {&operateOn, OperateOn::Derivation}, + }); +} + BuiltPathsCommand::BuiltPathsCommand(bool recursive) : recursive(recursive) { diff --git a/src/libcmd/command.hh b/src/libcmd/command.hh index 3b4b40981..d16bdbc4b 100644 --- a/src/libcmd/command.hh +++ b/src/libcmd/command.hh @@ -96,9 +96,6 @@ struct SourceExprCommand : virtual Args, MixFlakeOptions std::optional expr; bool readOnlyMode = false; - // FIXME: move this; not all commands (e.g. 'nix run') use it. - OperateOn operateOn = OperateOn::Output; - SourceExprCommand(bool supportReadOnlyMode = false); std::vector> parseInstallables( @@ -153,8 +150,15 @@ private: std::string _installable{"."}; }; +struct MixOperateOnOptions : virtual Args +{ + OperateOn operateOn = OperateOn::Output; + + MixOperateOnOptions(); +}; + /* A command that operates on zero or more store paths. */ -struct BuiltPathsCommand : public InstallablesCommand +struct BuiltPathsCommand : InstallablesCommand, virtual MixOperateOnOptions { private: diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index 24f458f1a..ff8261b09 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -167,13 +167,6 @@ SourceExprCommand::SourceExprCommand(bool supportReadOnlyMode) .handler = {&expr} }); - addFlag({ - .longName = "derivation", - .description = "Operate on the [store derivation](../../glossary.md#gloss-store-derivation) rather than its outputs.", - .category = installablesCategory, - .handler = {&operateOn, OperateOn::Derivation}, - }); - if (supportReadOnlyMode) { addFlag({ .longName = "read-only", diff --git a/src/nix/diff-closures.cc b/src/nix/diff-closures.cc index 0621d662c..3489cc132 100644 --- a/src/nix/diff-closures.cc +++ b/src/nix/diff-closures.cc @@ -106,7 +106,7 @@ void printClosureDiff( using namespace nix; -struct CmdDiffClosures : SourceExprCommand +struct CmdDiffClosures : SourceExprCommand, MixOperateOnOptions { std::string _before, _after; diff --git a/src/nix/why-depends.cc b/src/nix/why-depends.cc index 76125e5e4..a3a9dc698 100644 --- a/src/nix/why-depends.cc +++ b/src/nix/why-depends.cc @@ -27,7 +27,7 @@ static std::string filterPrintable(const std::string & s) return res; } -struct CmdWhyDepends : SourceExprCommand +struct CmdWhyDepends : SourceExprCommand, MixOperateOnOptions { std::string _package, _dependency; bool all = false; From 81e75e4bf6083046155a0c1fd6e312b43a60f7da Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 3 Feb 2023 21:42:42 +0100 Subject: [PATCH 06/40] Add some progress indication when fetching submodules --- src/libfetchers/git.cc | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc index 1f7d7c07d..fb80d248c 100644 --- a/src/libfetchers/git.cc +++ b/src/libfetchers/git.cc @@ -615,15 +615,24 @@ struct GitInputScheme : InputScheme AutoDelete delTmpGitDir(tmpGitDir, true); runProgram("git", true, { "-c", "init.defaultBranch=" + gitInitialBranch, "init", tmpDir, "--separate-git-dir", tmpGitDir }); - // TODO: repoDir might lack the ref (it only checks if rev - // exists, see FIXME above) so use a big hammer and fetch - // everything to ensure we get the rev. - runProgram("git", true, { "-C", tmpDir, "fetch", "--quiet", "--force", - "--update-head-ok", "--", repoDir, "refs/*:refs/*" }); + + { + // TODO: repoDir might lack the ref (it only checks if rev + // exists, see FIXME above) so use a big hammer and fetch + // everything to ensure we get the rev. + Activity act(*logger, lvlTalkative, actUnknown, fmt("making temporary clone of '%s'", tmpDir)); + runProgram("git", true, { "-C", tmpDir, "fetch", "--quiet", "--force", + "--update-head-ok", "--", repoDir, "refs/*:refs/*" }); + } runProgram("git", true, { "-C", tmpDir, "checkout", "--quiet", input.getRev()->gitRev() }); + runProgram("git", true, { "-C", tmpDir, "remote", "add", "origin", actualUrl }); - runProgram("git", true, { "-C", tmpDir, "submodule", "--quiet", "update", "--init", "--recursive" }); + + { + Activity act(*logger, lvlTalkative, actUnknown, fmt("fetching submodules of '%s'", tmpDir)); + runProgram("git", true, { "-C", tmpDir, "submodule", "--quiet", "update", "--init", "--recursive" }); + } filter = isNotDotGitDirectory; } else { From 2edd5cf6184a7955ba82a2aed8bbfa870bfeb15f Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 7 Feb 2023 15:35:32 +0100 Subject: [PATCH 07/40] Fix the origin URL used for fetching submodules We cannot use 'actualUrl', because for file:// repos that's not the original URL that the repo was fetched from. This is a problem since submodules may be relative to the original URL. Fixes e.g. nix eval --impure --json --expr 'builtins.fetchTree { type = "git"; url = "/path/to/blender"; submodules = true; }' where /path/to/blender is a clone of https://github.com/blender/blender.git (which has several relative submodules like '../blender-addons.git'). --- src/libfetchers/git.cc | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc index fb80d248c..8981476e1 100644 --- a/src/libfetchers/git.cc +++ b/src/libfetchers/git.cc @@ -627,7 +627,17 @@ struct GitInputScheme : InputScheme runProgram("git", true, { "-C", tmpDir, "checkout", "--quiet", input.getRev()->gitRev() }); - runProgram("git", true, { "-C", tmpDir, "remote", "add", "origin", actualUrl }); + /* Ensure that we use the correct origin for fetching + submodules. This matters for submodules with relative + URLs. */ + if (isLocal) { + writeFile(tmpGitDir + "/config", readFile(repoDir + "/" + gitDir + "/config")); + + /* Restore the config.bare setting we may have just + nuked. */ + runProgram("git", true, { "-C", tmpDir, "config", "core.bare", "false" }); + } else + runProgram("git", true, { "-C", tmpDir, "config", "remote.origin.url", actualUrl }); { Activity act(*logger, lvlTalkative, actUnknown, fmt("fetching submodules of '%s'", tmpDir)); From a8fe0dc16c18c260a1bb9e88f467e4c929cf22dc Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 7 Feb 2023 15:50:35 +0100 Subject: [PATCH 08/40] Speed up fetching submodules Previously we would completely refetch the submodules from the network, even though the repo might already have them. Now we copy the .git/modules directory from the repo as an optimisation. This speeds up evaluating builtins.fetchTree { type = "git"; url = "/path/to/blender"; submodules = true; } (where /path/to/blender already has the needed submodules) from 121s to 57s. This is still pretty inefficient and a hack, but a better solution is best done on the lazy-trees branch. This change also help in the case where the repo already has the submodules but the origin is unfetchable for whatever reason (e.g. there have been cases where Nix in a GitHub action doesn't have the right authentication set up). --- src/libfetchers/git.cc | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc index 8981476e1..9908db214 100644 --- a/src/libfetchers/git.cc +++ b/src/libfetchers/git.cc @@ -639,6 +639,14 @@ struct GitInputScheme : InputScheme } else runProgram("git", true, { "-C", tmpDir, "config", "remote.origin.url", actualUrl }); + /* As an optimisation, copy the modules directory of the + source repo if it exists. */ + auto modulesPath = repoDir + "/" + gitDir + "/modules"; + if (pathExists(modulesPath)) { + Activity act(*logger, lvlTalkative, actUnknown, fmt("copying submodules of '%s'", actualUrl)); + runProgram("cp", true, { "-R", "--", modulesPath, tmpGitDir + "/modules" }); + } + { Activity act(*logger, lvlTalkative, actUnknown, fmt("fetching submodules of '%s'", tmpDir)); runProgram("git", true, { "-C", tmpDir, "submodule", "--quiet", "update", "--init", "--recursive" }); From 72b18f05a22f46f383e0476262dc363ac0318aa6 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 7 Feb 2023 16:36:10 +0100 Subject: [PATCH 09/40] Add a basic daemon authorization test --- flake.nix | 2 + tests/nixos/authorization.nix | 79 +++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+) create mode 100644 tests/nixos/authorization.nix diff --git a/flake.nix b/flake.nix index 0325f33b1..7f0b738f8 100644 --- a/flake.nix +++ b/flake.nix @@ -499,6 +499,8 @@ }; # System tests. + tests.authorization = runNixOSTestFor "x86_64-linux" ./tests/nixos/authorization.nix; + tests.remoteBuilds = runNixOSTestFor "x86_64-linux" ./tests/nixos/remote-builds.nix; tests.nix-copy-closure = runNixOSTestFor "x86_64-linux" ./tests/nixos/nix-copy-closure.nix; diff --git a/tests/nixos/authorization.nix b/tests/nixos/authorization.nix new file mode 100644 index 000000000..4b13ec65e --- /dev/null +++ b/tests/nixos/authorization.nix @@ -0,0 +1,79 @@ +{ + name = "authorization"; + + nodes.machine = { + virtualisation.writableStore = true; + # TODO add a test without allowed-users setting. allowed-users is uncommon among NixOS users. + nix.settings.allowed-users = ["alice" "bob"]; + nix.settings.trusted-users = ["alice"]; + + users.users.alice.isNormalUser = true; + users.users.bob.isNormalUser = true; + users.users.mallory.isNormalUser = true; + + nix.settings.experimental-features = "nix-command"; + }; + + testScript = + let + pathFour = "/nix/store/20xfy868aiic0r0flgzq4n5dq1yvmxkn-four"; + in + '' + machine.wait_for_unit("multi-user.target") + machine.succeed(""" + exec 1>&2 + echo kSELDhobKaF8/VdxIxdP7EQe+Q > one + diff $(nix store add-file one) one + """) + machine.succeed(""" + su --login alice -c ' + set -x + cd ~ + echo ehHtmfuULXYyBV6NBk6QUi8iE0 > two + ls + diff $(echo $(nix store add-file two)) two' 1>&2 + """) + machine.succeed(""" + su --login bob -c ' + set -x + cd ~ + echo 0Jw8RNp7cK0W2AdNbcquofcOVk > three + diff $(nix store add-file three) three + ' 1>&2 + """) + + # We're going to check that a path is not created + machine.succeed(""" + ! [[ -e ${pathFour} ]] + """) + machine.succeed(""" + su --login mallory -c ' + set -x + cd ~ + echo 5mgtDj0ohrWkT50TLR0f4tIIxY > four; + (! diff $(nix store add-file four) four 2>&1) | grep -F "cannot open connection to remote store" + (! diff $(nix store add-file four) four 2>&1) | grep -F "Connection reset by peer" + ! [[ -e ${pathFour} ]] + ' 1>&2 + """) + + # Check that the file _can_ be added, and matches the expected path we were checking + machine.succeed(""" + exec 1>&2 + echo 5mgtDj0ohrWkT50TLR0f4tIIxY > four + four="$(nix store add-file four)" + diff $four four + diff <(echo $four) <(echo ${pathFour}) + """) + + machine.succeed(""" + su --login alice -c 'nix-store --verify --repair' + """) + + machine.succeed(""" + set -x + su --login bob -c '(! nix-store --verify --repair 2>&1)' | tee diag 1>&2 + grep -F "you are not privileged to repair paths" diag + """) + ''; +} From 7a6daf61e8a3b6fd91e6d89334fe6b59fe07a2a6 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 7 Feb 2023 22:22:50 +0100 Subject: [PATCH 10/40] Fix activity message --- src/libfetchers/git.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc index 9908db214..3871cf0dd 100644 --- a/src/libfetchers/git.cc +++ b/src/libfetchers/git.cc @@ -620,7 +620,7 @@ struct GitInputScheme : InputScheme // TODO: repoDir might lack the ref (it only checks if rev // exists, see FIXME above) so use a big hammer and fetch // everything to ensure we get the rev. - Activity act(*logger, lvlTalkative, actUnknown, fmt("making temporary clone of '%s'", tmpDir)); + Activity act(*logger, lvlTalkative, actUnknown, fmt("making temporary clone of '%s'", repoDir)); runProgram("git", true, { "-C", tmpDir, "fetch", "--quiet", "--force", "--update-head-ok", "--", repoDir, "refs/*:refs/*" }); } @@ -648,7 +648,7 @@ struct GitInputScheme : InputScheme } { - Activity act(*logger, lvlTalkative, actUnknown, fmt("fetching submodules of '%s'", tmpDir)); + Activity act(*logger, lvlTalkative, actUnknown, fmt("fetching submodules of '%s'", repoDir)); runProgram("git", true, { "-C", tmpDir, "submodule", "--quiet", "update", "--init", "--recursive" }); } From 9a7dc5d71879a32ff06305aa843abf299b39c90f Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 7 Feb 2023 22:46:25 +0100 Subject: [PATCH 11/40] Add some tests --- tests/fetchGitSubmodules.sh | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/tests/fetchGitSubmodules.sh b/tests/fetchGitSubmodules.sh index 50da4cb97..08ccaa3cd 100644 --- a/tests/fetchGitSubmodules.sh +++ b/tests/fetchGitSubmodules.sh @@ -104,3 +104,28 @@ noSubmoduleRepoBaseline=$(nix eval --raw --expr "(builtins.fetchGit { url = file noSubmoduleRepo=$(nix eval --raw --expr "(builtins.fetchGit { url = file://$subRepo; rev = \"$subRev\"; submodules = true; }).outPath") [[ $noSubmoduleRepoBaseline == $noSubmoduleRepo ]] + +# Test relative submodule URLs. +rm $TEST_HOME/.cache/nix/fetcher-cache* +rm -rf $rootRepo/.git $rootRepo/.gitmodules $rootRepo/sub +initGitRepo $rootRepo +git -C $rootRepo submodule add ../gitSubmodulesSub sub +git -C $rootRepo commit -m "Add submodule" +rev2=$(git -C $rootRepo rev-parse HEAD) +pathWithRelative=$(nix eval --raw --expr "(builtins.fetchGit { url = file://$rootRepo; rev = \"$rev2\"; submodules = true; }).outPath") +diff -r -x .gitmodules $pathWithSubmodules $pathWithRelative + +# Test clones that have an upstream with relative submodule URLs. +rm $TEST_HOME/.cache/nix/fetcher-cache* +cloneRepo=$TEST_ROOT/a/b/gitSubmodulesClone # NB /a/b to make the relative path not work relative to $cloneRepo +git clone $rootRepo $cloneRepo +pathIndirect=$(nix eval --raw --expr "(builtins.fetchGit { url = file://$cloneRepo; rev = \"$rev2\"; submodules = true; }).outPath") +[[ $pathIndirect = $pathWithRelative ]] + +# Test that if the clone has the submodule already, we're not fetching +# it again. +git -C $cloneRepo submodule update --init +rm $TEST_HOME/.cache/nix/fetcher-cache* +rm -rf $subRepo +pathSubmoduleGone=$(nix eval --raw --expr "(builtins.fetchGit { url = file://$cloneRepo; rev = \"$rev2\"; submodules = true; }).outPath") +[[ $pathSubmoduleGone = $pathWithRelative ]] From 1a86d3e98ebcc9a670c3c39499823298ce7fa1da Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 17 Jan 2023 19:52:19 +0100 Subject: [PATCH 12/40] local.mk: Don't log docroot comments These were accidentally logged and do not need to appear in make's log output. --- doc/manual/local.mk | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/manual/local.mk b/doc/manual/local.mk index 190f0258a..f43510b6d 100644 --- a/doc/manual/local.mk +++ b/doc/manual/local.mk @@ -51,13 +51,13 @@ $(d)/src/SUMMARY.md: $(d)/src/SUMMARY.md.in $(d)/src/command-ref/new-cli $(d)/src/command-ref/new-cli: $(d)/nix.json $(d)/generate-manpage.nix $(bindir)/nix @rm -rf $@ $(trace-gen) $(nix-eval) --write-to $@.tmp --expr 'import doc/manual/generate-manpage.nix { toplevel = builtins.readFile $<; }' - # @docroot@: https://nixos.org/manual/nix/unstable/contributing/hacking.html#docroot-variable + @# @docroot@: https://nixos.org/manual/nix/unstable/contributing/hacking.html#docroot-variable $(trace-gen) sed -i $@.tmp/*.md -e 's^@docroot@^../..^g' @mv $@.tmp $@ $(d)/src/command-ref/conf-file.md: $(d)/conf-file.json $(d)/generate-options.nix $(d)/src/command-ref/conf-file-prefix.md $(bindir)/nix @cat doc/manual/src/command-ref/conf-file-prefix.md > $@.tmp - # @docroot@: https://nixos.org/manual/nix/unstable/contributing/hacking.html#docroot-variable + @# @docroot@: https://nixos.org/manual/nix/unstable/contributing/hacking.html#docroot-variable $(trace-gen) $(nix-eval) --expr 'import doc/manual/generate-options.nix (builtins.fromJSON (builtins.readFile $<))' \ | sed -e 's^@docroot@^..^g'>> $@.tmp @mv $@.tmp $@ @@ -72,7 +72,7 @@ $(d)/conf-file.json: $(bindir)/nix $(d)/src/language/builtins.md: $(d)/builtins.json $(d)/generate-builtins.nix $(d)/src/language/builtins-prefix.md $(bindir)/nix @cat doc/manual/src/language/builtins-prefix.md > $@.tmp - # @docroot@: https://nixos.org/manual/nix/unstable/contributing/hacking.html#docroot-variable + @# @docroot@: https://nixos.org/manual/nix/unstable/contributing/hacking.html#docroot-variable $(trace-gen) $(nix-eval) --expr 'import doc/manual/generate-builtins.nix (builtins.fromJSON (builtins.readFile $<))' \ | sed -e 's^@docroot@^..^g' >> $@.tmp @cat doc/manual/src/language/builtins-suffix.md >> $@.tmp From 8a0ef5d58e0bb59eda0b8fd5622f2d731016f7a3 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 17 Jan 2023 19:53:21 +0100 Subject: [PATCH 13/40] sqlite.cc: Add SQL tracing Set environment variable NIX_DEBUG_SQLITE_TRACES=1 to log all sql statements. --- src/libstore/sqlite.cc | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/libstore/sqlite.cc b/src/libstore/sqlite.cc index 353dff9fa..871f2f3be 100644 --- a/src/libstore/sqlite.cc +++ b/src/libstore/sqlite.cc @@ -41,6 +41,15 @@ SQLiteError::SQLiteError(const char *path, const char *errMsg, int errNo, int ex throw SQLiteError(path, errMsg, err, exterr, offset, std::move(hf)); } +static void traceSQL(void * x, const char * sql) +{ + // wacky delimiters: + // so that we're quite unambiguous without escaping anything + // notice instead of trace: + // so that this can be enabled without getting the firehose in our face. + notice("SQL<[%1%]>", sql); +}; + SQLite::SQLite(const Path & path, bool create) { // useSQLiteWAL also indicates what virtual file system we need. Using @@ -58,6 +67,11 @@ SQLite::SQLite(const Path & path, bool create) if (sqlite3_busy_timeout(db, 60 * 60 * 1000) != SQLITE_OK) SQLiteError::throw_(db, "setting timeout"); + if (getEnv("NIX_DEBUG_SQLITE_TRACES") == "1") { + // To debug sqlite statements; trace all of them + sqlite3_trace(db, &traceSQL, nullptr); + } + exec("pragma foreign_keys = 1"); } From 29f0b196f44d273a5a85637168348c8a2e057049 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 17 Jan 2023 19:54:47 +0100 Subject: [PATCH 14/40] NarInfoDiskCache: Rename cacheExists -> upToDateCacheExists This is slightly more accurate considering that an outdated record may exist in the persistent cache. Possibly-outdated records are quite relevant as they may be foreign keys to more recent information that we want to keep, but we will not return them here. --- src/libstore/http-binary-cache-store.cc | 2 +- src/libstore/nar-info-disk-cache.cc | 2 +- src/libstore/nar-info-disk-cache.hh | 2 +- src/libstore/s3-binary-cache-store.cc | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libstore/http-binary-cache-store.cc b/src/libstore/http-binary-cache-store.cc index 73bcd6e81..1479822a9 100644 --- a/src/libstore/http-binary-cache-store.cc +++ b/src/libstore/http-binary-cache-store.cc @@ -56,7 +56,7 @@ public: void init() override { // FIXME: do this lazily? - if (auto cacheInfo = diskCache->cacheExists(cacheUri)) { + if (auto cacheInfo = diskCache->upToDateCacheExists(cacheUri)) { wantMassQuery.setDefault(cacheInfo->wantMassQuery); priority.setDefault(cacheInfo->priority); } else { diff --git a/src/libstore/nar-info-disk-cache.cc b/src/libstore/nar-info-disk-cache.cc index 3e0689534..883f364e5 100644 --- a/src/libstore/nar-info-disk-cache.cc +++ b/src/libstore/nar-info-disk-cache.cc @@ -200,7 +200,7 @@ public: }); } - std::optional cacheExists(const std::string & uri) override + std::optional upToDateCacheExists(const std::string & uri) override { return retrySQLite>([&]() -> std::optional { auto state(_state.lock()); diff --git a/src/libstore/nar-info-disk-cache.hh b/src/libstore/nar-info-disk-cache.hh index 2dcaa76a4..c185ca5e4 100644 --- a/src/libstore/nar-info-disk-cache.hh +++ b/src/libstore/nar-info-disk-cache.hh @@ -22,7 +22,7 @@ public: int priority; }; - virtual std::optional cacheExists(const std::string & uri) = 0; + virtual std::optional upToDateCacheExists(const std::string & uri) = 0; virtual std::pair> lookupNarInfo( const std::string & uri, const std::string & hashPart) = 0; diff --git a/src/libstore/s3-binary-cache-store.cc b/src/libstore/s3-binary-cache-store.cc index 844553ad3..8d76eee99 100644 --- a/src/libstore/s3-binary-cache-store.cc +++ b/src/libstore/s3-binary-cache-store.cc @@ -238,7 +238,7 @@ struct S3BinaryCacheStoreImpl : virtual S3BinaryCacheStoreConfig, public virtual void init() override { - if (auto cacheInfo = diskCache->cacheExists(getUri())) { + if (auto cacheInfo = diskCache->upToDateCacheExists(getUri())) { wantMassQuery.setDefault(cacheInfo->wantMassQuery); priority.setDefault(cacheInfo->priority); } else { From 79f62d2dda8603c1f2f471ce20557548db932296 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 30 Jan 2023 21:04:19 +0100 Subject: [PATCH 15/40] NarInfoDiskCacheImpl: Make dbPath a parameter This allows testing with a clean database. --- src/libstore/nar-info-disk-cache.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/libstore/nar-info-disk-cache.cc b/src/libstore/nar-info-disk-cache.cc index 883f364e5..c460100de 100644 --- a/src/libstore/nar-info-disk-cache.cc +++ b/src/libstore/nar-info-disk-cache.cc @@ -84,11 +84,10 @@ public: Sync _state; - NarInfoDiskCacheImpl() + NarInfoDiskCacheImpl(Path dbPath = getCacheDir() + "/nix/binary-cache-v6.sqlite") { auto state(_state.lock()); - Path dbPath = getCacheDir() + "/nix/binary-cache-v6.sqlite"; createDirs(dirOf(dbPath)); state->db = SQLite(dbPath); From 2ceece3ef384385d886f6aed5311d9b6dbbdd6dd Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 30 Jan 2023 22:15:23 +0100 Subject: [PATCH 16/40] NarInfoDiskCache: Prepare reproducer for #3898 --- src/libstore/nar-info-disk-cache.cc | 6 ++ src/libstore/nar-info-disk-cache.hh | 3 + src/libstore/tests/nar-info-disk-cache.cc | 87 +++++++++++++++++++++++ 3 files changed, 96 insertions(+) create mode 100644 src/libstore/tests/nar-info-disk-cache.cc diff --git a/src/libstore/nar-info-disk-cache.cc b/src/libstore/nar-info-disk-cache.cc index c460100de..494318af9 100644 --- a/src/libstore/nar-info-disk-cache.cc +++ b/src/libstore/nar-info-disk-cache.cc @@ -207,6 +207,7 @@ public: if (!cache) return std::nullopt; return CacheInfo { + .id = cache->id, .wantMassQuery = cache->wantMassQuery, .priority = cache->priority }; @@ -370,4 +371,9 @@ ref getNarInfoDiskCache() return cache; } +ref getTestNarInfoDiskCache(Path dbPath) +{ + return make_ref(dbPath); +} + } diff --git a/src/libstore/nar-info-disk-cache.hh b/src/libstore/nar-info-disk-cache.hh index c185ca5e4..adc14f3bc 100644 --- a/src/libstore/nar-info-disk-cache.hh +++ b/src/libstore/nar-info-disk-cache.hh @@ -18,6 +18,7 @@ public: struct CacheInfo { + int id; bool wantMassQuery; int priority; }; @@ -45,4 +46,6 @@ public: multiple threads. */ ref getNarInfoDiskCache(); +ref getTestNarInfoDiskCache(Path dbPath); + } diff --git a/src/libstore/tests/nar-info-disk-cache.cc b/src/libstore/tests/nar-info-disk-cache.cc new file mode 100644 index 000000000..1e9cbaa78 --- /dev/null +++ b/src/libstore/tests/nar-info-disk-cache.cc @@ -0,0 +1,87 @@ +#include "nar-info-disk-cache.hh" + +#include +#include +#include "sqlite.hh" +#include + + +namespace nix { + +RC_GTEST_PROP( + NarInfoDiskCacheImpl, + create_and_read, + (int prio, bool wantMassQuery) + ) +{ + Path tmpDir = createTempDir(); + AutoDelete delTmpDir(tmpDir); + Path dbPath(tmpDir + "/test-narinfo-disk-cache.sqlite"); + auto cache = getTestNarInfoDiskCache(dbPath); + + cache->createCache("other://uri", "/nix/storedir", wantMassQuery, prio); + cache->createCache("other://uri-2", "/nix/storedir", wantMassQuery, prio); + + cache->createCache("the://uri", "/nix/storedir", wantMassQuery, prio); + + { + auto r = cache->upToDateCacheExists("the://uri"); + ASSERT_TRUE(r); + ASSERT_EQ(r->priority, prio); + ASSERT_EQ(r->wantMassQuery, wantMassQuery); + } + + SQLite db(dbPath); + SQLiteStmt getIds; + getIds.create(db, "SELECT id FROM BinaryCaches WHERE url = 'the://uri'"); + + int savedId; + { + auto q(getIds.use()); + ASSERT_TRUE(q.next()); + savedId = q.getInt(0); + ASSERT_FALSE(q.next()); + } + + + db.exec("UPDATE BinaryCaches SET timestamp = timestamp - 1 - 7 * 24 * 3600;"); + + // Relies on memory cache + { + auto r = cache->upToDateCacheExists("the://uri"); + ASSERT_TRUE(r); + ASSERT_EQ(r->priority, prio); + ASSERT_EQ(r->wantMassQuery, wantMassQuery); + } + + auto cache2 = getTestNarInfoDiskCache(dbPath); + + { + auto r = cache2->upToDateCacheExists("the://uri"); + ASSERT_FALSE(r); + } + + cache2->createCache("the://uri", "/nix/storedir", wantMassQuery, prio); + + { + auto r = cache->upToDateCacheExists("the://uri"); + ASSERT_TRUE(r); + ASSERT_EQ(r->priority, prio); + ASSERT_EQ(r->wantMassQuery, wantMassQuery); + // FIXME, reproduces #3898 + // ASSERT_EQ(r->id, savedId); + (void) savedId; + } + + { + auto q(getIds.use()); + ASSERT_TRUE(q.next()); + auto currentId = q.getInt(0); + ASSERT_FALSE(q.next()); + // FIXME, reproduces #3898 + // ASSERT_EQ(currentId, savedId); + (void) currentId; + } +} + +} From fb94d5cabd51d57aa82a9857ab20f5f4bd323378 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 17 Jan 2023 19:56:06 +0100 Subject: [PATCH 17/40] NarInfoDiskCache: Keep BinaryCache.id stable and improve test Fixes #3898 The entire `BinaryCaches` row used to get replaced after it became stale according to the `timestamp` column. In a concurrent scenario, this leads to foreign key conflicts as different instances of the in-process `state.caches` cache now differ, with the consequence that the older process still tries to use the `id` number of the old record. Furthermore, this phenomenon appears to have caused the cache for actual narinfos to be erased about every week, while the default ttl for narinfos was supposed to be 30 days. --- src/libstore/nar-info-disk-cache.cc | 38 ++++++++++--- src/libstore/nar-info-disk-cache.hh | 2 +- src/libstore/tests/nar-info-disk-cache.cc | 69 ++++++++++++++--------- 3 files changed, 73 insertions(+), 36 deletions(-) diff --git a/src/libstore/nar-info-disk-cache.cc b/src/libstore/nar-info-disk-cache.cc index 494318af9..2645f468b 100644 --- a/src/libstore/nar-info-disk-cache.cc +++ b/src/libstore/nar-info-disk-cache.cc @@ -97,7 +97,7 @@ public: state->db.exec(schema); state->insertCache.create(state->db, - "insert or replace into BinaryCaches(url, timestamp, storeDir, wantMassQuery, priority) values (?, ?, ?, ?, ?)"); + "insert into BinaryCaches(url, timestamp, storeDir, wantMassQuery, priority) values (?1, ?2, ?3, ?4, ?5) on conflict (url) do update set timestamp = ?2, storeDir = ?3, wantMassQuery = ?4, priority = ?5 returning id;"); state->queryCache.create(state->db, "select id, storeDir, wantMassQuery, priority from BinaryCaches where url = ? and timestamp > ?"); @@ -165,6 +165,8 @@ public: return i->second; } +private: + std::optional queryCacheRaw(State & state, const std::string & uri) { auto i = state.caches.find(uri); @@ -172,15 +174,21 @@ public: auto queryCache(state.queryCache.use()(uri)(time(0) - cacheInfoTtl)); if (!queryCache.next()) return std::nullopt; - state.caches.emplace(uri, - Cache{(int) queryCache.getInt(0), queryCache.getStr(1), queryCache.getInt(2) != 0, (int) queryCache.getInt(3)}); + auto cache = Cache { + .id = (int) queryCache.getInt(0), + .storeDir = queryCache.getStr(1), + .wantMassQuery = queryCache.getInt(2) != 0, + .priority = (int) queryCache.getInt(3), + }; + state.caches.emplace(uri, cache); } return getCache(state, uri); } - void createCache(const std::string & uri, const Path & storeDir, bool wantMassQuery, int priority) override +public: + int createCache(const std::string & uri, const Path & storeDir, bool wantMassQuery, int priority) override { - retrySQLite([&]() { + return retrySQLite([&]() { auto state(_state.lock()); SQLiteTxn txn(state->db); @@ -189,13 +197,25 @@ public: auto cache(queryCacheRaw(*state, uri)); if (cache) - return; + return cache->id; - state->insertCache.use()(uri)(time(0))(storeDir)(wantMassQuery)(priority).exec(); - assert(sqlite3_changes(state->db) == 1); - state->caches[uri] = Cache{(int) sqlite3_last_insert_rowid(state->db), storeDir, wantMassQuery, priority}; + Cache ret { + .id = -1, // set below + .storeDir = storeDir, + .wantMassQuery = wantMassQuery, + .priority = priority, + }; + + { + auto r(state->insertCache.use()(uri)(time(0))(storeDir)(wantMassQuery)(priority)); + assert(r.next()); + ret.id = (int) r.getInt(0); + } + + state->caches[uri] = ret; txn.commit(); + return ret.id; }); } diff --git a/src/libstore/nar-info-disk-cache.hh b/src/libstore/nar-info-disk-cache.hh index adc14f3bc..4877f56d8 100644 --- a/src/libstore/nar-info-disk-cache.hh +++ b/src/libstore/nar-info-disk-cache.hh @@ -13,7 +13,7 @@ public: virtual ~NarInfoDiskCache() { } - virtual void createCache(const std::string & uri, const Path & storeDir, + virtual int createCache(const std::string & uri, const Path & storeDir, bool wantMassQuery, int priority) = 0; struct CacheInfo diff --git a/src/libstore/tests/nar-info-disk-cache.cc b/src/libstore/tests/nar-info-disk-cache.cc index 1e9cbaa78..0597fdc41 100644 --- a/src/libstore/tests/nar-info-disk-cache.cc +++ b/src/libstore/tests/nar-info-disk-cache.cc @@ -8,69 +8,68 @@ namespace nix { -RC_GTEST_PROP( - NarInfoDiskCacheImpl, - create_and_read, - (int prio, bool wantMassQuery) - ) -{ +TEST(NarInfoDiskCacheImpl, create_and_read) { + int prio = 12345; + bool wantMassQuery = true; + Path tmpDir = createTempDir(); AutoDelete delTmpDir(tmpDir); Path dbPath(tmpDir + "/test-narinfo-disk-cache.sqlite"); auto cache = getTestNarInfoDiskCache(dbPath); - cache->createCache("other://uri", "/nix/storedir", wantMassQuery, prio); - cache->createCache("other://uri-2", "/nix/storedir", wantMassQuery, prio); - - cache->createCache("the://uri", "/nix/storedir", wantMassQuery, prio); - { - auto r = cache->upToDateCacheExists("the://uri"); + auto bc1 = cache->createCache("https://bar", "/nix/storedir", wantMassQuery, prio); + auto bc2 = cache->createCache("https://xyz", "/nix/storedir", false, 12); + ASSERT_NE(bc1, bc2); + } + + int savedId = cache->createCache("http://foo", "/nix/storedir", wantMassQuery, prio);; + { + auto r = cache->upToDateCacheExists("http://foo"); ASSERT_TRUE(r); ASSERT_EQ(r->priority, prio); ASSERT_EQ(r->wantMassQuery, wantMassQuery); + ASSERT_EQ(savedId, r->id); } SQLite db(dbPath); SQLiteStmt getIds; - getIds.create(db, "SELECT id FROM BinaryCaches WHERE url = 'the://uri'"); + getIds.create(db, "SELECT id FROM BinaryCaches WHERE url = 'http://foo'"); - int savedId; { auto q(getIds.use()); ASSERT_TRUE(q.next()); - savedId = q.getInt(0); + ASSERT_EQ(savedId, q.getInt(0)); ASSERT_FALSE(q.next()); } - db.exec("UPDATE BinaryCaches SET timestamp = timestamp - 1 - 7 * 24 * 3600;"); // Relies on memory cache { - auto r = cache->upToDateCacheExists("the://uri"); + auto r = cache->upToDateCacheExists("http://foo"); ASSERT_TRUE(r); ASSERT_EQ(r->priority, prio); ASSERT_EQ(r->wantMassQuery, wantMassQuery); } + // We can't clear the in-memory cache, so we use a new cache object. auto cache2 = getTestNarInfoDiskCache(dbPath); { - auto r = cache2->upToDateCacheExists("the://uri"); + auto r = cache2->upToDateCacheExists("http://foo"); ASSERT_FALSE(r); } - cache2->createCache("the://uri", "/nix/storedir", wantMassQuery, prio); + // Update, same data, check that the id number is reused + cache2->createCache("http://foo", "/nix/storedir", wantMassQuery, prio); { - auto r = cache->upToDateCacheExists("the://uri"); + auto r = cache2->upToDateCacheExists("http://foo"); ASSERT_TRUE(r); ASSERT_EQ(r->priority, prio); ASSERT_EQ(r->wantMassQuery, wantMassQuery); - // FIXME, reproduces #3898 - // ASSERT_EQ(r->id, savedId); - (void) savedId; + ASSERT_EQ(r->id, savedId); } { @@ -78,10 +77,28 @@ RC_GTEST_PROP( ASSERT_TRUE(q.next()); auto currentId = q.getInt(0); ASSERT_FALSE(q.next()); - // FIXME, reproduces #3898 - // ASSERT_EQ(currentId, savedId); - (void) currentId; + ASSERT_EQ(currentId, savedId); } + + // Check that the fields can be modified + { + auto r0 = cache2->upToDateCacheExists("https://bar"); + ASSERT_FALSE(r0); + + cache2->createCache("https://bar", "/nix/storedir", !wantMassQuery, prio + 10); + auto r = cache2->upToDateCacheExists("https://bar"); + ASSERT_EQ(r->wantMassQuery, !wantMassQuery); + ASSERT_EQ(r->priority, prio + 10); + } + + // // Force update (no use case yet; we only retrieve cache metadata when stale based on timestamp) + // { + // cache2->createCache("https://bar", "/nix/storedir", wantMassQuery, prio + 20); + // auto r = cache2->upToDateCacheExists("https://bar"); + // ASSERT_EQ(r->wantMassQuery, wantMassQuery); + // ASSERT_EQ(r->priority, prio + 20); + // } + } } From 19b495a48a45ac4f04d9b362c0bbaa6fc122c404 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 31 Jan 2023 15:46:54 +0100 Subject: [PATCH 18/40] NarInfoDiskCache: Also test id consistency with updated fields And clarify test --- src/libstore/tests/nar-info-disk-cache.cc | 159 ++++++++++++---------- 1 file changed, 89 insertions(+), 70 deletions(-) diff --git a/src/libstore/tests/nar-info-disk-cache.cc b/src/libstore/tests/nar-info-disk-cache.cc index 0597fdc41..b4bdb8329 100644 --- a/src/libstore/tests/nar-info-disk-cache.cc +++ b/src/libstore/tests/nar-info-disk-cache.cc @@ -9,96 +9,115 @@ namespace nix { TEST(NarInfoDiskCacheImpl, create_and_read) { + // This is a large single test to avoid some setup overhead. + int prio = 12345; bool wantMassQuery = true; Path tmpDir = createTempDir(); AutoDelete delTmpDir(tmpDir); Path dbPath(tmpDir + "/test-narinfo-disk-cache.sqlite"); - auto cache = getTestNarInfoDiskCache(dbPath); - { - auto bc1 = cache->createCache("https://bar", "/nix/storedir", wantMassQuery, prio); - auto bc2 = cache->createCache("https://xyz", "/nix/storedir", false, 12); - ASSERT_NE(bc1, bc2); - } - - int savedId = cache->createCache("http://foo", "/nix/storedir", wantMassQuery, prio);; - { - auto r = cache->upToDateCacheExists("http://foo"); - ASSERT_TRUE(r); - ASSERT_EQ(r->priority, prio); - ASSERT_EQ(r->wantMassQuery, wantMassQuery); - ASSERT_EQ(savedId, r->id); - } - - SQLite db(dbPath); + int savedId; + int barId; + SQLite db; SQLiteStmt getIds; - getIds.create(db, "SELECT id FROM BinaryCaches WHERE url = 'http://foo'"); { - auto q(getIds.use()); - ASSERT_TRUE(q.next()); - ASSERT_EQ(savedId, q.getInt(0)); - ASSERT_FALSE(q.next()); - } + auto cache = getTestNarInfoDiskCache(dbPath); - db.exec("UPDATE BinaryCaches SET timestamp = timestamp - 1 - 7 * 24 * 3600;"); + // Set up "background noise" and check that different caches receive different ids + { + auto bc1 = cache->createCache("https://bar", "/nix/storedir", wantMassQuery, prio); + auto bc2 = cache->createCache("https://xyz", "/nix/storedir", false, 12); + ASSERT_NE(bc1, bc2); + barId = bc1; + } - // Relies on memory cache - { - auto r = cache->upToDateCacheExists("http://foo"); - ASSERT_TRUE(r); - ASSERT_EQ(r->priority, prio); - ASSERT_EQ(r->wantMassQuery, wantMassQuery); - } + // Check that the fields are saved and returned correctly. This does not test + // the select statement yet, because of in-memory caching. + savedId = cache->createCache("http://foo", "/nix/storedir", wantMassQuery, prio);; + { + auto r = cache->upToDateCacheExists("http://foo"); + ASSERT_TRUE(r); + ASSERT_EQ(r->priority, prio); + ASSERT_EQ(r->wantMassQuery, wantMassQuery); + ASSERT_EQ(savedId, r->id); + } - // We can't clear the in-memory cache, so we use a new cache object. - auto cache2 = getTestNarInfoDiskCache(dbPath); + // We're going to pay special attention to the id field because we had a bug + // that changed it. + db = SQLite(dbPath); + getIds.create(db, "select id from BinaryCaches where url = 'http://foo'"); - { - auto r = cache2->upToDateCacheExists("http://foo"); - ASSERT_FALSE(r); - } + { + auto q(getIds.use()); + ASSERT_TRUE(q.next()); + ASSERT_EQ(savedId, q.getInt(0)); + ASSERT_FALSE(q.next()); + } - // Update, same data, check that the id number is reused - cache2->createCache("http://foo", "/nix/storedir", wantMassQuery, prio); + // Pretend that the caches are older, but keep one up to date, as "background noise" + db.exec("update BinaryCaches set timestamp = timestamp - 1 - 7 * 24 * 3600 where url <> 'https://xyz';"); - { - auto r = cache2->upToDateCacheExists("http://foo"); - ASSERT_TRUE(r); - ASSERT_EQ(r->priority, prio); - ASSERT_EQ(r->wantMassQuery, wantMassQuery); - ASSERT_EQ(r->id, savedId); + // This shows that the in-memory cache works + { + auto r = cache->upToDateCacheExists("http://foo"); + ASSERT_TRUE(r); + ASSERT_EQ(r->priority, prio); + ASSERT_EQ(r->wantMassQuery, wantMassQuery); + } } { - auto q(getIds.use()); - ASSERT_TRUE(q.next()); - auto currentId = q.getInt(0); - ASSERT_FALSE(q.next()); - ASSERT_EQ(currentId, savedId); + // We can't clear the in-memory cache, so we use a new cache object. This is + // more realistic anyway. + auto cache2 = getTestNarInfoDiskCache(dbPath); + + { + auto r = cache2->upToDateCacheExists("http://foo"); + ASSERT_FALSE(r); + } + + // "Update", same data, check that the id number is reused + cache2->createCache("http://foo", "/nix/storedir", wantMassQuery, prio); + + { + auto r = cache2->upToDateCacheExists("http://foo"); + ASSERT_TRUE(r); + ASSERT_EQ(r->priority, prio); + ASSERT_EQ(r->wantMassQuery, wantMassQuery); + ASSERT_EQ(r->id, savedId); + } + + { + auto q(getIds.use()); + ASSERT_TRUE(q.next()); + auto currentId = q.getInt(0); + ASSERT_FALSE(q.next()); + ASSERT_EQ(currentId, savedId); + } + + // Check that the fields can be modified, and the id remains the same + { + auto r0 = cache2->upToDateCacheExists("https://bar"); + ASSERT_FALSE(r0); + + cache2->createCache("https://bar", "/nix/storedir", !wantMassQuery, prio + 10); + auto r = cache2->upToDateCacheExists("https://bar"); + ASSERT_EQ(r->wantMassQuery, !wantMassQuery); + ASSERT_EQ(r->priority, prio + 10); + ASSERT_EQ(r->id, barId); + } + + // // Force update (no use case yet; we only retrieve cache metadata when stale based on timestamp) + // { + // cache2->createCache("https://bar", "/nix/storedir", wantMassQuery, prio + 20); + // auto r = cache2->upToDateCacheExists("https://bar"); + // ASSERT_EQ(r->wantMassQuery, wantMassQuery); + // ASSERT_EQ(r->priority, prio + 20); + // } } - - // Check that the fields can be modified - { - auto r0 = cache2->upToDateCacheExists("https://bar"); - ASSERT_FALSE(r0); - - cache2->createCache("https://bar", "/nix/storedir", !wantMassQuery, prio + 10); - auto r = cache2->upToDateCacheExists("https://bar"); - ASSERT_EQ(r->wantMassQuery, !wantMassQuery); - ASSERT_EQ(r->priority, prio + 10); - } - - // // Force update (no use case yet; we only retrieve cache metadata when stale based on timestamp) - // { - // cache2->createCache("https://bar", "/nix/storedir", wantMassQuery, prio + 20); - // auto r = cache2->upToDateCacheExists("https://bar"); - // ASSERT_EQ(r->wantMassQuery, wantMassQuery); - // ASSERT_EQ(r->priority, prio + 20); - // } - } } From 15313bfdb7139d0fef53a5845dfdd7be90bd68c8 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 9 Feb 2023 16:42:14 +0100 Subject: [PATCH 19/40] Fix activity message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Josef Kemetmüller --- src/libfetchers/git.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc index 3871cf0dd..1fafc7df8 100644 --- a/src/libfetchers/git.cc +++ b/src/libfetchers/git.cc @@ -648,7 +648,7 @@ struct GitInputScheme : InputScheme } { - Activity act(*logger, lvlTalkative, actUnknown, fmt("fetching submodules of '%s'", repoDir)); + Activity act(*logger, lvlTalkative, actUnknown, fmt("fetching submodules of '%s'", actualUrl)); runProgram("git", true, { "-C", tmpDir, "submodule", "--quiet", "update", "--init", "--recursive" }); } From 862e56c23d9ee0dafc1902f27b20f58ccf421313 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 9 Feb 2023 16:42:45 +0100 Subject: [PATCH 20/40] Improve comment Co-authored-by: Robert Hensing --- src/libfetchers/git.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc index 1fafc7df8..309a143f5 100644 --- a/src/libfetchers/git.cc +++ b/src/libfetchers/git.cc @@ -634,7 +634,7 @@ struct GitInputScheme : InputScheme writeFile(tmpGitDir + "/config", readFile(repoDir + "/" + gitDir + "/config")); /* Restore the config.bare setting we may have just - nuked. */ + copied erroneously from the user's repo. */ runProgram("git", true, { "-C", tmpDir, "config", "core.bare", "false" }); } else runProgram("git", true, { "-C", tmpDir, "config", "remote.origin.url", actualUrl }); From 9813e54a74eb9c12494d1870dc660fd1cf2efd1b Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Thu, 9 Feb 2023 22:14:53 +0100 Subject: [PATCH 21/40] tests: Add command source locations to test log --- tests/common.sh.in | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/common.sh.in b/tests/common.sh.in index 74bbbc8ca..e72a8e12e 100644 --- a/tests/common.sh.in +++ b/tests/common.sh.in @@ -4,6 +4,8 @@ if [[ -z "$COMMON_SH_SOURCED" ]]; then COMMON_SH_SOURCED=1 +export PS4='+(${BASH_SOURCE[0]}:$LINENO) ' + export TEST_ROOT=$(realpath ${TMPDIR:-/tmp}/nix-test)/${TEST_NAME:-default} export NIX_STORE_DIR if ! NIX_STORE_DIR=$(readlink -f $TEST_ROOT/store 2> /dev/null); then From a0f1cb0ce7e82de8490645dde240f71d71d9225e Mon Sep 17 00:00:00 2001 From: Pico Geyer Date: Fri, 10 Feb 2023 11:55:22 +0200 Subject: [PATCH 22/40] Fix minor syntax issue in the one of the examples. Attribute set expressions need to end with a ; --- doc/manual/src/language/index.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/manual/src/language/index.md b/doc/manual/src/language/index.md index 31300631c..3eabe1a02 100644 --- a/doc/manual/src/language/index.md +++ b/doc/manual/src/language/index.md @@ -91,7 +91,7 @@ This is an incomplete overview of language features, by example. - `"hello ${ { a = "world" }.a }"` + `"hello ${ { a = "world"; }.a }"` `"1 2 ${toString 3}"` From 7908a416310bb6a1a9c68361985d0bb7429d3569 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 10 Feb 2023 13:03:24 +0100 Subject: [PATCH 23/40] tests/authorization: Simplify assertion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Théophane Hufschmitt <7226587+thufschmitt@users.noreply.github.com> --- tests/nixos/authorization.nix | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/nixos/authorization.nix b/tests/nixos/authorization.nix index 4b13ec65e..7e8744dd9 100644 --- a/tests/nixos/authorization.nix +++ b/tests/nixos/authorization.nix @@ -51,8 +51,8 @@ set -x cd ~ echo 5mgtDj0ohrWkT50TLR0f4tIIxY > four; - (! diff $(nix store add-file four) four 2>&1) | grep -F "cannot open connection to remote store" - (! diff $(nix store add-file four) four 2>&1) | grep -F "Connection reset by peer" + (! nix store add-file four 2>&1) | grep -F "cannot open connection to remote store" + (! nix store add-file four 2>&1) | grep -F "Connection reset by peer" ! [[ -e ${pathFour} ]] ' 1>&2 """) From f094ba7386fc5fbb3df5fd84008ca07d2289ff26 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 10 Feb 2023 14:38:14 +0100 Subject: [PATCH 24/40] Simplify the PID namespace check: just try to mount /proc Fixes #7783. --- src/libstore/build/local-derivation-goal.cc | 2 +- src/libutil/namespaces.cc | 81 ++++++++++----------- src/libutil/namespaces.hh | 4 +- src/libutil/util.cc | 29 +++++++- src/libutil/util.hh | 1 + 5 files changed, 69 insertions(+), 48 deletions(-) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index e1cc504f8..e5ba3ac0d 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -209,7 +209,7 @@ void LocalDerivationGoal::tryLocalBuild() #if __linux__ if (useChroot) { - if (!mountNamespacesSupported() || !pidNamespacesSupported()) { + if (!mountAndPidNamespacesSupported()) { if (!settings.sandboxFallback) throw Error("this system does not support the kernel namespaces that are required for sandboxing; use '--no-sandbox' to disable sandboxing"); debug("auto-disabling sandboxing because the prerequisite namespaces are not available"); diff --git a/src/libutil/namespaces.cc b/src/libutil/namespaces.cc index fdd52d92b..f66accb10 100644 --- a/src/libutil/namespaces.cc +++ b/src/libutil/namespaces.cc @@ -4,7 +4,7 @@ #include "util.hh" #include "finally.hh" -#include +#include namespace nix { @@ -33,63 +33,60 @@ bool userNamespacesSupported() return false; } - Pid pid = startProcess([&]() - { - auto res = unshare(CLONE_NEWUSER); - _exit(res ? 1 : 0); - }); + try { + Pid pid = startProcess([&]() + { + _exit(0); + }, { + .cloneFlags = CLONE_NEWUSER + }); - bool supported = pid.wait() == 0; + auto r = pid.wait(); + assert(!r); + } catch (SysError & e) { + debug("user namespaces do not work on this system: %s", e.msg()); + return false; + } - if (!supported) - debug("user namespaces do not work on this system"); - - return supported; + return true; }(); return res; } -bool mountNamespacesSupported() +bool mountAndPidNamespacesSupported() { static auto res = [&]() -> bool { - bool useUserNamespace = userNamespacesSupported(); + try { - Pid pid = startProcess([&]() - { - auto res = unshare(CLONE_NEWNS | (useUserNamespace ? CLONE_NEWUSER : 0)); - _exit(res ? 1 : 0); - }); + Pid pid = startProcess([&]() + { + /* Make sure we don't remount the parent's /proc. */ + if (mount(0, "/", 0, MS_PRIVATE | MS_REC, 0) == -1) + _exit(1); - bool supported = pid.wait() == 0; + /* Test whether we can remount /proc. The kernel disallows + this if /proc is not fully visible, i.e. if there are + filesystems mounted on top of files inside /proc. See + https://lore.kernel.org/lkml/87tvsrjai0.fsf@xmission.com/T/. */ + if (mount("none", "/proc", "proc", 0, 0) == -1) + _exit(2); - if (!supported) - debug("mount namespaces do not work on this system"); + _exit(0); + }, { + .cloneFlags = CLONE_NEWNS | CLONE_NEWPID | (userNamespacesSupported() ? CLONE_NEWUSER : 0) + }); - return supported; - }(); - return res; -} - -bool pidNamespacesSupported() -{ - static auto res = [&]() -> bool - { - /* Check whether /proc is fully visible, i.e. there are no - filesystems mounted on top of files inside /proc. If this - is not the case, then we cannot mount a new /proc inside - the sandbox that matches the sandbox's PID namespace. - See https://lore.kernel.org/lkml/87tvsrjai0.fsf@xmission.com/T/. */ - auto fp = fopen("/proc/mounts", "r"); - if (!fp) return false; - Finally delFP = [&]() { fclose(fp); }; - - while (auto ent = getmntent(fp)) - if (hasPrefix(std::string_view(ent->mnt_dir), "/proc/")) { - debug("PID namespaces do not work because /proc is not fully visible; disabling sandboxing"); + if (pid.wait()) { + debug("PID namespaces do not work on this system: cannot remount /proc"); return false; } + } catch (SysError & e) { + debug("mount namespaces do not work on this system: %s", e.msg()); + return false; + } + return true; }(); return res; diff --git a/src/libutil/namespaces.hh b/src/libutil/namespaces.hh index 34e54d5ad..e82379b9c 100644 --- a/src/libutil/namespaces.hh +++ b/src/libutil/namespaces.hh @@ -6,9 +6,7 @@ namespace nix { bool userNamespacesSupported(); -bool mountNamespacesSupported(); - -bool pidNamespacesSupported(); +bool mountAndPidNamespacesSupported(); #endif diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 40faa4bf2..94da37561 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -36,6 +36,7 @@ #ifdef __linux__ #include #include +#include #include #endif @@ -1051,9 +1052,17 @@ static pid_t doFork(bool allowVfork, std::function fun) } +static int childEntry(void * arg) +{ + auto main = (std::function *) arg; + (*main)(); + return 1; +} + + pid_t startProcess(std::function fun, const ProcessOptions & options) { - auto wrapper = [&]() { + std::function wrapper = [&]() { if (!options.allowVfork) logger = makeSimpleLogger(); try { @@ -1073,7 +1082,23 @@ pid_t startProcess(std::function fun, const ProcessOptions & options) _exit(1); }; - pid_t pid = doFork(options.allowVfork, wrapper); + pid_t pid = -1; + + if (options.cloneFlags) { + // Not supported, since then we don't know when to free the stack. + assert(!(options.cloneFlags & CLONE_VM)); + + size_t stackSize = 1 * 1024 * 1024; + auto stack = (char *) mmap(0, stackSize, + PROT_WRITE | PROT_READ, MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0); + if (stack == MAP_FAILED) throw SysError("allocating stack"); + + Finally freeStack([&]() { munmap(stack, stackSize); }); + + pid = clone(childEntry, stack + stackSize, options.cloneFlags | SIGCHLD, &wrapper); + } else + pid = doFork(options.allowVfork, wrapper); + if (pid == -1) throw SysError("unable to fork"); return pid; diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 266da0ae3..95562280e 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -301,6 +301,7 @@ struct ProcessOptions bool dieWithParent = true; bool runExitHandlers = false; bool allowVfork = false; + int cloneFlags = 0; // use clone() with the specified flags (Linux only) }; pid_t startProcess(std::function fun, const ProcessOptions & options = ProcessOptions()); From 37b1e93f4be2348ce343bca4a609732d04b62a7b Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 10 Feb 2023 14:35:04 +0100 Subject: [PATCH 25/40] daemon.cc: Rename UserSettings -> AuthorizationSettings This is a bit more accurate. It's a private name, but before you know it, someone might make it public! --- src/nix/daemon.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/nix/daemon.cc b/src/nix/daemon.cc index 2ba56ee26..a22bccba1 100644 --- a/src/nix/daemon.cc +++ b/src/nix/daemon.cc @@ -34,7 +34,7 @@ using namespace nix; using namespace nix::daemon; -struct UserSettings : Config { +struct AuthorizationSettings : Config { Setting trustedUsers{ this, {"root"}, "trusted-users", @@ -67,9 +67,9 @@ struct UserSettings : Config { )"}; }; -UserSettings userSettings; +AuthorizationSettings authorizationSettings; -static GlobalConfig::Register rSettings(&userSettings); +static GlobalConfig::Register rSettings(&authorizationSettings); #ifndef __linux__ #define SPLICE_F_MOVE 0 @@ -240,8 +240,8 @@ static void daemonLoop() struct group * gr = peer.gidKnown ? getgrgid(peer.gid) : 0; std::string group = gr ? gr->gr_name : std::to_string(peer.gid); - Strings trustedUsers = userSettings.trustedUsers; - Strings allowedUsers = userSettings.allowedUsers; + Strings trustedUsers = authorizationSettings.trustedUsers; + Strings allowedUsers = authorizationSettings.allowedUsers; if (matchUser(user, group, trustedUsers)) trusted = Trusted; From 3e6e34cdf589d67c767bffe19483a6c9e53233a7 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 10 Feb 2023 14:44:25 +0100 Subject: [PATCH 26/40] LocalDerivationGoal::startBuilder(): Use startProcess() to clone --- src/libstore/build/local-derivation-goal.cc | 22 +++++---------------- 1 file changed, 5 insertions(+), 17 deletions(-) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index e5ba3ac0d..7c4892c96 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -385,12 +385,6 @@ void LocalDerivationGoal::cleanupPostOutputsRegisteredModeNonCheck() } -int childEntry(void * arg) -{ - ((LocalDerivationGoal *) arg)->runChild(); - return 1; -} - #if __linux__ static void linkOrCopy(const Path & from, const Path & to) { @@ -916,21 +910,15 @@ void LocalDerivationGoal::startBuilder() if (getuid() == 0 && setgroups(0, 0) == -1) throw SysError("setgroups failed"); - size_t stackSize = 1 * 1024 * 1024; - char * stack = (char *) mmap(0, stackSize, - PROT_WRITE | PROT_READ, MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0); - if (stack == MAP_FAILED) throw SysError("allocating stack"); - - int flags = CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS | CLONE_PARENT | SIGCHLD; + ProcessOptions options; + options.cloneFlags = CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS | CLONE_PARENT | SIGCHLD; if (privateNetwork) - flags |= CLONE_NEWNET; + options.cloneFlags |= CLONE_NEWNET; if (usingUserNamespace) - flags |= CLONE_NEWUSER; + options.cloneFlags |= CLONE_NEWUSER; - pid_t child = clone(childEntry, stack + stackSize, flags, this); + pid_t child = startProcess([&]() { runChild(); }, options); - if (child == -1) - throw SysError("creating sandboxed builder process using clone()"); writeFull(builderOut.writeSide.get(), fmt("%d %d\n", usingUserNamespace, child)); _exit(0); From c49b7472eaa7a6501aa937b40f80702ecf0f43e6 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 10 Feb 2023 16:32:30 +0100 Subject: [PATCH 27/40] Fix macOS build --- src/libutil/util.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 94da37561..795b66dc3 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -1085,6 +1085,7 @@ pid_t startProcess(std::function fun, const ProcessOptions & options) pid_t pid = -1; if (options.cloneFlags) { + #ifdef __linux__ // Not supported, since then we don't know when to free the stack. assert(!(options.cloneFlags & CLONE_VM)); @@ -1096,6 +1097,9 @@ pid_t startProcess(std::function fun, const ProcessOptions & options) Finally freeStack([&]() { munmap(stack, stackSize); }); pid = clone(childEntry, stack + stackSize, options.cloneFlags | SIGCHLD, &wrapper); + #else + throw Error("clone flags are only supported on Linux"); + #endif } else pid = doFork(options.allowVfork, wrapper); From 2384d360839e27edb3c928da858ec911415c8b4d Mon Sep 17 00:00:00 2001 From: Alexander Bantyev Date: Wed, 17 Nov 2021 23:35:21 +0300 Subject: [PATCH 28/40] A setting to follow XDG Base Directory standard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit XDG Base Directory is a standard for locations for storing various files. Nix has a few files which seem to fit in the standard, but currently use a custom location directly in the user's ~, polluting it: - ~/.nix-profile - ~/.nix-defexpr - ~/.nix-channels This commit adds a config option (use-xdg-base-directories) to follow the XDG spec and instead use the following locations: - $XDG_STATE_HOME/nix/profile - $XDG_STATE_HOME/nix/defexpr - $XDG_STATE_HOME/nix/channels If $XDG_STATE_HOME is not set, it is assumed to be ~/.local/state. Co-authored-by: Théophane Hufschmitt <7226587+thufschmitt@users.noreply.github.com> Co-authored-by: Tim Fenney Co-authored-by: pasqui23 Co-authored-by: Artturin Co-authored-by: John Ericson --- doc/manual/src/command-ref/env-common.md | 13 ++++++++++ scripts/install-multi-user.sh | 2 +- scripts/install-nix-from-closure.sh | 8 +++--- scripts/nix-profile-daemon.sh.in | 31 ++++++++++++++++++++++-- scripts/nix-profile.sh.in | 30 ++++++++++++++++++++--- src/libexpr/eval.cc | 2 +- src/libstore/globals.hh | 21 ++++++++++++++++ src/libstore/profiles.cc | 4 +-- src/libstore/profiles.hh | 5 ++-- src/libutil/util.cc | 13 ++++++++++ src/libutil/util.hh | 6 +++++ src/nix-channel/nix-channel.cc | 4 +-- src/nix-env/nix-env.cc | 7 ++++-- tests/common.sh.in | 6 +++-- tests/nix-channel.sh | 13 ++++++++++ tests/nix-profile.sh | 8 ++++++ 16 files changed, 153 insertions(+), 20 deletions(-) diff --git a/doc/manual/src/command-ref/env-common.md b/doc/manual/src/command-ref/env-common.md index bb85a6b07..c5d38db47 100644 --- a/doc/manual/src/command-ref/env-common.md +++ b/doc/manual/src/command-ref/env-common.md @@ -91,3 +91,16 @@ Most Nix commands interpret the following environment variables: variable sets the initial size of the heap in bytes. It defaults to 384 MiB. Setting it to a low value reduces memory consumption, but will increase runtime due to the overhead of garbage collection. + +## XDG Base Directory + +New Nix commands conform to the [XDG Base Directory Specification], and use the following environment variables to determine locations of various state and configuration files: + +- [`XDG_CONFIG_HOME`]{#env-XDG_CONFIG_HOME} (default `~/.config`) +- [`XDG_STATE_HOME`]{#env-XDG_STATE_HOME} (default `~/.local/state`) +- [`XDG_CACHE_HOME`]{#env-XDG_CACHE_HOME} (default `~/.cache`) + +Classic Nix commands can also be made to follow this standard using the [`use-xdg-base-directories`] configuration option. + +[XDG Base Directory Specification]: https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html +[`use-xdg-base-directories`]: ../command-ref/conf-file.md#conf-use-xdg-base-directories \ No newline at end of file diff --git a/scripts/install-multi-user.sh b/scripts/install-multi-user.sh index f149ea0d7..7c66538b0 100644 --- a/scripts/install-multi-user.sh +++ b/scripts/install-multi-user.sh @@ -136,7 +136,7 @@ EOF cat <&2 exit 1 @@ -196,7 +198,7 @@ fi # Install an SSL certificate bundle. if [ -z "$NIX_SSL_CERT_FILE" ] || ! [ -f "$NIX_SSL_CERT_FILE" ]; then "$nix/bin/nix-env" -i "$cacert" - export NIX_SSL_CERT_FILE="$HOME/.nix-profile/etc/ssl/certs/ca-bundle.crt" + export NIX_SSL_CERT_FILE="$NIX_LINK/etc/ssl/certs/ca-bundle.crt" fi # Subscribe the user to the Nixpkgs channel and fetch it. @@ -214,8 +216,8 @@ fi added= p= -p_sh=$HOME/.nix-profile/etc/profile.d/nix.sh -p_fish=$HOME/.nix-profile/etc/profile.d/nix.fish +p_sh=$NIX_LINK/etc/profile.d/nix.sh +p_fish=$NIX_LINK/etc/profile.d/nix.fish if [ -z "$NIX_INSTALLER_NO_MODIFY_PROFILE" ]; then # Make the shell source nix.sh during login. for i in .bash_profile .bash_login .profile; do diff --git a/scripts/nix-profile-daemon.sh.in b/scripts/nix-profile-daemon.sh.in index 0a47571ac..235536c65 100644 --- a/scripts/nix-profile-daemon.sh.in +++ b/scripts/nix-profile-daemon.sh.in @@ -2,7 +2,33 @@ if [ -n "${__ETC_PROFILE_NIX_SOURCED:-}" ]; then return; fi __ETC_PROFILE_NIX_SOURCED=1 -export NIX_PROFILES="@localstatedir@/nix/profiles/default $HOME/.nix-profile" +NIX_LINK=$HOME/.nix-profile +if [ -n "$XDG_STATE_HOME" ]; then + NIX_LINK_NEW="$XDG_STATE_HOME/nix/profile" +else + NIX_LINK_NEW=$HOME/.local/state/nix/profile +fi +if ! [ -e "$NIX_LINK" ]; then + NIX_LINK="$NIX_LINK_NEW" +else + if [ -t 2 ] && [ -e "$NIX_LINK_NEW" ]; then + warning="\033[1;35mwarning:\033[0m" + printf "$warning Both %s and legacy %s exist; using the latter.\n" "$NIX_LINK_NEW" "$NIX_LINK" 1>&2 + if [ "$(realpath "$NIX_LINK")" = "$(realpath "$NIX_LINK_NEW")" ]; then + printf " Since the profiles match, you can safely delete either of them.\n" 1>&2 + else + # This should be an exceptionally rare occasion: the only way to get it would be to + # 1. Update to newer Nix; + # 2. Remove .nix-profile; + # 3. Set the $NIX_LINK_NEW to something other than the default user profile; + # 4. Roll back to older Nix. + # If someone did all that, they can probably figure out how to migrate the profile. + printf "$warning Profiles do not match. You should manually migrate from %s to %s.\n" "$NIX_LINK" "$NIX_LINK_NEW" 1>&2 + fi + fi +fi + +export NIX_PROFILES="@localstatedir@/nix/profiles/default $NIX_LINK" # Set $NIX_SSL_CERT_FILE so that Nixpkgs applications like curl work. if [ -n "${NIX_SSL_CERT_FILE:-}" ]; then @@ -34,4 +60,5 @@ else unset -f check_nix_profiles fi -export PATH="$HOME/.nix-profile/bin:@localstatedir@/nix/profiles/default/bin:$PATH" +export PATH="$NIX_LINK/bin:@localstatedir@/nix/profiles/default/bin:$PATH" +unset NIX_LINK diff --git a/scripts/nix-profile.sh.in b/scripts/nix-profile.sh.in index 5636085d4..264d9a8e2 100644 --- a/scripts/nix-profile.sh.in +++ b/scripts/nix-profile.sh.in @@ -2,11 +2,35 @@ if [ -n "$HOME" ] && [ -n "$USER" ]; then # Set up the per-user profile. - NIX_LINK=$HOME/.nix-profile + NIX_LINK="$HOME/.nix-profile" + if [ -n "$XDG_STATE_HOME" ]; then + NIX_LINK_NEW="$XDG_STATE_HOME/nix/profile" + else + NIX_LINK_NEW="$HOME/.local/state/nix/profile" + fi + if ! [ -e "$NIX_LINK" ]; then + NIX_LINK="$NIX_LINK_NEW" + else + if [ -t 2 ] && [ -e "$NIX_LINK_NEW" ]; then + warning="\033[1;35mwarning:\033[0m" + printf "$warning Both %s and legacy %s exist; using the latter.\n" "$NIX_LINK_NEW" "$NIX_LINK" 1>&2 + if [ "$(realpath "$NIX_LINK")" = "$(realpath "$NIX_LINK_NEW")" ]; then + printf " Since the profiles match, you can safely delete either of them.\n" 1>&2 + else + # This should be an exceptionally rare occasion: the only way to get it would be to + # 1. Update to newer Nix; + # 2. Remove .nix-profile; + # 3. Set the $NIX_LINK_NEW to something other than the default user profile; + # 4. Roll back to older Nix. + # If someone did all that, they can probably figure out how to migrate the profile. + printf "$warning Profiles do not match. You should manually migrate from %s to %s.\n" "$NIX_LINK" "$NIX_LINK_NEW" 1>&2 + fi + fi + fi # Set up environment. # This part should be kept in sync with nixpkgs:nixos/modules/programs/environment.nix - export NIX_PROFILES="@localstatedir@/nix/profiles/default $HOME/.nix-profile" + export NIX_PROFILES="@localstatedir@/nix/profiles/default $NIX_LINK" # Set $NIX_SSL_CERT_FILE so that Nixpkgs applications like curl work. if [ -e /etc/ssl/certs/ca-certificates.crt ]; then # NixOS, Ubuntu, Debian, Gentoo, Arch @@ -31,5 +55,5 @@ if [ -n "$HOME" ] && [ -n "$USER" ]; then fi export PATH="$NIX_LINK/bin:$PATH" - unset NIX_LINK + unset NIX_LINK NIX_LINK_NEW fi diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index a48968656..3e37c7f60 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -2496,7 +2496,7 @@ Strings EvalSettings::getDefaultNixPath() res.push_back(s ? *s + "=" + p : p); }; - add(getHome() + "/.nix-defexpr/channels"); + add(settings.useXDGBaseDirectories ? getStateDir() + "/nix/defexpr/channels" : getHome() + "/.nix-defexpr/channels"); add(settings.nixStateDir + "/profiles/per-user/root/channels/nixpkgs", "nixpkgs"); add(settings.nixStateDir + "/profiles/per-user/root/channels"); diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index c3ccb5e11..40f10fde3 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -975,6 +975,27 @@ public: resolves to a different location from that of the build machine. You can enable this setting if you are sure you're not going to do that. )"}; + + Setting useXDGBaseDirectories{ + this, false, "use-xdg-base-directories", + R"( + If set to `true`, Nix will conform to the [XDG Base Directory Specification] for files in `$HOME`. + The environment variables used to implement this are documented in the [Environment Variables section](@docroot@/installation/env-variables.md). + + [XDG Base Directory Specification]: https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html + + > **Warning** + > This changes the location of some well-known symlinks that Nix creates, which might break tools that rely on the old, non-XDG-conformant locations. + + In particular, the following locations change: + + | Old | New | + |-------------------|--------------------------------| + | `~/.nix-profile` | `$XDG_STATE_HOME/nix/profile` | + | `~/.nix-defexpr` | `$XDG_STATE_HOME/nix/defexpr` | + | `~/.nix-channels` | `$XDG_STATE_HOME/nix/channels` | + )" + }; }; diff --git a/src/libstore/profiles.cc b/src/libstore/profiles.cc index b202351ce..c551c5f3e 100644 --- a/src/libstore/profiles.cc +++ b/src/libstore/profiles.cc @@ -282,7 +282,7 @@ std::string optimisticLockProfile(const Path & profile) Path profilesDir() { - auto profileRoot = getDataDir() + "/nix/profiles"; + auto profileRoot = createNixStateDir() + "/profiles"; createDirs(profileRoot); return profileRoot; } @@ -290,7 +290,7 @@ Path profilesDir() Path getDefaultProfile() { - Path profileLink = getHome() + "/.nix-profile"; + Path profileLink = settings.useXDGBaseDirectories ? createNixStateDir() + "/profile" : getHome() + "/.nix-profile"; try { auto profile = getuid() == 0 diff --git a/src/libstore/profiles.hh b/src/libstore/profiles.hh index 73667a798..fbf95b850 100644 --- a/src/libstore/profiles.hh +++ b/src/libstore/profiles.hh @@ -72,8 +72,9 @@ std::string optimisticLockProfile(const Path & profile); profiles. */ Path profilesDir(); -/* Resolve ~/.nix-profile. If ~/.nix-profile doesn't exist yet, create - it. */ +/* Resolve the default profile (~/.nix-profile by default, $XDG_STATE_HOME/ + nix/profile if XDG Base Directory Support is enabled), and create if doesn't + exist */ Path getDefaultProfile(); } diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 40faa4bf2..40a54b010 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -608,6 +608,19 @@ Path getDataDir() return dataDir ? *dataDir : getHome() + "/.local/share"; } +Path getStateDir() +{ + auto stateDir = getEnv("XDG_STATE_HOME"); + return stateDir ? *stateDir : getHome() + "/.local/state"; +} + +Path createNixStateDir() +{ + Path dir = getStateDir() + "/nix"; + createDirs(dir); + return dir; +} + std::optional getSelfExe() { diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 266da0ae3..4fadafaf2 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -158,6 +158,12 @@ Path getDataDir(); /* Return the path of the current executable. */ std::optional getSelfExe(); +/* Return $XDG_STATE_HOME or $HOME/.local/state. */ +Path getStateDir(); + +/* Create the Nix state directory and return the path to it. */ +Path createNixStateDir(); + /* Create a directory and all its parents, if necessary. Returns the list of created directories, in order of creation. */ Paths createDirs(const Path & path); diff --git a/src/nix-channel/nix-channel.cc b/src/nix-channel/nix-channel.cc index 263d85eea..338a7d18e 100755 --- a/src/nix-channel/nix-channel.cc +++ b/src/nix-channel/nix-channel.cc @@ -164,8 +164,8 @@ static int main_nix_channel(int argc, char ** argv) { // Figure out the name of the `.nix-channels' file to use auto home = getHome(); - channelsList = home + "/.nix-channels"; - nixDefExpr = home + "/.nix-defexpr"; + channelsList = settings.useXDGBaseDirectories ? createNixStateDir() + "/channels" : home + "/.nix-channels"; + nixDefExpr = settings.useXDGBaseDirectories ? createNixStateDir() + "/defexpr" : home + "/.nix-defexpr"; // Figure out the name of the channels profile. profile = profilesDir() + "/channels"; diff --git a/src/nix-env/nix-env.cc b/src/nix-env/nix-env.cc index 406e548c0..0daf374de 100644 --- a/src/nix-env/nix-env.cc +++ b/src/nix-env/nix-env.cc @@ -1289,7 +1289,7 @@ static void opSwitchProfile(Globals & globals, Strings opFlags, Strings opArgs) throw UsageError("exactly one argument expected"); Path profile = absPath(opArgs.front()); - Path profileLink = getHome() + "/.nix-profile"; + Path profileLink = settings.useXDGBaseDirectories ? createNixStateDir() + "/profile" : getHome() + "/.nix-profile"; switchLink(profileLink, profile); } @@ -1393,7 +1393,10 @@ static int main_nix_env(int argc, char * * argv) Globals globals; globals.instSource.type = srcUnknown; - globals.instSource.nixExprPath = getHome() + "/.nix-defexpr"; + { + Path nixExprPath = settings.useXDGBaseDirectories ? createNixStateDir() + "/defexpr" : getHome() + "/.nix-defexpr"; + globals.instSource.nixExprPath = nixExprPath; + } globals.instSource.systemFilter = "*"; if (!pathExists(globals.instSource.nixExprPath)) { diff --git a/tests/common.sh.in b/tests/common.sh.in index 74bbbc8ca..bbe97da68 100644 --- a/tests/common.sh.in +++ b/tests/common.sh.in @@ -27,6 +27,8 @@ export NIX_REMOTE=$NIX_REMOTE_ unset NIX_PATH export TEST_HOME=$TEST_ROOT/test-home export HOME=$TEST_HOME +unset XDG_STATE_HOME +unset XDG_DATA_HOME unset XDG_CONFIG_HOME unset XDG_CONFIG_DIRS unset XDG_CACHE_HOME @@ -62,8 +64,8 @@ readLink() { } clearProfiles() { - profiles="$HOME"/.local/share/nix/profiles - rm -rf $profiles + profiles="$HOME"/.local/state/nix/profiles + rm -rf "$profiles" } clearStore() { diff --git a/tests/nix-channel.sh b/tests/nix-channel.sh index 54b8f5979..b64283f48 100644 --- a/tests/nix-channel.sh +++ b/tests/nix-channel.sh @@ -12,6 +12,19 @@ nix-channel --remove xyzzy [ -e $TEST_HOME/.nix-channels ] [ "$(cat $TEST_HOME/.nix-channels)" = '' ] +# Test the XDG Base Directories support + +export NIX_CONFIG="use-xdg-base-directories = true" + +nix-channel --add http://foo/bar xyzzy +nix-channel --list | grep -q http://foo/bar +nix-channel --remove xyzzy + +unset NIX_CONFIG + +[ -e $TEST_HOME/.local/state/nix/channels ] +[ "$(cat $TEST_HOME/.local/state/nix/channels)" = '' ] + # Create a channel. rm -rf $TEST_ROOT/foo mkdir -p $TEST_ROOT/foo diff --git a/tests/nix-profile.sh b/tests/nix-profile.sh index 7ba3235fa..266dc9e49 100644 --- a/tests/nix-profile.sh +++ b/tests/nix-profile.sh @@ -56,6 +56,14 @@ nix profile history nix profile history | grep "packages.$system.default: ∅ -> 1.0" nix profile diff-closures | grep 'env-manifest.nix: ε → ∅' +# Test XDG Base Directories support + +export NIX_CONFIG="use-xdg-base-directories = true" +nix profile remove 1 +nix profile install $flake1Dir +[[ $($TEST_HOME/.local/state/nix/profile/bin/hello) = "Hello World" ]] +unset NIX_CONFIG + # Test upgrading a package. printf NixOS > $flake1Dir/who printf 2.0 > $flake1Dir/version From a21405a4e8a5ca4bfbe8df8de2f76d69c4608a9f Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 10 Feb 2023 17:51:44 +0100 Subject: [PATCH 29/40] Add regression test --- tests/nixos/remote-builds.nix | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/nixos/remote-builds.nix b/tests/nixos/remote-builds.nix index 696cd2652..1c96cc787 100644 --- a/tests/nixos/remote-builds.nix +++ b/tests/nixos/remote-builds.nix @@ -11,6 +11,11 @@ let { services.openssh.enable = true; virtualisation.writableStore = true; nix.settings.sandbox = true; + + # Regression test for use of PID namespaces when /proc has + # filesystems mounted on top of it + # (i.e. /proc/sys/fs/binfmt_misc). + boot.binfmt.emulatedSystems = [ "aarch64-linux" ]; }; # Trivial Nix expression to build remotely. From a537095e1f42d3de726e3c4d8bfc018a4c43ad86 Mon Sep 17 00:00:00 2001 From: Philipp Jungkamp Date: Fri, 10 Feb 2023 18:03:19 +0100 Subject: [PATCH 30/40] Infer short completion descriptions for commandline flags Descriptions for commandline flags may not include newlines and should be rather short for display in a shell. Truncate the description string of a flag on '\n' or '.' to and add an ellipsis if needed. --- src/libutil/args.cc | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/libutil/args.cc b/src/libutil/args.cc index 2930913d6..348f1bca0 100644 --- a/src/libutil/args.cc +++ b/src/libutil/args.cc @@ -29,7 +29,16 @@ void Args::removeFlag(const std::string & longName) void Completions::add(std::string completion, std::string description) { - assert(description.find('\n') == std::string::npos); + // strip whitespace/empty lines from the front of the description + description.erase(0, description.find_first_not_of(" \t\n")); + // ellipsize overflowing content on the back of the description + auto end_index = description.find_first_of(".\n"); + if (end_index != std::string::npos) { + auto needs_ellipsis = end_index != description.size() - 1; + description.resize(end_index); + if (needs_ellipsis) + description.append(" [...]"); + } insert(Completion { .completion = completion, .description = description From 5978ceb2715127cd081146272fc910f17232b5ec Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 10 Feb 2023 18:38:57 +0100 Subject: [PATCH 31/40] Fix building with GCC 9 Nixpkgs on aarch64-linux is currently stuck on GCC 9 (https://github.com/NixOS/nixpkgs/issues/208412) and using gcc11Stdenv doesn't work either. So use c++2a instead of c++20 for now. Unfortunately this means we can't use some C++20 features for now (like std::span). --- Makefile | 2 +- perl/Makefile | 2 +- src/libcmd/nix-cmd.pc.in | 2 +- src/libexpr/nix-expr.pc.in | 2 +- src/libmain/nix-main.pc.in | 2 +- src/libstore/nix-store.pc.in | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/Makefile b/Makefile index 42d11638b..8675c9925 100644 --- a/Makefile +++ b/Makefile @@ -36,4 +36,4 @@ endif include mk/lib.mk -GLOBAL_CXXFLAGS += -g -Wall -include config.h -std=c++20 -I src +GLOBAL_CXXFLAGS += -g -Wall -include config.h -std=c++2a -I src diff --git a/perl/Makefile b/perl/Makefile index 2d759e6fc..c2c95f255 100644 --- a/perl/Makefile +++ b/perl/Makefile @@ -1,6 +1,6 @@ makefiles = local.mk -GLOBAL_CXXFLAGS += -g -Wall -std=c++20 -I ../src +GLOBAL_CXXFLAGS += -g -Wall -std=c++2a -I ../src -include Makefile.config diff --git a/src/libcmd/nix-cmd.pc.in b/src/libcmd/nix-cmd.pc.in index a21d93f1d..39575f222 100644 --- a/src/libcmd/nix-cmd.pc.in +++ b/src/libcmd/nix-cmd.pc.in @@ -6,4 +6,4 @@ Name: Nix Description: Nix Package Manager Version: @PACKAGE_VERSION@ Libs: -L${libdir} -lnixcmd -Cflags: -I${includedir}/nix -std=c++20 +Cflags: -I${includedir}/nix -std=c++2a diff --git a/src/libexpr/nix-expr.pc.in b/src/libexpr/nix-expr.pc.in index 95d452ca8..60ffb5dba 100644 --- a/src/libexpr/nix-expr.pc.in +++ b/src/libexpr/nix-expr.pc.in @@ -7,4 +7,4 @@ Description: Nix Package Manager Version: @PACKAGE_VERSION@ Requires: nix-store bdw-gc Libs: -L${libdir} -lnixexpr -Cflags: -I${includedir}/nix -std=c++20 +Cflags: -I${includedir}/nix -std=c++2a diff --git a/src/libmain/nix-main.pc.in b/src/libmain/nix-main.pc.in index b46ce1990..fb3ead6fa 100644 --- a/src/libmain/nix-main.pc.in +++ b/src/libmain/nix-main.pc.in @@ -6,4 +6,4 @@ Name: Nix Description: Nix Package Manager Version: @PACKAGE_VERSION@ Libs: -L${libdir} -lnixmain -Cflags: -I${includedir}/nix -std=c++20 +Cflags: -I${includedir}/nix -std=c++2a diff --git a/src/libstore/nix-store.pc.in b/src/libstore/nix-store.pc.in index 385169a13..dc42d0bca 100644 --- a/src/libstore/nix-store.pc.in +++ b/src/libstore/nix-store.pc.in @@ -6,4 +6,4 @@ Name: Nix Description: Nix Package Manager Version: @PACKAGE_VERSION@ Libs: -L${libdir} -lnixstore -lnixutil -Cflags: -I${includedir}/nix -std=c++20 +Cflags: -I${includedir}/nix -std=c++2a From 30edd7af532bb893f76d5d4e7ec0fadfcdc941c6 Mon Sep 17 00:00:00 2001 From: Philipp Jungkamp Date: Fri, 10 Feb 2023 22:17:09 +0100 Subject: [PATCH 32/40] Completions::add use libutil trim() --- src/libutil/args.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/libutil/args.cc b/src/libutil/args.cc index 348f1bca0..35686a8aa 100644 --- a/src/libutil/args.cc +++ b/src/libutil/args.cc @@ -29,8 +29,7 @@ void Args::removeFlag(const std::string & longName) void Completions::add(std::string completion, std::string description) { - // strip whitespace/empty lines from the front of the description - description.erase(0, description.find_first_not_of(" \t\n")); + description = trim(description); // ellipsize overflowing content on the back of the description auto end_index = description.find_first_of(".\n"); if (end_index != std::string::npos) { From 55016b6fcda70836df0320a84a7d5fd348b4c02f Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 1 Mar 2021 00:15:05 +0000 Subject: [PATCH 33/40] Test `nix build --json` return output paths in floating CA case Adding a test to ensure there is no regression. The tests that are split out of `tests/build.sh` are ones that don't yet work with CA derivation. I have not yet evaluated whether they should or not. This behavior, reported missing in issue #4661, already got fixed in PR #4818, but didn't get a test case then. --- tests/build-delete.sh | 56 +++++++++++++++++++++++++++++++++ tests/build.sh | 59 ----------------------------------- tests/ca/new-build-cmd.sh | 5 +++ tests/ca/recursive.sh | 2 -- tests/local.mk | 3 ++ tests/output-normalization.sh | 9 ++++++ 6 files changed, 73 insertions(+), 61 deletions(-) create mode 100644 tests/build-delete.sh create mode 100644 tests/ca/new-build-cmd.sh create mode 100644 tests/output-normalization.sh diff --git a/tests/build-delete.sh b/tests/build-delete.sh new file mode 100644 index 000000000..636681f64 --- /dev/null +++ b/tests/build-delete.sh @@ -0,0 +1,56 @@ +source common.sh + +clearStore + +set -o pipefail + +# https://github.com/NixOS/nix/issues/6572 +issue_6572_independent_outputs() { + nix build -f multiple-outputs.nix --json independent --no-link > $TEST_ROOT/independent.json + + # Make sure that 'nix build' can build a derivation that depends on both outputs of another derivation. + p=$(nix build -f multiple-outputs.nix use-independent --no-link --print-out-paths) + nix-store --delete "$p" # Clean up for next test + + # Make sure that 'nix build' tracks input-outputs correctly when a single output is already present. + nix-store --delete "$(jq -r <$TEST_ROOT/independent.json .[0].outputs.first)" + p=$(nix build -f multiple-outputs.nix use-independent --no-link --print-out-paths) + cmp $p < $TEST_ROOT/a.json + + # # Make sure that 'nix build' can build a derivation that depends on both outputs of another derivation. + p=$(nix build -f multiple-outputs.nix use-a --no-link --print-out-paths) + nix-store --delete "$p" # Clean up for next test + + # Make sure that 'nix build' tracks input-outputs correctly when a single output is already present. + nix-store --delete "$(jq -r <$TEST_ROOT/a.json .[0].outputs.second)" + p=$(nix build -f multiple-outputs.nix use-a --no-link --print-out-paths) + cmp $p < $TEST_ROOT/independent.json - - # Make sure that 'nix build' can build a derivation that depends on both outputs of another derivation. - p=$(nix build -f multiple-outputs.nix use-independent --no-link --print-out-paths) - nix-store --delete "$p" # Clean up for next test - - # Make sure that 'nix build' tracks input-outputs correctly when a single output is already present. - nix-store --delete "$(jq -r <$TEST_ROOT/independent.json .[0].outputs.first)" - p=$(nix build -f multiple-outputs.nix use-independent --no-link --print-out-paths) - cmp $p < $TEST_ROOT/a.json - - # # Make sure that 'nix build' can build a derivation that depends on both outputs of another derivation. - p=$(nix build -f multiple-outputs.nix use-a --no-link --print-out-paths) - nix-store --delete "$p" # Clean up for next test - - # Make sure that 'nix build' tracks input-outputs correctly when a single output is already present. - nix-store --delete "$(jq -r <$TEST_ROOT/a.json .[0].outputs.second)" - p=$(nix build -f multiple-outputs.nix use-a --no-link --print-out-paths) - cmp $p < Date: Sun, 12 Feb 2023 05:45:25 +0100 Subject: [PATCH 34/40] parser: use implicit rule --- src/libexpr/parser.y | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index ffb364a90..6f1041d2d 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -438,7 +438,7 @@ expr_select function named ‘or’, allow stuff like ‘map or [...]’. */ expr_simple OR_KW { $$ = new ExprCall(CUR_POS, $1, {new ExprVar(CUR_POS, data->symbols.create("or"))}); } - | expr_simple { $$ = $1; } + | expr_simple ; expr_simple @@ -455,7 +455,7 @@ expr_simple | IND_STRING_OPEN ind_string_parts IND_STRING_CLOSE { $$ = stripIndentation(CUR_POS, data->symbols, *$2); } - | path_start PATH_END { $$ = $1; } + | path_start PATH_END | path_start string_parts_interpolated PATH_END { $2->insert($2->begin(), {makeCurPos(@1, data), $1}); $$ = new ExprConcatStrings(CUR_POS, false, $2); @@ -596,7 +596,7 @@ attrpath ; attr - : ID { $$ = $1; } + : ID | OR_KW { $$ = {"or", 2}; } ; From fa89d317b7c7590a59db44bb369fcfe6baee4296 Mon Sep 17 00:00:00 2001 From: Et7f3 Date: Sat, 11 Feb 2023 00:34:31 +0100 Subject: [PATCH 35/40] ExprString: Avoid copy of string --- src/libexpr/nixexpr.hh | 2 +- src/libexpr/parser.y | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh index ffe67f97d..20a586f60 100644 --- a/src/libexpr/nixexpr.hh +++ b/src/libexpr/nixexpr.hh @@ -186,7 +186,7 @@ struct ExprString : Expr { std::string s; Value v; - ExprString(std::string s) : s(std::move(s)) { v.mkString(this->s.data()); }; + ExprString(std::string &&s) : s(std::move(s)) { v.mkString(this->s.data()); }; Value * maybeThunk(EvalState & state, Env & env) override; COMMON_METHODS }; diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index 6f1041d2d..1dd4a801f 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -268,7 +268,7 @@ static Expr * stripIndentation(const PosIdx pos, SymbolTable & symbols, s2 = std::string(s2, 0, p + 1); } - es2->emplace_back(i->first, new ExprString(s2)); + es2->emplace_back(i->first, new ExprString(std::move(s2))); }; for (; i != es.end(); ++i, --n) { std::visit(overloaded { trimExpr, trimString }, i->second); @@ -465,7 +465,7 @@ expr_simple $$ = new ExprCall(CUR_POS, new ExprVar(data->symbols.create("__findFile")), {new ExprVar(data->symbols.create("__nixPath")), - new ExprString(path)}); + new ExprString(std::move(path))}); } | URI { static bool noURLLiterals = settings.isExperimentalFeatureEnabled(Xp::NoUrlLiterals); From db41f74af39850d0aeb2741304c1eacf90ceea88 Mon Sep 17 00:00:00 2001 From: Yorick van Pelt Date: Tue, 14 Feb 2023 12:03:34 +0100 Subject: [PATCH 36/40] Don't allow writing to /etc --- src/libstore/build/local-derivation-goal.cc | 4 +++- tests/linux-sandbox.sh | 3 +++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 7c4892c96..de023f336 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -670,7 +670,6 @@ void LocalDerivationGoal::startBuilder() nobody account. The latter is kind of a hack to support Samba-in-QEMU. */ createDirs(chrootRootDir + "/etc"); - chownToBuilder(chrootRootDir + "/etc"); if (parsedDrv->useUidRange() && (!buildUser || buildUser->getUIDCount() < 65536)) throw Error("feature 'uid-range' requires the setting '%s' to be enabled", settings.autoAllocateUids.name); @@ -970,6 +969,9 @@ void LocalDerivationGoal::startBuilder() "nobody:x:65534:65534:Nobody:/:/noshell\n", sandboxUid(), sandboxGid(), settings.sandboxBuildDir)); + /* Make /etc unwritable */ + chmod_(chrootRootDir + "/etc", 0555); + /* Save the mount- and user namespace of the child. We have to do this *before* the child does a chroot. */ sandboxMountNamespace = open(fmt("/proc/%d/ns/mnt", (pid_t) pid).c_str(), O_RDONLY); diff --git a/tests/linux-sandbox.sh b/tests/linux-sandbox.sh index 3f304ac2f..e62039567 100644 --- a/tests/linux-sandbox.sh +++ b/tests/linux-sandbox.sh @@ -37,3 +37,6 @@ nix-build check.nix -A nondeterministic --sandbox-paths /nix/store --no-out-link (! nix-build check.nix -A nondeterministic --sandbox-paths /nix/store --no-out-link --check -K 2> $TEST_ROOT/log) if grep -q 'error: renaming' $TEST_ROOT/log; then false; fi grep -q 'may not be deterministic' $TEST_ROOT/log + +# Test that sandboxed builds cannot write to /etc easily +(! nix-build -E 'with import ./config.nix; mkDerivation { name = "etc-write"; buildCommand = "echo > /etc/test"; }' --no-out-link --sandbox-paths /nix/store) From ad1f61c39b716f4876d5f4c1dd9e37681631edb3 Mon Sep 17 00:00:00 2001 From: Yorick van Pelt Date: Tue, 14 Feb 2023 12:26:40 +0100 Subject: [PATCH 37/40] container test: make /etc writable --- tests/nixos/containers/systemd-nspawn.nix | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/nixos/containers/systemd-nspawn.nix b/tests/nixos/containers/systemd-nspawn.nix index 424436b3f..457af6064 100644 --- a/tests/nixos/containers/systemd-nspawn.nix +++ b/tests/nixos/containers/systemd-nspawn.nix @@ -62,6 +62,7 @@ runCommand "test" mkdir -p $out + chmod +w /etc touch /etc/os-release echo a5ea3f98dedc0278b6f3cc8c37eeaeac > /etc/machine-id From 49fd72a903b7bc2fdc4735111ca5569122cf55ee Mon Sep 17 00:00:00 2001 From: Yorick van Pelt Date: Tue, 14 Feb 2023 13:29:30 +0100 Subject: [PATCH 38/40] Make /etc writability conditional on uid-range feature --- src/libstore/build/local-derivation-goal.cc | 5 ++++- tests/nixos/containers/systemd-nspawn.nix | 1 - 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index de023f336..7b125f5d2 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -670,6 +670,8 @@ void LocalDerivationGoal::startBuilder() nobody account. The latter is kind of a hack to support Samba-in-QEMU. */ createDirs(chrootRootDir + "/etc"); + if (parsedDrv->useUidRange()) + chownToBuilder(chrootRootDir + "/etc"); if (parsedDrv->useUidRange() && (!buildUser || buildUser->getUIDCount() < 65536)) throw Error("feature 'uid-range' requires the setting '%s' to be enabled", settings.autoAllocateUids.name); @@ -970,7 +972,8 @@ void LocalDerivationGoal::startBuilder() sandboxUid(), sandboxGid(), settings.sandboxBuildDir)); /* Make /etc unwritable */ - chmod_(chrootRootDir + "/etc", 0555); + if (!parsedDrv->useUidRange()) + chmod_(chrootRootDir + "/etc", 0555); /* Save the mount- and user namespace of the child. We have to do this *before* the child does a chroot. */ diff --git a/tests/nixos/containers/systemd-nspawn.nix b/tests/nixos/containers/systemd-nspawn.nix index 457af6064..f54f32f2a 100644 --- a/tests/nixos/containers/systemd-nspawn.nix +++ b/tests/nixos/containers/systemd-nspawn.nix @@ -56,7 +56,6 @@ runCommand "test" # Make /run a tmpfs to shut up a systemd warning. mkdir /run mount -t tmpfs none /run - chmod 0700 /run mount -t cgroup2 none /sys/fs/cgroup From 35049389cdc50d4e3a460d40808a72a8c50207e7 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 14 Feb 2023 15:54:19 +0100 Subject: [PATCH 39/40] Fix static build For static builds, we need to propagate all the static library dependencies to the link of the program. E.g. if libstore-tests-exe depends on libnixstore-tests, and libnixstore-tests depends on libstore, then libstore-tests-exe needs to link against libstore. https://hydra.nixos.org/build/209007480 --- mk/libraries.mk | 5 ++++- mk/programs.mk | 4 ++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/mk/libraries.mk b/mk/libraries.mk index 6541775f3..02e4d47f9 100644 --- a/mk/libraries.mk +++ b/mk/libraries.mk @@ -67,6 +67,7 @@ define build-library $(1)_LDFLAGS_USE := $(1)_LDFLAGS_USE_INSTALLED := + $(1)_LIB_CLOSURE := $(1) $$(eval $$(call create-dir, $$(_d))) @@ -128,10 +129,12 @@ define build-library +$$(trace-ld) $(LD) -Ur -o $$(_d)/$$($(1)_NAME).o $$^ $$(trace-ar) $(AR) crs $$@ $$(_d)/$$($(1)_NAME).o - $(1)_LDFLAGS_USE += $$($(1)_PATH) $$($(1)_LDFLAGS) + $(1)_LDFLAGS_USE += $$($(1)_PATH) $$($(1)_LDFLAGS) $$(foreach lib, $$($(1)_LIBS), $$($$(lib)_LDFLAGS_USE)) $(1)_INSTALL_PATH := $$(libdir)/$$($(1)_NAME).a + $(1)_LIB_CLOSURE += $$($(1)_LIBS) + endif $(1)_LDFLAGS_USE += $$($(1)_LDFLAGS_PROPAGATED) diff --git a/mk/programs.mk b/mk/programs.mk index 204409332..1ee1d3fa5 100644 --- a/mk/programs.mk +++ b/mk/programs.mk @@ -30,7 +30,7 @@ define build-program _d := $(buildprefix)$$($(1)_DIR) _srcs := $$(sort $$(foreach src, $$($(1)_SOURCES), $$(src))) $(1)_OBJS := $$(addprefix $(buildprefix), $$(addsuffix .o, $$(basename $$(_srcs)))) - _libs := $$(foreach lib, $$($(1)_LIBS), $$($$(lib)_PATH)) + _libs := $$(foreach lib, $$($(1)_LIBS), $$(foreach lib2, $$($$(lib)_LIB_CLOSURE), $$($$(lib2)_PATH))) $(1)_PATH := $$(_d)/$$($(1)_NAME) $$(eval $$(call create-dir, $$(_d))) @@ -58,7 +58,7 @@ define build-program else $(DESTDIR)$$($(1)_INSTALL_PATH): $$($(1)_PATH) | $(DESTDIR)$$($(1)_INSTALL_DIR)/ - install -t $(DESTDIR)$$($(1)_INSTALL_DIR) $$< + +$$(trace-install) install -t $(DESTDIR)$$($(1)_INSTALL_DIR) $$< endif endif From cec23f5ddab31fb7704aa7d822a508690e416ab6 Mon Sep 17 00:00:00 2001 From: Et7f3 Date: Sat, 11 Feb 2023 00:37:23 +0100 Subject: [PATCH 40/40] ExprOpHasAttr,ExprSelect,stripIndentation,binds,formals: delete losts objects We are looking for *$ because it indicate that it was constructed with a new but not release. De-referencing shallow copy so deleting as whole might create dangling pointer that's why we move it so we delete a empty containers + the nice perf boost. --- src/libexpr/nixexpr.hh | 4 ++-- src/libexpr/parser.y | 21 ++++++++++++--------- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh index 20a586f60..4a81eaa47 100644 --- a/src/libexpr/nixexpr.hh +++ b/src/libexpr/nixexpr.hh @@ -233,7 +233,7 @@ struct ExprSelect : Expr PosIdx pos; Expr * e, * def; AttrPath attrPath; - ExprSelect(const PosIdx & pos, Expr * e, const AttrPath & attrPath, Expr * def) : pos(pos), e(e), def(def), attrPath(attrPath) { }; + ExprSelect(const PosIdx & pos, Expr * e, const AttrPath && attrPath, Expr * def) : pos(pos), e(e), def(def), attrPath(std::move(attrPath)) { }; ExprSelect(const PosIdx & pos, Expr * e, Symbol name) : pos(pos), e(e), def(0) { attrPath.push_back(AttrName(name)); }; PosIdx getPos() const override { return pos; } COMMON_METHODS @@ -243,7 +243,7 @@ struct ExprOpHasAttr : Expr { Expr * e; AttrPath attrPath; - ExprOpHasAttr(Expr * e, const AttrPath & attrPath) : e(e), attrPath(attrPath) { }; + ExprOpHasAttr(Expr * e, const AttrPath && attrPath) : e(e), attrPath(std::move(attrPath)) { }; PosIdx getPos() const override { return e->getPos(); } COMMON_METHODS }; diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index 1dd4a801f..dec5818fc 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -90,7 +90,7 @@ static void dupAttr(const EvalState & state, Symbol attr, const PosIdx pos, cons } -static void addAttr(ExprAttrs * attrs, AttrPath & attrPath, +static void addAttr(ExprAttrs * attrs, AttrPath && attrPath, Expr * e, const PosIdx pos, const nix::EvalState & state) { AttrPath::iterator i; @@ -188,7 +188,7 @@ static Formals * toFormals(ParseData & data, ParserFormals * formals, static Expr * stripIndentation(const PosIdx pos, SymbolTable & symbols, - std::vector>> & es) + std::vector>> && es) { if (es.empty()) return new ExprString(""); @@ -408,7 +408,7 @@ expr_op | expr_op OR expr_op { $$ = new ExprOpOr(makeCurPos(@2, data), $1, $3); } | expr_op IMPL expr_op { $$ = new ExprOpImpl(makeCurPos(@2, data), $1, $3); } | expr_op UPDATE expr_op { $$ = new ExprOpUpdate(makeCurPos(@2, data), $1, $3); } - | expr_op '?' attrpath { $$ = new ExprOpHasAttr($1, *$3); } + | expr_op '?' attrpath { $$ = new ExprOpHasAttr($1, std::move(*$3)); delete $3; } | expr_op '+' expr_op { $$ = new ExprConcatStrings(makeCurPos(@2, data), false, new std::vector >({{makeCurPos(@1, data), $1}, {makeCurPos(@3, data), $3}})); } | expr_op '-' expr_op { $$ = new ExprCall(makeCurPos(@2, data), new ExprVar(data->symbols.create("__sub")), {$1, $3}); } @@ -431,9 +431,9 @@ expr_app expr_select : expr_simple '.' attrpath - { $$ = new ExprSelect(CUR_POS, $1, *$3, 0); } + { $$ = new ExprSelect(CUR_POS, $1, std::move(*$3), nullptr); delete $3; } | expr_simple '.' attrpath OR_KW expr_select - { $$ = new ExprSelect(CUR_POS, $1, *$3, $5); } + { $$ = new ExprSelect(CUR_POS, $1, std::move(*$3), $5); delete $3; } | /* Backwards compatibility: because Nixpkgs has a rarely used function named ‘or’, allow stuff like ‘map or [...]’. */ expr_simple OR_KW @@ -453,7 +453,8 @@ expr_simple | FLOAT { $$ = new ExprFloat($1); } | '"' string_parts '"' { $$ = $2; } | IND_STRING_OPEN ind_string_parts IND_STRING_CLOSE { - $$ = stripIndentation(CUR_POS, data->symbols, *$2); + $$ = stripIndentation(CUR_POS, data->symbols, std::move(*$2)); + delete $2; } | path_start PATH_END | path_start string_parts_interpolated PATH_END { @@ -533,7 +534,7 @@ ind_string_parts ; binds - : binds attrpath '=' expr ';' { $$ = $1; addAttr($$, *$2, $4, makeCurPos(@2, data), data->state); } + : binds attrpath '=' expr ';' { $$ = $1; addAttr($$, std::move(*$2), $4, makeCurPos(@2, data), data->state); delete $2; } | binds INHERIT attrs ';' { $$ = $1; for (auto & i : *$3) { @@ -542,6 +543,7 @@ binds auto pos = makeCurPos(@3, data); $$->attrs.emplace(i.symbol, ExprAttrs::AttrDef(new ExprVar(CUR_POS, i.symbol), pos, true)); } + delete $3; } | binds INHERIT '(' expr ')' attrs ';' { $$ = $1; @@ -551,6 +553,7 @@ binds dupAttr(data->state, i.symbol, makeCurPos(@6, data), $$->attrs[i.symbol].pos); $$->attrs.emplace(i.symbol, ExprAttrs::AttrDef(new ExprSelect(CUR_POS, $4, i.symbol), makeCurPos(@6, data))); } + delete $6; } | { $$ = new ExprAttrs(makeCurPos(@0, data)); } ; @@ -612,9 +615,9 @@ expr_list formals : formal ',' formals - { $$ = $3; $$->formals.push_back(*$1); } + { $$ = $3; $$->formals.emplace_back(*$1); delete $1; } | formal - { $$ = new ParserFormals; $$->formals.push_back(*$1); $$->ellipsis = false; } + { $$ = new ParserFormals; $$->formals.emplace_back(*$1); $$->ellipsis = false; delete $1; } | { $$ = new ParserFormals; $$->ellipsis = false; } | ELLIPSIS