Merged
Conversation
This was referenced Jan 17, 2023
roberth
reviewed
Jan 17, 2023
Member
roberth
left a comment
roberth
left a comment
There was a problem hiding this comment.
Looks decent; just a suggestion and two nitpicks.
I don't think I know enough about how the initialization order of the settings to judge that aspect, but otherwise lgtm.
| both_ways store gc --help | ||
|
|
||
| ! nix --experimental-features 'nix-command' show-config --flake-registry 'https://no' | ||
| nix --experimental-features 'nix-command flakes' show-config --flake-registry 'https://no' |
Member
There was a problem hiding this comment.
A unit test would be more future proof, but this will do.
Member
Author
There was a problem hiding this comment.
Thanks. Yeah I would have to mock up a whole command thingy and parse it, at which point I'd be worried the mocking and the real world got out of sync.
Ericson2314
commented
Jan 17, 2023
Ericson2314
commented
Jan 17, 2023
Ericson2314
commented
Jan 17, 2023
Ericson2314
force-pushed
the
hide-experimental-settings
branch
from
8429ccc to
0463611
Compare
This was referenced Feb 13, 2023
fricklerhandwerk
added
UX
Ericson2314
force-pushed
the
hide-experimental-settings
branch
from
0463611 to
8a6591b
Compare
github-actions
bot
added
with-tests
roberth
reviewed
Mar 17, 2023
roberth
reviewed
Mar 17, 2023
thufschmitt
added
the
idea approved
Ericson2314
force-pushed
the
hide-experimental-settings
branch
2 times, most recently
from
577c1b2 to
c0e7b70
Compare
Ericson2314
force-pushed
the
hide-experimental-settings
branch
2 times, most recently
from
e64f893 to
359e1a6
Compare
roberth
reviewed
Mar 20, 2023
This is needed in subsequent commits to allow the settings and CLI args infrastructure itself to read this setting.
We hide them in various ways if the experimental feature isn't enabled. To do this, we had to move the experimental features list out of libnixstore, because the setting machinary itself depends on it. To do that, we made a new `ExperimentalFeatureSettings`.
If we conditionally "declare" the argument, as we did before, based upon
weather the feature is enabled, commands like
nix --experimental-features=foo ... --thing-gated-on-foo
won't work, because the experimental feature isn't enabled until *after*
we start parsing.
Instead, allow arguments to also be associated with experimental
features (just as we did for builtins and settings), and then the
command line parser will filter out the experimental ones.
Since the effects of arguments (handler functions) are performed right
away, we get the required behavior: earlier arguments can enable later
arguments enabled!
There is just one catch: we want to keep non-positional
flags...non-positional. So if
nix --experimental-features=foo ... --thing-gated-on-foo
works, then
nix --thing-gated-on-foo --experimental-features=foo ...
should also work.
This is not my favorite long-term solution, but for now this is
implemented by delaying the requirement of needed experimental features
until *after* all the arguments have been parsed.
Ericson2314
force-pushed
the
hide-experimental-settings
branch
from
359e1a6 to
4607ac7
Compare
Contributor
|
Discussed in the Nix team meeting 2023-03-17:
|
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
roberth
approved these changes
Mar 27, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Settings for experimental features should not be accessible without that experimental feature enabled. For example, if
nix-commandis enabled, butflakesis disabled, the CLI should refuse to:helpandshow-configThis commit allows marking settings experimental, like we do for built-ins already, in a declarative way.
It also allows flags to likewise be marked experimental, which is needed to implement the above but also useful in its own right.
Even if we were to stabilize
flakesandnix-commandat once (and I hope not!), it is still nice to have this functionality for future experimental features. For example, we might end up needing settings and flags that exist forca-derivations. We should likewise be able to hide those from the stable CLI.Context
In #7608 @roberth started investigating what would be needed to make a very simple command
nix store gc, Hiding theseflakesettings was one of the items in his check list.The first commit message as a test that fails, and the latter two fix the test, until it passes. The latter two have somewhat in-depth commit messages explaining the implementation strategy.
Needed for #7608.
Checklist for maintainers
Maintainers: tick if completed or explain if not relevant
tests/**.shsrc/*/tests