nixos-option C++ rewrite with --all #68193
Conversation
|
(To make review easier: I'm not sure what the normal way to build packages inside nixos is, but I found that I could build this with this invocation: ) |
|
This should be supported by the @NixOS/nix-core team as we would need to keep |
|
For the C++ code, could you use the same formatting as Nix (e.g. 4 space indent)? See .dir-locals in the Nix source tree. 👍 on dropping the XML option. It would make more sense to have a |
|
@chkno The canonical way to build NixOS-specific tools is like this: |
There was a problem hiding this comment.
First of all thanks a lot for working on this! The current implementation of nixos-option is indeed rather fragile and should be replaced entirely IMHO.
However I'd like to express a few concerns about the current approach:
-
When we work on
nixos-optionwe should also take a refactoring ofnixops show-optioninto account: this command uses strict evaluation to introspect attributes of the current deployment which happens to cause evaluation failures in several cases and doesn't provide a human-friendly output e.g. when querying an attribute set. So I guess that it would make sense to build a solution that can be easily used withnixops show-option. -
Not sure if evaluating the entire system config every time is worth the overhead since it needs to be re-implemented by several other tools (e.g. I built a wrapper for
evalModulesinnix-zsh-completions). Some months ago I discussed that with @fpletz and @elseym and we had the idea that a config activation should dump the current config (including details about where a value was declared) as JSON in/run/current-system, in that case tools likenixops show-option,nixos-option, their autocompletions and any other arbitrary tool would simply need to parse the JSON. Unfortunately I lacked time to build a proper implementation for that, but mostly out of interest, what would you think of that? :)
|
Ma27 - Thanks for your comments! I'm new to Nix. I don't know what nixops is, so I will read about nixops and get back to you. |
|
@Ma27 Do you think this should be done as part of this pull request or can this be solved in future contributions? |
There was a problem hiding this comment.
Wow, an impressive first contribution! I love it!
I've skimmed over the source, but won't claim to have reviewed it exhaustively. With:
- the adjustments I've commented
- a review from someone else (more familiar with nix's code, ideally!)
- and an entry in the release notes,
I'd be happy to merge this. A few more questions:
-
Is there any particular reason not to have this as a standalone tool that can be built from nixpkgs without going through nixos, i.e. add it to
all-packages.nix? -
re @Ma27's suggestion — I think this approach is nicer. Evaluating individual options isn't nearly as expensive as evaluating the whole system config, and I like the ability to look at system options before building the system. That said, this would also be an excellent base for creating that dump and having both ways available!
-
@Ma27 re
nixops show-option— that should be fairly easy to implement on top of this with the help of the--config_exprand--option_exproptions?
We've missed the branch-off for 19.09, but I'm already excited to have this as a feature in 20.03! 😊
| if (evaluated_type.type != tString) { | ||
| return false; | ||
| } | ||
| return evaluated_type.string.s == static_cast<std::string>("option"); |
There was a problem hiding this comment.
Shouldn't this be std::string(evaluated_type.string.s) == "option"?
There was a problem hiding this comment.
I don't think it matters. As long as one or the other of them is a std::string, I get an operator== that looks at content rather than pointer equality. I picked casting the constant over casting the dynamic thing in hopes that the compiler would have a better chance at optimizing away any runtime cost.
But, I don't think this matters, so I switched it as you suggested.
For consistency with the Nix C++ convention. :~(
I don't think this matters. As long as one or the other of these is a std::string, I get an operator== that looks at content rather than pointer equality. I picked casting the constant over casting the dynamic thing in hopes that the compiler would have a better chance at optimizing away any runtime cost. Deferring to reviewer.
Switch from convention "appease clang-tidy --checks='*'" to "References are like non-nullptr pointers". The clang-tidy check "google-runtime-references" complains about non-const reference arguments, but this is not a convention used in Nix.
Switch from convention "appease clang-tidy --checks='*'" to "References are like non-nullptr pointers". The clang-tidy check "google-runtime-references" complains about non-const reference arguments, but this is not a convention used in Nix.
Switch from convention "appease clang-tidy --checks='*'" to "References are like non-nullptr pointers". The clang-tidy check "google-runtime-references" complains about non-const reference arguments, but this is not a convention used in Nix.
Automated consumers can use 'sed 1d' or similar to remove this header. This probably makes this output *easier* to consume correctly. Having this header show up in consumers' terminal or log output is probably not useful, but hiding it without hiding all error messages would have been more troublesome that just stripping it from stdout. I.e., previously, unsophisticated use would show undesired output: $ some-other-tool This attribute set contains: This attribute set contains: This attribute set contains: This attribute set contains: <Actual some-other-tool output> The simplest way to hide this undesired output would have been nixos-option ... 2>/dev/null, which would hide all error messages. We do not wish to encourage that. Correct use would have been something like: nixos-option ... 2> >( grep --line-buffered -v 'This attribute set contains:') After this change, correct use is simpler: nixos-option ... | sed 1d or nixos-option ... | sed '1/This attribute set contains:/d' if the caller don't know if this invocation of nixos-option will yield an attribute listing or an option description.
lheckemann
force-pushed
the
nixos-option-rewrite
branch
from
05f91bc
to
754e315
Compare
nixos-option C++ rewrite with --all (cherry picked from commit 6f41b1c)
Motivation for this change
I wanted a --all flag for nixos-option. As a shell script, this took tens of hours to run and felt brittle. As a direct client of the C++ nix library, it takes six seconds & feels robust. Looking at diffs of the --all output before and after making changes in configuration.nix is amazingly helpful in understanding what's going on inside and for doing refactoring.
This implementation's output format is slightly different, and where different it is mostly an improvement (eg: indented). I didn't try to exactly match the shell script's output format, but I am happy to pursue that if there are important automated consumers that cannot be atomically updated.
The shell script says it has an XML output mode, but it seems to do very little (it only works on non-leaf paths --
soundbut notsound.enable), so I made no attempt to preserve XML output mode.Things done
(I'm not sure which of these things make sense, how to do some of them, or what other testing would be useful. I'm new to Nix. Thanks for your help!)
sandboxinnix.confon non-NixOS)nix-shell -p nix-review --run "nix-review wip"./result/bin/)nix path-info -Sbefore and after)Notify maintainers
cc @nbp @Ma27