-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add setting handlers (side effects when settings are changed) #9922
base: master
Are you sure you want to change the base?
Conversation
`BaseSetting` is never used on its own, so it's fairly trivial to remove.
80c9083
to
e22cbab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bunch of small nitpicks, but I think that makes sense overall. Thanks!
The idea of adding arbitrary side effects to configuration settings is scary to me. It makes it hard to reason about the configuration system. E.g. currently a configuration |
@edolstra wouldn't that be OK as long as we are in control of the side effects we want to have? |
`fun` has a default value, so we no longer need to be able to convert `SettingHandler` to `bool`.
Yeah, I was thinking about that. I think it would be pretty easy to replace the setting values with thunks, and then it would be easier to make the config values non-static. |
not sure why this is controversial. we already have the facility to add arbitrary side effects, it's just less convenient (requiring an explicit template instantiation and an implementation of |
I have given this more thought, and do think as part of the translation / compat layer we will end up needing something like this. But I want to work on what we are translating to first. In particular, I would like to migrate the store settings off the current Once that is done, I'll be more more comfortable adding bells and whistles like this to Conversely, if we don't do things this way, I am afraid getting stuck half-way through with an even-more-complicated |
#10562 reminds me I want the global variables gone, which reminds me that we're gonna need this as part of the transition. |
Motivation
One of the biggest differences between command-line arguments and settings is that command-line arguments have handlers, arbitrary functions that are run when the command-line argument is set, which may perform arbitrary side-effects. This is used by arguments like
--log-format
and--print-build-logs
to make changes to global state in response to argument values.This PR adds simple handler functions to settings, to gain parity with command-line arguments.
(Note: The use of global state in handlers for
--log-format
and similar arguments is unnerving and error prone. I believe this design will scale if we try to reduce global state in Nix — in the future, we can add parameters likeEvalState
to handler functions to make data flow explicit.)I intend to use this functionality to implement two features:
Settings with side-effects, like
log-format
andprint-build-logs
(to close tickets like Nix.conf for print-build-logs and verbose #5858 and allow settings log-format in nix.conf #5561).I have a PR which adds
log-format
andlog-format-legacy
settings written already, building on this PR: Convert--log-format
to a setting #9923Settings that change at runtime, to support commands like
:set print-build-logs
in the REPL/debugger (seenix repl
should be able to change settings #9944)This PR also combines
BaseSetting
andSetting
(there were no usages ofBaseSetting
on its own).Priorities and Process
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.