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

Fix terminal size detection #299

Merged
merged 1 commit into from Jun 16, 2023
Merged

Conversation

Secrus
Copy link
Member

@Secrus Secrus commented Dec 12, 2022

Closes: python-poetry/poetry#7184

This fixes the issue, where in Python <3.11 shutil.get_terminal_size() would return (0, 0) size in non-TTY terminals.

@TheKevJames
Copy link

I don't believe this solves the problem, see sample CI run here.

Note that python3 -c 'import shutil; print(shutil.get_terminal_size(fallback=(80, 24)))' still returns os.terminal_size(columns=0, lines=0).

Using the fallback when the original answer is zero only occurs in Python 3.11+:

Changed in version 3.11: The fallback values are also used if os.get_terminal_size() returns zeroes.

Source: https://docs.python.org/3/library/shutil.html#shutil.get_terminal_size

@Secrus
Copy link
Member Author

Secrus commented Dec 13, 2022

I don't believe this solves the problem, see sample CI run here.

Note that python3 -c 'import shutil; print(shutil.get_terminal_size(fallback=(80, 24)))' still returns os.terminal_size(columns=0, lines=0).

Using the fallback when the original answer is zero only occurs in Python 3.11+:

Changed in version 3.11: The fallback values are also used if os.get_terminal_size() returns zeroes.

Source: https://docs.python.org/3/library/shutil.html#shutil.get_terminal_size

@TheKevJames

I have tested it with Docker. Non-tty terminals are handled correctly when passing the fallback value.
Zrzut ekranu z 2022-12-13 21-24-55

the exact commands I used:

docker run --rm python:3.11-slim python3 -c "import shutil; print(shutil.get_terminal_size(fallback=(80,24)))"

docker run --rm -t python:3.11-slim python3 -c "import shutil; print(shutil.get_terminal_size(fallback=(80,24)))"

docker run --rm python:3.10-slim python3 -c "import shutil; print(shutil.get_terminal_size(fallback=(80,24)))"

docker run --rm -t python:3.10-slim python3 -c "import shutil; print(shutil.get_terminal_size(fallback=(80,24)))"

And as you see on the screen, in no case, it returns (0, 0). We are currently working on designing a proper unit testing approach to detect it.

@TheKevJames
Copy link

TheKevJames commented Dec 13, 2022

Ah, interesting! Ok, then I think there has been a miscommunication:

The original issue reported to poetry was that poetry was breaking in CircleCI. After our investigation in python-poetry/poetry#7184, we ultimately determined the root cause was the columns value being 0.

@neersighted proposed here that it may be possible to reproduce this error by running in docker without a tty, but that was meant to be a reproduction case rather than actually being the central issue.

Given your results show that your change here fixes the issue in docker, but my results here show that CircleCI continues to receive columns=0 even when a fallback value is provided, I think we've successfully proven that using docker without -t does not reproduce the original issue.

EDIT: I guess this implies the original issue in CircleCI is not definitely related to the existence/non-existence of an attached tty.

@Secrus
Copy link
Member Author

Secrus commented Dec 13, 2022

Interesting... Seems like a Circle CI thing (online-judge-tools/oj#611, aws/aws-cdk#2355)

@neersighted
Copy link
Member

It appears that they do allocate a TTY with a (reported) size of zero, which is fascinating and going to break other tools... e.g. bash provides variables for this that just report the value of TIOCGWINSZ.

@istankovic
Copy link

Is there any blocker for this fix?

@Secrus Secrus changed the title Fix terminal sizing for non-tty terminals Fix terminal size detection Jun 14, 2023
src/cleo/terminal.py Show resolved Hide resolved
src/cleo/terminal.py Show resolved Hide resolved
@Secrus Secrus requested a review from neersighted June 16, 2023 08:21
@Secrus Secrus merged commit 8430044 into python-poetry:main Jun 16, 2023
17 checks passed
@Secrus Secrus deleted the fix-terminal branch June 16, 2023 11:15
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.

Poetry 1.3 dies when run with a TTY reporting size 0/0
5 participants