nix
nix copied to clipboard
New store settings system
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:
- 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
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.
@L-as I think I need to UBSAN, because the store unit tests are failing for what looks like a random variable corruption.
We should just enable it by default, but it causes some weird test failures.
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 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.
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
Fis 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.
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.
Yay, fixed the UBSAN issues!
:tada: All dependencies have been resolved !