gh-112105: Make completer delims work on libedit #112106

gaogaotiantian · 2023-11-15T04:52:48Z

Issue: readline.set_completer_delims has no effect with libedit #112105

gaogaotiantian · 2023-11-15T05:07:01Z

@gpshead @vstinner are you familiar with readline? Could you take a look at the PR? Thanks!

gaogaotiantian · 2023-11-21T01:30:55Z

Hi @corona10 , I just realized you have spent sometime on libedit when I was reviewing the history of the file. Could you share your thoughts of the PR? Thanks!

corona10 · 2023-11-21T01:32:10Z

@gaogaotiantian I will take a look at this PR in this weekend :)

corona10 · 2023-11-27T06:37:16Z

@gaogaotiantian Sorry for the delay, I will take a look at in 2-3 days.

gaogaotiantian · 2023-11-27T07:08:01Z

@gaogaotiantian Sorry for the delay, I will take a look at in 2-3 days.

No worries!

corona10 · 2023-11-28T01:57:22Z

Modules/readline.c

@@ -576,6 +576,7 @@ readline_set_completer_delims(PyObject *module, PyObject *string)
    if (break_chars) {
        free(completer_word_break_characters);
        completer_word_break_characters = break_chars;
+        rl_basic_word_break_characters = break_chars;


Okay so it's kinds of implementation detail.
Since we enable the flag for libedit in following way.

cpython/configure.ac

Line 5916 in 2e632fa

AH_TEMPLATE([WITH_EDITLINE], [Define to build the readline module against libedit.])

What about changing the code for the libedit implementation only?

Suggested change

rl_basic_word_break_characters = break_chars;

#ifdef WITH_EDITLINE

// Comment why this is needed

rl_basic_word_break_characters = break_chars;

#endif

From the comments in the file:

* This emulation library is not 100% API compatible with the "real" readline * and cannot be detected at compile-time, * hence we use a runtime check to detect if the Python readline module is * linked to libedit.

It seems like we are not able to detect if libedit is used at compile time. I doubt if the macro protection would actually work in all cases.

Do our official binaries build with libedit ever?

It seems like we are not able to detect if libedit is used at compile time

I am not sure when the comment was added.
But we can detect editline at build-time from Python 3.10
https://docs.python.org/3/using/configure.html?highlight=with_editline#cmdoption-with-readline

Do our official binaries build with libedit ever?

Even if our official build does not use it, we should consider downstream distros :)

That's actually not what I meant. What I meant is - we could have been using libedit without WITH_EDITLINE. So even if we do not define WITH_EDITLINE, we could still be using libedit. I just confirmed on my mac that this change will break the test.

The reason I quoted that comment was that I believed such case exists - where we don't know if libedit is being used at compile time.

we could have been using libedit without WITH_EDITLINE.
From what I can observe, writing to both variables has no unexpected side effect, because rl_basic_word_break_characters is not used on CPython with GNU deadline.

Got it, then should we consider using both using_libedit_emulation and WITH_EDITLINE?
If you think that we don't have to consider side effect from the implementation detail, then we can just add a comment why we do this.

I checked that following suggestion pass the test on my macOS.

Suggested change

rl_basic_word_break_characters = break_chars;

#ifdef WITH_EDITLINE

rl_basic_word_break_characters = break_chars;

#else

if (using_libedit_emulation) rl_basic_word_break_characters = break_chars;

#endif

Sorry I did not use your suggestion directly. I do like the brackets for the if statement. I applied the protection to avoid side effects for GNU readline on both cases.

corona10 · 2023-11-28T01:57:33Z

Modules/readline.c

@@ -1267,6 +1268,7 @@ setup_readline(readlinestate *mod_state)
    completer_word_break_characters =
        strdup(" \t\n`~!@#$%^&*()-=+[{]}\\|;:'\",<>/?");
        /* All nonalphanums except '.' */
+    rl_basic_word_break_characters = completer_word_break_characters;


ditto
https://github.com/python/cpython/pull/112106/files#r1406981744

corona10

lgtm!!
Thanks for the work @gaogaotiantian

miss-islington-app · 2023-11-28T06:23:27Z

Thanks @gaogaotiantian for the PR, and @corona10 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

(cherry picked from commit 2df26d8) Co-authored-by: Tian Gao <gaogaotiantian@hotmail.com>

bedevere-app · 2023-11-28T06:23:46Z

GH-112487 is a backport of this pull request to the 3.12 branch.

(cherry picked from commit 2df26d8) Co-authored-by: Tian Gao <gaogaotiantian@hotmail.com>

bedevere-app · 2023-11-28T06:23:50Z

GH-112488 is a backport of this pull request to the 3.11 branch.

…h-112488) gh-112105: Make completer delims work on libedit (gh-112106) (cherry picked from commit 2df26d8) Co-authored-by: Tian Gao <gaogaotiantian@hotmail.com>

…h-112487) gh-112105: Make completer delims work on libedit (gh-112106) (cherry picked from commit 2df26d8) Co-authored-by: Tian Gao <gaogaotiantian@hotmail.com>

Make completer delims work on libedit

b7b05fc

bedevere-app bot added the awaiting review label Nov 15, 2023

bedevere-app bot mentioned this pull request Nov 15, 2023

readline.set_completer_delims has no effect with libedit #112105

Closed

📜🤖 Added by blurb_it.

b0841a7

corona10 self-assigned this Nov 21, 2023

corona10 added needs backport to 3.11 only security fixes needs backport to 3.12 bug and security fixes labels Nov 28, 2023

corona10 reviewed Nov 28, 2023

View reviewed changes

Reduce side effects for rl_basic_word_break_characters

1941e94

corona10 approved these changes Nov 28, 2023

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting review labels Nov 28, 2023

corona10 enabled auto-merge (squash) November 28, 2023 06:15

corona10 merged commit 2df26d8 into python:main Nov 28, 2023
32 checks passed

bedevere-app bot removed the awaiting merge label Nov 28, 2023

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 28, 2023

pythongh-112105: Make completer delims work on libedit (pythongh-112106)

b27d93f

(cherry picked from commit 2df26d8) Co-authored-by: Tian Gao <gaogaotiantian@hotmail.com>

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 28, 2023

pythongh-112105: Make completer delims work on libedit (pythongh-112106)

a3b6f8a

(cherry picked from commit 2df26d8) Co-authored-by: Tian Gao <gaogaotiantian@hotmail.com>

bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Nov 28, 2023

bedevere-app bot removed the needs backport to 3.11 only security fixes label Nov 28, 2023

gaogaotiantian deleted the readline-delim branch November 28, 2023 06:48

aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024

pythongh-112105: Make completer delims work on libedit (pythongh-112106)

13e3f2e

Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024

pythongh-112105: Make completer delims work on libedit (pythongh-112106)

c749966

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-112105: Make completer delims work on libedit #112106

gh-112105: Make completer delims work on libedit #112106

gaogaotiantian commented Nov 15, 2023 •

edited by bedevere-app bot

Loading

gaogaotiantian commented Nov 15, 2023

gaogaotiantian commented Nov 21, 2023

corona10 commented Nov 21, 2023

corona10 commented Nov 27, 2023

gaogaotiantian commented Nov 27, 2023

corona10 Nov 28, 2023

gaogaotiantian Nov 28, 2023

gaogaotiantian Nov 28, 2023

corona10 Nov 28, 2023 •

edited

Loading

corona10 Nov 28, 2023

corona10 Nov 28, 2023 •

edited

Loading

gaogaotiantian Nov 28, 2023

corona10 Nov 28, 2023 •

edited

Loading

corona10 Nov 28, 2023 •

edited

Loading

gaogaotiantian Nov 28, 2023

corona10 Nov 28, 2023

corona10 left a comment

miss-islington-app bot commented Nov 28, 2023

bedevere-app bot commented Nov 28, 2023

bedevere-app bot commented Nov 28, 2023

gh-112105: Make completer delims work on libedit #112106

gh-112105: Make completer delims work on libedit #112106

Conversation

gaogaotiantian commented Nov 15, 2023 • edited by bedevere-app bot Loading

gaogaotiantian commented Nov 15, 2023

gaogaotiantian commented Nov 21, 2023

corona10 commented Nov 21, 2023

corona10 commented Nov 27, 2023

gaogaotiantian commented Nov 27, 2023

corona10 Nov 28, 2023

Choose a reason for hiding this comment

gaogaotiantian Nov 28, 2023

Choose a reason for hiding this comment

gaogaotiantian Nov 28, 2023

Choose a reason for hiding this comment

corona10 Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

corona10 Nov 28, 2023

Choose a reason for hiding this comment

corona10 Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

gaogaotiantian Nov 28, 2023

Choose a reason for hiding this comment

corona10 Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

corona10 Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

gaogaotiantian Nov 28, 2023

Choose a reason for hiding this comment

corona10 Nov 28, 2023

Choose a reason for hiding this comment

corona10 left a comment

Choose a reason for hiding this comment

miss-islington-app bot commented Nov 28, 2023

bedevere-app bot commented Nov 28, 2023

bedevere-app bot commented Nov 28, 2023

gaogaotiantian commented Nov 15, 2023 •

edited by bedevere-app bot

Loading

corona10 Nov 28, 2023 •

edited

Loading

corona10 Nov 28, 2023 •

edited

Loading

corona10 Nov 28, 2023 •

edited

Loading

corona10 Nov 28, 2023 •

edited

Loading