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: don't break if builtins.trace is used in <nixos-config> #68121

Closed
wants to merge 1 commit into from

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Sep 4, 2019

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 @

…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
@infinisil
Copy link
Member

Why is stderr redirected to stdout anyways?

@Ma27
Copy link
Member Author

Ma27 commented Sep 5, 2019

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

@infinisil
Copy link
Member

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

@deliciouslytyped
Copy link
Contributor

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
Copy link
Contributor

bjornfor commented Sep 7, 2019

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

@Ma27
Copy link
Member Author

Ma27 commented Sep 7, 2019

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
Copy link
Member Author

Ma27 commented Sep 9, 2019

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

@infinisil
Copy link
Member

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
Copy link
Contributor

deliciouslytyped commented Sep 10, 2019

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
Copy link
Member

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

@lheckemann lheckemann added this to the 19.09 milestone Sep 10, 2019
@infinisil
Copy link
Member

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

@lheckemann
Copy link
Member

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

lheckemann pushed a commit that referenced this pull request Sep 13, 2019
…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 #67659

(cherry picked from commit 588aefc)
closes #68121
@lheckemann lheckemann closed this Sep 13, 2019
@Ma27 Ma27 deleted the fix-nixos-option-with-trace branch September 13, 2019 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nixos-option fails if configuration has trace output
5 participants