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: don't break if builtins.trace
is used in <nixos-config>
#68121
Conversation
…ig>` 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 evaluated by `nixos-option`. Fixes NixOS#67659
Why is stderr redirected to stdout anyways? |
To provide a meaningful output if one of the evaluations fail (e.g. due to an evaluation error). |
How about capturing stdout and stderr into separate variables instead of mixing them together? Something like https://stackoverflow.com/a/13806678/6605742 |
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? |
FYI, this PR rewrites nixos-option from sh to C++: #68193. |
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? :) |
@infinisil any further objections? Otherwise I'd like to merge :) |
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. |
Looks like it's handling special cases already, this "just" adds another one? shrug 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 |
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 :) |
I guess. I'm just more fond of the notion of "leave code better than you saw it" than the opposite :) |
Generally agree! But this is a case of "minimise effort for fixing almost-obsolete code" IMHO ;) |
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 isused that breaks the Nix expression returned by
nix-instantiate
.Fixes #67659
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)Notify maintainers
cc @