-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Add colored print to test harness output #25495
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
Conversation
tools/diagnostics.py
Outdated
|
||
|
||
def with_color(color, text): | ||
return f'\033[9{color}m{text}\033[0m' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why wouldn't this be output_color() + text + reset_color()
?
Why is ok to use ansi here, even on windows, when we use different mechanism for windows above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
output_color() and reset_color() adjust the current terminal state already before printing. That works for full lines to be printed, e.g.
output_color(...)
print('asdf')
reset_color()
but not for adjusting color on an individual line.
Why is ok to use ansi here
That I actually don't know. In general Python it is not ok on Windows to do that (i.e. if you write a standalone python file with a print('\033[92m HELLO \033[0m')
, it will not work on Windows.
But we have run some code somewhere else to enable the current terminal to use ANSI escape codes on Windows. Modules like colorama seem to do that, but I don't know what it is that enables that in Emscripten's case for Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But output_color
and reset_color
just return strings don't they? it should be possible to construct your string here by using those function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
output_color()
calls to output_color_windows()
, which calls to SetConsoleTextAttribute, which is a global state for the current terminal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, but given that with_color
in this PR doesn't work on windows anyway why not just do:
def with_color(color, text):
if WINDOWS:
ruturn text
return output_color() + text + reset_color()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with_color()
does work on both Windows 10 and 11.. the screenshot I posted in the PR description is a screenshot from Windows cmd.exe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, in that case I think we should probably just delete the current output_color_windows
code.
I'm assuming more users are on windows 10 or 11 these days? Users of older versions of windows just wouldn't get color?
i.e. if we are going to assume ANSI support on windows lets just do that everywhere maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we can detect it using ENABLE_VIRTUAL_TERMINAL_PROCESSING
... (I will take a stab at it unless you are feeling like it?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds like a good idea, though I'd rather leave that for a future refactor PR, since I don't yet know the extent here.
In general for Windows, I think we can call all pre-Windows 10 versions unsupported. (i.e. XP, Vista, 7, 8, 8.1 all are obsolete). On Windows 10, we could take e.g. Win10 21H1 (build 19043, May 18, 2021) as the minimum version (not that it would matter for Emscripten I believe)
msg = f'{test} ... ERROR' | ||
errlog(f'{self.compute_progress()}{with_color(RED, msg)}') | ||
self.buffered_result = BufferedTestError(test, err) | ||
self.test_result = 'errored' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this work for -j1
too? i.e. the non-parallel runner? Maybe as a followup? Maybe the default python runner already has some color support?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be nice (also to add the [35%] progress bar), though looks more difficult, since in non-parallel runner the printing is handled by the built-in TextTestResult class, which I don't know how to replace.
Another thing I'd want to do is remove the long wall of test results dump at the very end of the test run, which repeats the same results from the test run a second time.. that is a bit spammy.
Windows 10 and above support ANSI coloring and there should be very few emscripten users on older version of windows at this point (if any?). The downside is that windows 7 will no longer have nice colored error messages, but the upside is that we can cleanup and refactor this code now (see emscripten-core#25495).
Windows 10 and above support ANSI coloring and there should be very few emscripten users on older version of windows at this point (if any?). The downside is that windows 7 will no longer have nice colored error messages, but the upside is that we can cleanup and refactor this code now (see emscripten-core#25495).
Windows 10 and above support ANSI coloring and there should be very few emscripten users on older version of windows at this point (if any?). The downside is that windows 7 will no longer have nice colored error messages, but the upside is that we can cleanup and refactor this code now (see emscripten-core#25495).
Windows 10 and above support ANSI coloring and there should be very few emscripten users on older version of windows at this point (if any?). The downside is that windows 7 will no longer have nice colored error messages, but the upside is that we can cleanup and refactor this code now (see emscripten-core#25495).
Windows 10 and above support ANSI coloring and there should be very few emscripten users on older version of windows at this point (if any?). The downside is that windows 7 will no longer have nice colored error messages, but the upside is that we can cleanup and refactor this code now (see emscripten-core#25495).
Windows 10 and above support ANSI coloring and there should be very few emscripten users on older version of windows at this point (if any?). The downside is that windows 7 will no longer have nice colored error messages, but the upside is that we can cleanup and refactor this code now (see emscripten-core#25495).
Windows 10 and above support ANSI coloring and there should be very few emscripten users on older version of windows at this point (if any?). The downside is that windows 7 will no longer have nice colored error messages, but the upside is that we can cleanup and refactor this code now (see emscripten-core#25495).
Windows 10 and above support ANSI coloring and there should be very few emscripten users on older version of windows at this point (if any?). emscripten already does not support windows 7 IIUC, since its dependencies (both python 3 and node 18) do not support windows 7 This means that only windows 8 users should be effected by this change. The upside is that we can cleanup and refactor this code now (see #25495).
tools/diagnostics.py
Outdated
|
||
|
||
def with_color(color, text): | ||
return f'\033[9{color}m{text}\033[0m' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can now be output_color() + text + reset_color()
right? Now that #25502 has landed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm,
def output_color(color):
assert color_enabled
return '\033[3%sm' % color
this generates a different code than \033[9{color}m
that I was using. I wonder where that \033[3
comes from. Gives blue instead of cyan.. but green and red are intact.
But maybe blue looks nicer than cyan, I'll let that be like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was wondering that myself, certainly worth not have two ways to output a given color (at least not without documenting).
9cc9962
to
8fc7709
Compare
CircleCI fails on
although locally for me on Windows 10, test run is good with colors. |
Demoted the code to output non-colors instead of asserting, if colors not enabled.. Guess that should be per design? |
Ah I guess we should do |
Another take at adding colors to test harness:
Tested to work on: