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

enable ANSI color (instead of ANSI color codes) in the Windows terminal #4393 #4403

Merged
merged 68 commits into from Jul 20, 2020

Conversation

akshaysharmajs
Copy link
Contributor

@akshaysharmajs akshaysharmajs commented Mar 4, 2020

modified scrapy/utils/display.py to stop ANSI color sequences in the Windows terminal (issue #4393)

@codecov
Copy link

codecov bot commented Mar 4, 2020

Codecov Report

Merging #4403 into master will increase coverage by 0.34%.
The diff coverage is 86.66%.

@@            Coverage Diff             @@
##           master    #4403      +/-   ##
==========================================
+ Coverage   84.90%   85.25%   +0.34%     
==========================================
  Files         162      162              
  Lines        9748     9760      +12     
  Branches     1437     1439       +2     
==========================================
+ Hits         8277     8321      +44     
+ Misses       1211     1178      -33     
- Partials      260      261       +1     
Impacted Files Coverage Δ
scrapy/utils/display.py 92.85% <86.66%> (+61.60%) ⬆️
scrapy/core/downloader/contextfactory.py 90.00% <0.00%> (-6.67%) ⬇️
scrapy/utils/trackref.py 82.85% <0.00%> (-2.86%) ⬇️
scrapy/utils/spider.py 77.77% <0.00%> (+11.11%) ⬆️
scrapy/robotstxt.py 97.53% <0.00%> (+22.22%) ⬆️
scrapy/utils/py36.py 100.00% <0.00%> (+80.00%) ⬆️

scrapy/utils/display.py Outdated Show resolved Hide resolved
@wRAR
Copy link
Contributor

wRAR commented Mar 5, 2020

Ideally the colorization should stay enabled on the terminals that actually support it, such as the ones from WSL or MSYS, but I suspect we need to check terminfo for that which is a lot of work for no significant gain.

OTOH this PR doesn't allow enabling the colorization explicitly on Windows, maybe the check should be earlier.

@akshaysharmajs
Copy link
Contributor Author

akshaysharmajs commented Mar 5, 2020

Ideally the colorization should stay enabled on the terminals that actually support it, such as the ones from WSL or MSYS, but I suspect we need to check terminfo for that which is a lot of work for no significant gain.
I completely agree with this.

I just made a read for the problem and found out that Windows 10's console recently began interpreting escapes, but only with ENABLE_VIRTUAL_TERMINAL_PROCESSING which is off by default. So i tried enabling it and i think it works just fine.

@akshaysharmajs akshaysharmajs changed the title stop ANSI color sequences in the Windows terminal #4393 enable ANSI color sequences in the Windows terminal #4393 Mar 8, 2020
@akshaysharmajs akshaysharmajs changed the title enable ANSI color sequences in the Windows terminal #4393 enable ANSI color (instead of ANSI color codes) in the Windows terminal #4393 Mar 8, 2020
@wRAR
Copy link
Contributor

wRAR commented Mar 11, 2020

I wonder if this new code works on pre-Windows 10 terminals? And again, what about non-cmd.exe terminals?

And I'd want some error handling there.

@akshaysharmajs
Copy link
Contributor Author

akshaysharmajs commented Mar 11, 2020

I wonder if this new code works on pre-Windows 10 terminals?

I just added a check for the windows 10 release and for specific version 10.0.14393 as the ANSI color codes problem only persist after this release of windows. So, there ll' be no problem for pre-windows 10 terminals. Also, ENABLE_VIRTUAL_TERMINAL_PROCESSING flag is only available in windows 10 and up.

@akshaysharmajs
Copy link
Contributor Author

akshaysharmajs commented Mar 11, 2020

what about non-cmd.exe terminals

For unix terminals running on windows its working fine according to me, @wRAR Please check it once.
WINDOWS CMD.EXE
1
CYGWIN TERMINAL (RUNNING ON WINDOWS)
2

@Gallaecio
Copy link
Member

Gallaecio commented Mar 12, 2020

Just for reference, Django’s implementation: https://github.com/django/django/blob/master/django/core/management/color.py#L12

@akshaysharmajs
Copy link
Contributor Author

akshaysharmajs commented Mar 23, 2020

@wRAR Is there anything that concerns you about the changes that I have made?
or what further changes can be done?

@Gallaecio
Copy link
Member

Gallaecio commented Mar 24, 2020

Django’s implementation seems less prone to issues to me, as it does not rely on specific Windows versions. Could you check if it also works for us?

@wRAR
Copy link
Contributor

wRAR commented Mar 24, 2020

@akshaysharmajs my point about error handling is still valid.

@akshaysharmajs
Copy link
Contributor Author

akshaysharmajs commented Mar 24, 2020

@wRAR @Gallaecio thanks for reply. I have added some error handling (used django's implementation for reference).
windows versions check is still there as windows has no real support for ANSI escape sequences.

@akshaysharmajs
Copy link
Contributor Author

akshaysharmajs commented Jun 6, 2020

@Gallaecio @elacuesta Please go through the changes. I have used only normal terminal formatter for all platforms and added a separate test for windows by mocking ctypes.

@akshaysharmajs
Copy link
Contributor Author

akshaysharmajs commented Jun 6, 2020

I figured out that GetStdhandle() is giving wrong handle during test I don't know why?. I tried passing actual handle to SetConsoleMode() then test is passing fine.

This test is passing fine as GetStdHandle() giving valid handle.

@Gallaecio
Copy link
Member

Gallaecio commented Jun 9, 2020

@akshaysharmajs
Copy link
Contributor Author

akshaysharmajs commented Jun 10, 2020

Okay, I am working on it.

if sys.platform == "win32" and parse_version(version()) >= parse_version("10.0.14393"):
try:
import ctypes
kernel32 = ctypes.windll.kernel32
handle = kernel32.GetStdHandle(-11)
# set `ENABLE_VIRTUAL_TERMINAL_PROCESSING` flag
if not kernel32.SetConsoleMode(handle, 7):
raise ValueError
except ValueError:
return text
Copy link
Contributor Author

@akshaysharmajs akshaysharmajs Jun 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Gallaecio please suggest how should I cover this part. I tried mocking windows platform but I think parse_version is still failing.

Copy link
Member

@Gallaecio Gallaecio Jun 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please push the test code that fails, so that I can have a look?

Copy link
Contributor Author

@akshaysharmajs akshaysharmajs Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have pushed the test code.Please have a look, windows mocking part is failing giving this error AttributeError: <module 'ctypes' from '/usr/lib/python3.6/ctypes/__init__.py'> does not have the attribute 'windll'

@Gallaecio
Copy link
Member

Gallaecio commented Jun 16, 2020

Please, have a look at akshaysharmajs#1

It’s a refactoring proposal, where separate methods are used in the color-handling implementation for easier testing and better code readability.

Note that I removed the curses stuff, because I still don’t understand why it is needed, and fear it may result in terminals supporting colors not displaying them just because curses is not installed. But I may be wrong here.

Refactor output color handling
@akshaysharmajs
Copy link
Contributor Author

akshaysharmajs commented Jun 16, 2020

@Gallaecio thanks for the changes, looks perfect and precise to me. I see removing curses is completely fine and also helping in testing.
I don't know if we are fine with windows versions < 10.0.14393 with no colors?

@Gallaecio
Copy link
Member

Gallaecio commented Jun 17, 2020

I don't know if we are fine with windows versions < 10.0.14393 with no colors?

I thought those Windows versions would not support color.

Is that what curses was for? Do older Windows versions correctly parse ANSI color codes if curses is installed?

If so, maybe it makes sense to add back the curses code (but only for those old Windows versions?).

On a separate note, my refactoring broke the Flake8 checks, as well as Python 3.5 tests through the use of f-strings (not the first time I forgot Scrapy cannot use them yet 🤦 ).

@akshaysharmajs
Copy link
Contributor Author

akshaysharmajs commented Jun 17, 2020

curses was only for different formatters for different platform. but now we are only using normal formatter so there is no use of it.

Is that what curses was for? Do older Windows versions correctly parse ANSI color codes if curses is installed?

Older versions of windows also interpret ansi escape codes as terminal processing is enabled by default and in newer versions we have to enable it separately.

@akshaysharmajs
Copy link
Contributor Author

akshaysharmajs commented Jun 17, 2020

On a separate note, my refactoring broke the Flake8 checks, as well as Python 3.5 tests through the use of f-strings (not the first time I forgot Scrapy cannot use them yet 🤦 ).

😄😄 no problem I will fix it.

@Gallaecio
Copy link
Member

Gallaecio commented Jun 18, 2020

Older versions of windows also interpret ansi escape codes as terminal processing is enabled by default and in newer versions we have to enable it separately.

Oh, I had no idea. I see you’ve already fixed the logic accordingly, so 👍.

@akshaysharmajs
Copy link
Contributor Author

akshaysharmajs commented Jun 18, 2020

Yes I have fixed it accordingly. 😃
Thanks @Gallaecio

@akshaysharmajs akshaysharmajs requested a review from elacuesta Jun 19, 2020
@akshaysharmajs
Copy link
Contributor Author

akshaysharmajs commented Jun 29, 2020

Please suggest, what else can be done here??

wRAR
wRAR approved these changes Jul 20, 2020
@wRAR
Copy link
Contributor

wRAR commented Jul 20, 2020

Thank you!

@wRAR wRAR merged commit de297a3 into scrapy:master Jul 20, 2020
2 checks passed
@akshaysharmajs akshaysharmajs deleted the Scrapychanges branch Jul 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants