Skip to content

gh-119102: Fix REPL for dumb terminal - #119269

Merged
vstinner merged 1 commit into
python:mainfrom
vstinner:pyrepl_dumb
May 21, 2024
Merged

gh-119102: Fix REPL for dumb terminal#119269
vstinner merged 1 commit into
python:mainfrom
vstinner:pyrepl_dumb

Conversation

@vstinner

@vstinnervstinner commented May 20, 2024•
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Member

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-appbedevere-appBot mentioned this pull request May 20, 2024
Closed
@vstinner

vstinner commented May 20, 2024

Copy link
Copy Markdown
MemberAuthor

cc @pablogamboa@ambv

eugenetriguba
eugenetriguba reviewed May 20, 2024
View reviewed changes
Comment threadLib/site.py Outdated
import _pyrepl
try:
if os.getenv("PYTHON_BASIC_REPL"):
if os.getenv("PYTHON_BASIC_REPL") or not _pyrepl._CAN_USE_PYREPL:

@eugenetrigubaeugenetrigubaMay 20, 2024

Copy link
Copy Markdown
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

@vstinnervstinnerMay 21, 2024

Copy link
Copy Markdown
MemberAuthor

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

vstinner commented May 21, 2024

Copy link
Copy Markdown
MemberAuthor

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

@eugenetriguba

eugenetriguba commented May 21, 2024•
edited
Loading

Copy link
Copy Markdown
Contributor

@vstinnerUsing 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

vstinner commented May 21, 2024

Copy link
Copy Markdown
MemberAuthor

@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

vstinner commented May 21, 2024

Copy link
Copy Markdown
MemberAuthor

I rewrote the fix to please the typing gods.

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

miss-islington-appBot commented May 21, 2024

Copy link
Copy Markdown

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

bedevere-appBot commented May 21, 2024

Copy link
Copy Markdown

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

@bedevere-appbedevere-appBot 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

danielhollas commented May 21, 2024

Copy link
Copy Markdown
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

lysnikolaou commented May 21, 2024

Copy link
Copy Markdown
Member

Same behavior for me that @danielhollas described.

@vstinner

vstinner commented May 21, 2024

Copy link
Copy Markdown
MemberAuthor

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

@vstinner

vstinner commented May 21, 2024

Copy link
Copy Markdown
MemberAuthor

Please check my second fix: PR gh-119332.

@ambvambv added the topic-repl Related to the interactive shell label May 23, 2024
@JelleZijlstraJelleZijlstra 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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pablogsalpablogsalpablogsal approved these changes
+1 more reviewer
@eugenetrigubaeugenetrigubaeugenetriguba left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

skip newstopic-replRelated 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