From 8c09888de4ada45ffdcb733879847aa24d3b00d8 Mon Sep 17 00:00:00 2001 From: Clemens Tolboom Date: Sat, 13 Feb 2021 15:18:08 +0100 Subject: [PATCH 01/30] Use long options instead of short ones It is a little hard to learn what the options mean. --- doc/manual/src/quick-start.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/doc/manual/src/quick-start.md b/doc/manual/src/quick-start.md index 651134c25..829e50d8a 100644 --- a/doc/manual/src/quick-start.md +++ b/doc/manual/src/quick-start.md @@ -19,7 +19,7 @@ to subsequent chapters. channel: ```console - $ nix-env -qaP + $ nix-env --query --available --attr-path nixpkgs.docbook_xml_dtd_43 docbook-xml-4.3 nixpkgs.docbook_xml_dtd_45 docbook-xml-4.5 nixpkgs.firefox firefox-33.0.2 @@ -31,7 +31,7 @@ to subsequent chapters. 1. Install some packages from the channel: ```console - $ nix-env -iA nixpkgs.hello + $ nix-env --install --attr nixpkgs.hello ``` This should download pre-built packages; it should not build them @@ -49,13 +49,13 @@ to subsequent chapters. 1. Uninstall a package: ```console - $ nix-env -e hello + $ nix-env --uninstall hello ``` 1. You can also test a package without installing it: ```console - $ nix-shell -p hello + $ nix-shell --profile hello ``` This builds or downloads GNU Hello and its dependencies, then drops @@ -76,7 +76,7 @@ to subsequent chapters. ```console $ nix-channel --update nixpkgs - $ nix-env -u '*' + $ nix-env --upgrade '*' ``` The latter command will upgrade each installed package for which @@ -95,5 +95,5 @@ to subsequent chapters. them: ```console - $ nix-collect-garbage -d + $ nix-collect-garbage --delete-old ``` From ddb40ddd476164a316d5cee299d807856c251040 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sun, 19 Feb 2023 20:00:02 -0500 Subject: [PATCH 02/30] Update doc/manual/src/quick-start.md Co-authored-by: Valentin Gagarin --- doc/manual/src/quick-start.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/manual/src/quick-start.md b/doc/manual/src/quick-start.md index 829e50d8a..1d2688ede 100644 --- a/doc/manual/src/quick-start.md +++ b/doc/manual/src/quick-start.md @@ -55,7 +55,7 @@ to subsequent chapters. 1. You can also test a package without installing it: ```console - $ nix-shell --profile hello + $ nix-shell --packages hello ``` This builds or downloads GNU Hello and its dependencies, then drops From 214f1d67910e491e0bd7e1f09f0623fdffd82d48 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Fri, 7 Apr 2023 09:16:40 +0200 Subject: [PATCH 03/30] Rename and protect `BufferedSink::write` The `write` name is ambiguous and could lead to some funny bugs like https://github.com/NixOS/nix/pull/8173#issuecomment-1500009480. So rename it to the more explicit `writeUnbuffered`. Besides, this method shouldn't be (and isn't) used outside of the class implementation, so mark it `protected`. This makes it more symetrical to `BufferedSource` which uses a `protected readUnbuffered` method. --- src/libutil/compression.cc | 6 +++--- src/libutil/compression.hh | 2 +- src/libutil/hash.cc | 2 +- src/libutil/hash.hh | 2 +- src/libutil/serialise.cc | 6 +++--- src/libutil/serialise.hh | 8 +++++--- 6 files changed, 14 insertions(+), 12 deletions(-) diff --git a/src/libutil/compression.cc b/src/libutil/compression.cc index 89180e7a7..ba0847cde 100644 --- a/src/libutil/compression.cc +++ b/src/libutil/compression.cc @@ -23,7 +23,7 @@ struct ChunkedCompressionSink : CompressionSink { uint8_t outbuf[32 * 1024]; - void write(std::string_view data) override + void writeUnbuffered(std::string_view data) override { const size_t CHUNK_SIZE = sizeof(outbuf) << 2; while (!data.empty()) { @@ -103,7 +103,7 @@ struct ArchiveCompressionSink : CompressionSink throw Error(reason, archive_error_string(this->archive)); } - void write(std::string_view data) override + void writeUnbuffered(std::string_view data) override { ssize_t result = archive_write_data(archive, data.data(), data.length()); if (result <= 0) check(result); @@ -136,7 +136,7 @@ struct NoneSink : CompressionSink warn("requested compression level '%d' not supported by compression method 'none'", level); } void finish() override { flush(); } - void write(std::string_view data) override { nextSink(data); } + void writeUnbuffered(std::string_view data) override { nextSink(data); } }; struct BrotliDecompressionSink : ChunkedCompressionSink diff --git a/src/libutil/compression.hh b/src/libutil/compression.hh index 3892831c2..4e53a7b3c 100644 --- a/src/libutil/compression.hh +++ b/src/libutil/compression.hh @@ -12,7 +12,7 @@ namespace nix { struct CompressionSink : BufferedSink, FinishSink { using BufferedSink::operator (); - using BufferedSink::write; + using BufferedSink::writeUnbuffered; using FinishSink::finish; }; diff --git a/src/libutil/hash.cc b/src/libutil/hash.cc index 5735e4715..0c8f5727d 100644 --- a/src/libutil/hash.cc +++ b/src/libutil/hash.cc @@ -343,7 +343,7 @@ HashSink::~HashSink() delete ctx; } -void HashSink::write(std::string_view data) +void HashSink::writeUnbuffered(std::string_view data) { bytes += data.size(); update(ht, *ctx, data); diff --git a/src/libutil/hash.hh b/src/libutil/hash.hh index be1fdba2a..ae3ee40f4 100644 --- a/src/libutil/hash.hh +++ b/src/libutil/hash.hh @@ -197,7 +197,7 @@ public: HashSink(HashType ht); HashSink(const HashSink & h); ~HashSink(); - void write(std::string_view data) override; + void writeUnbuffered(std::string_view data) override; HashResult finish() override; HashResult currentHash(); }; diff --git a/src/libutil/serialise.cc b/src/libutil/serialise.cc index 7476e6f6c..62d426e5c 100644 --- a/src/libutil/serialise.cc +++ b/src/libutil/serialise.cc @@ -20,7 +20,7 @@ void BufferedSink::operator () (std::string_view data) buffer size. */ if (bufPos + data.size() >= bufSize) { flush(); - write(data); + writeUnbuffered(data); break; } /* Otherwise, copy the bytes to the buffer. Flush the buffer @@ -38,7 +38,7 @@ void BufferedSink::flush() if (bufPos == 0) return; size_t n = bufPos; bufPos = 0; // don't trigger the assert() in ~BufferedSink() - write({buffer.get(), n}); + writeUnbuffered({buffer.get(), n}); } @@ -48,7 +48,7 @@ FdSink::~FdSink() } -void FdSink::write(std::string_view data) +void FdSink::writeUnbuffered(std::string_view data) { written += data.size(); try { diff --git a/src/libutil/serialise.hh b/src/libutil/serialise.hh index 2cf527023..cdb991036 100644 --- a/src/libutil/serialise.hh +++ b/src/libutil/serialise.hh @@ -53,7 +53,9 @@ struct BufferedSink : virtual Sink void flush(); - virtual void write(std::string_view data) = 0; +protected: + + virtual void writeUnbuffered(std::string_view data) = 0; }; @@ -133,7 +135,7 @@ struct FdSink : BufferedSink ~FdSink(); - void write(std::string_view data) override; + void writeUnbuffered(std::string_view data) override; bool good() override; @@ -520,7 +522,7 @@ struct FramedSink : nix::BufferedSink } } - void write(std::string_view data) override + void writeUnbuffered(std::string_view data) override { /* Don't send more data if the remote has encountered an error. */ From 6e0b7109abb40ded327b15599b29f861d9acb3c9 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Wed, 1 Feb 2023 13:34:32 +0100 Subject: [PATCH 04/30] Move OpenSSL init to initLibUtil Part of an effort to make it easier to initialize the right things, by moving code into the appropriate libraries. --- src/libmain/shared.cc | 22 +--------------------- src/libutil/hash.cc | 23 +++++++++++++++++++++++ src/libutil/util.cc | 4 ++++ src/libutil/util.hh | 3 +++ 4 files changed, 31 insertions(+), 21 deletions(-) diff --git a/src/libmain/shared.cc b/src/libmain/shared.cc index 37664c065..2ed310cba 100644 --- a/src/libmain/shared.cc +++ b/src/libmain/shared.cc @@ -115,22 +115,6 @@ std::string getArg(const std::string & opt, return *i; } - -#if OPENSSL_VERSION_NUMBER < 0x10101000L -/* OpenSSL is not thread-safe by default - it will randomly crash - unless the user supplies a mutex locking function. So let's do - that. */ -static std::vector opensslLocks; - -static void opensslLockCallback(int mode, int type, const char * file, int line) -{ - if (mode & CRYPTO_LOCK) - opensslLocks[type].lock(); - else - opensslLocks[type].unlock(); -} -#endif - static std::once_flag dns_resolve_flag; static void preloadNSS() { @@ -177,11 +161,7 @@ void initNix() std::cerr.rdbuf()->pubsetbuf(buf, sizeof(buf)); #endif -#if OPENSSL_VERSION_NUMBER < 0x10101000L - /* Initialise OpenSSL locking. */ - opensslLocks = std::vector(CRYPTO_num_locks()); - CRYPTO_set_locking_callback(opensslLockCallback); -#endif + initLibUtil(); if (sodium_init() == -1) throw Error("could not initialise libsodium"); diff --git a/src/libutil/hash.cc b/src/libutil/hash.cc index 5735e4715..9df8bcfb4 100644 --- a/src/libutil/hash.cc +++ b/src/libutil/hash.cc @@ -1,6 +1,7 @@ #include #include +#include #include #include @@ -16,6 +17,28 @@ namespace nix { +#if OPENSSL_VERSION_NUMBER < 0x10101000L +/* OpenSSL is not thread-safe by default - it will randomly crash + unless the user supplies a mutex locking function. So let's do + that. */ +static std::vector opensslLocks; + +static void opensslLockCallback(int mode, int type, const char * file, int line) +{ + if (mode & CRYPTO_LOCK) + opensslLocks[type].lock(); + else + opensslLocks[type].unlock(); +} +#endif + +void initOpenSSL() { +#if OPENSSL_VERSION_NUMBER < 0x10101000L + /* Initialise OpenSSL locking. */ + opensslLocks = std::vector(CRYPTO_num_locks()); + CRYPTO_set_locking_callback(opensslLockCallback); +#endif +} static size_t regularHashSize(HashType type) { switch (type) { diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 843a10eab..0099f7ebc 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -47,6 +47,10 @@ extern char * * environ __attribute__((weak)); namespace nix { +void initLibUtil() { + initOpenSSL(); +} + std::optional getEnv(const std::string & key) { char * value = getenv(key.c_str()); diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 56160baaf..783a4a601 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -32,6 +32,9 @@ namespace nix { struct Sink; struct Source; +void initLibUtil(); + +void initOpenSSL(); /** * The system for which Nix is compiled. From a692c437298ad59004583f193ef3d73a378fd837 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Wed, 1 Feb 2023 16:46:26 +0100 Subject: [PATCH 05/30] Move loadConfFile() to initLibStore Part of an effort to make it easier to initialize the right things, by moving code into the appropriate libraries. Using libstore without loading the config file is risky, as sqlite may then be misconfigured. See https://github.com/cachix/cachix/issues/475 --- perl/lib/Nix/Store.xs | 1 - src/libmain/shared.cc | 2 -- src/libstore/globals.cc | 3 +++ 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/perl/lib/Nix/Store.xs b/perl/lib/Nix/Store.xs index de91dc28d..b3f192810 100644 --- a/perl/lib/Nix/Store.xs +++ b/perl/lib/Nix/Store.xs @@ -27,7 +27,6 @@ static ref store() if (!_store) { try { initLibStore(); - loadConfFile(); settings.lockCPU = false; _store = openStore(); } catch (Error & e) { diff --git a/src/libmain/shared.cc b/src/libmain/shared.cc index 2ed310cba..6dd64c6c7 100644 --- a/src/libmain/shared.cc +++ b/src/libmain/shared.cc @@ -166,8 +166,6 @@ void initNix() if (sodium_init() == -1) throw Error("could not initialise libsodium"); - loadConfFile(); - startSignalHandlerThread(); /* Reset SIGCHLD to its default. */ diff --git a/src/libstore/globals.cc b/src/libstore/globals.cc index 823b4af74..b18525dd7 100644 --- a/src/libstore/globals.cc +++ b/src/libstore/globals.cc @@ -291,6 +291,9 @@ void assertLibStoreInitialized() { } void initLibStore() { + + loadConfFile(); + initLibStoreDone = true; } From 969307671500cb6cb9c01ab91c1d815ebd6a644b Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Wed, 1 Feb 2023 17:09:35 +0100 Subject: [PATCH 06/30] Move initLibStore() immediately after initLibUtil() Part of an effort to make it easier to initialize the right things, by moving code into the appropriate libraries. The goal of this reordering is to make initLibStore self-sufficient in a following commit. --- src/libmain/shared.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libmain/shared.cc b/src/libmain/shared.cc index 6dd64c6c7..5e19bddb7 100644 --- a/src/libmain/shared.cc +++ b/src/libmain/shared.cc @@ -162,6 +162,7 @@ void initNix() #endif initLibUtil(); + initLibStore(); if (sodium_init() == -1) throw Error("could not initialise libsodium"); @@ -223,7 +224,6 @@ void initNix() #endif preloadNSS(); - initLibStore(); } From a58be394769fb174ee4b6ff5ce16744cf5806485 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Wed, 1 Feb 2023 17:12:11 +0100 Subject: [PATCH 07/30] Move sodium_init() to initLibStore() Part of an effort to make it easier to initialize the right things, by moving code into the appropriate libraries. --- src/libmain/shared.cc | 5 ----- src/libstore/globals.cc | 5 +++++ 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/libmain/shared.cc b/src/libmain/shared.cc index 5e19bddb7..8e693fd8d 100644 --- a/src/libmain/shared.cc +++ b/src/libmain/shared.cc @@ -28,8 +28,6 @@ #include -#include - namespace nix { @@ -164,9 +162,6 @@ void initNix() initLibUtil(); initLibStore(); - if (sodium_init() == -1) - throw Error("could not initialise libsodium"); - startSignalHandlerThread(); /* Reset SIGCHLD to its default. */ diff --git a/src/libstore/globals.cc b/src/libstore/globals.cc index b18525dd7..1e66838c5 100644 --- a/src/libstore/globals.cc +++ b/src/libstore/globals.cc @@ -13,6 +13,8 @@ #include +#include + namespace nix { @@ -292,6 +294,9 @@ void assertLibStoreInitialized() { void initLibStore() { + if (sodium_init() == -1) + throw Error("could not initialise libsodium"); + loadConfFile(); initLibStoreDone = true; From e706ffa007120249deace149dc4ba7cacf2c8beb Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Wed, 1 Feb 2023 17:24:14 +0100 Subject: [PATCH 08/30] Move preloadNSS() from initNix to initLibStore It is required for the sandbox, which is a libstore responsibility; not just libmain. Part of an effort to make it easier to initialize the right things, by moving code into the appropriate libraries. --- src/libmain/shared.cc | 42 --------------------------------------- src/libstore/globals.cc | 44 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 42 deletions(-) diff --git a/src/libmain/shared.cc b/src/libmain/shared.cc index 8e693fd8d..cbd80756e 100644 --- a/src/libmain/shared.cc +++ b/src/libmain/shared.cc @@ -10,7 +10,6 @@ #include #include #include -#include #include #include @@ -20,11 +19,6 @@ #ifdef __linux__ #include #endif -#ifdef __GLIBC__ -#include -#include -#include -#endif #include @@ -113,41 +107,6 @@ std::string getArg(const std::string & opt, return *i; } -static std::once_flag dns_resolve_flag; - -static void preloadNSS() { - /* builtin:fetchurl can trigger a DNS lookup, which with glibc can trigger a dynamic library load of - one of the glibc NSS libraries in a sandboxed child, which will fail unless the library's already - been loaded in the parent. So we force a lookup of an invalid domain to force the NSS machinery to - load its lookup libraries in the parent before any child gets a chance to. */ - std::call_once(dns_resolve_flag, []() { -#ifdef __GLIBC__ - /* On linux, glibc will run every lookup through the nss layer. - * That means every lookup goes, by default, through nscd, which acts as a local - * cache. - * Because we run builds in a sandbox, we also remove access to nscd otherwise - * lookups would leak into the sandbox. - * - * But now we have a new problem, we need to make sure the nss_dns backend that - * does the dns lookups when nscd is not available is loaded or available. - * - * We can't make it available without leaking nix's environment, so instead we'll - * load the backend, and configure nss so it does not try to run dns lookups - * through nscd. - * - * This is technically only used for builtins:fetch* functions so we only care - * about dns. - * - * All other platforms are unaffected. - */ - if (!dlopen(LIBNSS_DNS_SO, RTLD_NOW)) - warn("unable to load nss_dns backend"); - // FIXME: get hosts entry from nsswitch.conf. - __nss_configure_lookup("hosts", "files dns"); -#endif - }); -} - static void sigHandler(int signo) { } @@ -218,7 +177,6 @@ void initNix() unsetenv("TMPDIR"); #endif - preloadNSS(); } diff --git a/src/libstore/globals.cc b/src/libstore/globals.cc index 1e66838c5..6848991a2 100644 --- a/src/libstore/globals.cc +++ b/src/libstore/globals.cc @@ -7,6 +7,7 @@ #include #include +#include #include #include #include @@ -15,6 +16,11 @@ #include +#ifdef __GLIBC__ +#include +#include +#include +#endif namespace nix { @@ -283,6 +289,42 @@ void initPlugins() settings.pluginFiles.pluginsLoaded = true; } +static void preloadNSS() +{ + /* builtin:fetchurl can trigger a DNS lookup, which with glibc can trigger a dynamic library load of + one of the glibc NSS libraries in a sandboxed child, which will fail unless the library's already + been loaded in the parent. So we force a lookup of an invalid domain to force the NSS machinery to + load its lookup libraries in the parent before any child gets a chance to. */ + static std::once_flag dns_resolve_flag; + + std::call_once(dns_resolve_flag, []() { +#ifdef __GLIBC__ + /* On linux, glibc will run every lookup through the nss layer. + * That means every lookup goes, by default, through nscd, which acts as a local + * cache. + * Because we run builds in a sandbox, we also remove access to nscd otherwise + * lookups would leak into the sandbox. + * + * But now we have a new problem, we need to make sure the nss_dns backend that + * does the dns lookups when nscd is not available is loaded or available. + * + * We can't make it available without leaking nix's environment, so instead we'll + * load the backend, and configure nss so it does not try to run dns lookups + * through nscd. + * + * This is technically only used for builtins:fetch* functions so we only care + * about dns. + * + * All other platforms are unaffected. + */ + if (!dlopen(LIBNSS_DNS_SO, RTLD_NOW)) + warn("unable to load nss_dns backend"); + // FIXME: get hosts entry from nsswitch.conf. + __nss_configure_lookup("hosts", "files dns"); +#endif + }); +} + static bool initLibStoreDone = false; void assertLibStoreInitialized() { @@ -299,6 +341,8 @@ void initLibStore() { loadConfFile(); + preloadNSS(); + initLibStoreDone = true; } From 52d6ce6515ff1e8462b67b2adb1942477ce122f8 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Wed, 1 Feb 2023 17:35:28 +0100 Subject: [PATCH 09/30] Move macOS TMPDIR hack from initNix to initLibStore This code is bad. We shouldn't unset variables in programs whose children may need them. Fixing one issue at a time, so postponing. See https://github.com/NixOS/nix/issues/7731 Part of an effort to make it easier to initialize the right things, by moving code into the appropriate libraries. --- src/libmain/shared.cc | 8 -------- src/libstore/globals.cc | 8 ++++++++ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/libmain/shared.cc b/src/libmain/shared.cc index cbd80756e..2a7e09e65 100644 --- a/src/libmain/shared.cc +++ b/src/libmain/shared.cc @@ -169,14 +169,6 @@ void initNix() gettimeofday(&tv, 0); srandom(tv.tv_usec); - /* On macOS, don't use the per-session TMPDIR (as set e.g. by - sshd). This breaks build users because they don't have access - to the TMPDIR, in particular in ‘nix-store --serve’. */ -#if __APPLE__ - if (hasPrefix(getEnv("TMPDIR").value_or("/tmp"), "/var/folders/")) - unsetenv("TMPDIR"); -#endif - } diff --git a/src/libstore/globals.cc b/src/libstore/globals.cc index 6848991a2..5a8825be5 100644 --- a/src/libstore/globals.cc +++ b/src/libstore/globals.cc @@ -343,6 +343,14 @@ void initLibStore() { preloadNSS(); + /* On macOS, don't use the per-session TMPDIR (as set e.g. by + sshd). This breaks build users because they don't have access + to the TMPDIR, in particular in ‘nix-store --serve’. */ +#if __APPLE__ + if (hasPrefix(getEnv("TMPDIR").value_or("/tmp"), "/var/folders/")) + unsetenv("TMPDIR"); +#endif + initLibStoreDone = true; } From 1107ea363f600f37152e2b144d03c4071c2a6b6b Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Wed, 1 Feb 2023 17:41:24 +0100 Subject: [PATCH 10/30] libmain: Clarify the lack of initLibExpr() Quote Why not initLibExpr()? initGC() is essentially that, but detectStackOverflow is not an instance of the init function concept, as it may have to be invoked more than once per process. Furthermore, renaming initGC to initLibExpr is more trouble than it's worth at this time. --- src/libmain/shared.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/libmain/shared.cc b/src/libmain/shared.cc index 2a7e09e65..a25865aad 100644 --- a/src/libmain/shared.cc +++ b/src/libmain/shared.cc @@ -156,7 +156,10 @@ void initNix() if (sigaction(SIGTRAP, &act, 0)) throw SysError("handling SIGTRAP"); #endif - /* Register a SIGSEGV handler to detect stack overflows. */ + /* Register a SIGSEGV handler to detect stack overflows. + Why not initLibExpr()? initGC() is essentially that, but + detectStackOverflow is not an instance of the init function concept, as + it may have to be invoked more than once per process. */ detectStackOverflow(); /* There is no privacy in the Nix system ;-) At least not for From 781d3dceb303d9fceabe9a39eae0f7f986e1adcc Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Wed, 1 Feb 2023 17:43:14 +0100 Subject: [PATCH 11/30] Move initLibUtil() from initNix to initLibStore libutil is a dependency of libstore, so it should always be initialized as such. libutil is also a dependency of libmain. Being explicit about this dependency might be good, but not worth the slight code complexity until the library structure gets more advanced. Part of an effort to make it easier to initialize the right things, by moving code into the appropriate libraries. --- src/libmain/shared.cc | 1 - src/libstore/globals.cc | 2 ++ 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libmain/shared.cc b/src/libmain/shared.cc index a25865aad..56f47a4ac 100644 --- a/src/libmain/shared.cc +++ b/src/libmain/shared.cc @@ -118,7 +118,6 @@ void initNix() std::cerr.rdbuf()->pubsetbuf(buf, sizeof(buf)); #endif - initLibUtil(); initLibStore(); startSignalHandlerThread(); diff --git a/src/libstore/globals.cc b/src/libstore/globals.cc index 5a8825be5..3f944f024 100644 --- a/src/libstore/globals.cc +++ b/src/libstore/globals.cc @@ -336,6 +336,8 @@ void assertLibStoreInitialized() { void initLibStore() { + initLibUtil(); + if (sodium_init() == -1) throw Error("could not initialise libsodium"); From 2196fd1146aa077419a113059ced924a648f9766 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Wed, 1 Feb 2023 18:38:54 +0100 Subject: [PATCH 12/30] libutil: Provide alternatives to startSignalHandlerThread How signals should be handled depends on what kind of process Nix is integrated into. The signal handler thread used by the stand-alone Nix commands / processes may not work well in the context of other runtime systems, such as those of Python, Perl, or Haskell. --- src/libutil/util.cc | 44 ++++++++++++++++++++++++++++++++++++++++++-- src/libutil/util.hh | 19 +++++++++++++++++++ 2 files changed, 61 insertions(+), 2 deletions(-) diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 0099f7ebc..5c19dc737 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -1748,13 +1748,39 @@ void triggerInterrupt() } static sigset_t savedSignalMask; +static bool savedSignalMaskIsSet = false; + +void setChildSignalMask(sigset_t * sigs) +{ + assert(sigs); // C style function, but think of sigs as a reference + +#if _POSIX_C_SOURCE >= 1 || _XOPEN_SOURCE || _POSIX_SOURCE + sigemptyset(&savedSignalMask); + // There's no "assign" or "copy" function, so we rely on (math) idempotence + // of the or operator: a or a = a. + sigorset(&savedSignalMask, sigs, sigs); +#else + // Without sigorset, our best bet is to assume that sigset_t is a type that + // can be assigned directly, such as is the case for a sigset_t defined as + // an integer type. + savedSignalMask = *sigs; +#endif + + savedSignalMaskIsSet = true; +} + +void saveSignalMask() { + if (sigprocmask(SIG_BLOCK, nullptr, &savedSignalMask)) + throw SysError("querying signal mask"); + + savedSignalMaskIsSet = true; +} void startSignalHandlerThread() { updateWindowSize(); - if (sigprocmask(SIG_BLOCK, nullptr, &savedSignalMask)) - throw SysError("querying signal mask"); + saveSignalMask(); sigset_t set; sigemptyset(&set); @@ -1771,6 +1797,20 @@ void startSignalHandlerThread() static void restoreSignals() { + // If startSignalHandlerThread wasn't called, that means we're not running + // in a proper libmain process, but a process that presumably manages its + // own signal handlers. Such a process should call either + // - initNix(), to be a proper libmain process + // - startSignalHandlerThread(), to resemble libmain regarding signal + // handling only + // - saveSignalMask(), for processes that define their own signal handling + // thread + // TODO: Warn about this? Have a default signal mask? The latter depends on + // whether we should generally inherit signal masks from the caller. + // I don't know what the larger unix ecosystem expects from us here. + if (!savedSignalMaskIsSet) + return; + if (sigprocmask(SIG_SETMASK, &savedSignalMask, nullptr)) throw SysError("restoring signals"); } diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 783a4a601..08993e1cf 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -448,6 +448,8 @@ void setStackSize(size_t stackSize); /** * Restore the original inherited Unix process context (such as signal * masks, stack size). + + * See startSignalHandlerThread(), saveSignalMask(). */ void restoreProcessContext(bool restoreMounts = true); @@ -817,9 +819,26 @@ class Callback; /** * Start a thread that handles various signals. Also block those signals * on the current thread (and thus any threads created by it). + * Saves the signal mask before changing the mask to block those signals. + * See saveSignalMask(). */ void startSignalHandlerThread(); +/** + * Saves the signal mask, which is the signal mask that nix will restore + * before creating child processes. + * See setChildSignalMask() to set an arbitrary signal mask instead of the + * current mask. + */ +void saveSignalMask(); + +/** + * Sets the signal mask. Like saveSignalMask() but for a signal set that doesn't + * necessarily match the current thread's mask. + * See saveSignalMask() to set the saved mask to the current mask. + */ +void setChildSignalMask(sigset_t *sigs); + struct InterruptCallback { virtual ~InterruptCallback() { }; From 2445afd92c99ec0901a0e1a00fadda12aad15220 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 3 Feb 2023 18:07:47 +0100 Subject: [PATCH 13/30] Require openssl >= 1.1.1 Versions older this are sufficiently old that we don't want to support them, and they require extra support code. --- configure.ac | 2 +- src/libutil/hash.cc | 23 ----------------------- src/libutil/util.cc | 1 - src/libutil/util.hh | 2 -- 4 files changed, 1 insertion(+), 27 deletions(-) diff --git a/configure.ac b/configure.ac index f1f45f868..ba5756169 100644 --- a/configure.ac +++ b/configure.ac @@ -184,7 +184,7 @@ fi # Look for OpenSSL, a required dependency. FIXME: this is only (maybe) # used by S3BinaryCacheStore. -PKG_CHECK_MODULES([OPENSSL], [libcrypto], [CXXFLAGS="$OPENSSL_CFLAGS $CXXFLAGS"]) +PKG_CHECK_MODULES([OPENSSL], [libcrypto >= 1.1.1], [CXXFLAGS="$OPENSSL_CFLAGS $CXXFLAGS"]) # Look for libarchive. diff --git a/src/libutil/hash.cc b/src/libutil/hash.cc index 9df8bcfb4..02bddc8d9 100644 --- a/src/libutil/hash.cc +++ b/src/libutil/hash.cc @@ -17,29 +17,6 @@ namespace nix { -#if OPENSSL_VERSION_NUMBER < 0x10101000L -/* OpenSSL is not thread-safe by default - it will randomly crash - unless the user supplies a mutex locking function. So let's do - that. */ -static std::vector opensslLocks; - -static void opensslLockCallback(int mode, int type, const char * file, int line) -{ - if (mode & CRYPTO_LOCK) - opensslLocks[type].lock(); - else - opensslLocks[type].unlock(); -} -#endif - -void initOpenSSL() { -#if OPENSSL_VERSION_NUMBER < 0x10101000L - /* Initialise OpenSSL locking. */ - opensslLocks = std::vector(CRYPTO_num_locks()); - CRYPTO_set_locking_callback(opensslLockCallback); -#endif -} - static size_t regularHashSize(HashType type) { switch (type) { case htMD5: return md5HashSize; diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 5c19dc737..21d1c8dcd 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -48,7 +48,6 @@ extern char * * environ __attribute__((weak)); namespace nix { void initLibUtil() { - initOpenSSL(); } std::optional getEnv(const std::string & key) diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 08993e1cf..6ff9d2524 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -34,8 +34,6 @@ struct Source; void initLibUtil(); -void initOpenSSL(); - /** * The system for which Nix is compiled. */ From 1c0b680ef9ca9604ff993a9d693355254ddc5bf4 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sun, 5 Feb 2023 14:16:27 +0100 Subject: [PATCH 14/30] libstore: Remove lockCPU dead code Left over from 9747ea84b, https://github.com/NixOS/nix/pull/5821 --- perl/lib/Nix/Store.xs | 1 - src/libstore/globals.cc | 1 - 2 files changed, 2 deletions(-) diff --git a/perl/lib/Nix/Store.xs b/perl/lib/Nix/Store.xs index b3f192810..10a0c4067 100644 --- a/perl/lib/Nix/Store.xs +++ b/perl/lib/Nix/Store.xs @@ -27,7 +27,6 @@ static ref store() if (!_store) { try { initLibStore(); - settings.lockCPU = false; _store = openStore(); } catch (Error & e) { croak("%s", e.what()); diff --git a/src/libstore/globals.cc b/src/libstore/globals.cc index 3f944f024..1b38e32fb 100644 --- a/src/libstore/globals.cc +++ b/src/libstore/globals.cc @@ -49,7 +49,6 @@ Settings::Settings() , nixDaemonSocketFile(canonPath(getEnvNonEmpty("NIX_DAEMON_SOCKET_PATH").value_or(nixStateDir + DEFAULT_SOCKET_PATH))) { buildUsersGroup = getuid() == 0 ? "nixbld" : ""; - lockCPU = getEnv("NIX_AFFINITY_HACK") == "1"; allowSymlinkedStore = getEnv("NIX_IGNORE_SYMLINK_STORE") == "1"; auto sslOverride = getEnv("NIX_SSL_CERT_FILE").value_or(getEnv("SSL_CERT_FILE").value_or("")); From ddebeb934a20225eec518520c96768bf00f0810a Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sun, 5 Feb 2023 14:16:27 +0100 Subject: [PATCH 15/30] libstore: Remove lockCPU dead code Left over from 9747ea84b, https://github.com/NixOS/nix/pull/5821 --- src/libstore/globals.hh | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index 63c7389da..c29ad5f89 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -478,11 +478,6 @@ public: )", {"env-keep-derivations"}}; - /** - * Whether to lock the Nix client and worker to the same CPU. - */ - bool lockCPU; - Setting sandboxMode{ this, #if __linux__ From 4e0804c920558575a4b3486df1e595445bf67555 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sun, 9 Apr 2023 22:42:20 +0200 Subject: [PATCH 16/30] Deduplicate string literal rendering, fix 4909 --- src/libcmd/repl.cc | 20 ++++++-------------- src/libexpr/eval.cc | 13 +++---------- src/libexpr/nixexpr.cc | 19 ++++--------------- src/libexpr/value/print.cc | 26 ++++++++++++++++++++++++++ src/libexpr/value/print.hh | 30 ++++++++++++++++++++++++++++++ src/libstore/derivations.cc | 9 +++++++++ tests/repl.sh | 8 ++++++++ 7 files changed, 86 insertions(+), 39 deletions(-) create mode 100644 src/libexpr/value/print.cc create mode 100644 src/libexpr/value/print.hh diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index 57848a5d3..1366622c7 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -40,6 +40,7 @@ extern "C" { #include "markdown.hh" #include "local-fs-store.hh" #include "progress-bar.hh" +#include "value/print.hh" #if HAVE_BOEHMGC #define GC_INCLUDE_NEW @@ -894,17 +895,6 @@ std::ostream & NixRepl::printValue(std::ostream & str, Value & v, unsigned int m } -std::ostream & printStringValue(std::ostream & str, const char * string) { - str << "\""; - for (const char * i = string; *i; i++) - if (*i == '\"' || *i == '\\') str << "\\" << *i; - else if (*i == '\n') str << "\\n"; - else if (*i == '\r') str << "\\r"; - else if (*i == '\t') str << "\\t"; - else str << *i; - str << "\""; - return str; -} // FIXME: lot of cut&paste from Nix's eval.cc. @@ -922,12 +912,14 @@ std::ostream & NixRepl::printValue(std::ostream & str, Value & v, unsigned int m break; case nBool: - str << ANSI_CYAN << (v.boolean ? "true" : "false") << ANSI_NORMAL; + str << ANSI_CYAN; + printLiteral(str, v.boolean); + str << ANSI_NORMAL; break; case nString: str << ANSI_WARNING; - printStringValue(str, v.string.s); + printLiteral(str, v.string.s); str << ANSI_NORMAL; break; @@ -967,7 +959,7 @@ std::ostream & NixRepl::printValue(std::ostream & str, Value & v, unsigned int m if (isVarName(i.first)) str << i.first; else - printStringValue(str, i.first.c_str()); + printLiteral(str, i.first); str << " = "; if (seen.count(i.second)) str << "«repeated»"; diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 18cfd9531..06208897f 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -9,6 +9,7 @@ #include "filetransfer.hh" #include "function-trace.hh" #include "profiles.hh" +#include "value/print.hh" #include #include @@ -104,18 +105,10 @@ void Value::print(const SymbolTable & symbols, std::ostream & str, str << integer; break; case tBool: - str << (boolean ? "true" : "false"); + printLiteral(str, boolean); break; case tString: - str << "\""; - for (const char * i = string.s; *i; i++) - if (*i == '\"' || *i == '\\') str << "\\" << *i; - else if (*i == '\n') str << "\\n"; - else if (*i == '\r') str << "\\r"; - else if (*i == '\t') str << "\\t"; - else if (*i == '$' && *(i+1) == '{') str << "\\" << *i; - else str << *i; - str << "\""; + printLiteral(str, string.s); break; case tPath: str << path; // !!! escaping? diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index eb6f062b4..ca6df0af3 100644 --- a/src/libexpr/nixexpr.cc +++ b/src/libexpr/nixexpr.cc @@ -3,6 +3,7 @@ #include "eval.hh" #include "symbol-table.hh" #include "util.hh" +#include "value/print.hh" #include @@ -62,18 +63,6 @@ Pos::operator std::shared_ptr() const /* Displaying abstract syntax trees. */ -static void showString(std::ostream & str, std::string_view s) -{ - str << '"'; - for (auto c : s) - if (c == '"' || c == '\\' || c == '$') str << "\\" << c; - else if (c == '\n') str << "\\n"; - else if (c == '\r') str << "\\r"; - else if (c == '\t') str << "\\t"; - else str << c; - str << '"'; -} - std::ostream & operator <<(std::ostream & str, const SymbolStr & symbol) { std::string_view s = symbol; @@ -85,7 +74,7 @@ std::ostream & operator <<(std::ostream & str, const SymbolStr & symbol) else { char c = s[0]; if (!((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || c == '_')) { - showString(str, s); + printLiteral(str, s); return str; } for (auto c : s) @@ -93,7 +82,7 @@ std::ostream & operator <<(std::ostream & str, const SymbolStr & symbol) (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '_' || c == '\'' || c == '-')) { - showString(str, s); + printLiteral(str, s); return str; } str << s; @@ -118,7 +107,7 @@ void ExprFloat::show(const SymbolTable & symbols, std::ostream & str) const void ExprString::show(const SymbolTable & symbols, std::ostream & str) const { - showString(str, s); + printLiteral(str, s); } void ExprPath::show(const SymbolTable & symbols, std::ostream & str) const diff --git a/src/libexpr/value/print.cc b/src/libexpr/value/print.cc new file mode 100644 index 000000000..cf97def46 --- /dev/null +++ b/src/libexpr/value/print.cc @@ -0,0 +1,26 @@ +#include "value/print.hh" + +namespace nix { + +std::ostream & +printLiteral(std::ostream & str, const std::string_view string) { + str << "\""; + for (auto i = string.begin(); i != string.end(); ++i) { + if (*i == '\"' || *i == '\\') str << "\\" << *i; + else if (*i == '\n') str << "\\n"; + else if (*i == '\r') str << "\\r"; + else if (*i == '\t') str << "\\t"; + else if (*i == '$' && *(i+1) == '{') str << "\\" << *i; + else str << *i; + } + str << "\""; + return str; +} + +std::ostream & +printLiteral(std::ostream & str, bool boolean) { + str << (boolean ? "true" : "false"); + return str; +} + +} diff --git a/src/libexpr/value/print.hh b/src/libexpr/value/print.hh new file mode 100644 index 000000000..31c94eb85 --- /dev/null +++ b/src/libexpr/value/print.hh @@ -0,0 +1,30 @@ +#pragma once +/** + * @file + * @brief Common printing functions for the Nix language + * + * While most types come with their own methods for printing, they share some + * functions that are placed here. + */ + +#include + +namespace nix { + /** + * Print a string as a Nix string literal. + * + * Quotes and fairly minimal escaping are added. + * + * @param s The logical string + */ + std::ostream & printLiteral(std::ostream & o, std::string_view s); + inline std::ostream & printLiteral(std::ostream & o, const char * s) { + return printLiteral(o, std::string_view(s)); + } + inline std::ostream & printLiteral(std::ostream & o, const std::string & s) { + return printLiteral(o, std::string_view(s)); + } + + /** Print `true` or `false`. */ + std::ostream & printLiteral(std::ostream & o, bool b); +} diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index abdfb1978..9948862e5 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -313,6 +313,15 @@ Derivation parseDerivation(const Store & store, std::string && s, std::string_vi } +/** + * Print a derivation string literal to an std::string. + * + * This syntax does not generalize to the expression language, which needs to + * escape `$`. + * + * @param res Where to print to + * @param s Which logical string to print + */ static void printString(std::string & res, std::string_view s) { boost::container::small_vector buffer; diff --git a/tests/repl.sh b/tests/repl.sh index be8adb742..2b3789521 100644 --- a/tests/repl.sh +++ b/tests/repl.sh @@ -79,6 +79,14 @@ testReplResponse ' "result: ${a}" ' "result: 2" +# check dollar escaping https://github.com/NixOS/nix/issues/4909 +# note the escaped \, +# \\ +# because the second argument is a regex +testReplResponse ' +"$" + "{hi}" +' '"\\${hi}"' + testReplResponse ' drvPath ' '".*-simple.drv"' \ From 7f5ca6192d091090bc71ab7bf96dd4acf0f1d376 Mon Sep 17 00:00:00 2001 From: Ben Radford Date: Mon, 10 Apr 2023 14:12:10 +0100 Subject: [PATCH 17/30] Add script to reproduce issue by inducing heavy load. --- repro-7998.sh | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100755 repro-7998.sh diff --git a/repro-7998.sh b/repro-7998.sh new file mode 100755 index 000000000..b022d31ab --- /dev/null +++ b/repro-7998.sh @@ -0,0 +1,33 @@ +#!/usr/bin/env bash +set -eux +LOG_FILE=/tmp/repro-7998.log +rm -f "$LOG_FILE" +for i in {1..8}; do + ( + while true; do + nix-build \ + --argstr uuid $(uuidgen) \ + --arg drvCount $((RANDOM % 256)) \ + -E ' + { uuid ? "00000000-0000-0000-0000-000000000000", drvCount ? 0 }: + with import { }; + let + mkDrv = name: buildInputs: + stdenv.mkDerivation { + inherit name; + inherit buildInputs; + unpackPhase = "date +\"${uuid} %F %T\" >date.txt"; + installPhase = "mkdir -p $out; cp date.txt $out/"; + }; + mkDrvs = n: + let + name = "repro-7998-${toString n}"; + buildInputs = if n == 0 then [ ] else [ (mkDrvs (n - 1)) ]; + in mkDrv name buildInputs; + in mkDrvs drvCount + ' + done 2>&1 | tee -a "$LOG_FILE" + ) & +done +read # Press enter to stop +pkill -KILL -f repro-7998.sh From 7c56e842133afe14812270c34cda3dc0a3da8aa6 Mon Sep 17 00:00:00 2001 From: Ben Radford Date: Tue, 11 Apr 2023 10:22:07 +0100 Subject: [PATCH 18/30] Warn after a second of being busy instead of immediately. Getting the occasional SQLITE_BUSY is expected when the database is being accessed concurrently. The retry will likely succeed so it is pointless to warn immediately. Instead we track how long each retrySQLite block has been running, and only begin warning after a second has elapsed (and then every 10 seconds subsequently). --- src/libstore/sqlite.cc | 9 ++------- src/libstore/sqlite.hh | 15 +++++++++++++-- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/src/libstore/sqlite.cc b/src/libstore/sqlite.cc index 871f2f3be..c57e58fe0 100644 --- a/src/libstore/sqlite.cc +++ b/src/libstore/sqlite.cc @@ -239,14 +239,9 @@ SQLiteTxn::~SQLiteTxn() } } -void handleSQLiteBusy(const SQLiteBusy & e) +void handleSQLiteBusy(const SQLiteBusy & e, bool shouldWarn) { - static std::atomic lastWarned{0}; - - time_t now = time(0); - - if (now > lastWarned + 10) { - lastWarned = now; + if (shouldWarn) { logWarning({ .msg = hintfmt(e.what()) }); diff --git a/src/libstore/sqlite.hh b/src/libstore/sqlite.hh index b735838ec..e2c9e28f8 100644 --- a/src/libstore/sqlite.hh +++ b/src/libstore/sqlite.hh @@ -139,7 +139,7 @@ protected: MakeError(SQLiteBusy, SQLiteError); -void handleSQLiteBusy(const SQLiteBusy & e); +void handleSQLiteBusy(const SQLiteBusy & e, bool shouldWarn); /** * Convenience function for retrying a SQLite transaction when the @@ -148,11 +148,22 @@ void handleSQLiteBusy(const SQLiteBusy & e); template T retrySQLite(F && fun) { + time_t nextWarning = time(0) + 1; + while (true) { try { return fun(); + } catch (SQLiteBusy & e) { - handleSQLiteBusy(e); + time_t now = time(0); + bool shouldWarn = false; + + if (now > nextWarning) { + nextWarning = now + 10; + shouldWarn = true; + } + + handleSQLiteBusy(e, shouldWarn); } } } From da322ebda62470acbf5373374c4cee8236705c2f Mon Sep 17 00:00:00 2001 From: Ben Radford Date: Tue, 11 Apr 2023 10:47:53 +0100 Subject: [PATCH 19/30] Revert "Add script to reproduce issue by inducing heavy load." This reverts commit 213b838f9cfb820d2bc76d7c6edc468b27029945. --- repro-7998.sh | 33 --------------------------------- 1 file changed, 33 deletions(-) delete mode 100755 repro-7998.sh diff --git a/repro-7998.sh b/repro-7998.sh deleted file mode 100755 index b022d31ab..000000000 --- a/repro-7998.sh +++ /dev/null @@ -1,33 +0,0 @@ -#!/usr/bin/env bash -set -eux -LOG_FILE=/tmp/repro-7998.log -rm -f "$LOG_FILE" -for i in {1..8}; do - ( - while true; do - nix-build \ - --argstr uuid $(uuidgen) \ - --arg drvCount $((RANDOM % 256)) \ - -E ' - { uuid ? "00000000-0000-0000-0000-000000000000", drvCount ? 0 }: - with import { }; - let - mkDrv = name: buildInputs: - stdenv.mkDerivation { - inherit name; - inherit buildInputs; - unpackPhase = "date +\"${uuid} %F %T\" >date.txt"; - installPhase = "mkdir -p $out; cp date.txt $out/"; - }; - mkDrvs = n: - let - name = "repro-7998-${toString n}"; - buildInputs = if n == 0 then [ ] else [ (mkDrvs (n - 1)) ]; - in mkDrv name buildInputs; - in mkDrvs drvCount - ' - done 2>&1 | tee -a "$LOG_FILE" - ) & -done -read # Press enter to stop -pkill -KILL -f repro-7998.sh From de3df3009bf003f327d35e246d5904d93273e2e9 Mon Sep 17 00:00:00 2001 From: Ben Radford Date: Tue, 11 Apr 2023 16:03:37 +0100 Subject: [PATCH 20/30] Move warning timing logic into handleSQLiteBusy. --- src/libstore/sqlite.cc | 6 ++++-- src/libstore/sqlite.hh | 13 ++----------- 2 files changed, 6 insertions(+), 13 deletions(-) diff --git a/src/libstore/sqlite.cc b/src/libstore/sqlite.cc index c57e58fe0..df334c23c 100644 --- a/src/libstore/sqlite.cc +++ b/src/libstore/sqlite.cc @@ -239,9 +239,11 @@ SQLiteTxn::~SQLiteTxn() } } -void handleSQLiteBusy(const SQLiteBusy & e, bool shouldWarn) +void handleSQLiteBusy(const SQLiteBusy & e, time_t & nextWarning) { - if (shouldWarn) { + time_t now = time(0); + if (now > nextWarning) { + nextWarning = now + 10; logWarning({ .msg = hintfmt(e.what()) }); diff --git a/src/libstore/sqlite.hh b/src/libstore/sqlite.hh index e2c9e28f8..6e14852cb 100644 --- a/src/libstore/sqlite.hh +++ b/src/libstore/sqlite.hh @@ -139,7 +139,7 @@ protected: MakeError(SQLiteBusy, SQLiteError); -void handleSQLiteBusy(const SQLiteBusy & e, bool shouldWarn); +void handleSQLiteBusy(const SQLiteBusy & e, time_t & nextWarning); /** * Convenience function for retrying a SQLite transaction when the @@ -153,17 +153,8 @@ T retrySQLite(F && fun) while (true) { try { return fun(); - } catch (SQLiteBusy & e) { - time_t now = time(0); - bool shouldWarn = false; - - if (now > nextWarning) { - nextWarning = now + 10; - shouldWarn = true; - } - - handleSQLiteBusy(e, shouldWarn); + handleSQLiteBusy(e, nextWarning); } } } From ee97f107e8749d61ae5e310a2d457637ccc109ce Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 13 Apr 2023 13:38:12 -0400 Subject: [PATCH 21/30] Push `getFSAccessor` `unsupported(...)` down `Store` class hierarchy More progress on issue #5729. Instead of having it by the default method in `Store` itself, have it be the implementation in `DummyStore` and `LegacySSHStore`. Then just the implementations which fail to provide the method pay the "penalty" of dealing with the icky `unimplemented` function for non-compliance. Combined with my other recent PRs, this finally makes `Store` have no `unsupported` calls! --- src/libstore/dummy-store.cc | 3 +++ src/libstore/legacy-ssh-store.cc | 3 +++ src/libstore/store-api.hh | 3 +-- 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/libstore/dummy-store.cc b/src/libstore/dummy-store.cc index ae2777d0c..74d6ed3b5 100644 --- a/src/libstore/dummy-store.cc +++ b/src/libstore/dummy-store.cc @@ -71,6 +71,9 @@ struct DummyStore : public virtual DummyStoreConfig, public virtual Store void queryRealisationUncached(const DrvOutput &, Callback> callback) noexcept override { callback(nullptr); } + + virtual ref getFSAccessor() override + { unsupported("getFSAccessor"); } }; static RegisterStoreImplementation regDummyStore; diff --git a/src/libstore/legacy-ssh-store.cc b/src/libstore/legacy-ssh-store.cc index eb471d8fc..d2ddbbe5f 100644 --- a/src/libstore/legacy-ssh-store.cc +++ b/src/libstore/legacy-ssh-store.cc @@ -342,6 +342,9 @@ public: void ensurePath(const StorePath & path) override { unsupported("ensurePath"); } + virtual ref getFSAccessor() override + { unsupported("getFSAccessor"); } + void computeFSClosure(const StorePathSet & paths, StorePathSet & out, bool flipDirection = false, bool includeOutputs = false, bool includeDerivers = false) override diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 3cb48bff5..5bee272bf 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -678,8 +678,7 @@ public: /** * @return An object to access files in the Nix store. */ - virtual ref getFSAccessor() - { unsupported("getFSAccessor"); } + virtual ref getFSAccessor() = 0; /** * Repair the contents of the given path by redownloading it using From 9e8f2090365a509656dead69bc91fb6615cf9d05 Mon Sep 17 00:00:00 2001 From: Raphael Robatsch Date: Thu, 13 Apr 2023 16:22:45 +0200 Subject: [PATCH 22/30] Display valid installable in InstallableDerivedPath::parse warning The warning message should produce an installable name that can be passed to `nix build`, `nix path-info`, etc. again. Since the CLI expects that the .drv path and the output names are separated by a caret, the warning message must also separate the .drv path and output names with a caret. However, `DerivedPath::Built.to_string()` uses an exclamation point as the separator instead. This commit adds a `separator` argument to the to_string method. This changes the warning message from: If this command is now failing try again with '/nix/store/foo.drv!*' to: If this command is now failing try again with '/nix/store/foo.drv^*' --- src/libcmd/installable-derived-path.cc | 2 +- src/libstore/derived-path.cc | 13 +++++++------ src/libstore/derived-path.hh | 4 ++-- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/libcmd/installable-derived-path.cc b/src/libcmd/installable-derived-path.cc index 6ecf54b7c..35f6c5bfe 100644 --- a/src/libcmd/installable-derived-path.cc +++ b/src/libcmd/installable-derived-path.cc @@ -47,7 +47,7 @@ InstallableDerivedPath InstallableDerivedPath::parse( }; warn( "The interpretation of store paths arguments ending in `.drv` recently changed. If this command is now failing try again with '%s'", - oldDerivedPath.to_string(*store)); + oldDerivedPath.to_string(*store, '^')); }; return DerivedPath::Opaque { .path = std::move(storePath), diff --git a/src/libstore/derived-path.cc b/src/libstore/derived-path.cc index e5f0f1b33..6baca70a3 100644 --- a/src/libstore/derived-path.cc +++ b/src/libstore/derived-path.cc @@ -59,18 +59,19 @@ std::string DerivedPath::Opaque::to_string(const Store & store) const return store.printStorePath(path); } -std::string DerivedPath::Built::to_string(const Store & store) const +std::string DerivedPath::Built::to_string(const Store & store, char separator) const { return store.printStorePath(drvPath) - + "!" + + separator + outputs.to_string(); } -std::string DerivedPath::to_string(const Store & store) const +std::string DerivedPath::to_string(const Store & store, char separator) const { - return std::visit( - [&](const auto & req) { return req.to_string(store); }, - this->raw()); + return std::visit(overloaded { + [&](const DerivedPath::Built & req) { return req.to_string(store, separator); }, + [&](const DerivedPath::Opaque & req) { return req.to_string(store); }, + }, this->raw()); } diff --git a/src/libstore/derived-path.hh b/src/libstore/derived-path.hh index 2155776b1..9fc64e2f2 100644 --- a/src/libstore/derived-path.hh +++ b/src/libstore/derived-path.hh @@ -48,7 +48,7 @@ struct DerivedPathBuilt { StorePath drvPath; OutputsSpec outputs; - std::string to_string(const Store & store) const; + std::string to_string(const Store & store, char separator = '!') const; static DerivedPathBuilt parse(const Store & store, std::string_view, std::string_view); nlohmann::json toJSON(ref store) const; @@ -81,7 +81,7 @@ struct DerivedPath : _DerivedPathRaw { return static_cast(*this); } - std::string to_string(const Store & store) const; + std::string to_string(const Store & store, char separator = '!') const; static DerivedPath parse(const Store & store, std::string_view); }; From d93e76fbb80c9f407ddac128357350f54add68fb Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 6 Apr 2023 11:09:01 -0400 Subject: [PATCH 23/30] Start cross-referencing experimental features - Create a glossary entry for experimental features. - Have the man page experimental feature notice link `nix-commmand`. (Eventually this should be programmed, based on whether the command is experimental, and if so what experimental feature does it depend on.) - Document which installables depend on which experimental features. I tried to use the same style (bold warning and block quote) that the top of the man page uses. Co-authored-by: Valentin Gagarin --- doc/manual/generate-manpage.nix | 4 ++- .../src/command-ref/experimental-commands.md | 2 +- doc/manual/src/glossary.md | 6 +++++ .../src/language/advanced-attributes.md | 26 +++++++++++++++---- src/nix/nix.md | 14 +++++++++- 5 files changed, 44 insertions(+), 8 deletions(-) diff --git a/doc/manual/generate-manpage.nix b/doc/manual/generate-manpage.nix index 86f2ca567..0b3a23801 100644 --- a/doc/manual/generate-manpage.nix +++ b/doc/manual/generate-manpage.nix @@ -10,7 +10,9 @@ let result = '' > **Warning** \ - > This program is **experimental** and its interface is subject to change. + > This program is + > [**experimental**](@docroot@/contributing/experimental-features.md#xp-feature-nix-command) + > and its interface is subject to change. # Name diff --git a/doc/manual/src/command-ref/experimental-commands.md b/doc/manual/src/command-ref/experimental-commands.md index cfa6f8b73..286ddc6d6 100644 --- a/doc/manual/src/command-ref/experimental-commands.md +++ b/doc/manual/src/command-ref/experimental-commands.md @@ -1,6 +1,6 @@ # Experimental Commands -This section lists experimental commands. +This section lists [experimental commands](@docroot@/contributing/experimental-features.md#xp-feature-nix-command). > **Warning** > diff --git a/doc/manual/src/glossary.md b/doc/manual/src/glossary.md index 4eedb2e93..a9782be5c 100644 --- a/doc/manual/src/glossary.md +++ b/doc/manual/src/glossary.md @@ -225,3 +225,9 @@ [string]: ./language/values.md#type-string [path]: ./language/values.md#type-path [attribute name]: ./language/values.md#attribute-set + + - [experimental feature]{#gloss-experimental-feature}\ + Not yet stabilized functionality guarded by named experimental feature flags. + These flags are enabled or disabled with the [`experimental-features`](./command-ref/conf-file.html#conf-experimental-features) setting. + + See the contribution guide on the [purpose and lifecycle of experimental feaures](@docroot@/contributing/experimental-features.md). diff --git a/doc/manual/src/language/advanced-attributes.md b/doc/manual/src/language/advanced-attributes.md index 3e8c48890..307971434 100644 --- a/doc/manual/src/language/advanced-attributes.md +++ b/doc/manual/src/language/advanced-attributes.md @@ -208,12 +208,26 @@ Derivations can declare some infrequently used optional attributes. about converting to and from base-32 notation.) - [`__contentAddressed`]{#adv-attr-__contentAddressed} - If this **experimental** attribute is set to true, then the derivation + > **Warning** + > This attribute is part of an [experimental feature](@docroot@/contributing/experimental-features.md). + > + > To use this attribute, you must enable the + > [`ca-derivations`](@docroot@/contributing/experimental-features.md#xp-feature-ca-derivations) experimental feature. + > For example, in [nix.conf](../command-ref/conf-file.md) you could add: + > + > ``` + > extra-experimental-features = ca-derivations + > ``` + + If this attribute is set to `true`, then the derivation outputs will be stored in a content-addressed location rather than the traditional input-addressed one. - This only has an effect if the `ca-derivations` experimental feature is enabled. - Setting this attribute also requires setting `outputHashMode` and `outputHashAlgo` like for *fixed-output derivations* (see above). + Setting this attribute also requires setting + [`outputHashMode`](#adv-attr-outputHashMode) + and + [`outputHashAlgo`](#adv-attr-outputHashAlgo) + like for *fixed-output derivations* (see above). - [`passAsFile`]{#adv-attr-passAsFile}\ A list of names of attributes that should be passed via files rather @@ -307,9 +321,11 @@ Derivations can declare some infrequently used optional attributes. - [`unsafeDiscardReferences`]{#adv-attr-unsafeDiscardReferences}\ > **Warning** - > This is an experimental feature. + > This attribute is part of an [experimental feature](@docroot@/contributing/experimental-features.md). > - > To enable it, add the following to [nix.conf](../command-ref/conf-file.md): + > To use this attribute, you must enable the + > [`discard-references`](@docroot@/contributing/experimental-features.md#xp-feature-discard-references) experimental feature. + > For example, in [nix.conf](../command-ref/conf-file.md) you could add: > > ``` > extra-experimental-features = discard-references diff --git a/src/nix/nix.md b/src/nix/nix.md index e1865b31c..1ef6c7fcd 100644 --- a/src/nix/nix.md +++ b/src/nix/nix.md @@ -48,12 +48,17 @@ manual](https://nixos.org/manual/nix/stable/). # Installables +> **Warning** \ +> Installables are part of the unstable +> [`nix-command` experimental feature](@docroot@/contributing/experimental-features.md#xp-feature-nix-command), +> and subject to change without notice. + Many `nix` subcommands operate on one or more *installables*. These are command line arguments that represent something that can be realised in the Nix store. The following types of installable are supported by most commands: -- [Flake output attribute](#flake-output-attribute) +- [Flake output attribute](#flake-output-attribute) (experimental) - [Store path](#store-path) - [Nix file](#nix-file), optionally qualified by an attribute path - [Nix expression](#nix-expression), optionally qualified by an attribute path @@ -63,6 +68,13 @@ That is, Nix will operate on the default flake output attribute of the flake in ### Flake output attribute +> **Warning** \ +> Flake output attribute installables depend on both the +> [`flakes`](@docroot@/contributing/experimental-features.md#xp-feature-flakes) +> and +> [`nix-command`](@docroot@/contributing/experimental-features.md#xp-feature-nix-command) +> experimental features, and subject to change without notice. + Example: `nixpkgs#hello` These have the form *flakeref*[`#`*attrpath*], where *flakeref* is a From ee420ac64e7d1f51f5abcb069dbe84cd6ff707ce Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 14 Apr 2023 20:45:11 -0400 Subject: [PATCH 24/30] Legacy vs non-legacy `to_string`/`parse` for `DerivedPath` As requested by @roberth, it is good to call out the specific instances we care about, which is `!` for the RPC protocols, and `^` for humans. This doesn't take advantage of parametricity as much, but since the human and computer interfaces are good to decouple anyways (we don't care if they drift further apart over time in the slightest) some separation and slight duplication is fine. Also, unit test both round trips. --- src/libcmd/installable-derived-path.cc | 2 +- src/libstore/derived-path.cc | 37 +++++++++++++++++++++----- src/libstore/derived-path.hh | 30 ++++++++++++++++++--- src/libstore/remote-store.cc | 4 +-- src/libstore/tests/derived-path.cc | 8 ++++++ 5 files changed, 69 insertions(+), 12 deletions(-) diff --git a/src/libcmd/installable-derived-path.cc b/src/libcmd/installable-derived-path.cc index 35f6c5bfe..6ecf54b7c 100644 --- a/src/libcmd/installable-derived-path.cc +++ b/src/libcmd/installable-derived-path.cc @@ -47,7 +47,7 @@ InstallableDerivedPath InstallableDerivedPath::parse( }; warn( "The interpretation of store paths arguments ending in `.drv` recently changed. If this command is now failing try again with '%s'", - oldDerivedPath.to_string(*store, '^')); + oldDerivedPath.to_string(*store)); }; return DerivedPath::Opaque { .path = std::move(storePath), diff --git a/src/libstore/derived-path.cc b/src/libstore/derived-path.cc index 6baca70a3..9a2ffda39 100644 --- a/src/libstore/derived-path.cc +++ b/src/libstore/derived-path.cc @@ -59,17 +59,32 @@ std::string DerivedPath::Opaque::to_string(const Store & store) const return store.printStorePath(path); } -std::string DerivedPath::Built::to_string(const Store & store, char separator) const +std::string DerivedPath::Built::to_string(const Store & store) const { return store.printStorePath(drvPath) - + separator + + '^' + outputs.to_string(); } -std::string DerivedPath::to_string(const Store & store, char separator) const +std::string DerivedPath::Built::to_string_legacy(const Store & store) const +{ + return store.printStorePath(drvPath) + + '!' + + outputs.to_string(); +} + +std::string DerivedPath::to_string(const Store & store) const { return std::visit(overloaded { - [&](const DerivedPath::Built & req) { return req.to_string(store, separator); }, + [&](const DerivedPath::Built & req) { return req.to_string(store); }, + [&](const DerivedPath::Opaque & req) { return req.to_string(store); }, + }, this->raw()); +} + +std::string DerivedPath::to_string_legacy(const Store & store) const +{ + return std::visit(overloaded { + [&](const DerivedPath::Built & req) { return req.to_string_legacy(store); }, [&](const DerivedPath::Opaque & req) { return req.to_string(store); }, }, this->raw()); } @@ -88,14 +103,24 @@ DerivedPath::Built DerivedPath::Built::parse(const Store & store, std::string_vi }; } -DerivedPath DerivedPath::parse(const Store & store, std::string_view s) +static inline DerivedPath parseWith(const Store & store, std::string_view s, std::string_view separator) { - size_t n = s.find("!"); + size_t n = s.find(separator); return n == s.npos ? (DerivedPath) DerivedPath::Opaque::parse(store, s) : (DerivedPath) DerivedPath::Built::parse(store, s.substr(0, n), s.substr(n + 1)); } +DerivedPath DerivedPath::parse(const Store & store, std::string_view s) +{ + return parseWith(store, s, "^"); +} + +DerivedPath DerivedPath::parseLegacy(const Store & store, std::string_view s) +{ + return parseWith(store, s, "!"); +} + RealisedPath::Set BuiltPath::toRealisedPaths(Store & store) const { RealisedPath::Set res; diff --git a/src/libstore/derived-path.hh b/src/libstore/derived-path.hh index 9fc64e2f2..5f7acbebc 100644 --- a/src/libstore/derived-path.hh +++ b/src/libstore/derived-path.hh @@ -48,8 +48,18 @@ struct DerivedPathBuilt { StorePath drvPath; OutputsSpec outputs; - std::string to_string(const Store & store, char separator = '!') const; - static DerivedPathBuilt parse(const Store & store, std::string_view, std::string_view); + /** + * Uses `^` as the separator + */ + std::string to_string(const Store & store) const; + /** + * Uses `!` as the separator + */ + std::string to_string_legacy(const Store & store) const; + /** + * The caller splits on the separator, so it works for both variants. + */ + static DerivedPathBuilt parse(const Store & store, std::string_view drvPath, std::string_view outputs); nlohmann::json toJSON(ref store) const; GENERATE_CMP(DerivedPathBuilt, me->drvPath, me->outputs); @@ -81,8 +91,22 @@ struct DerivedPath : _DerivedPathRaw { return static_cast(*this); } - std::string to_string(const Store & store, char separator = '!') const; + /** + * Uses `^` as the separator + */ + std::string to_string(const Store & store) const; + /** + * Uses `!` as the separator + */ + std::string to_string_legacy(const Store & store) const; + /** + * Uses `^` as the separator + */ static DerivedPath parse(const Store & store, std::string_view); + /** + * Uses `!` as the separator + */ + static DerivedPath parseLegacy(const Store & store, std::string_view); }; /** diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index e128c3a29..b862902d1 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -90,12 +90,12 @@ void write(const Store & store, Sink & out, const ContentAddress & ca) DerivedPath read(const Store & store, Source & from, Phantom _) { auto s = readString(from); - return DerivedPath::parse(store, s); + return DerivedPath::parseLegacy(store, s); } void write(const Store & store, Sink & out, const DerivedPath & req) { - out << req.to_string(store); + out << req.to_string_legacy(store); } diff --git a/src/libstore/tests/derived-path.cc b/src/libstore/tests/derived-path.cc index d1ac2c5e7..e6d32dbd0 100644 --- a/src/libstore/tests/derived-path.cc +++ b/src/libstore/tests/derived-path.cc @@ -51,6 +51,14 @@ TEST_F(DerivedPathTest, force_init) { } +RC_GTEST_FIXTURE_PROP( + DerivedPathTest, + prop_legacy_round_rip, + (const DerivedPath & o)) +{ + RC_ASSERT(o == DerivedPath::parseLegacy(*store, o.to_string_legacy(*store))); +} + RC_GTEST_FIXTURE_PROP( DerivedPathTest, prop_round_rip, From 9c74df5bb4f41c938a4f6942492f5ce92b0ef371 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 15 Apr 2023 20:56:51 +0200 Subject: [PATCH 25/30] Format Co-authored-by: Eelco Dolstra Co-authored-by: John Ericson --- src/libexpr/value/print.cc | 6 ++++-- src/libstore/derivations.cc | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/libexpr/value/print.cc b/src/libexpr/value/print.cc index cf97def46..c67ff9f87 100644 --- a/src/libexpr/value/print.cc +++ b/src/libexpr/value/print.cc @@ -3,7 +3,8 @@ namespace nix { std::ostream & -printLiteral(std::ostream & str, const std::string_view string) { +printLiteral(std::ostream & str, const std::string_view string) +{ str << "\""; for (auto i = string.begin(); i != string.end(); ++i) { if (*i == '\"' || *i == '\\') str << "\\" << *i; @@ -18,7 +19,8 @@ printLiteral(std::ostream & str, const std::string_view string) { } std::ostream & -printLiteral(std::ostream & str, bool boolean) { +printLiteral(std::ostream & str, bool boolean) +{ str << (boolean ? "true" : "false"); return str; } diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index 9948862e5..7eb5cd275 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -314,7 +314,7 @@ Derivation parseDerivation(const Store & store, std::string && s, std::string_vi /** - * Print a derivation string literal to an std::string. + * Print a derivation string literal to an `std::string`. * * This syntax does not generalize to the expression language, which needs to * escape `$`. From 1e2dd669bcdd8df6cdaac061e035828626906447 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sun, 16 Apr 2023 12:56:31 +0200 Subject: [PATCH 26/30] printLiteral: Do not overload --- src/libcmd/repl.cc | 6 +++--- src/libexpr/eval.cc | 4 ++-- src/libexpr/nixexpr.cc | 6 +++--- src/libexpr/value/print.cc | 4 ++-- src/libexpr/value/print.hh | 12 ++++++------ 5 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index 1366622c7..41cf77424 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -913,13 +913,13 @@ std::ostream & NixRepl::printValue(std::ostream & str, Value & v, unsigned int m case nBool: str << ANSI_CYAN; - printLiteral(str, v.boolean); + printLiteralBool(str, v.boolean); str << ANSI_NORMAL; break; case nString: str << ANSI_WARNING; - printLiteral(str, v.string.s); + printLiteralString(str, v.string.s); str << ANSI_NORMAL; break; @@ -959,7 +959,7 @@ std::ostream & NixRepl::printValue(std::ostream & str, Value & v, unsigned int m if (isVarName(i.first)) str << i.first; else - printLiteral(str, i.first); + printLiteralString(str, i.first); str << " = "; if (seen.count(i.second)) str << "«repeated»"; diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 06208897f..bd05fc156 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -105,10 +105,10 @@ void Value::print(const SymbolTable & symbols, std::ostream & str, str << integer; break; case tBool: - printLiteral(str, boolean); + printLiteralBool(str, boolean); break; case tString: - printLiteral(str, string.s); + printLiteralString(str, string.s); break; case tPath: str << path; // !!! escaping? diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index ca6df0af3..1b5d522d3 100644 --- a/src/libexpr/nixexpr.cc +++ b/src/libexpr/nixexpr.cc @@ -74,7 +74,7 @@ std::ostream & operator <<(std::ostream & str, const SymbolStr & symbol) else { char c = s[0]; if (!((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || c == '_')) { - printLiteral(str, s); + printLiteralString(str, s); return str; } for (auto c : s) @@ -82,7 +82,7 @@ std::ostream & operator <<(std::ostream & str, const SymbolStr & symbol) (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '_' || c == '\'' || c == '-')) { - printLiteral(str, s); + printLiteralString(str, s); return str; } str << s; @@ -107,7 +107,7 @@ void ExprFloat::show(const SymbolTable & symbols, std::ostream & str) const void ExprString::show(const SymbolTable & symbols, std::ostream & str) const { - printLiteral(str, s); + printLiteralString(str, s); } void ExprPath::show(const SymbolTable & symbols, std::ostream & str) const diff --git a/src/libexpr/value/print.cc b/src/libexpr/value/print.cc index c67ff9f87..bd241d9d8 100644 --- a/src/libexpr/value/print.cc +++ b/src/libexpr/value/print.cc @@ -3,7 +3,7 @@ namespace nix { std::ostream & -printLiteral(std::ostream & str, const std::string_view string) +printLiteralString(std::ostream & str, const std::string_view string) { str << "\""; for (auto i = string.begin(); i != string.end(); ++i) { @@ -19,7 +19,7 @@ printLiteral(std::ostream & str, const std::string_view string) } std::ostream & -printLiteral(std::ostream & str, bool boolean) +printLiteralBool(std::ostream & str, bool boolean) { str << (boolean ? "true" : "false"); return str; diff --git a/src/libexpr/value/print.hh b/src/libexpr/value/print.hh index 31c94eb85..98dd2008d 100644 --- a/src/libexpr/value/print.hh +++ b/src/libexpr/value/print.hh @@ -17,14 +17,14 @@ namespace nix { * * @param s The logical string */ - std::ostream & printLiteral(std::ostream & o, std::string_view s); - inline std::ostream & printLiteral(std::ostream & o, const char * s) { - return printLiteral(o, std::string_view(s)); + std::ostream & printLiteralString(std::ostream & o, std::string_view s); + inline std::ostream & printLiteralString(std::ostream & o, const char * s) { + return printLiteralString(o, std::string_view(s)); } - inline std::ostream & printLiteral(std::ostream & o, const std::string & s) { - return printLiteral(o, std::string_view(s)); + inline std::ostream & printLiteralString(std::ostream & o, const std::string & s) { + return printLiteralString(o, std::string_view(s)); } /** Print `true` or `false`. */ - std::ostream & printLiteral(std::ostream & o, bool b); + std::ostream & printLiteralBool(std::ostream & o, bool b); } From 28a5cdde02964306e7eb443f696c8d5d59ebf9e9 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sun, 16 Apr 2023 13:10:45 +0200 Subject: [PATCH 27/30] libexpr/value/print.* -> libexpr/print.* Generalizes the file to sensibly allow printing any part of the language syntax. --- src/libcmd/repl.cc | 2 +- src/libexpr/eval.cc | 2 +- src/libexpr/nixexpr.cc | 2 +- src/libexpr/{value => }/print.cc | 2 +- src/libexpr/{value => }/print.hh | 0 5 files changed, 4 insertions(+), 4 deletions(-) rename src/libexpr/{value => }/print.cc (96%) rename src/libexpr/{value => }/print.hh (100%) diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index 41cf77424..806dce024 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -40,7 +40,7 @@ extern "C" { #include "markdown.hh" #include "local-fs-store.hh" #include "progress-bar.hh" -#include "value/print.hh" +#include "print.hh" #if HAVE_BOEHMGC #define GC_INCLUDE_NEW diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index bd05fc156..6668add8c 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -9,7 +9,7 @@ #include "filetransfer.hh" #include "function-trace.hh" #include "profiles.hh" -#include "value/print.hh" +#include "print.hh" #include #include diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index 1b5d522d3..d8f3cd701 100644 --- a/src/libexpr/nixexpr.cc +++ b/src/libexpr/nixexpr.cc @@ -3,7 +3,7 @@ #include "eval.hh" #include "symbol-table.hh" #include "util.hh" -#include "value/print.hh" +#include "print.hh" #include diff --git a/src/libexpr/value/print.cc b/src/libexpr/print.cc similarity index 96% rename from src/libexpr/value/print.cc rename to src/libexpr/print.cc index bd241d9d8..282903b72 100644 --- a/src/libexpr/value/print.cc +++ b/src/libexpr/print.cc @@ -1,4 +1,4 @@ -#include "value/print.hh" +#include "print.hh" namespace nix { diff --git a/src/libexpr/value/print.hh b/src/libexpr/print.hh similarity index 100% rename from src/libexpr/value/print.hh rename to src/libexpr/print.hh From b6125772d7d5f82d48873fc93a7f261832154b14 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sun, 16 Apr 2023 14:07:35 +0200 Subject: [PATCH 28/30] libexpr: Move identifier-like printing to print.cc --- src/libcmd/repl.cc | 6 ++--- src/libexpr/nixexpr.cc | 27 +++-------------------- src/libexpr/print.cc | 50 ++++++++++++++++++++++++++++++++++++++++++ src/libexpr/print.hh | 18 +++++++++++++++ 4 files changed, 73 insertions(+), 28 deletions(-) diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index 806dce024..80c08bf1c 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -426,6 +426,7 @@ StringSet NixRepl::completePrefix(const std::string & prefix) } +// FIXME: DRY and match or use the parser static bool isVarName(std::string_view s) { if (s.size() == 0) return false; @@ -956,10 +957,7 @@ std::ostream & NixRepl::printValue(std::ostream & str, Value & v, unsigned int m sorted.emplace(state->symbols[i.name], i.value); for (auto & i : sorted) { - if (isVarName(i.first)) - str << i.first; - else - printLiteralString(str, i.first); + printAttributeName(str, i.first); str << " = "; if (seen.count(i.second)) str << "«repeated»"; diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index d8f3cd701..1557cbbeb 100644 --- a/src/libexpr/nixexpr.cc +++ b/src/libexpr/nixexpr.cc @@ -61,33 +61,12 @@ Pos::operator std::shared_ptr() const return pos; } -/* Displaying abstract syntax trees. */ - +// FIXME: remove, because *symbols* are abstract and do not have a single +// textual representation; see printIdentifier() std::ostream & operator <<(std::ostream & str, const SymbolStr & symbol) { std::string_view s = symbol; - - if (s.empty()) - str << "\"\""; - else if (s == "if") // FIXME: handle other keywords - str << '"' << s << '"'; - else { - char c = s[0]; - if (!((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || c == '_')) { - printLiteralString(str, s); - return str; - } - for (auto c : s) - if (!((c >= 'a' && c <= 'z') || - (c >= 'A' && c <= 'Z') || - (c >= '0' && c <= '9') || - c == '_' || c == '\'' || c == '-')) { - printLiteralString(str, s); - return str; - } - str << s; - } - return str; + return printIdentifier(str, s); } void Expr::show(const SymbolTable & symbols, std::ostream & str) const diff --git a/src/libexpr/print.cc b/src/libexpr/print.cc index 282903b72..d08672cfc 100644 --- a/src/libexpr/print.cc +++ b/src/libexpr/print.cc @@ -25,4 +25,54 @@ printLiteralBool(std::ostream & str, bool boolean) return str; } +std::ostream & +printIdentifier(std::ostream & str, std::string_view s) { + if (s.empty()) + str << "\"\""; + else if (s == "if") // FIXME: handle other keywords + str << '"' << s << '"'; + else { + char c = s[0]; + if (!((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || c == '_')) { + printLiteralString(str, s); + return str; + } + for (auto c : s) + if (!((c >= 'a' && c <= 'z') || + (c >= 'A' && c <= 'Z') || + (c >= '0' && c <= '9') || + c == '_' || c == '\'' || c == '-')) { + printLiteralString(str, s); + return str; + } + str << s; + } + return str; +} + +// FIXME: keywords +static bool isVarName(std::string_view s) +{ + if (s.size() == 0) return false; + char c = s[0]; + if ((c >= '0' && c <= '9') || c == '-' || c == '\'') return false; + for (auto & i : s) + if (!((i >= 'a' && i <= 'z') || + (i >= 'A' && i <= 'Z') || + (i >= '0' && i <= '9') || + i == '_' || i == '-' || i == '\'')) + return false; + return true; +} + +std::ostream & +printAttributeName(std::ostream & str, std::string_view name) { + if (isVarName(name)) + str << name; + else + printLiteralString(str, name); + return str; +} + + } diff --git a/src/libexpr/print.hh b/src/libexpr/print.hh index 98dd2008d..f9cfc3964 100644 --- a/src/libexpr/print.hh +++ b/src/libexpr/print.hh @@ -27,4 +27,22 @@ namespace nix { /** Print `true` or `false`. */ std::ostream & printLiteralBool(std::ostream & o, bool b); + + /** + * Print a string as an attribute name in the Nix expression language syntax. + * + * Prints a quoted string if necessary. + */ + std::ostream & printAttributeName(std::ostream & o, std::string_view s); + + /** + * Print a string as an identifier in the Nix expression language syntax. + * + * FIXME: "identifier" is ambiguous. Identifiers do not have a single + * textual representation. They can be used in variable references, + * let bindings, left-hand sides or attribute names in a select + * expression, or something else entirely, like JSON. Use one of the + * `print*` functions instead. + */ + std::ostream & printIdentifier(std::ostream & o, std::string_view s); } From ba9ae691b6b0d8f6e23a2a9468aef51a75f16cfe Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sun, 16 Apr 2023 10:44:03 -0400 Subject: [PATCH 29/30] Add `optionalString` to manual Nix lang utilities Use it everywhere it could be also. --- doc/manual/generate-manpage.nix | 28 +++++++++++++++------------- doc/manual/utils.nix | 4 +++- 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/doc/manual/generate-manpage.nix b/doc/manual/generate-manpage.nix index 0b3a23801..d04eecf55 100644 --- a/doc/manual/generate-manpage.nix +++ b/doc/manual/generate-manpage.nix @@ -31,19 +31,18 @@ let showSynopsis = command: args: let - showArgument = arg: "*${arg.label}*" + (if arg ? arity then "" else "..."); + showArgument = arg: "*${arg.label}*" + optionalString (! arg ? arity) "..."; arguments = concatStringsSep " " (map showArgument args); in '' `${command}` [*option*...] ${arguments} ''; - maybeSubcommands = if details ? commands && details.commands != {} - then '' + maybeSubcommands = optionalString (details ? commands && details.commands != {}) + '' where *subcommand* is one of the following: ${subcommands} - '' - else ""; + ''; subcommands = if length categories > 1 then listCategories @@ -65,12 +64,11 @@ let * [`${command} ${name}`](./${appendName filename name}.md) - ${subcmd.description} ''; - maybeDocumentation = - if details ? doc - then replaceStrings ["@stores@"] [storeDocs] details.doc - else ""; + maybeDocumentation = optionalString + (details ? doc) + (replaceStrings ["@stores@"] [storeDocs] details.doc); - maybeOptions = if details.flags == {} then "" else '' + maybeOptions = optionalString (details.flags != {}) '' # Options ${showOptions details.flags toplevel.flags} @@ -80,15 +78,19 @@ let let allOptions = options // commonOptions; showCategory = cat: '' - ${if cat != "" then "**${cat}:**" else ""} + ${optionalString (cat != "") "**${cat}:**"} ${listOptions (filterAttrs (n: v: v.category == cat) allOptions)} ''; listOptions = opts: concatStringsSep "\n" (attrValues (mapAttrs showOption opts)); showOption = name: option: let - shortName = if option ? shortName then "/ `-${option.shortName}`" else ""; - labels = if option ? labels then (concatStringsSep " " (map (s: "*${s}*") option.labels)) else ""; + shortName = optionalString + (option ? shortName) + ("/ `-${option.shortName}`"); + labels = optionalString + (option ? labels) + (concatStringsSep " " (map (s: "*${s}*") option.labels)); in trim '' - `--${name}` ${shortName} ${labels} diff --git a/doc/manual/utils.nix b/doc/manual/utils.nix index 82544935a..e69cbe658 100644 --- a/doc/manual/utils.nix +++ b/doc/manual/utils.nix @@ -42,6 +42,8 @@ rec { filterAttrs = pred: set: listToAttrs (concatMap (name: let v = set.${name}; in if pred name v then [(nameValuePair name v)] else []) (attrNames set)); + optionalString = cond: string: if cond then string else ""; + showSetting = { useAnchors }: name: { description, documentDefault, defaultValue, aliases, value }: let result = squash '' @@ -74,7 +76,7 @@ rec { else "*machine-specific*"; showAliases = aliases: - if aliases == [] then "" else + optionalString (aliases != []) "**Deprecated alias:** ${(concatStringsSep ", " (map (s: "`${s}`") aliases))}"; in result; From 9800c1e8074d248f75ea9bed1b5a0f76e799863d Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 10 Apr 2023 12:02:35 -0400 Subject: [PATCH 30/30] Mark experimental configuration settings programmatically Fix #8162 The test is changed to compare `nlohmann::json` values, not strings of dumped JSON, which allows us to format things more nicely. --- doc/manual/utils.nix | 20 +++++++++++++++- src/libstore/globals.hh | 20 ---------------- src/libutil/config.cc | 4 ++++ src/libutil/tests/config.cc | 48 ++++++++++++++++++++++++++++++++++--- 4 files changed, 68 insertions(+), 24 deletions(-) diff --git a/doc/manual/utils.nix b/doc/manual/utils.nix index e69cbe658..9043dd8cd 100644 --- a/doc/manual/utils.nix +++ b/doc/manual/utils.nix @@ -44,7 +44,7 @@ rec { optionalString = cond: string: if cond then string else ""; - showSetting = { useAnchors }: name: { description, documentDefault, defaultValue, aliases, value }: + showSetting = { useAnchors }: name: { description, documentDefault, defaultValue, aliases, value, experimentalFeature }: let result = squash '' - ${if useAnchors @@ -54,10 +54,28 @@ rec { ${indent " " body} ''; + experimentalFeatureNote = optionalString (experimentalFeature != null) '' + > **Warning** + > This setting is part of an + > [experimental feature](@docroot@/contributing/experimental-features.md). + + To change this setting, you need to make sure the corresponding experimental feature, + [`${experimentalFeature}`](@docroot@/contributing/experimental-features.md#xp-feature-${experimentalFeature}), + is enabled. + For example, include the following in [`nix.conf`](#): + + ``` + extra-experimental-features = ${experimentalFeature} + ${name} = ... + ``` + ''; + # separate body to cleanly handle indentation body = '' ${description} + ${experimentalFeatureNote} + **Default:** ${showDefault documentDefault defaultValue} ${showAliases aliases} diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index 63c7389da..f598ed4a8 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -328,16 +328,6 @@ public: users in `build-users-group`. UIDs are allocated starting at 872415232 (0x34000000) on Linux and 56930 on macOS. - - > **Warning** - > This is an experimental feature. - - To enable it, add the following to [`nix.conf`](#): - - ``` - extra-experimental-features = auto-allocate-uids - auto-allocate-uids = true - ``` )"}; Setting startId{this, @@ -367,16 +357,6 @@ public: Cgroups are required and enabled automatically for derivations that require the `uid-range` system feature. - - > **Warning** - > This is an experimental feature. - - To enable it, add the following to [`nix.conf`](#): - - ``` - extra-experimental-features = cgroups - use-cgroups = true - ``` )"}; #endif diff --git a/src/libutil/config.cc b/src/libutil/config.cc index 5ff8d91ba..a42f3a849 100644 --- a/src/libutil/config.cc +++ b/src/libutil/config.cc @@ -191,6 +191,10 @@ std::map AbstractSetting::toJSONObject() std::map obj; obj.emplace("description", description); obj.emplace("aliases", aliases); + if (experimentalFeature) + obj.emplace("experimentalFeature", *experimentalFeature); + else + obj.emplace("experimentalFeature", nullptr); return obj; } diff --git a/src/libutil/tests/config.cc b/src/libutil/tests/config.cc index 8be6730dd..f250e934e 100644 --- a/src/libutil/tests/config.cc +++ b/src/libutil/tests/config.cc @@ -156,12 +156,54 @@ namespace nix { } TEST(Config, toJSONOnNonEmptyConfig) { + using nlohmann::literals::operator "" _json; Config config; - std::map settings; - Setting setting{&config, "", "name-of-the-setting", "description"}; + Setting setting{ + &config, + "", + "name-of-the-setting", + "description", + }; setting.assign("value"); - ASSERT_EQ(config.toJSON().dump(), R"#({"name-of-the-setting":{"aliases":[],"defaultValue":"","description":"description\n","documentDefault":true,"value":"value"}})#"); + ASSERT_EQ(config.toJSON(), + R"#({ + "name-of-the-setting": { + "aliases": [], + "defaultValue": "", + "description": "description\n", + "documentDefault": true, + "value": "value", + "experimentalFeature": null + } + })#"_json); + } + + TEST(Config, toJSONOnNonEmptyConfigWithExperimentalSetting) { + using nlohmann::literals::operator "" _json; + Config config; + Setting setting{ + &config, + "", + "name-of-the-setting", + "description", + {}, + true, + Xp::Flakes, + }; + setting.assign("value"); + + ASSERT_EQ(config.toJSON(), + R"#({ + "name-of-the-setting": { + "aliases": [], + "defaultValue": "", + "description": "description\n", + "documentDefault": true, + "value": "value", + "experimentalFeature": "flakes" + } + })#"_json); } TEST(Config, setSettingAlias) {