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

Triage and break up the global settings #5638

Open
Ericson2314 opened this issue Nov 23, 2021 · 13 comments
Open

Triage and break up the global settings #5638

Ericson2314 opened this issue Nov 23, 2021 · 13 comments
Assignees
Labels
contributor-experience Developer experience for Nix contributors new-cli Relating to the "nix" command settings Settings, global flags, nix.conf UX The way in which users interact with Nix. Higher level than UI.

Comments

@Ericson2314
Copy link
Member

Ericson2314 commented Nov 23, 2021

Thinking through #5603, and the two getDefaultSystemFeatures reminded me of this:

  1. We have many global settings that should be per-store. thisSytem and extraPlatforms are good examples. This works best after Split Store class #5729 so that we don't pollute stores like HttpBinaryCacheStore with these new per-store options that don't make sense for them.

  2. We have too much ad-hoc logic around build remotes that should be fixed up to use per-store settings instead, as we did before with system features in 4720853

    • We should probably make stores that can build a subclass of stores that just are read/written. Then we don't have to do silly things like say what "extraPlatforms" s3://... has.
  3. Separate stores for evaluation, building and storing the result #5025 (comment) remains a very good idea. Then many of the remaining settings which aren't per-store would make sense as an additional config parameter to this now-freestanding (not Store method) function.

  4. Finally, there may be global settings that are for the libstore layer at all, and they can be moved to another downstream library.

@rickynils
Copy link
Member

rickynils commented Feb 17, 2022

I'm dumping some references here, where discussions about options propagation to remote builders can be found:

#5600
#2994
#4180

The issue is that when using remote builders or remote stores, it is not at all clear what options will be in effect. For ssh:// there is a small set of settings propagated, for ssh-ng:// almost no options (I think builders-use-substitutes is the only one) are propagated. It doesn't make sense to propagate all Nix options indiscriminately, because many settings are related to aspects of the local machine. If I understand this issue correctly, we could identify options that are related to specific types of stores and then propagate these when such remote stores are used?

Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Mar 1, 2022
Starting work on NixOS#5638

The exact boundary between `FetchSettings` and `EvalSettings` is not
clear to me, but that's fine. First lets clean out `libstore`, and then
worry about what, if anything, should be the separation between those
two.
@stale stale bot added the stale label Nov 13, 2022
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Feb 2, 2023
These settings are not needed for libstore at all, they are just used by
the nix daemon *command* for authorization on unix domain sockets. My
moving them to a new configuration struct just in that file, we avoid
them leaking anywhere else.

Also, it is good to break up the mammoth `Settings` struct in general.
Issue NixOS#5638 tracks this.

The message is not changed because I do not want to regress in
convenience to the user. Just saying "this connection is not trusted"
doesn't tell them out to fix the issue. The ideal thing to do would be
to somehow parameterize `processCommand` on how the error should be
displayed, so different sorts of connections can display different
information to the user based on how authentication is performed for the
connection in question. This, however, is a good bit more work, so it is
left for the future.

This came up with me thinking about the tcp:// store (NixOS#5265). The larger
project is not TCP *per se*, but the idea that it should be possible for
something else to manage access control to services like the Nix Daemon,
and those services simply trust or trust the incoming connection as they
are told. This is a more capability-oriented way of thinking about trust
than "every server implements its own auth separately" as we are used to today.

Its very great that libstore itself already implements just this model,
and so via this refactor I basically want to "enshrine" that so it
continues to be the case.
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Feb 2, 2023
These settings are not needed for libstore at all, they are just used by
the nix daemon *command* for authorization on unix domain sockets. My
moving them to a new configuration struct just in that file, we avoid
them leaking anywhere else.

Also, it is good to break up the mammoth `Settings` struct in general.
Issue NixOS#5638 tracks this.

The message is not changed because I do not want to regress in
convenience to the user. Just saying "this connection is not trusted"
doesn't tell them out to fix the issue. The ideal thing to do would be
to somehow parameterize `processCommand` on how the error should be
displayed, so different sorts of connections can display different
information to the user based on how authentication is performed for the
connection in question. This, however, is a good bit more work, so it is
left for the future.

This came up with me thinking about the tcp:// store (NixOS#5265). The larger
project is not TCP *per se*, but the idea that it should be possible for
something else to manage access control to services like the Nix Daemon,
and those services simply trust or trust the incoming connection as they
are told. This is a more capability-oriented way of thinking about trust
than "every server implements its own auth separately" as we are used to today.

Its very great that libstore itself already implements just this model,
and so via this refactor I basically want to "enshrine" that so it
continues to be the case.
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Feb 3, 2023
Trying to do NixOS#5638 but this is not working very well at all.
@Ericson2314 Ericson2314 removed the stale label Feb 3, 2023
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Feb 3, 2023
Trying to do NixOS#5638 but this is not working very well at all.
@fricklerhandwerk
Copy link
Contributor

fricklerhandwerk commented Feb 20, 2023

Triaged in the Nix team meeting 2023-02-17:

  • @edolstra: this should be low priority because it will cause a lot of code churn and have little use to the user
    • @fricklerhandwerk: given most of our users are developers and we want to improve contributor experience it may actually have value to users

@nixos-discourse
Copy link

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-02-17-nix-team-meeting-minutes-33/25624/1

@Ericson2314
Copy link
Member Author

The author of #6980, @virusdave, talks it about it being confusing that an arbitrary Nix invocation looks up the users home directory. I agree this seems off!

The reason for this is ultimately that our settings are stored in these giant global variables. The default value settings must be created and start-up time, and since profiles optionally got moved to home directories, that means looking up the user's home directory.

The new location is good, and the warning on looking up the home directory is also good. The problem is, once again, the global. Global settings mean a violation of "pay for what you use, only". We have to do all this work for settings we may well not end up using, simply because global variables mean all configuration must be done eagerly.

This is not good!

@fricklerhandwerk fricklerhandwerk added new-cli Relating to the "nix" command UX The way in which users interact with Nix. Higher level than UI. contributor-experience Developer experience for Nix contributors labels Mar 7, 2023
@Ericson2314
Copy link
Member Author

Ericson2314 commented Mar 13, 2023

We discussed this today during the Nix Team meeting at length.

As part of this, we discussed the sort of user facing configuration changes we could also do that that relate to this. There should be a separate "overhaul the configuration system" ticket for that to track these user-facing points distinct from this info which is really about implementation details (though each does inform the other!). That said, that issue doesn't yet exist, and the conversation went back and forth, so so I will post everything here for now.


  • @Ericson2314: would like to pair the new CLI with a better config system and abolishing global variables
  • @edolstra: per-store settings is not a good idea at this point, it's a massive user-facing change, and we don't have a mechanism for discovering settings per store
    • is the system parameter even per store? it's a parameter of Nix's own ability to build
  • @Ericson2314: if a store cannot build, there's no system parameter
    • but then there's this whole remote store issue (e.g. check-trusted):
      • the remote store would have to tell you which build it supports
    • @edolstra: theoretically it's true, but how is the user going to configure this, concretely?
    • @Ericson2314: this is why we should discuss this as a design issue including the entire UX, as @roberth proposed last time we talked about this
  • agreement: do some brain storming/requirement analysis now, @roberth and @Ericson2314 will use that to make a draft for discussion

Brainstorming

  • @Ericson2314: would be good to have hierarchical settings

    • in theory we know how to do this, we have NixOS
  • @edolstra: it would be much easier if nix.conf was a JSON file

    • but this is unrelated, because how many settings do we really want to have for a single program?

    • having a flat namespace for settings is not bad in itself

    • documentation can be solved by having settings in categories

      • @fricklerhandwerk: how does that differ from hierarchical settings?
        • @edolstra: it only appears in documentation, the config file is still flat
          • in a sense, we already have this encoded in name, such as with gc-<foo>
  • @edolstra clarifying a "limited" JSON file he supports:

    • illustrating
      {
        # bad according to Eelco
        "gc": {
          "auto": {
            "enable": true
          }
        }
        # good acording to Eelco
        "gc-auto-enable": true,
        "use-substitutes": false, 
        "substituters": [
          {"url": "https://cache.nixos.org", "priority": 10}
        ],
        # Today, not so good
        "store": "local?system=i686-linux"
        # Better:
        "store": {
          "type": "local",
          "system": "i686-linux"
        },
        "eval-store": null, # defaults to `.store`
        "builders": [
          {
            # there may not be "hostname": "builder.example.org", 
            "systemTypes": ["i686-linux"],
            "store": {
              "type": "local",
              "chroot": "/tmp/nix",
                
            }
          }
        ]
      }
    • flat namespace of settings keys
    • values however can be structured, e.g. build remotes
    • --extra-builders '[{....}]' good
    • --builders.0.system-types '[...]' not good
  • @edolstra: in the NixOS module system we have an ambiguity, where although it looks like a hierarchical configuration it's in fact a flat namespace that collides with actually hierarchical settings

    • @roberth: agreed, we should instead focus on the keys oriented towards functionality. for example, gc is about the store
  • @Ericson2314: one way of thinking about this is that it should be possible to rig up two Nix processes which run on different configurations, such as different daemons and GCs

  • @tomberek: in flakes we have two methods of expressing inputs, as attributes and as URL; may work similarly for remote stores/builders

    • @edolstra: we may actually re-use much of that code for this
  • @roberth: we should pay attention the rules of database design; we're designing a database for configurations

  • @roberth: should also consider e.g. settings for S3 stores, we may have multiple of those

    • main concern is usability
  • @edolstra: with lists there is no way to override elements inside a list

    • don't know if that is even a desirable property
  • @Ericson2314:

    • Nice to not list the same store under builders and substuters twice, but instead write down the store once and then what it is used for.
    • Likewise the "trusted" (allowed to use, not use by default) vs other settings today is very confusing, can do better by replacing "builder": true with "builder": "allowed" vs "builder": "used-by-default".
    • tranposing the settings options is now an option we can take advantage of
  • @roberth: concerning aliases: thinking along the lines of the SSH config file

    • the semantics makes for a good example (not the syntax). Can use URL or can use non-URL alias.
    • Again see candidate/functional keys
    • @fricklerhandwerk: really like that way of looking at it, but that begs the question if JSON is the reasonable representation for it
      • @roberth: it forces keys to be strings
  • @Ericson2314: related to installer:

    • Some different directions of work going on:

      • people working on the installer seem to assume the installation process will always be very involved, and making that less painful
      • people working on Nix want there to be less install/configure work to do in the first place.
        • Example: auto UID allocation means don't need to install-time set up build users
        • Example NIX_PATH default value means OK not to set up the environment variable.
    • @ericson3214 ultimately thinks that second approach is the the right way forward: Make the complexity lazy / on demand, rather than trying to deal with setting up everything the user might possibly need to do, before they actually might do it.

    • @roberth: this is interface segragation (or "pay for what you use"): SOLID

    • @Ericson2314:

      • Only configure/initialize what you use:

        • if you don't use nix-env you don't need to worry about profile dirs
        • if you don't use impure eval, you don't need worry about NIX_PATH
        • if you don't use a local store, you don't need worry about 50 or so misc settings
      • Only set up state for what you use

        • The installer today has to assume people will use many of these features, and set things up accordingly
    • @edolstra: how does this differ from the current state? if you don't use values in the configuration, they don't matter

      • @Ericson2314: config is evaluated eagerly and then generates warnings even if those settings don't matter
      • @edolstra: the question is whether nix.conf is a fully generic configuration file or a generic one for Nix
        • options for Hydra should not go in there
        • we should not overengineer this to become a universal configuration system
        • @fricklerhandwerk: ...which Nix is?
      • @Ericson2314: (more arguments why this still may work)
  • @edolstra: lazy and hierarchical options seem to be orthogonal concepts

    • @Ericson2314: Yes, but those two form a dialectic :)
    • @roberth: there is a subtelty we can exploit:
      • say there is an unused component such as nix-env, then we can validate the root attrset and allow a profile setting that contains garbage by just ignoring it
        • @edolstra: is that indeed what we want?
        • @roberth: it simplifies the code and is more robust
        • @fricklerhandwerk: allows localising validation to the component that uses the code, seems reasonable
        • @Ericson2314: right now the CLI allows setting anything regardless of if they matter
          • presenting all the things is overwhelming to the user
          • setting it up that way allows presenting only the relevant settings quite naturally
  • @fricklerhandwerk: is this enough for @Ericson2314 and @roberth to make a design draft and a sketch of a migration path?

    • @Ericson2314: could sketch what it should look like
      • an initial idea for migration: move specific settings into separate structs, but don't change the outward interface yet
        • wouldn't capture the lazy part, but the hierarchical part
      • start with better separation of concerns in the code
      • implementation leads the way, interface catches up
      • old configuration would stick around as long as possible in a thorny compatibility layer
        • @roberth: which would also be there to provide defaults, flat names, etc.
    • @fricklerhandwerk: is there a way to design it without getting into the particularities of current components yet?
      • looks like there is a risk of getting bogged down by side discussions
      • @Ericson2314: when you don't have to worry about UI it's very easy. the mapping to the CLI conventions and configuration files is the really tricky part
        • the easy part is imagining Nix to be a library
        • @fricklerhandwerk: strongly agree we should think about Nix in terms of a library with all desired properties (simplicity, composability, ...) and not look at the peculiarities of a particular representation of its interface to avoid obscuring sight on the really important, foundational aspects

@virusdave
Copy link
Contributor

virusdave commented Mar 13, 2023

  • illustrating
    {
      # bad according to Eelco
      "gc": {
        "auto": {
          "enable": true
        }
      }
      # good acording to Eelco
      "gc-auto-enable": true,
      "use-substitutes": false, 
      "substituters": [
        {"url": "https://cache.nixos.org", "priority": 10}
      ],
      # Today, not so good
      "store": "local?system=i686-linux"
      # Better:
      "store": {
        "type": "local",
        "system": "i686-linux"
      },
      "eval-store": null, # defaults to `.store`
      "builders": [
        {
          # there may not be "hostname": "builder.example.org", 
          "systemTypes": ["i686-linux"],
          "store": {
            "type": "local",
            "chroot": "/tmp/nix",
              
          }
        }
      ]
    }

At the risk of opening up an unintended deluge of derision, response spam, or bikeshedding...

I want to quickly suggest consideration of something other than json for things like this. Clearly Eelco doesn't like the verbosity of namespacing that json requires, but something like HOCON (with c++ impl) solves this completely and gives you familiar json-like syntax but in a human-friendly form without the downsides of abominations like YAML or resorting to "namespacing-via-string-concat-in-key-names" like in the above hypothetical example. Other options would include things like cue, or many other common or standard-ish options that don't suck like json does.

I do realize there's already use of json in nix, so using something else would require adding an additional dependency, but for anything likely to attract human interaction (like configuration as discussed here), i would beseech consideration of a better, less human-hostile format for config state storage. I, at least, believe that the benefits outweigh the dependency cost.

[@virusdave ducks backs back out of the discussion here...]

@roberth
Copy link
Member

roberth commented Mar 13, 2023

I think finding the right data model is orders of magnitude more important than the syntax.
That said...

[@virusdave ducks backs back out of the discussion here...]

Wise choice ;)

HOCON

The references are too much for me. I've had the misfortune of using HOCON with an Akka application once, and I would describe the use of references for some sort of overriding as haunting at best.
We may also consider reusing TOML.

[@roberth joins @virusdave in the nice place where syntax does not matter... Anyone else joining?]

@nixos-discourse
Copy link

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-03-13-nix-team-meeting-minutes-40/26309/1

@roberth
Copy link
Member

roberth commented Mar 16, 2023

Doing this turns bugs into errors, which is a tangible improvement.

@nixos-discourse
Copy link

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-01-30-nix-team-meeting-minutes-119/39185/1

@nixos-discourse
Copy link

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-06-05-nix-team-meeting-minutes-150/46583/1

Ericson2314 added a commit that referenced this issue Jun 14, 2024
Progress on #5638

There is still a global eval settings, but it pushed down into
`libnixcmd`, which is a lot less bad a place for this sort of thing.
Ericson2314 added a commit that referenced this issue Jun 14, 2024
Progress on #5638

There is still a global eval settings, but it pushed down into
`libnixcmd`, which is a lot less bad a place for this sort of thing.
Ericson2314 added a commit to haenoe/nix that referenced this issue Jun 14, 2024
…sting

This allows us to avoid needing to generalize `dummy://` into a
`transient://`  at this time.

Setting the global settings in unit tests like this is very ugly, but
the real problem is not changing the settings but having the code being
unit tested depend on global variables at all. NixOS#5638 tracks fixing this.
Ericson2314 added a commit to haenoe/nix that referenced this issue Jun 14, 2024
…sting

This allows us to avoid needing to generalize `dummy://` into a
`transient://`  at this time.

Setting the global settings in unit tests like this is very ugly, but
the real problem is not changing the settings but having the code being
unit tested depend on global variables at all. NixOS#5638 tracks fixing this.
Ericson2314 added a commit to Ericson2314/nix that referenced this issue Jun 23, 2024
…sting

This allows us to avoid needing to generalize `dummy://` into a
`transient://`  at this time.

Setting the global settings in unit tests like this is very ugly, but
the real problem is not changing the settings but having the code being
unit tested depend on global variables at all. NixOS#5638 tracks fixing this.
Ericson2314 added a commit to Ericson2314/nix that referenced this issue Jun 24, 2024
…sting

This allows us to avoid needing to generalize `dummy://` into a
`transient://`  at this time.

Setting the global settings in unit tests like this is very ugly, but
the real problem is not changing the settings but having the code being
unit tested depend on global variables at all. NixOS#5638 tracks fixing this.
Ericson2314 added a commit that referenced this issue Jun 24, 2024
Progress on #5638

There is still a global eval settings, but it pushed down into
`libnixcmd`, which is a lot less bad a place for this sort of thing.
Ericson2314 added a commit that referenced this issue Jun 24, 2024
Progress on #5638

There is still a global eval settings, but it pushed down into
`libnixcmd`, which is a lot less bad a place for this sort of thing.
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Jul 1, 2024
Progress on NixOS#5638

There are still a global fetcher and eval settings, but they are pushed
down into `libnixcmd`, which is a lot less bad a place for this sort of
thing.

Continuing process pioneered in
52bfccf.
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Jul 1, 2024
Progress on NixOS#5638

There are still a global fetcher and eval settings, but they are pushed
down into `libnixcmd`, which is a lot less bad a place for this sort of
thing.

Continuing process pioneered in
52bfccf.
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Jul 2, 2024
Progress on NixOS#5638

There are still a global fetcher and eval settings, but they are pushed
down into `libnixcmd`, which is a lot less bad a place for this sort of
thing.

Continuing process pioneered in
52bfccf.
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Jul 2, 2024
Progress on NixOS#5638

There are still a global fetcher and eval settings, but they are pushed
down into `libnixcmd`, which is a lot less bad a place for this sort of
thing.

Continuing process pioneered in
52bfccf.
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Jul 2, 2024
They are not actually part of the store layer, but instead part of the
Nix executable infra (libraries don't need plugins, executables do).

This is part of a larger project of moving all of our legacy settings
infra to libmain, and having the underlying libraries just have plain
configuration structs detached from any settings infra / UI layer.

Progress on NixOS#5638
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Jul 8, 2024
Progress on NixOS#5638

There are still a global fetcher and eval settings, but they are pushed
down into `libnixcmd`, which is a lot less bad a place for this sort of
thing.

Continuing process pioneered in
52bfccf.
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Jul 8, 2024
Progress on NixOS#5638

There are still a global fetcher and eval settings, but they are pushed
down into `libnixcmd`, which is a lot less bad a place for this sort of
thing.

Continuing process pioneered in
52bfccf.
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Jul 8, 2024
Progress on NixOS#5638

There are still a global fetcher and eval settings, but they are pushed
down into `libnixcmd`, which is a lot less bad a place for this sort of
thing.

Continuing process pioneered in
52bfccf.
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Jul 8, 2024
Progress on NixOS#5638

There are still a global fetcher and eval settings, but they are pushed
down into `libnixcmd`, which is a lot less bad a place for this sort of
thing.

Continuing process pioneered in
52bfccf.
@roberth roberth added the settings Settings, global flags, nix.conf label Jul 11, 2024
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Jul 12, 2024
Progress on NixOS#5638

There are still a global fetcher and eval settings, but they are pushed
down into `libnixcmd`, which is a lot less bad a place for this sort of
thing.

Continuing process pioneered in
52bfccf.
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Jul 15, 2024
They are not actually part of the store layer, but instead part of the
Nix executable infra (libraries don't need plugins, executables do).

This is part of a larger project of moving all of our legacy settings
infra to libmain, and having the underlying libraries just have plain
configuration structs detached from any settings infra / UI layer.

Progress on NixOS#5638
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Jul 15, 2024
They are not actually part of the store layer, but instead part of the
Nix executable infra (libraries don't need plugins, executables do).

This is part of a larger project of moving all of our legacy settings
infra to libmain, and having the underlying libraries just have plain
configuration structs detached from any settings infra / UI layer.

Progress on NixOS#5638
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor-experience Developer experience for Nix contributors new-cli Relating to the "nix" command settings Settings, global flags, nix.conf UX The way in which users interact with Nix. Higher level than UI.
Projects
Status: Defined work
Development

No branches or pull requests

7 participants