nixos-option: don't break if builtins.trace is used in <nixos-config>

[Ma27]

Motivation for this change

By default everything from stderr will be recorded in case of errors,
however this shouldn't break nixos-option if a simple trace call is
used that breaks the Nix expression returned by nix-instantiate.

Fixes #67659

Things done

• 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 @

[infinisil]

Why is stderr redirected to stdout anyways?

[Ma27]

To provide a meaningful output if one of the evaluations fail (e.g. due to an evaluation error).

[infinisil]

How about capturing stdout and stderr into separate variables instead of mixing them together? Something like https://stackoverflow.com/a/13806678/6605742

[deliciouslytyped]

Without having read any of the stuff here (which I admittedly should have) - it's probably not ergonomic to separate the streams for outputting to the user, because related lines should be located in their appropriate location.

So maybe display to the user should have merged output and the code should use separated streams for processing. This is starting to feel like "should not be written in bash" territory though?

[bjornfor]

FYI, this PR rewrites nixos-option from sh to C++: #68193.

[Ma27]

Yeah, seen it. But as this is a bigger change, I'm not sure if this will make it into the upcoming release and is this is mainly a bugfix, that should be good to go, right? :)

[Ma27]

@infinisil any further objections? Otherwise I'd like to merge :)

[infinisil]

Well I'm not a fan of this change because it just makes code uglier. I still think by separating stdout and stderr from the start this could be made a lot cleaner.

[deliciouslytyped]

Looks like it's handling special cases already, this "just" adds another one? shrug
Is it worth blocking a hotfix with "the whole thing needs a rewrite"? (or am I being too melodramatic?)

Effort should be transferred to the C++ issue for any significant work I think?

Edit: Oh wow I didn't notice that PR is so recent and seems to have momentum. C++ isn't my thing, but fingers crossed :D

[lheckemann]

I'm in favour of cherry-picking this (as is, no major refactoring with changing stderr and such) to 19.09 so that the bug is fixed, but not putting it in master, given the wonderful rewrite that we should be able to merge soon :)

[infinisil]

I guess. I'm just more fond of the notion of "leave code better than you saw it" than the opposite :)

[lheckemann]

Generally agree! But this is a case of "minimise effort for fixing almost-obsolete code" IMHO ;)