Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

unix: upgrade libedit 20210910-3.1 -> 20240808-3.1 #466

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

indygreg
Copy link
Collaborator

@indygreg indygreg commented Jan 1, 2025

We were soft blocked on upgrading due to musl compatibility issues.

It looks like these got fixed upstream. So we refreshed the configure
patch and libedit build just worked.

However, Python 3.9 and 3.10 encountered compile errors with the
newer version.

On 3.10 we worked around this bug by backporting a patch from 3.11.
On 3.9, the backport was non-trivial, so I just hacked up the existing
3.9 patch to manually change some C preprocessor checks to key off
libedit.

While diffing Modules/readline.c I found another patch related to
fixing completer delims. While strictly not required, it was trivial
to backport to 3.10 to fix some missing functionality. So I did.

3.13 initially didn't like the upgraded libedit because
we were manually defining a preprocessor variable (introduced in 3.13
by upstream commit 8515fd79fef1ac16d7848cec5ec1797294cb5366). Removing
the variable and letting configure deduce things with the newer libedit
appears to just work. Perhaps upstream configure doesn't implement
the feature detection properly on older libedit versions?

@zanieb
Copy link
Member

zanieb commented Jan 2, 2025

Failures for 3.9 / 3.10 on aarch64, armv7, etc.

cpython-3.9> ./Modules/readline.c:43:42: error: unknown type name 'CPFunction'
cpython-3.9>  extern char **completion_matches(char *, CPFunction *);
cpython-3.9>                                           ^~~~~~~~~~
cpython-3.9> ./Modules/readline.c:918:1: warning: function declaration isn't a prototype [-Wstrict-prototypes]
cpython-3.9>  on_startup_hook()
cpython-3.9>  ^~~~~~~~~~~~~~~
cpython-3.9> Makefile:2162: recipe for target 'Modules/readline.o' failed
cpython-3.9> make: *** [Modules/readline.o] Error 1

Failures for more versions on x86_64 GNU

cpython-3.13> clang  -DPy_RL_STARTUP_HOOK_TAKES_ARGS -I/tools/deps/libedit/include -I/tools/deps/libedit/include/ncursesw -fno-strict-overflow -Wsign-compare -Wunreachable-code -DNDEBUG -g -O3 -Wall -march=x86-64-v4  -fdebug-default-version=4 -fPIC -I/tools/deps/include -I/tools/deps/include/ncursesw   -flto=thin -g -std=c11 -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -Wstrict-prototypes -Werror=implicit-function-declaration -fvisibility=hidden  -I./Include/internal -I./Include/internal/mimalloc  -I. -I./Include -march=x86-64-v4  -fdebug-default-version=4 -fPIC -I/tools/deps/include -I/tools/deps/include/ncursesw  -fPIC -DUSE_LIBEDIT=1 -DPy_BUILD_CORE_BUILTIN -c ./Modules/readline.c -o Modules/readline.o
cpython-3.13> ./Modules/readline.c
cpython-3.13> :
cpython-3.13> 1305
cpython-3.13> :
cpython-3.13> 21
cpython-3.13> :
cpython-3.13> error:
cpython-3.13> incompatible function pointer types assigning to 'rl_hook_func_t *' (aka 'int (*)(void)') from 'int (const char *, int)' [-Wincompatible-function-pointer-types]
cpython-3.13> 1305
cpython-3.13> |
cpython-3.13> r
cpython-3.13> l
cpython-3.13> _
cpython-3.13> s
cpython-3.13> t
cpython-3.13> a
cpython-3.13> r
cpython-3.13> t
cpython-3.13> u
cpython-3.13> p
cpython-3.13> _
cpython-3.13> h
cpython-3.13> o
cpython-3.13> o
cpython-3.13> k
cpython-3.13> =
cpython-3.13> o
cpython-3.13> n
cpython-3.13> _
cpython-3.13> s
cpython-3.13> t
cpython-3.13> a
cpython-3.13> r
cpython-3.13> t
cpython-3.13> u
cpython-3.13> p
cpython-3.13> _
cpython-3.13> h
cpython-3.13> o
cpython-3.13> o
cpython-3.13> k;
cpython-3.13>       |                     ^ ~~~~~~~~~~~~~~~
cpython-3.13> ./Modules/readline.c:1307:23: error: incompatible function pointer types assigning to 'rl_hook_func_t *' (aka 'int (*)(void)') from 'int (const char *, int)' [-Wincompatible-function-pointer-types]
cpython-3.13>  1307 |     rl_pre_input_hook = on_pre_input_hook;
cpython-3.13>       |                       ^ ~~~~~~~~~~~~~~~~~
cpython-3.13> 2
cpython-3.13> error
cpython-3.13> s
cpython-3.13> generated
cpython-3.13> .
cpython-3.13> Makefile:3420: recipe for target 'Modules/readline.o' failed

Failures for x86_64 musl

cpython-3.10> ./Modules/readline.c:444:21: error: expected expression
cpython-3.10>         (VFunction *)on_completion_display_matches_hook : 0;
cpython-3.10>                     ^
cpython-3.10> ./Modules/readline.c:444:10: error: use of undeclared identifier 'VFunction'; did you mean 'function'?
cpython-3.10>         (VFunction *)on_completion_display_matches_hook : 0;
cpython-3.10>          ^~~~~~~~~
cpython-3.10>          function
cpython-3.10> ./Modules/readline.c:430:61: note: 'function' declared here
cpython-3.10>                                                   PyObject *function)
cpython-3.10>                                                             ^
cpython-3.10> ./Modules/readline.c:444:22: error: expected ':'
cpython-3.10>         (VFunction *)on_completion_display_matches_hook : 0;
cpython-3.10>                      ^
cpython-3.10>                      : 
cpython-3.10> ./Modules/readline.c:440:63: note: to match this '?'
cpython-3.10>         readlinestate_global->completion_display_matches_hook ?
cpython-3.10>                                                               ^
cpython-3.10> 3 errors generated.
cpython-3.10> make: *** [Modules/readline.o] Error 1

@indygreg indygreg force-pushed the gps/libedit-upgrade branch 2 times, most recently from ea9ff0b to 402bcdc Compare January 2, 2025 02:15
@indygreg
Copy link
Collaborator Author

indygreg commented Jan 2, 2025

PR is still draft for a reason :)

@indygreg indygreg changed the title downloads: upgrade libedit 20210910-3.1 -> 20240808-3.1 unix: upgrade libedit 20210910-3.1 -> 20240808-3.1 Jan 2, 2025
@zanieb
Copy link
Member

zanieb commented Jan 2, 2025

Just trying to save you some time :) probably should write some CI tooling for that too...

@indygreg indygreg force-pushed the gps/libedit-upgrade branch from 402bcdc to 0a887b6 Compare January 2, 2025 02:32
@indygreg indygreg marked this pull request as ready for review January 2, 2025 02:34
@indygreg
Copy link
Collaborator Author

indygreg commented Jan 2, 2025

I think I fixed the CI failures on Linux.

Still don't have results on macOS since CI is backlogged. But usually Linux and macOS work similarly for libedit, at least on newer Python versions. So 🤞. If macOS is busted, it should hopefully be easy enough to fix.

We were soft blocked on upgrading due to musl compatibility issues.

It looks like these got fixed upstream. So we refreshed the configure
patch and libedit build _just worked_.

However, Python 3.9 and 3.10 encountered compile errors with the
newer version.

On 3.10 we worked around this bug by backporting a patch from 3.11.
On 3.9, the backport was non-trivial, so I just hacked up the existing
3.9 patch to manually change some C preprocessor checks to key off
libedit.

While diffing `Modules/readline.c` I found another patch related to
fixing completer delims. While strictly not required, it was trivial
to backport to 3.10 to fix some missing functionality. So I did.

3.13 initially didn't like the upgraded libedit because
we were manually defining a preprocessor variable (introduced in 3.13
by upstream commit 8515fd79fef1ac16d7848cec5ec1797294cb5366). Removing
the variable and letting configure deduce things with the newer libedit
appears to _just work_. Perhaps upstream configure doesn't implement
the feature detection properly on older libedit versions?
@indygreg indygreg force-pushed the gps/libedit-upgrade branch from 0a887b6 to 98c7d2a Compare January 2, 2025 18:29
@indygreg
Copy link
Collaborator Author

indygreg commented Jan 2, 2025

The refreshed 3.9 patch broke macOS because we're using the macOS SDK libedit, which is older.

I modified the code to only apply the 3.9 patch on !macOS. Hopefully CI passes now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants