gh-121245: Refactor site.register_readline()

[skirpichev]

also initialize CAN_USE_PYREPL at import from _pyrepl.main

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

[skirpichev]

@smontanaro, if you can reproduce #121245, then please try this patch.

[smontanaro]

I can't reproduce #121245: ``` % cat ~/.python_history 'Save this' 'Save this' ```

…

On Fri, Jul 12, 2024 at 10:52 PM Sergey B Kirpichev < ***@***.***> wrote: @smontanaro <https://github.com/smontanaro>, if you can reproduce #121245 <#121245>, then please try this patch. — Reply to this email directly, view it on GitHub <#121659 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA2E3IMUEY3VAZEH2VFUTZLZMCQAFAVCNFSM6AAAAABKZ4WHXOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRWG42TIOJWHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[smontanaro]

That said, I agree now my idea for skipping the test is wrong. Rebuilding the readline module to use the readline 8.2.10 library in /opt/homebrew, I still get the same failure.

[skirpichev]

Rebuilding the readline module to use the readline 8.2.10 library in /opt/homebrew, I still get the same failure.

Then I would guess, as noted in #121422, that problem is related to your local customization. Try unset all PYTHON* environment variables and run tests.

[smontanaro]

Rebuilding the readline module to use the readline 8.2.10 library in /opt/homebrew, I still get the same failure.

Then I would guess, as noted in #121422, that problem is related to your local customization. Try unset all PYTHON* environment variables and run tests.

I get that, and yes, I agree, my personal interactive settings are interfering. I have long had readline customizations in my setup. I think the correct solution is for the test suite to be run in isolation, at least that part of it which purports to test interactive features such as readline. See Victor's fix in #121414.

[skirpichev]

See Victor's fix in #121414.

Unfortunately, this fix already applied and it's not enough. We can do a local fix:

diff --git a/Lib/test/test_pyrepl/test_pyrepl.py b/Lib/test/test_pyrepl/test_pyrepl.py
index 015b690566..88b1dc38f0 100644
--- a/Lib/test/test_pyrepl/test_pyrepl.py
+++ b/Lib/test/test_pyrepl/test_pyrepl.py
@@ -909,6 +909,12 @@ def test_not_wiping_history_file(self):
         hfile = tempfile.NamedTemporaryFile(delete=False)
         self.addCleanup(unlink, hfile.name)
         env = os.environ.copy()
+
+        # cleanup from PYTHON* variables
+        for k in env.copy():
+            if k.startswith("PYTHON"):
+                env.pop(k)
+
         env["PYTHON_HISTORY"] = hfile.name
         commands = "123\nspam\nexit()\n"
 

Let me know if this does work for you.

But I'm thinking on a more generic solution: probably we should instead make a default environment for all tests (say, my_env), clean PYTHON* variables in one and then in every test do my_env.copy().

Edit: typo fixed, sorry :(

[skirpichev]

See #121672

[ZeroIntensity]

site.register_readline is undocumented, it should get documented by this patch.

[ZeroIntensity]

Note that I'm pretty sure this suggestion will break buildbots while site.register_readline is undocumented. Document that before adding this.

Suggested change

	Simplify handling of the history file in ``site.register_readline()``
	Simplify handling of the history file in :func:`site.register_readline`

[skirpichev]

I think this out of the scope for this pr.

[ZeroIntensity]

Suggested change

	my_readline.read_history_file(history)
	target_readline.read_history_file(history)

[pablogsal]

Thanks for the PR @skirpichev. Unfortunately you are mixing what seems to be a fix for #121245 with a general refactor, what makes more difficult to understand what is happening here and what is and what isn't necessary. What is worse, it makes it not possible to back port because 3.13 accepts bug fixes but in no way refactors. Can you please keep the changes to only the minimum set of things require to fix #121245?

[skirpichev]

Unfortunately you are mixing what seems to be a fix for #121245 with a general refactor

@pablogsal, in fact I think that that issue is fixed (but I'm not sure, heh). It was reopened by my request, because added regression has many failures on build bots. But this seems to be fixed by #121422.

I still think, that refactoring belongs to the #121245. With new code it's obvious that we are using same readline both while reading the history and while writing...

Does this make sense for you?