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.

Sign up for GitHub

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

Jump to bottom

gh-119102: Fix REPL for dumb terminal #119269

Merged
merged 1 commit into from
May 21, 2024
Merged

gh-119102: Fix REPL for dumb terminal #119269

merged 1 commit into from
May 21, 2024

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented May 20, 2024 •
edited by bedevere-app bot
Loading

Move CAN_USE_PYREPL variable from _pyrepl.main to _pyrepl and rename it to _CAN_USE_PYREPL. Use the variable in the site module to decide if _pyrepl.write_history_file() can be used.

@bedevere-app bedevere-app bot mentioned this pull request May 20, 2024
Closed
@vstinner
Copy link
Member Author

cc @pablogamboa @ambv

eugenetriguba
eugenetriguba reviewed May 20, 2024
View reviewed changes
Lib/site.py Outdated
try:
if os.getenv("PYTHON_BASIC_REPL"):
if os.getenv("PYTHON_BASIC_REPL") or not _pyrepl._CAN_USE_PYREPL:
Copy link
Contributor

Choose a reason for hiding this comment

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

question: This is working, I presume, because we're hitting the except block in __main__ when we have a dumb TERM setting? I was initially a bit confused because I wasn't seeing why an extra conditional on a setting for a win32 check would fix this

Copy link
Member Author

Choose a reason for hiding this comment

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

_pyrepl._CAN_USE_PYREPL is set to False if pyrepl fails at startup.

@vstinner
Copy link
Member Author

@ambv: Do you have an idea on how to fix mypy?

@eugenetriguba
Copy link
Contributor

eugenetriguba commented May 21, 2024 •
edited
Loading

@vstinner Using from . import _CAN_USE_PYREPL instead of import _pyrepl seems to work. Not sure if there is a better solution It seems to work in that it makes mypy happy, but it wouldn't allow updating the variable in the other module 🙂

@vstinner
Copy link
Member Author

@vstinner Using from . import _CAN_USE_PYREPL instead of import _pyrepl seems to work.

In that case, _CAN_USE_PYREPL = False only sets the variable in __main__.py, not in __init__.py.

@vstinner
pythongh-119102: Fix REPL for dumb terminal
37dc157 
Use CAN_USE_PYREPL of _pyrepl.__main__ in the site module to decide
if _pyrepl.write_history_file() can be used.
@vstinner
Copy link
Member Author

I rewrote the fix to please the typing gods.

@vstinner vstinner merged commit 73f4a58 into python:main May 21, 2024
31 checks passed
@vstinner vstinner deleted the pyrepl_dumb branch May 21, 2024 12:53
@vstinner vstinner added the needs backport to 3.13 bugs and security fixes label May 21, 2024
@miss-islington-app
Copy link

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 21, 2024
@vstinner @miss-islington
pythongh-119102: Fix REPL for dumb terminal (pythonGH-119269)
35c9446 
Use CAN_USE_PYREPL of _pyrepl.__main__ in the site module to decide
if _pyrepl.write_history_file() can be used.
(cherry picked from commit 73f4a58)

Co-authored-by: Victor Stinner <vstinner@python.org>
@bedevere-app
Copy link

bedevere-app bot commented May 21, 2024

GH-119308 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label May 21, 2024
vstinner added a commit that referenced this pull request May 21, 2024
@miss-islington @vstinner
[3.13] gh-119102: Fix REPL for dumb terminal (GH-119269) (#119308)
f028451 
gh-119102: Fix REPL for dumb terminal (GH-119269)

Use CAN_USE_PYREPL of _pyrepl.__main__ in the site module to decide
if _pyrepl.write_history_file() can be used.
(cherry picked from commit 73f4a58)

Co-authored-by: Victor Stinner <vstinner@python.org>
@danielhollas
Copy link
Contributor

I rewrote the fix to please the typing gods.

@vstinner the new version doesn't seem to fix the issue for me (tested on Fedora 39).

I've ran into the same problem when I was trying to fix this. It looks like from _pyrepl.__main__ import CAN_USE_PYREPL re-executes the code in __main__, and therefore CAN_USE_PYREPL will always be True on non-windows systems.

@lysnikolaou
Copy link
Contributor

Same behavior for me that @danielhollas described.

@vstinner
Copy link
Member Author

Sorry, I didn't retest functionally after fixing mypy 😬

@vstinner
Copy link
Member Author

Please check my second fix: PR gh-119332.

@ambv ambv added the topic-repl Related to the interactive shell label May 23, 2024
@JelleZijlstra JelleZijlstra mentioned this pull request May 28, 2024
Closed
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
@vstinner @estyxx
pythongh-119102: Fix REPL for dumb terminal (python#119269)
c620799 
Use CAN_USE_PYREPL of _pyrepl.__main__ in the site module to decide
if _pyrepl.write_history_file() can be used.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eugenetriguba eugenetriguba eugenetriguba left review comments

@pablogsal pablogsal pablogsal approved these changes

Assignees
No one assigned
Labels
skip news topic-repl Related to the interactive shell
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@vstinner @eugenetriguba @danielhollas @lysnikolaou @pablogsal @ambv