nix icon indicating copy to clipboard operation
nix copied to clipboard

New store settings system

Open Ericson2314 opened this issue 1 year ago • 4 comments

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.

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

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

Depends on #11033

Part of #11106 Part of #10766

Priorities and Process

Add :+1: to pull requests you find important.

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

Ericson2314 avatar Jul 19 '24 03:07 Ericson2314

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

Ericson2314 avatar Jul 19 '24 03:07 Ericson2314

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

L-as avatar Jul 19 '24 10:07 L-as

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.

edolstra avatar Jul 19 '24 13:07 edolstra

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.

roberth avatar Aug 28 '24 21:08 roberth

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 avatar Apr 13 '25 00:04 Ericson2314

Yay, fixed the UBSAN issues!

Ericson2314 avatar Apr 13 '25 22:04 Ericson2314

:tada: All dependencies have been resolved !

dpulls[bot] avatar May 14 '25 19:05 dpulls[bot]