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

support mintty (MSYS/Cygwin terminals) #226

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

SSE4
Copy link

@SSE4 SSE4 commented Aug 5, 2019

closes: #224
detect mintty (MSYS bash, Git bash, Cygwin bash, MinGW bash, etc)
based on https://github.com/msys2/MINGW-packages/pull/2675/files

@SSE4 SSE4 changed the title - support MSYS/Cygwin terminals support mintty (MSYS/Cygwin terminals) Aug 5, 2019
@charlesbaynham
Copy link

Just a note that I came across this PR while trying to get colorama working with git's bash environment on Windows, and it worked like a charm! Would be great to see it merged.

@tartley
Copy link
Owner

tartley commented Oct 13, 2020

Hey. FYI, Yesterday I created a PR to test releases before we push them to PyPI. When that is merged, I'll be more confident about resuming merges and releases. I'll try to look at this PR soon. Thank you for creating it!

@3tilley
Copy link
Contributor

3tilley commented Oct 1, 2021

Hello, what's the latest on this? Our team is suffering the same issue described above

@tartley
Copy link
Owner

tartley commented Oct 7, 2021

Hi @3tilley. I have been AWOL for years, but am now rounding up PRs that could get merged. I'll take a look at this. I'm currently biased against it because there are no tests, but I can see people want it.

import msvcrt
import ctypes
import sys
import re
Copy link
Owner

Choose a reason for hiding this comment

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

Hey. Imports should be at the top of the file, not in functions. Thanks!

Copy link
Author

Choose a reason for hiding this comment

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

moved to the top

@@ -13,6 +13,44 @@
winterm = WinTerm()


def is_msys_cygwin_tty(stream):
# https://github.com/msys2/MINGW-packages/pull/2675/files
Copy link
Owner

Choose a reason for hiding this comment

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

Not a biggie, but I'd vote for putting this URL in the commit message (with a sentence explaining what it is, as you have done) not in the code. If people want more info about the function and where it came from, they can git blame it, and will find the URL.

Copy link
Author

Choose a reason for hiding this comment

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

I've changed the commit message

@tartley tartley added the 0.4.5 Get merged for 0.4.5 release label Oct 7, 2021
inspired by https://github.com/msys2/MINGW-packages/pull/2675/files
MSYS/Cygwin terminals (mintty) have filenames of the specific pattterns:
(msys|cygwin)-<GUID>-pty<NUMBER>-(from|to)-master
GetFileInformationByHandleEx is used to obtain a filename from the file descriptor
https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-getfileinformationbyhandleex

Signed-off-by: SSE4 <[email protected]>
@SSE4
Copy link
Author

SSE4 commented Oct 7, 2021

master:
image
my branch:
image

@Delgan
Copy link
Contributor

Delgan commented Oct 7, 2021

Hi!

Out of curiosity, is there any reason for implementing this using WinAPI and not simply checking for the "TERM" environment variable?

@SSE4
Copy link
Author

SSE4 commented Oct 8, 2021

Hi!

Out of curiosity, is there any reason for implementing this using WinAPI and not simply checking for the "TERM" environment variable?

to be honest, I am not sure how is it reliable. environment variable could be set to anything, e.g. after few quick tests, I can say spawning a cmd.exe from the MSYS2 terminal inherits TERM=xterm-256color for me. vice versa, spawning MSYS2 bash.exe from the cmd.exe also inherits TERM it was previously set in cmd.exe.

I also don't know for which values to check. AFAIR ncurses has a (large) terminal database, so it might be not so simple to maintain a white-list of good/known TERM values.

@Delgan
Copy link
Contributor

Delgan commented Oct 8, 2021

@SSE4 Thanks for answering!

I agree checking for "TERM" is probably not as reliable as your solution.

This is what is used by pytest though: terminalwriter.py#L36.
However it can become very complex if more accuracy is desired as illustrated by chalk implementation: index.js#L139.

I was suggesting "TERM" as it's a simple solution which does not require dealing with Windows stuff, but it's surely not enough to cover all possible cases.

@3tilley
Copy link
Contributor

3tilley commented Oct 22, 2021

Is there anything outstanding with this PR? I know there was a test mention above, I don't know how the above could be tested sensibly though. How is it done for other terminals?

colorama/ansitowin32.py Outdated Show resolved Hide resolved
@tartley
Copy link
Owner

tartley commented Oct 22, 2021

@3tilley Hi there! Thank you for responding to my earlier comment from some time ago. Now that I look again, it reminds me of a 2nd comment I have about the 'import' statement. Sorry I didn't think of it earlier, and thank you for your patience.

In reply to your question about testing: Yes, I agree having an end-to-end test for working with a particular terminal type, that is difficult to do, and such a test would have limited utility, since it would only be useful when executing the tests in that terminal type (we'd presumably have to skip the test if running elsewhere.) So other contributors which don't use that type of terminal would be unlikely to ever run your test, and CI would not be able to usefully run your test. It would be of very little use. (in many ways, that's why the various 'demo' scripts exist - so that people on a particular platform can eyeball the actual output as a sanity check)

However, we can still write unit tests. The goal of these is that If someone comes along and modifies your code in the future, accidentally completely breaking it, would anything alert us to that? Or would we deploy the broken feature? If we add code without tests which would warn us of this problem, then IF someone breaks your code in the future, and we deploy that, annoying many users (we have a billion and a half downloads at this point) then the root cause of that failure would be the PR that introduced untested code, not the merge or deploy of the broken code.

I'll write more in another comment, one sec...

@tartley
Copy link
Owner

tartley commented Oct 22, 2021

First, we'd need a unit test that isatty() calls your new function correctly. (ie. that it does call it, that it combines the result with something else using an 'or', and that it returns the result which uses this expression.)

To do this, first write as simple a test as we can for which isatty returns False. Hopefully we could copy an existing test to obtain this. As a sanity check, run that test and watch it pass. Now modify the test, to assert that isatty returns True. To make that pass, modify the test so that in its setup, it does something that makes is_msys_cygwin_tty return True. There are a number of approaches you might use to make that happen. Most conceptually straightforward (although probably not the simplest resulting test) is to make sure that the stream our test gives isatty to work on does have a fileno attribute, and a "windll" attribute, etc. Eventually, once your test sets things up just right, is_mysys_cygwin_tty' will start returning True, which will make isatty return True, which will make your test pass.

One non-trivial part of doing the above is that on non-windows platforms, the check for whether msvcrt imports will always return False, making the test fail no matter what we do. There are ways we could work around this, and right now I'm favoring the following:

Instead of your test trying to make is_mysys_cygwin_tty actually return true, we won't bother, and will instead patch out that function, so that when isatty calls is_msys_cygwin_tty(), it is actually calling a function (or mock) that your test has provided, which always returns True.

At this point, some people feel uncomfortable that the test isn't actually calling is_msys_cygwin_tty. So what's the point? The test isn't even running your new function. But remember, THIS test is supposed to be demonstrating that the change to isatty works. It doesn't, and shouldn't, care about whether is_mysys_cygwin_tty works. That's the job of a different test, which I'll describe in the next comment.

@SSE4
Copy link
Author

SSE4 commented Oct 22, 2021

just in case, ssome more screenshots with other terminals
Cygwin terminal:
image
cmd.exe:
image
powershell:
image

@tartley
Copy link
Owner

tartley commented Oct 22, 2021

The 2nd kind of unit tests needed are those that call is_mysys_cygwin_tty, and verifies it produces the correct outputs for various inputs.

What we'd really like to do is start with a test that asserts is_mysys_cygwin_tty returns True. Then modify the test setup to do whatever we have to do to make that test pass. So, for example, the stream the test passes should have the correct attributes, etc.

Again, there will be complications about doing windows-specific logic when the test is running on non-windows platforms (or indeed, on non-cygwin platforms.) There are a few possible workarounds.

  1. We could write one test that only runs on non-windows platforms (using unittest's 'skipIf'), and asserts that is_mysys_cygwin_tty always returns false (because of the msvcrt import check.) Then we write another test that only runs on non-cygwin platforms, and again assert the result is always False (because of all the stream attribute checks, etc). Then finally, we write a test that only runs on cygwin, and asserts that the result is always True.

    But I don't like this much. The most important part of these tests is the third part, and that would only ever run when the tests get run in a cygwin terminal, which presumably won't be often, so we could easily merge broken commits.

  2. The alternative I prefer is to patch out whatever windows- or cygwin-specific symbols are used in the code-under-test, so that the test can successfully exercise all the logic, no matter what platform the test is running on. So, for example:

    1. One test that patches 'msvcrt' to be None (assuming you have implemented the change to the import above that I suggested a few minutes ago), and then asserts the result to be False. All other tests of this function will patch 'msvcrt' to be some object that contains the attributes the test needs.
    2. One test that provides a stream without a 'fileno' attribute, and asserts the result is False. All other tests will provide a stream that does have a fileno attribute.

In general, you'll need one test for each branch in the code-under-test. (If they interact, you might need to test the permutations of different branches, but I don't think that applies here.)

When creating these tests, it's surprisingly common to create a test where you THINK the code under test is producing the expected output for one reason, but there's actually a bug in your test, and you've accidentally produced a test that will always pass, no matter what. To avoid bugs like this, use the following sequence:

  1. The first test you should write is the one that makes is_msys_cygwin_tty return True. This will be the hardest test to write, since it has to patch msvcrt, provide a stream with all the right attributes, etc. So create it incrementally, maybe adding prints to the code-under test as you go, to sanity check how far your test is getting through the code-under-test. (Ideally, you'd write the test first, then write the code 2nd, which helps this process).
  2. Then, when the above test is working, copy it, and in the copy, change ONE of the items in the setup, and assert this now produces a False result. You can now be certain that this one change you made is responsible for the transition to a False result.
  3. Repeat step 2 until you've tested all branches and parts of logic in the code-under-test.

I'll stop. I hope this is useful and not just infuriating. Hugs!

@3tilley
Copy link
Contributor

3tilley commented Oct 22, 2021

It sounds like a philosophical debate about how useful mocks can be. I think you're probably right that it's appropriate here, but given that we have no control over the msvcrt symbols (or to be honest even know what they mean in my case), the mocking feels a little artificial.

I would propose option 2 but with a pytest mark to not mock, so someone can trivially check it's doing the right thing by running in their terminal of choice.

@SSE4 knock yourself out with the above, if you don't think you'll have time I can try and look sometime next week.

@SSE4
Copy link
Author

SSE4 commented Oct 23, 2021

unfortunately, it seems like I am not so good with mocks and don't know to properly mock modules like msvcrt or ctypes.
I've tried to play a bit with mock.patch and MagicMock, but I don't see how to make them control over ctypes.windll availability, for instance.

@3tilley
Copy link
Contributor

3tilley commented Oct 26, 2021

Tests implemented on this PR, if @SSE4 is happy with that and merges to his fork I think they will show up on here.

It was a lot more painful than I was expecting, but probably just because I'm too used to pytest mocks now.

@SSE4
Copy link
Author

SSE4 commented Oct 26, 2021

Tests implemented on this PR, if @SSE4 is happy with that and merges to his fork I think they will show up on here.

It was a lot more painful than I was expecting, but probably just because I'm too used to pytest mocks now.

thanks a lot, I have merged your tests, they look simple and good enough for me :)
@tartley anything else to move this forward?

@tartley
Copy link
Owner

tartley commented Oct 26, 2021

This is looking AMAZING. Thanks for all the hard work on getting this in such good shape.

I belatedly enabled CI for this PR (I didn't realize that was an opt-in step. Thanks cryptocurrency 🙄), and it shows that mock in Python3 is now unittest.mock, so some try...except fallback on that import is required.

I'll check in more detail this evening. THANK YOU!

colorama/tests/isatty_test.py Outdated Show resolved Hide resolved
@SSE4
Copy link
Author

SSE4 commented Oct 28, 2021

@3tilley there are some failed tests, can you take a look?

@3tilley
Copy link
Contributor

3tilley commented Oct 28, 2021

@SSE4 can you permission me to push to that branch to just make it a bit quicker?

@SSE4
Copy link
Author

SSE4 commented Oct 28, 2021

@SSE4 can you permission me to push to that branch to just make it a bit quicker?

sure, I have invited you as a collaborator to my fork. check your email.
still, as I understand, tests don't run automatically after the commit and have to be triggered manually :(

@3tilley
Copy link
Contributor

3tilley commented Oct 29, 2021

@tartley looks like you'll have to approve the CI run. I was hoping once you'd done it once I'd be safelisted

@3tilley
Copy link
Contributor

3tilley commented Nov 3, 2021

@tartley apologies I overlooked a ModuleNotFoundError. I'd be interested in what your thoughts are about supported 3.5 and 2.7 now they're EoL, but the above should now be fixed.

Do you mind approving another run at the CI?

@3tilley
Copy link
Contributor

3tilley commented Nov 8, 2021

Awesome, looks like it passed this time! Is there anything else I can help with on this PR?

@SSE4 SSE4 requested a review from tartley November 9, 2021 17:30
@3tilley
Copy link
Contributor

3tilley commented Dec 11, 2021

Hello @tartley , is there anything we can do to help here?

@tartley tartley added 0.4.6 Get merged for 0.4.6. release and removed 0.4.5 Get merged for 0.4.5 release labels Jun 14, 2022
@michalpravda
Copy link

michalpravda commented Sep 16, 2022

Hi,
it seems to me that it has been resolved (great collab, thanks). Is it gonna be merged and released any time soon? Can't find any roadmap on 0.4.6

@tartley
Copy link
Owner

tartley commented Sep 16, 2022

Hi. Thanks for the ping. The only "roadmap" that exists are the aspirational "0.4.6" (and similar) tags applied to some issues. The project is in maintenance mode, and doesn't receive enough attention to be adding new features, but aims to address bugs where possible. I'll take a look this weekend.

@michalpravda
Copy link

michalpravda commented Sep 19, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.4.6 Get merged for 0.4.6. release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Colorama not working with Git Bash (mintty): colors should not be stripped
6 participants