gh-121245: Refactor site.register_readline()

[skirpichev]

also initialize CAN_USE_PYREPL at import from _pyrepl.main

• Issue: gh-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 https://github.com/python/cpython/issues/121245, then please try this patch.

— Reply to this email directly, view it on GitHub https://github.com/python/cpython/pull/121659#issuecomment-2226754969, 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 https://github.com/python/cpython/pull/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 https://github.com/python/cpython/pull/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 https://github.com/python/cpython/pull/121672

[ZeroIntensity]

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

[pablogsal]

Thanks for the PR @skirpichev. Unfortunately you are mixing what seems to be a fix for https://github.com/python/cpython/issues/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 https://github.com/python/cpython/issues/121245?

[skirpichev]

Unfortunately you are mixing what seems to be a fix for https://github.com/python/cpython/issues/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?

[miss-islington-app[bot]]

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

[bedevere-app[bot]]

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