New store settings system #11139

Ericson2314 · 2024-07-19T03:37:18Z

Motivation

See the linked issues for details.

The most notable user-relevant bits are:

This cleans up the MountedSSHStore: decomposed into its orthogonal parts
This brings us pretty close to being able to then implement a JSON-based config.
- Store query parameters can be JSON
- Stores can entirely be specified via JSON objects, but this is not yet hooked up to anything.

Also behind the scenes have these benefits:

The docs are moved out of the headers, good for less rebuilding when they changes
Stores are always constructed from store configs
Use JSON, avoid custom serializers

Requirements

General requirements for settings:

Settings have names/paths for hierarchical settings
Settings have defaults when not explicitly set
Settings have descriptions
Have machine-readable descriptions and (some) defaults for docs
Can parse settings file and fill in default to have complete settings struct with all fields initialized.

Other requirements:

nix.conf can be in JSON, not a custom line oriented format
- This notably includes not just flat settings with JSON values, but allowing the settings structure itself to be hierarchical.
- This PR doesn't make the JSON settings file itself, but it does introduce some hierarchical store settings
No complicated inheritance hierarchy for the settings infrastructure itself
- If one looks at configuration.hh, there is a huge amount of inheritance. This new system avoids that entirely. Parsing is just a function.
No global variables (library user)
- fewer extraneous flags in CLI
- better unit tests (no uncertain coverage due to settings state)
Be able to open a store from a typed store configuration (parsing separate from actions/side effects)
- Do not want to force parsing untyped map (StoreReference) into doing actions to open a Store in one step.
UI logic (parsing both old and new settings) parsing separate from the settings themselves --- components being configured should get structs and don't have to care about how those structs were made.
- The library / structs in headers only should be the C++ field names and types. There should be no default value, human string-name, description, etc. leaking into the header.
- Once the old settings are reworked on top of this, tons of UI code (configuration and CLI arugments) can move out of libutil, where it does not belong, and pollutes libstore/libexpr in the library use-case.
Ability to use designated initializers
- The inheritance of the existing store config inheritance prevents using designated initializers. Pulling out the template stuff into separate data types, so it can be "plain old data" fixes this problem. Eventually it would be nice to not have as much inheritance on store configs, so we can go back to having fewer types, but that is future work.
Concision and ease of editing (contributor experience)
- <= 2 places (one header, one compilation unit with descriptions and such)

Context

Do not review by commits

contains commits from a different PR that are listed here because that PR was squashed
not much of useful separation anyway in this commit history

Part of #11106

Depends on #13154

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

Ericson2314 · 2024-07-19T03:37:55Z

@L-as I think I need to UBSAN, because the store unit tests are failing for what looks like a random variable corruption.

L-as · 2024-07-19T10:08:08Z

We should just enable it by default, but it causes some weird test failures.

edolstra · 2024-07-19T13:27:42Z

I don't really get the motivation for this change (another massive refactoring PR, which I assume will be followed by an even bigger change when this gets applied to the main settings). #10766 doesn't really give a motivation other than "we always go directly from a StoreReference to the Store data type directly" which is apparently a bad thing. #11106 mentions support for structured settings, but I don't see why that requires completely replacing the current Settings type (there's no reason why those cannot be constructed from JSON values).

This PR scatters the settings mechanism for a class like BinaryCacheStore across a new BinaryCacheStoreConfigT, BinaryCacheStoreConfig, BinaryCacheStore::Config::Descriptions::Descriptions and BinaryCacheStore::Config::BinaryCacheStoreConfig (the defaults declared in another class from the actual settings). That's not an improvement for readability!

I don't really like types like:

template<template<typename> class F>
struct BinaryCacheStoreConfigT
...
    const F<std::string> compression;

We should really keep the template metaprogramming to a minimum! Again for readability, this is not great since looking at this code, the reader has no idea what F is supposed to do.

roberth · 2024-08-28T21:06:19Z

We should really keep the template metaprogramming to a minimum! Again for readability, this is not great since looking at this code, the reader has no idea what F is supposed to do.

I believe it's quite valuable in this situation, and not all that confusing when documented properly.

For this we can make use of the language server. (not using one is cruel anyway)

Example:

diff --git a/src/libstore/binary-cache-store.hh b/src/libstore/binary-cache-store.hh
index af649aaab..a638248c5 100644
--- a/src/libstore/binary-cache-store.hh
+++ b/src/libstore/binary-cache-store.hh
@@ -13,6 +13,7 @@ namespace nix {
 
 struct NarInfo;
 
+/** @param T Either 'nix::config::JustValue' or `nix::config::SettingInfo` */
 template<template<typename> class F>
 struct BinaryCacheStoreConfigT
 {
diff --git a/src/libutil/config-abstract.hh b/src/libutil/config-abstract.hh
index 2c34ff50b..e7f71e700 100644
--- a/src/libutil/config-abstract.hh
+++ b/src/libutil/config-abstract.hh
@@ -5,6 +5,9 @@
 
 namespace nix::config {
 
+/**
+ * Just a value, no metadata. explain explain
+ */
 template<typename T>
 struct JustValue
 {

With docs in these positions, readers can easily navigate to the explanations in the two relevant types that instantiate the fields.

Note also that when hovering over a config.foo usage (this->config.foo), the LSP will show JustValue<bool> which gives a good intuition for what to expect from it.

Ericson2314 · 2025-04-13T00:09:24Z

If the Linux tests fail again, I think it might something to do with UBSan? I'll need to figure out how to run that locally.

Ericson2314 · 2025-04-13T22:20:08Z

Yay, fixed the UBSAN issues!

doc/manual/generate-settings.nix

src/libstore/include/nix/store/store-open.hh

src/libstore/include/nix/store/store-registration.hh

roberth · 2025-04-14T08:18:13Z

src/libstore/include/nix/store/uds-remote-store.hh

+    static const std::string name() { return "Local Daemon Store"; }

-    std::string doc() override;
+    static std::string doc();


Downgrade to constants?

We I think we could, but I am tempted to do in a separate commit since this one is already so big. How does that sound?

src/libstore/local-fs-store.cc

roberth · 2025-04-14T08:31:37Z

src/libstore/store-reference.cc

+            } catch (nlohmann::json::exception &) {
+                // if its not valid JSON...
+                if (k == "remote-program" || k == "system-features") {
+                    // Back compat hack! Split and take that array


Can we not do hacks? Presumably this needs to be part of the non-JSON parsing logic only.

I am trying to allow query parameters to also be JSON, however. For example, I made it so ssh://localhost?remote-program=["nix","store","--serve"] works (provided one does enough HTML % escaping).

roberth · 2025-04-14T08:34:12Z

src/libstore/store-registration.cc

+
+void adl_serializer<ref<const StoreConfig>>::to_json(json & obj, ref<const StoreConfig> s)
+{
+    // TODO, for tests maybe


Or nix store show/info

roberth · 2025-04-14T08:36:15Z

src/libutil/include/nix/util/config-abstract.hh

+#include <optional>
+
+namespace nix::config {
+


Needs documentation. I believe this was part of an earlier review.

src/libutil/include/nix/util/config-abstract.hh

tests/functional/build-remote-with-mounted-ssh-ng.sh

src/libstore/http-binary-cache-store.cc

src/libstore/include/nix/store/config-parse-impl.hh

src/libstore/include/nix/store/config-parse.hh

src/libstore/include/nix/store/local-overlay-store.hh

src/libstore/include/nix/store/store-registration.hh

src/libutil/include/nix/util/config-abstract.hh

roberth · 2025-04-30T20:09:09Z

src/libstore/include/nix/store/store-dir-config.hh

+ * settings record in order to ensure don't forgot to parse or document
+ * settings field.
+ */
+template<template<typename> class F>


Suggested change

template<template<typename> class F>

template<template<typename> class Mode>

Suggested change

template<template<typename> class F>

template<template<typename> class Container>

Suggested change

template<template<typename> class F>

template<template<typename> class Setting>

Suggested change

template<template<typename> class F>

template<template<typename> class Interpretation>

roberth · 2025-04-30T20:12:42Z

src/libstore/include/nix/store/store-dir-config.hh

-          between stores if they have the same `store` setting.
-        )"};
-    const Path storeDir = storeDir_;
+    F<Path> _storeDir;


If use Setting<F, Path> = F<Path>; works and needs no extra initializer braces:

Suggested change

F<Path> _storeDir;

Setting<F, Path> _storeDir;

xokdvium · 2025-04-28T18:55:21Z

src/libstore/include/nix/store/config-parse.hh

+#if 0
+    /**
+     * @see SettingInfo::aliases
+     */
+    std::set<std::string> aliases;
+#endif


It's not very clear why this is if 0'd out. Is this some placeholder for future work?

Yes, to subsume the old configuration system more completely (not just what is in-use for store settings) we would add this stuff also

src/libutil/include/nix/util/util.hh

xokdvium · 2025-05-01T08:52:31Z

src/libstore/unix/build/derivation-builder.cc

+            auto config = make_ref<LocalStore::Config>(*getLocalStore().config);
+            config->pathInfoCacheSize.value = 0;
+            config->stateDir.value = "/no-such-path";
+            config->logDir.value = "/no-such-path";


It might be good for readability if PlainValue overloaded operator= so that we could just do this:

Suggested change

auto config = make_ref<LocalStore::Config>(*getLocalStore().config);

config->pathInfoCacheSize.value = 0;

config->stateDir.value = "/no-such-path";

config->logDir.value = "/no-such-path";

auto config = make_ref<LocalStore::Config>(*getLocalStore().config);

config->pathInfoCacheSize = 0;

config->stateDir = "/no-such-path";

config->logDir = "/no-such-path";

Might be worth adding something like:

template <std::convertible_to<T> U> PlainValue & operator=(U rhs) & { value = static_cast<T>(rhs); return *this; }

xokdvium · 2025-05-01T08:56:24Z

src/libstore/s3-binary-cache-store.cc

-            wantMassQuery.setDefault(cacheInfo->wantMassQuery);
-            priority.setDefault(cacheInfo->priority);
+            resolvedSubstConfig.wantMassQuery.value =
+                config->storeConfig.wantMassQuery.optValue.value_or(cacheInfo->wantMassQuery);


This is quite a mouthful. Can't we just forward value_or method to the OptValue class so that this would work:

Suggested change

config->storeConfig.wantMassQuery.optValue.value_or(cacheInfo->wantMassQuery);

config->storeConfig.wantMassQuery.value_or(cacheInfo->wantMassQuery);

This works:

template<typename T> struct OptValue : private std::optional<T> { private: using OptBase = std::optional<T>; public: OptValue(OptBase opt) : OptBase(std::move(opt)) {} using OptBase::OptBase; using OptBase::value_or; };

I think I can just use std::optional directly

Don't really have a strong opinion on this, but that a good example of the Law of Demeter and saves on some typing. But again, not a blocking suggestion in any way.

Fix NixOS#10766 See that ticket for details. Progress (I hope!) towards NixOS#11139.

Fix NixOS#10766 See that ticket for details. Progress (I hope!) towards NixOS#11139. Co-Authored-By: Sergei Zimmerman <[email protected]>

dpulls · 2025-05-14T19:08:43Z

🎉 All dependencies have been resolved !

Co-authored-by: Sergei Zimmerman <[email protected]>

Motivation: See the linked issues for details. The most notable user-relevant bits are: - This cleans up the `MountedSSHStore`: decomposed into its orthogonal parts - This brings us pretty close to being able to then implement a JSON-based config. - Store query parameters can be JSON - Stores can entirely be specified via JSON objects, but this is not yet hooked up to anything. Also behind the scenes have these benefits: 1. The docs are moved out of the headers, good for less rebuilding when they changes 2. Stores are always constructed from store configs 3. Use JSON, avoid custom serializers Context: Part of NixOS#11106 Co-Authored-By: Robert Hensing <[email protected]> Co-authored-by: Sergei Zimmerman <[email protected]>

Ericson2314 mentioned this pull request Jul 31, 2024

Add setting 'client-only-settings' to prevent forwarding to the daemon #11223

Open

Ericson2314 mentioned this pull request Nov 13, 2024

Design and enable creation of remote gc roots #11812

Open

github-actions bot added the documentation label Feb 3, 2025

Ericson2314 mentioned this pull request Feb 3, 2025

Get settings info out of headers, out of libutil #11033

Draft

Ericson2314 mentioned this pull request Feb 13, 2025

More structured settings in nix store URLs #11106

Open

Ericson2314 force-pushed the new-store-settings branch from ede1c2b to 6ba8c57 Compare March 17, 2025 15:32

Ericson2314 marked this pull request as ready for review March 17, 2025 15:51

Ericson2314 requested a review from edolstra as a code owner March 17, 2025 15:51

Ericson2314 marked this pull request as draft March 17, 2025 15:52

Ericson2314 mentioned this pull request Apr 9, 2025

store URI: introduce multiple signatures support #12976

Merged

Ericson2314 force-pushed the new-store-settings branch 2 times, most recently from 382f994 to 38b376c Compare April 12, 2025 23:52

Ericson2314 force-pushed the new-store-settings branch 2 times, most recently from 9511fe1 to 668c465 Compare April 13, 2025 21:30

Ericson2314 marked this pull request as ready for review April 13, 2025 22:19

roberth reviewed Apr 14, 2025

View reviewed changes

Ericson2314 force-pushed the new-store-settings branch 3 times, most recently from d146a6e to c7a1caa Compare April 16, 2025 18:45

xokdvium reviewed Apr 26, 2025

View reviewed changes

Ericson2314 force-pushed the new-store-settings branch 2 times, most recently from 6249504 to 1ceada0 Compare April 28, 2025 18:46

roberth reviewed Apr 30, 2025

View reviewed changes

xokdvium reviewed May 1, 2025

View reviewed changes

Ericson2314 force-pushed the new-store-settings branch from 1ceada0 to 02446fd Compare May 7, 2025 19:25

Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request May 7, 2025

Stores no longer inherit from their configs

e1acf3d

Fix NixOS#10766 See that ticket for details. Progress (I hope!) towards NixOS#11139.

Ericson2314 force-pushed the new-store-settings branch from 02446fd to a4a23a7 Compare May 7, 2025 23:30

Ericson2314 mentioned this pull request May 7, 2025

Stores no longer inherit from their configs #13154

Merged

Ericson2314 marked this pull request as draft May 7, 2025 23:37

Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request May 7, 2025

Stores no longer inherit from their configs

d3a56fa

Fix NixOS#10766 See that ticket for details. Progress (I hope!) towards NixOS#11139.

Ericson2314 force-pushed the new-store-settings branch from a4a23a7 to 9a01d3b Compare May 7, 2025 23:47

Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request May 8, 2025

Stores no longer inherit from their configs

8598565

Fix NixOS#10766 See that ticket for details. Progress (I hope!) towards NixOS#11139.

Ericson2314 force-pushed the new-store-settings branch from 9a01d3b to 48a8c2c Compare May 8, 2025 16:42

Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request May 13, 2025

Stores no longer inherit from their configs

934918b

Fix NixOS#10766 See that ticket for details. Progress (I hope!) towards NixOS#11139. Co-Authored-By: Sergei Zimmerman <[email protected]>

Ericson2314 force-pushed the new-store-settings branch from 48a8c2c to e302ab6 Compare May 13, 2025 20:09

Mic92 pushed a commit to Mic92/nix-1 that referenced this pull request May 14, 2025

Stores no longer inherit from their configs

fb05644

Fix NixOS#10766 See that ticket for details. Progress (I hope!) towards NixOS#11139. Co-Authored-By: Sergei Zimmerman <[email protected]>

Ericson2314 force-pushed the new-store-settings branch from e302ab6 to 2daf1ee Compare May 14, 2025 19:15

Ericson2314 mentioned this pull request May 14, 2025

Split out store-open.hh and store-registration.hh #13188

Merged

Ericson2314 force-pushed the new-store-settings branch 3 times, most recently from 9b5b2b9 to 2ea9f59 Compare May 21, 2025 21:59

Ericson2314 force-pushed the new-store-settings branch from 2ea9f59 to 6c4c845 Compare May 23, 2025 19:21

More fexible typing for get in util.hh

09b2f1c

Co-authored-by: Sergei Zimmerman <[email protected]>

Ericson2314 force-pushed the new-store-settings branch from 93f958b to c7f126b Compare May 23, 2025 19:39

Ericson2314 force-pushed the new-store-settings branch from c7f126b to fe8b42d Compare May 23, 2025 20:50

Ericson2314 removed this from Nix team May 28, 2025

	template<template<typename> class F>
	template<template<typename> class Mode>

	template<template<typename> class F>
	template<template<typename> class Container>

	template<template<typename> class F>
	template<template<typename> class Setting>

	template<template<typename> class F>
	template<template<typename> class Interpretation>

	config->storeConfig.wantMassQuery.optValue.value_or(cacheInfo->wantMassQuery);
	config->storeConfig.wantMassQuery.value_or(cacheInfo->wantMassQuery);

Uh oh!

New store settings system #11139

Are you sure you want to change the base?

New store settings system #11139

Uh oh!

Conversation

Ericson2314 commented Jul 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Requirements

Context

Priorities and Process

Uh oh!

Ericson2314 commented Jul 19, 2024

Uh oh!

L-as commented Jul 19, 2024

Uh oh!

edolstra commented Jul 19, 2024

Uh oh!

roberth commented Aug 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ericson2314 commented Apr 13, 2025

Uh oh!

Ericson2314 commented Apr 13, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

roberth Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpulls bot commented May 14, 2025

Uh oh!

Uh oh!

Ericson2314 commented Jul 19, 2024 •

edited

Loading

roberth commented Aug 28, 2024 •

edited

Loading

roberth Apr 30, 2025 •

edited

Loading