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

nixos-option C++ rewrite with --all #68193

Merged
merged 31 commits into from Nov 4, 2019
Merged

Conversation

@chkno
Copy link
Contributor

chkno commented Sep 6, 2019

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 -- sound but not sound.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!)

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @nbp @Ma27

@chkno chkno requested a review from nbp as a code owner Sep 6, 2019
@chkno

This comment has been minimized.

Copy link
Contributor Author

chkno commented Sep 6, 2019

(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:

nix-build -E '((import <nixpkgs> {}).nixos {}).nixos-option' -I nixpkgs="$HOME/devel/nixpkgs"

)

@Mic92

This comment has been minimized.

Copy link
Contributor

Mic92 commented Sep 6, 2019

This should be supported by the @NixOS/nix-core team as we would need to keep nixos-option compatible with the nix API.

@edolstra

This comment has been minimized.

Copy link
Member

edolstra commented Sep 6, 2019

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 --json flag now.

@edolstra

This comment has been minimized.

Copy link
Member

edolstra commented Sep 6, 2019

@chkno The canonical way to build NixOS-specific tools is like this:

nix-build '<nixpkgs/nixos>' -A config.system.build.nixos-install
@FRidh FRidh added this to the 19.09 milestone Sep 6, 2019
@FRidh FRidh self-requested a review Sep 6, 2019
Copy link
Member

Ma27 left a 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-option we should also take a refactoring of nixops show-option into 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 with nixops 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 evalModules in nix-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 like nixops 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? :)

@chkno

This comment has been minimized.

Copy link
Contributor Author

chkno commented Sep 8, 2019

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.

@Mic92

This comment has been minimized.

Copy link
Contributor

Mic92 commented Sep 9, 2019

@Ma27 Do you think this should be done as part of this pull request or can this be solved in future contributions?

Copy link
Member

lheckemann left a 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_expr and --option_expr options?

We've missed the branch-off for 19.09, but I'm already excited to have this as a feature in 20.03! 😊

@lheckemann lheckemann modified the milestones: 19.09, 20.03 Sep 9, 2019
if (evaluated_type.type != tString) {
return false;
}
return evaluated_type.string.s == static_cast<std::string>("option");

This comment has been minimized.

Copy link
@edolstra

edolstra Sep 10, 2019

Member

Shouldn't this be std::string(evaluated_type.string.s) == "option"?

This comment has been minimized.

Copy link
@chkno

chkno Sep 16, 2019

Author Contributor

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.

chkno and others added 24 commits Sep 6, 2019
This is important because this contains some code copied from nix (as an
interim expediency until that functionality can be exported via nix's
API).  The license specified here must be compatible with this borrowing.
Select the same license that nix is released under: lgpl2Plus.
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 lheckemann force-pushed the chkno:nixos-option-rewrite branch from 05f91bc to 754e315 Nov 4, 2019
@lheckemann lheckemann merged commit 6f41b1c into NixOS:master Nov 4, 2019
12 checks passed
12 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
dtzWill added a commit to dtzWill/nixpkgs that referenced this pull request Nov 4, 2019
nixos-option C++ rewrite with --all

(cherry picked from commit 6f41b1c)
@chkno chkno deleted the chkno:nixos-option-rewrite branch Nov 4, 2019
@Mic92 Mic92 mentioned this pull request Dec 3, 2019
4 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.