gh-105323: Update readline module to detect apple editline variant #108665
Conversation
corona10
requested review from
erlend-aasland,
ezio-melotti and
hugovk
as code owners
corona10
changed the title
corona10
force-pushed
the
gh-105223-apple-readline
branch
from
8371b10 to
868cb8a
Compare
|
Without having time to look closely at this now. I feel pretty strongly against adding a new configuration option for what has always worked by default before. There should be a better way. |
Yeah let's discuss it after your travel :) Enjoy! |
The different signature has existed since macOS 10.5. (2007) |
|
I agree with @ned-deily. Before proposing new PRs, let's instead lay out the three scenarios in detail and then see how we can improve configure and readline.c preprocessor guards :) |
First of all, I love seamless approach, but here is the table for why IIUC, There is no way to distinguish between editline and Apple editline except using Suppose Apple editline is viewed as an independent implementation. In that case, I think it is better in terms of consistency to treat it separately as an Apple option as if the edlitline option was added. |
|
Hm, I wonder if there are really four flavours we need to take into account:
|
When I refer the libedit hompage, |
Minix Editline (https://troglobit.com/projects/editline/) provides editline/editline.h. |
Thanks @erlend-aasland, I updated the table :) |
Apple's lib edit isn't really an independent implementation, but is a very old copy of NetBSD libedit. This old revision in the NetBSD CVS(!) server has similar definitions. That said, this is just nitpicking on your statement. For handling the differences you might as well treat this as the "apple" implementation because I expect nobody else uses the old code base. Do you know if the newer libedit is consistently better than Apple's copy? If it is we could consider to just ship a current version with our installers, but that's for a different issue. |
corona10
changed the title
|
Okay, using CI for FreeBSD is unrelated to this change. |
configure.ac
Outdated
| AS_VAR_IF([with_readline], [readline], [ | ||
| AX_CHECK_TYPEDEF(Function, readline/readline.h, [,AC_DEFINE([WITH_APPLE_EDITLINE]),]) |
There was a problem hiding this comment.
I am not sure that we have to support for AS_VAR_IF([with_readline], [edit], [...]) case too
corona10
changed the title
|
Oops, I need more testing. |
configure.ac
Outdated
| @@ -5830,6 +5831,12 @@ AC_ARG_WITH( | |||
| [with_readline=readline] | |||
| ) | |||
|
|
|||
| dnl gh-105323: Need to handle the macOS editline as an alias of readline. | |||
| AS_CASE([$ac_sys_system/$ac_sys_release], | |||
| [Darwin/*], [AX_CHECK_TYPEDEF(Function, readline/readline.h, AC_DEFINE([WITH_APPLE_EDITLINE]))], | |||
There was a problem hiding this comment.
@erlend-aasland cc @ronaldoussoren
I am not sure why AC_DEFINE([WITH_APPLE_EDITLINE]) is not generated in configure file.
Checking itself looks okay but the problem is if the condition is true it doesn't do as my expectation.
checking whether right shift extends the sign bit... yes
checking for getc_unlocked() and friends... yes
checking for Function in readline/readline.h... yes
|
@ronaldoussoren @ned-deily @erlend-aasland Finally, I updated the PR to be the working solution. I checked both cases in my local environment. FYI, I found some comments about Thank you @ronaldoussoren and @ned-deily to suggest better solution :) |
|
@ronaldoussoren @ned-deily @erlend-aasland gentle ping! |
Co-authored-by: Erlend E. Aasland <[email protected]>
📚 Documentation preview 📚: https://cpython-previews--108665.org.readthedocs.build/