gh-121245: a regression test for site.register_readline() by skirpichev · Pull Request #121259 · python/cpython

skirpichev · 2024-07-02T10:29:08Z

Should finally address #121245.

Issue: Python 3.13.0b3 REPL empties ~/.python_history #121245

* make readline stuff more symmetrical wrt reading/writing * prevent truncation of the history file * add regression test Should finally address python#121245.

Lib/site.py

pablogsal · 2024-07-02T18:46:12Z

Lib/test/test_pyrepl/test_pyrepl.py

        self.assertNotIn("Exception", output)
        self.assertNotIn("Traceback", output)

+    def test_not_wiping_history_file(self):


I have tried and reverting the changes you did in site.py and running this test still succeeds

Example:

❯ git --no-pager diff --cached diff --git a/Lib/site.py b/Lib/site.py index fcfd2fe7bbe..9381f6f510e 100644 --- a/Lib/site.py +++ b/Lib/site.py @@ -509,10 +509,6 @@ def register_readline(): pass if readline.get_current_history_length() == 0: - try: - from _pyrepl.main import CAN_USE_PYREPL - except ImportError: - CAN_USE_PYREPL = False # If no history was loaded, default to .python_history, # or PYTHON_HISTORY. # The guard is necessary to avoid doubling history size at @@ -520,23 +516,29 @@ def register_readline(): # through a PYTHONSTARTUP hook, see: # http://bugs.python.org/issue5845#msg198636 history = gethistoryfile() - if os.getenv("PYTHON_BASIC_REPL") or not CAN_USE_PYREPL: - my_readline = readline - else: - my_readline = _pyrepl.readline try: - my_readline.read_history_file(history) + if os.getenv("PYTHON_BASIC_REPL"): + readline.read_history_file(history) + else: + _pyrepl.readline.read_history_file(history) except (OSError,* _pyrepl.unix_console._error): pass def write_history(): try: - my_readline.write_history_file(history) - except (FileNotFoundError, PermissionError, - _pyrepl.unix_console.InvalidTerminal): + # _pyrepl.__main__ is executed as the __main__ module + from __main__ import CAN_USE_PYREPL + except ImportError: + CAN_USE_PYREPL = False + + try: + if os.getenv("PYTHON_BASIC_REPL") or not CAN_USE_PYREPL: + 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 - # or terminal doesn't have the required capability pass atexit.register(write_history) ~/github/python/main fix-repl-121245-v2* ❯ ./python.exe -m test test_pyrepl -uall Using random seed: 2158058370 Raised RLIMIT_NOFILE: 256 -> 1024 0:00:00 load avg: 3.76 Run 1 test sequentially in a single process 0:00:00 load avg: 3.76 [1/1] test_pyrepl == Tests result: SUCCESS == 1 test OK. Total duration: 3.8 sec Total tests: run=118 skipped=1 Total test files: run=1/1 Result: SUCCESS

The test should fail when you revert #121255

Ah thanks for pointing that out! I was confused by the fact that we have a NEWS entry in this PR claiming that it fixes the bug.

This PR amends that news entry.

Sorry for a confusion. I was thinking that this pr doesn't deserve a separate news entry. Just a minor cleanup and adds a test. #121255 was a quick fix for annoying issue.

FYI: news entry slightly corrected as I've reverted a guard that prevents wiping history file (e.g. if user run readline.clear_history()).

Unfortunately, I can't reproduce build failure on "Address sanitizer" job. It looks as PYTHON_HISTORY either doesn't affect the interpreter in run_repl() (but it should, there is no -E option!) or write_history_file() just fails.

Misc/NEWS.d/next/Library/2024-07-02-11-34-06.gh-issue-121245.sSkDAr.rst

skirpichev · 2024-07-03T01:13:20Z

Lib/test/test_pyrepl/test_pyrepl.py

        self.assertNotIn("Exception", output)
        self.assertNotIn("Traceback", output)

+    def test_not_wiping_history_file(self):


Sorry for a confusion. I was thinking that this pr doesn't deserve a separate news entry. Just a minor cleanup and adds a test. #121255 was a quick fix for annoying issue.

FYI: news entry slightly corrected as I've reverted a guard that prevents wiping history file (e.g. if user run readline.clear_history()).

Unfortunately, I can't reproduce build failure on "Address sanitizer" job. It looks as PYTHON_HISTORY either doesn't affect the interpreter in run_repl() (but it should, there is no -E option!) or write_history_file() just fails.

…SkDAr.rst

skirpichev · 2024-07-03T04:10:32Z

Ok, I see the problem. It looks that CAN_USE_REPL can be dynamically reset after importing of the site module:

cpython/Lib/_pyrepl/main.py

Lines 38 to 52 in f09d184

    
           try: 
        
               import errno 
        
               if not os.isatty(sys.stdin.fileno()): 
        
                   raise OSError(errno.ENOTTY, "tty required", "stdin") 
        
               from .simple_interact import check 
        
               if err := check(): 
        
                   raise RuntimeError(err) 
        
               from .simple_interact import run_multiline_interactive_console 
        
               run_interactive = run_multiline_interactive_console 
        
           except Exception as e: 
        
               from .trace import trace 
        
               msg = f"warning: can't use pyrepl: {e}" 
        
               trace(msg) 
        
               print(msg, file=sys.stderr) 
        
               CAN_USE_PYREPL = False

That happens in the interactive_console(). Maybe we can run this code on import of the main module?

Meanwhile, marked as draft.

This reverts commit 77a7baa.

skirpichev · 2024-07-03T06:49:00Z

It seems #121255 fully address issue, lets just add a test.

miss-islington-app · 2024-07-03T10:45:46Z

Thanks @skirpichev for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

…onGH-121259) (cherry picked from commit afee76b) Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>

pablogsal · 2024-07-03T10:45:59Z

Thanks for your contribution @skirpichev 🤘

bedevere-app · 2024-07-03T10:46:00Z

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

…121259) (#121322)

…on#121259)

Refactor site.register_readline()

8195067

* make readline stuff more symmetrical wrt reading/writing * prevent truncation of the history file * add regression test Should finally address python#121245.

skirpichev requested review from ambv, lysnikolaou and pablogsal as code owners July 2, 2024 10:29

bedevere-app bot added the awaiting review label Jul 2, 2024

+ news entry

8cbf9e5

skirpichev changed the title ~~Refactor site.register_readline()~~ gh-121245: Refactor site.register_readline() Jul 2, 2024

bedevere-app bot mentioned this pull request Jul 2, 2024

Python 3.13.0b3 REPL empties ~/.python_history #121245

Closed

+ add cleanup for test file

9dd6305

This comment was marked as outdated.

Sign in to view

skirpichev commented Jul 2, 2024

View reviewed changes

Lib/site.py Outdated Show resolved Hide resolved

hroncok reviewed Jul 2, 2024

View reviewed changes

Lib/site.py Outdated Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

skirpichev added 2 commits July 2, 2024 17:19

Merge branch 'main' into fix-repl-121245-v2

4c57916

Update Lib/site.py (drop history length check)

41d0f3d

skirpichev mentioned this pull request Jul 2, 2024

test_pyrepl doesn't ignore PYTHONSTARTUP #121271

Closed

+ test exit_code

595e5da

pablogsal added the needs backport to 3.13 bugs and security fixes label Jul 2, 2024

pablogsal reviewed Jul 2, 2024

View reviewed changes

skirpichev commented Jul 3, 2024

View reviewed changes

skirpichev added 2 commits July 3, 2024 04:13

Update Misc/NEWS.d/next/Library/2024-07-02-11-34-06.gh-issue-121245.s…

e358963

…SkDAr.rst

XXX skip test_not_wiping_history_file() if pyrepl can't be used

77a7baa

skirpichev marked this pull request as draft July 3, 2024 04:06

bedevere-app bot removed the awaiting review label Jul 3, 2024

skirpichev added 5 commits July 3, 2024 07:11

Revert "XXX skip test_not_wiping_history_file() if pyrepl can't be used"

8ed572d

This reverts commit 77a7baa.

Merge branch 'master' into fix-repl-121245-v2

753ef78

XXX keep only test

f625be8

+ test with/without basic repl

d3dbbfb

Revert news; lets just add a test

d4b0f10

skirpichev changed the title ~~gh-121245: Refactor site.register_readline()~~ gh-121245: a regression test for site.register_readline() Jul 3, 2024

skirpichev marked this pull request as ready for review July 3, 2024 06:46

bedevere-app bot added the awaiting review label Jul 3, 2024

pablogsal added the skip news label Jul 3, 2024

pablogsal approved these changes Jul 3, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting review labels Jul 3, 2024

pablogsal merged commit afee76b into python:main Jul 3, 2024

bedevere-app bot removed the awaiting merge label Jul 3, 2024

bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jul 3, 2024

skirpichev deleted the fix-repl-121245-v2 branch July 3, 2024 11:04

pablogsal pushed a commit that referenced this pull request Jul 3, 2024

[3.13] gh-121245: a regression test for site.register_readline() (GH-…

2516673

…121259) (#121322)

noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024

pythongh-121245: a regression test for site.register_readline() (pyth…

8397060

…on#121259)

estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024

pythongh-121245: a regression test for site.register_readline() (pyth…

fb8ff95

…on#121259)

Uh oh!

Conversation

skirpichev commented Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

pablogsal Jul 2, 2024

Choose a reason for hiding this comment

Uh oh!

pablogsal Jul 2, 2024

Choose a reason for hiding this comment

Uh oh!

hroncok Jul 2, 2024

Choose a reason for hiding this comment

Uh oh!

pablogsal Jul 2, 2024

Choose a reason for hiding this comment

Uh oh!

hroncok Jul 2, 2024

Choose a reason for hiding this comment

Uh oh!

skirpichev Jul 3, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

skirpichev Jul 3, 2024

Choose a reason for hiding this comment

Uh oh!

skirpichev commented Jul 3, 2024

Uh oh!

skirpichev commented Jul 3, 2024

Uh oh!

miss-islington-app bot commented Jul 3, 2024

Uh oh!

pablogsal commented Jul 3, 2024

Uh oh!

bedevere-app bot commented Jul 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

skirpichev commented Jul 2, 2024 •

edited

Loading