New store settings system by Ericson2314 · Pull Request #11139 · NixOS/nix

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

This is good for e.g. `std::string_view` and `StringMap`. Needed by NixOS#11139 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 Allow putting description and default together, which works in most, but not all, cases. 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

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 3 times, most recently from c7f126b to fe8b42d Compare May 23, 2025 20:50

Ericson2314 removed this from Nix team May 28, 2025

Ericson2314 force-pushed the new-store-settings branch from fe8b42d to 62690d4 Compare July 23, 2025 21:00

Ericson2314 force-pushed the new-store-settings branch 5 times, most recently from c2610cc to 942bb19 Compare August 12, 2025 15:08

Ericson2314 force-pushed the new-store-settings branch 2 times, most recently from a51537f to 6f7ab17 Compare August 15, 2025 17:17

Ericson2314 mentioned this pull request Aug 15, 2025

More flexible typing for get in util.hh #13763

Merged

Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Aug 15, 2025

More flexible typing for get in util.hh

a15f8ca

This is good for e.g. `std::string_view` and `StringMap`. Needed by NixOS#11139 Co-authored-by: Sergei Zimmerman <[email protected]>

Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Aug 15, 2025

More flexible typing for get in util.hh

0ef818d

This is good for e.g. `std::string_view` and `StringMap`. Needed by NixOS#11139 Co-authored-by: Sergei Zimmerman <[email protected]>

Ericson2314 force-pushed the new-store-settings branch 3 times, most recently from 0a75222 to 736af35 Compare August 21, 2025 22:52

Ericson2314 force-pushed the new-store-settings branch from 736af35 to 782b014 Compare September 28, 2025 07:14

Ericson2314 force-pushed the new-store-settings branch 2 times, most recently from f4ae2c4 to bb7b84b Compare October 23, 2025 15:59

Ericson2314 mentioned this pull request Nov 5, 2025

Socket-only unix: store #14475

Draft

Ericson2314 force-pushed the new-store-settings branch from bb7b84b to 1b3c411 Compare November 21, 2025 06:15

Ericson2314 mentioned this pull request Feb 11, 2026

Move settings from Settings to LocalStoreConfig #7740

Draft

7 tasks

Uh oh!

Comments

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!

roberth Apr 14, 2025

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Apr 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

roberth Apr 14, 2025

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

roberth Apr 14, 2025

Choose a reason for hiding this comment

Uh oh!

roberth Apr 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Ericson2314 commented Jul 19, 2024 •

edited

Loading

roberth commented Aug 28, 2024 •

edited

Loading