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

Ansi codes are not properly handled on Windows Terminal #939

Closed
julie777 opened this issue Feb 26, 2022 · 15 comments
Closed

Ansi codes are not properly handled on Windows Terminal #939

julie777 opened this issue Feb 26, 2022 · 15 comments
Labels
🐛 bug Something isn't working
Milestone

Comments

@julie777
Copy link

  • [x ] I have searched the issue tracker and believe that this is not a duplicate.

Make sure you run commands with -v flag before pasting the output.

Steps to reproduce

run git bash in Windows Terminal
run pdm -h

Actual behavior

Terminal codes are output raw.

PDM - Python Development Master

←[33m←[1mCommands←[0m:
←[36m  {add,build,cache,completion,config,export,import,info,init,install,list,lock,plugin,remove,run,search,show,sync,update,use}
←[0m←[36m    add                 ←[0mAdd package(s) to pyproject.toml and install them
←[36m    build               ←[0mBuild artifacts for distribution
←[36m    cache               ←[0mControl the caches of PDM
←[36m    completion          ←[0mGenerate completion scripts for the given shell

Expected behavior

I expected output to be formatted and colored like it is in other environments.

Environment Information

# Paste the output of `pdm info && pdm info --env` below:
$ pdm info && pdm info --env
PDM version:        1.13.3
Python Interpreter: C:\Users\julie\AppData\Local\Programs\Python\Python310\python.EXE (3.10)
Project Root:       C:/tmp/a
Project Packages:   C:\tmp\a\__pypackages__\3.10
{
  "implementation_name": "cpython",
  "implementation_version": "3.10.2",
  "os_name": "nt",
  "platform_machine": "AMD64",
  "platform_release": "10",
  "platform_system": "Windows",
  "platform_version": "10.0.19043",
  "python_full_version": "3.10.2",
  "platform_python_implementation": "CPython",
  "python_version": "3.10",
  "sys_platform": "win32"
}
@julie777 julie777 added the 🐛 bug Something isn't working label Feb 26, 2022
@julie777
Copy link
Author

I will submit a pull request shortly, but I want to document the issue here.
Windows Terminal emulates and xterm unlike the Windows Console used for cmd.exe and powershell. This means that just checking for platform=='win32' is not sufficient to handle terminal configuration.

Colorama documents that calling initialise.py:init with no arguments should cause Colorama to do nothing if not on Windows. This fails to account for Windows Terminal.

Someone already recognized that and added code in install-pdm.py support_ansi and termui.py supports_ansi (which are almost the same) that detects Windows Terminal specifically os.getenv("WT_SESSION") is not None and sets ansi to True. This test causes colorama.deinit() to be called instead of colorama:init(). I don't understand why deinit should be called, but it may be related to the problem which causes the bad output.

In logsymbols/symbols.py there is a call to colorama init(autoreset=True) and coloram 'deinit(). Because colorama doesn't support Windows Terminal directly any calls to colorama.init` regardless of parameters will put it in a strange mode causing the problem above. Colorama init is not required to use Fore in symbols.py so I removed the calls and pdm works great in Windows Terminal.

After my changes, colorama:deinit() is called once and there are no init calls (when running pdm -h which is the only testing that I have done.

@julie777 julie777 mentioned this issue Feb 26, 2022
1 task
@pawamoy
Copy link
Contributor

pawamoy commented Feb 26, 2022

Getting this right always seemed way too complicated to me! Now it seems less complicated, thanks to your thorough explanation, so thanks a lot for sharing 🙂

@pawamoy
Copy link
Contributor

pawamoy commented Feb 27, 2022

For reference: tartley/colorama#306

@frostming
Copy link
Collaborator

@julie777 Thanks for your explanation but some part doesn't make sense to me. Can you kindly elaborate on that?

I don't understand why deinit should be called, but it may be related to the problem which causes the bad output.

deinit just restores stdout and stderr to the original unwrapped object if possible, and should be a no-op if they are not wrapped by any previous calls to init. It doesn't make sense it will bring any bad effects.

And I am also using Windows Terminal and it just works fine. From another look of the source code I think it might be caused by the second call to init, so can you try only removing the call and see if it works?

@julie777
Copy link
Author

@julie777 Thanks for your explanation but some part doesn't make sense to me. Can you kindly elaborate on that?

I don't understand why deinit should be called, but it may be related to the problem which causes the bad output.

It gets called in termui.py because supports_ansi is true.

deinit just restores stdout and stderr to the original unwrapped object if possible, and should be a no-op if they are not wrapped by any previous calls to init. It doesn't make sense it will bring any bad effects.

I understand, but if the code is not ever calling init (as above) then calling deinit doesn't have a purpose.

And I am also using Windows Terminal and it just works fine. From another look of the source code I think it might be caused by the second call to init, so can you try only removing the call and see if it works?

The init in termui.py never gets called on Windows Terminal because supports_ansi correctly identifies that Windows Terminal is running and supports ansi. It is the check for WT_SESSION below. Without my changes init only gets called once in symbols.py and that is what caused the problem.

 if sys.platform == "win32":
        return (
            os.getenv("ANSICON") is not None
            or os.getenv("WT_SESSION") is not None
            or "ON" == os.getenv("ConEmuANSI")
            or (os.getenv("Term") and os.getenv("Term").startswith('xterm'))
        )

Additional explanation

When I started there was only one call to init actually being called, which was the one in symbols.

When I talk about calling deinit, I am refering to terminal.py where it does the initialization of the terminal.

if not self.supports_ansi:
            colorama.init()
        else:
            colorama.deinit()

I ansi is supported it calls deinit() even though init was not called (at least after I took it out of symbols.)

@frostming
Copy link
Collaborator

I ansi is supported it calls deinit() even though init was not called (at least after I took it out of symbols.)

Correct, but deinit will become a no-op in this case and shouldn't do harm, or am I getting anything wrong?

@julie777
Copy link
Author

julie777 commented Mar 1, 2022

I ansi is supported it calls deinit() even though init was not called (at least after I took it out of symbols.)

Correct, but deinit will become a no-op in this case and shouldn't do harm, or am I getting anything wrong?

I agree with you. I just don't see a reason to call it. I am sorry if I am not clear enough with my writing.

@frostming
Copy link
Collaborator

I ansi is supported it calls deinit() even though init was not called (at least after I took it out of symbols.)

Correct, but deinit will become a no-op in this case and shouldn't do harm, or am I getting anything wrong?

I agree with you. I just don't see a reason to call it. I am sorry if I am not clear enough with my writing.

The point here is to try not to modify the _vendor directory. Fix it outside unless it is not possible.

@julie777
Copy link
Author

julie777 commented Mar 4, 2022

I understand your not wanting to change anything in the _vendor directory.
Sadly, the one line of my pull request that is absolutely necessary is removing the line

init(autoreset=True)

from pdm/_vendor/log_symbols/symbols.py which is in the _vendor directory.

@frostming
Copy link
Collaborator

frostming commented Mar 5, 2022

@julie777 Because I didn't see any arguments supporting the necessity of removing the line.

In logsymbols/symbols.py there is a call to colorama init(autoreset=True) and colorama deinit(). Because colorama doesn't support Windows Terminal directly any calls to colorama.init regardless of parameters will put it in a strange mode causing the problem above

As you said, init and deinit in the symbols.py appear pairly and any effects that may be brought by init() will be reverted by deinit(), right?

I don't understand why deinit should be called, but it may be related to the problem which causes the bad output.

It will revert if init takes effects, and no-op otherwise. Therefore, after deinit is called, stdout and stderr should revert to normal.

Most importantly, I am also using Windows Terminal and it works normally, so I think the issue might not be as what you analyzed.

I want to see a reason of "MUST", not "MAY" or "SHOULD", to change the code in _vendors

@julie777
Copy link
Author

julie777 commented Mar 5, 2022

@frostming
I'm glad you kept pushing. I tried again today and found that my fix no longer worked. I have no idea why it seemed to work before.

During my testing I found that piping the output to less causes the output to show ansi codes instead of colors. This may have contributed to my confusion when trying to diagnose the problem, as I was using less at times to stop some debugging output from scrolling off of the screen. Using head instead allows the colors to show.

I closed my previous pull request and opened a new one with a fix that works. I found a reference to the problem https://bugs.python.org/issue40134

I have run in multiple shells. I have created a new shell just before running pdm. It is always repeatable.
If I remove the os.system("") call it doesn't show colors and if I put it in the colors work as expected.

Sorry for all the confusion.

@frostming frostming added this to the Release 2.0 milestone Jun 29, 2022
@frostming
Copy link
Collaborator

Please test if it still occurs on 2.0.0a1

@ineshbose
Copy link

Still facing this issue in 2.1.2!

@iincer
Copy link

iincer commented Mar 17, 2023

I also get this on Windows. I have pdm 2.4.0. This is my pdm -V output on cmd, powershell, and bash:

←[1mPDM←[0m, version ←[32m2.4.0←[0m

@kalleswe
Copy link

ANSI codes should be treated as ANSI-BBS standard at CP437 and the line drawing and special characters should be rendered correctly, which they are not currently doing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants