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

Regression in terminal color auto-detection? #11675

Closed
mgeier opened this issue Sep 4, 2023 · 9 comments · Fixed by #11708
Closed

Regression in terminal color auto-detection? #11675

mgeier opened this issue Sep 4, 2023 · 9 comments · Fixed by #11708

Comments

@mgeier
Copy link
Contributor

mgeier commented Sep 4, 2023

Describe the bug

I'm not sure if this is caused by Sphinx or readthedocs.org, but when using Sphinx 7.1 and 7.2 on RTD, the log (which doesn't support color) contains some partial ANSI escape sequences like [2K.

See readthedocs/readthedocs.org#10702 for details.

How to Reproduce

Push to RTD using Sphinx >= 7.1 and look at the logs.

Environment Information

readthedocs.org

Sphinx extensions

No response

Additional context

No response

@picnixz
Copy link
Member

picnixz commented Sep 5, 2023

From commit d3c91f9, four months ago (I think it's still between 7.0 and 7.1):

https://github.com/sphinx-doc/sphinx/blob/master/sphinx/util/display.py#L44-L55

It's quite hard to bisect (even more on a phone...) so I suggest testing with a patched version of Sphinx to check if it works (technically, this commit could be reverted without too much work I think, to confirm the bug. If it's the culprit, we can first check if ANSI esc sequences are supported or not (especially the clear line (\x1b[2K) cs)).

cc @humitos

@picnixz
Copy link
Member

picnixz commented Sep 9, 2023

@mgeier (or anyone else) Do you think you can work out some PR? (otherwise I'll have a look in mid October).

@AA-Turner
Copy link
Member

@humitos do you have a link to where RTD does the log streaming please? You're right this is likely a problem with TTY detection or similar in Sphinx, just want to be able to replicate what RTD is doing on your side.

A

@humitos
Copy link
Contributor

humitos commented Sep 9, 2023

@humitos do you have a link to where RTD does the log streaming please?

The code that executes the command is at https://github.com/readthedocs/readthedocs.org/blob/a5052deea15c252179518432bcae4042b1684bc8/readthedocs/doc_builder/environments.py#L297-L357. It uses Docker behind the scenes, so may not be super easy to reproduce the same behavior.

@humitos
Copy link
Contributor

humitos commented Sep 9, 2023

I spun up a Read the Docs instance locally and I ran some build tests. I found the problem was introduced in 7.1.0. The latest Sphinx version that doesn't have this issue is 7.0.1.

The changes between those releases are: v7.0.1...v7.1.0

@humitos
Copy link
Contributor

humitos commented Sep 9, 2023

From commit d3c91f9, four months ago (I think it's still between 7.0 and 7.1):

I reverted this commit and try installing 7.2.5 with this commit reverted and the issue persists. So, there is something else that's breaking the terminal.

Edit: after git bisect I found this commit was the one that introduced the bug. I suspect that I did something wrong when solving the revert conflicts 🤷🏼

@humitos
Copy link
Contributor

humitos commented Sep 9, 2023

I'm doing a git bisect and testing each step on my Read the Docs local instance. I will post what I found once I finish...

@humitos
Copy link
Contributor

humitos commented Sep 9, 2023

▶ git bisect good
d3c91f951255c6729a53e38c895ddc0af036b5b9 is the first bad commit
commit d3c91f951255c6729a53e38c895ddc0af036b5b9
Author: Adam Turner <9087854+aa-turner@users.noreply.github.com>
Date:   Sat May 13 02:50:38 2023 +0100

    Refactor ``status_iterator``

 sphinx/util/display.py     | 34 ++++++++++++++++++++--------------
 tests/test_util_display.py | 37 ++++++++++++++++++++++---------------
 2 files changed, 42 insertions(+), 29 deletions(-)

From commit d3c91f9, four months ago (I think it's still between 7.0 and 7.1):

You are right. This is the bad commit 🎉

@mgeier
Copy link
Contributor Author

mgeier commented Sep 24, 2023

@mgeier (or anyone else) Do you think you can work out some PR? (otherwise I'll have a look in mid October).

Sorry, I currently do not have time to look into this.

@picnixz picnixz added this to the some future version milestone Sep 26, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants