Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New store settings system #11139

Draft
wants to merge 76 commits into
base: master
Choose a base branch
from

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Jul 19, 2024

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 👍 to pull requests you find important.

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

fzakaria and others added 30 commits July 15, 2024 11:10
Following what is outlined in NixOS#10766 refactor the uds-remote-store such
that the member variables (state) don't live in the store itself but in
the config object.

Additionally, the config object includes a new necessary constructor
that takes a scheme & authority.

Minor:
* code formatting
* cleanup of getting default path
* added some comments
Co-authored-by: John Ericson <git@JohnEricson.me>
Revert back to basic constructor in store-api.cc

Co-authored-by: John Ericson <git@JohnEricson.me>
Hopefully this fixes the macOS link error. It's also good for
compilation time.
Not having these causes some issues with the new unit tests.
It is a property of the configuration of a store --- how a store URL is
parsed into a store config, not a store itself.

Progress towards NixOS#10766
@github-actions github-actions bot added new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority store Issues and pull requests concerning the Nix store repl The Read Eval Print Loop, "nix repl" command and debugger c api Nix as a C library with a stable interface labels Jul 19, 2024
@Ericson2314
Copy link
Member Author

@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
Copy link
Member

L-as commented Jul 19, 2024

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

@edolstra
Copy link
Member

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
Copy link
Member

roberth commented Aug 28, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c api Nix as a C library with a stable interface new-cli Relating to the "nix" command repl The Read Eval Print Loop, "nix repl" command and debugger store Issues and pull requests concerning the Nix store with-tests Issues related to testing. PRs with tests have some priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants