ocamltest: explicitly set OCAML_ERROR_STYLE#2271
Merged
gasche merged 1 commit intoocaml:trunkfrom Feb 27, 2019
Merged
Conversation
gasche
reviewed
Feb 27, 2019
Member
gasche
left a comment
There was a problem hiding this comment.
The code generally looks good, but see comments for a small change suggestion.
ocamltest/ocaml_actions.ml
Outdated
| error_style_reader.env_var | ||
| (error_style_reader.print Misc.Error_style.default_setting) | ||
| in | ||
| [| "TERM=dumb"; error_style |] |
Member
There was a problem hiding this comment.
I would generalize this logic to deal with Error_style and Color uniformly.
let env_setting env_reader default_setting = ...
let ocaml_env = [|
"TERM=dumb";
env_setting Clflags.error_style_reader Misc.Error_style.default_setting;
env_setting Clflags.color_reader Misc.Color.default_setting
|]Today I think that this is unnecessary because TERM=dumb implies the default color setting, but this shape of code (being careful to list all those touchy environment-decided options) scales better to the potential addition of new readers in the future.
Member
There was a problem hiding this comment.
Also I think that default_ocaml_env would be a better variable name.
Member
Author
There was a problem hiding this comment.
Indeed, you're completely right. I just implemented your suggestion.
Explicitly set OCAML_ERROR_STYLE to its default value when running tests, instead of inheriting the one coming from the outside environment.
e47289e to
00411d4
Compare
gasche
approved these changes
Feb 27, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explicitly set
OCAML_ERROR_STYLEto its default value when running tests,instead of inheriting the one coming from the outside environment.
This should fix the concern raised by @mshinwell in #2096 .
NB: in theory an other way of doing that would be to unset the environment variable (so that the compiler uses the default value), but to my knowledge we do not bind unsetenv(3) (and would there be a windows counterpart?), so it's not possible to do that.
So we have to explicitly get and set the environment variable to the default value, which involves a bit of plumbing.