cpython icon indicating copy to clipboard operation
cpython copied to clipboard

New pyrepl gives a traceback on exit with "dumb" terminal

Open dgrisby opened this issue 1 year ago • 8 comments

Bug report

Bug description:

I was experimenting with terminal types:

$ export TERM=dumb
$ python3
Python 3.13.0b1 (main, May 12 2024, 23:38:03) [GCC 13.2.1 20240316 (Red Hat 13.2.1-7)] on linux
Type "help", "copyright", "credits" or "license" for more information.
warning: can't use pyrepl: terminal doesn't have the required clear capability
>>> quit
Use quit() or Ctrl-D (i.e. EOF) to exit
>>> quit()
Exception ignored in atexit callback <function register_readline.<locals>.write_history at 0x7f7204f537e0>:
Traceback (most recent call last):
  File "<frozen site>", line 530, in write_history
  File "/home/duncan/py313inst/lib/python3.13/_pyrepl/readline.py", line 361, in write_history_file
    history = self.get_reader().get_trimmed_history(maxlength)
  File "/home/duncan/py313inst/lib/python3.13/_pyrepl/readline.py", line 277, in get_reader
    console = UnixConsole(self.f_in, self.f_out, encoding=ENCODING)
  File "/home/duncan/omni/py313inst/lib/python3.13/_pyrepl/unix_console.py", line 170, in __init__
    self._clear = _my_getstr("clear")
  File "/home/duncan/py313inst/lib/python3.13/_pyrepl/unix_console.py", line 163, in _my_getstr
    raise InvalidTerminal(
_pyrepl.unix_console.InvalidTerminal: terminal doesn't have the required clear capability

It notices that the terminal is unsuitable, but then tries to use it on exit.

CPython versions tested on:

3.13

Operating systems tested on:

Linux

Linked PRs

  • gh-119269

dgrisby avatar May 16 '24 16:05 dgrisby

Looks like the write_history callback is registered in site.py here:

https://github.com/python/cpython/blob/65de194dd80bbc8cb7098d21cfd6aefd11d0d0ce/Lib/site.py#L525

        def write_history():
            try:
                if os.getenv("PYTHON_BASIC_REPL"):
                    readline.write_history_file(history)
                else:
                    _pyrepl.readline.write_history_file(history)
            except (FileNotFoundError, PermissionError):
                # home directory does not exist or is not writable
                # https://bugs.python.org/issue19891
                pass

It clearly needs to get more clever to detect whether pyrepl is actually in use.

Maybe it can just look at the _pyrepl.__main__.CAN_USE_PYREPL global.

danielhollas avatar May 17 '24 12:05 danielhollas

@danielhollas CAN_USE_PYREPL just checks whether we're on a Windows system, so I wouldn't think checking that would help in this case.

Maybe: We set PYTHON_BASIC_REPL to true if TERM is set to "dumb"?

eugenetriguba avatar May 20 '24 18:05 eugenetriguba

@eugenetriguba if you look at the _pyrepl/__main__.py, CAN_USE_PYREPL is indeed checking for windows at import, but then later it is set to False if for any reason the new repl cannot be used, on line 41.

I tried to fix this by checking CAN_USE_PYREPL in the write_history callback function but couldn't make it work -- accessing CAN_USE_PYREPL from __main__ module seems to be cursed, and moving it to __init__ did not help.

danielhollas avatar May 20 '24 19:05 danielhollas

Maybe: We set PYTHON_BASIC_REPL to true if TERM is set to "dumb"?

The following patch indeed fixes the issue, happy to make a PR if this seems like a right approach

diff --git a/Lib/_pyrepl/__main__.py b/Lib/_pyrepl/__main__.py
index c598019e7c..40cdd6d9fe 100644
--- a/Lib/_pyrepl/__main__.py
+++ b/Lib/_pyrepl/__main__.py
@@ -39,6 +39,7 @@ def interactive_console(mainmodule=None, quiet=False, pythonstartup=False):
         trace(msg)
         print(msg, file=sys.stderr)
         CAN_USE_PYREPL = False
+        os.environ["PYTHON_BASIC_REPL"] = "true"
     if run_interactive is None:
         return sys._baserepl()
     return run_interactive(mainmodule)

danielhollas avatar May 20 '24 19:05 danielhollas

@danielhollas I worked with @lysnikolaou during PyCon sprints on this one, we came to the same conclusion but he said this is not the best practice to export the env variable to the system. He said he will take a deeper look as to why this might be happening

mi1acl avatar May 20 '24 20:05 mi1acl

Maybe add a site._CAN_USE_PYREPL constant and modify _pyrepl/__main__.py to set site._CAN_USE_PYREPL?

vstinner avatar May 20 '24 20:05 vstinner

I can do this if it's ok

mi1acl avatar May 20 '24 20:05 mi1acl

I proposed PR gh-119269 to fix this issue.

vstinner avatar May 20 '24 20:05 vstinner

Leaving this open since #119269 did not fix the problem.

lysnikolaou avatar May 21 '24 14:05 lysnikolaou

Please check my second fix: PR gh-119332.

vstinner avatar May 21 '24 17:05 vstinner

This time, it should be fixed.

vstinner avatar May 22 '24 09:05 vstinner