gh-105323: Add special workround for macOS default editline #108633

corona10 · 2023-08-29T12:50:19Z

Issue: Incompatible function pointer types assigning to 'Function *' at readline module #105323

Misc/NEWS.d/next/Tests/2023-08-29-21-51-04.gh-issue-105223.lqw_5B.rst

configure.ac

corona10 · 2023-08-29T12:53:24Z

@erlend-aasland @ned-deily
The default readline header of macOS is always linked to editline https://github.com/phracker/MacOSX-SDKs/blob/041600eda65c6a668f66cb7d56b7d1da3e8bcc93/MacOSX11.3.sdk/usr/include/readline/readline.h

corona10 · 2023-08-29T13:17:37Z

@erlend-aasland @ned-deily
I double checked on my machine and it works as expected

./python.exe -E -c 'import sys ; from sysconfig import get_platform ; print("%s-%d.%d" % (get_platform(), *sys.version_info[:2]))' >platform
The necessary bits to build these optional modules were not found:
_gdbm                 _hashlib              _ssl
To find the necessary bits, look in configure.ac and config.log.

Could not build the ssl module!
Python requires a OpenSSL 1.1.1 or newer

Checked 107 modules (31 built-in, 73 shared, 0 n/a on macosx-12.5-arm64, 0 disabled, 3 missing, 0 failed on import


$> python.exe -m test test_readline -v
Raised RLIMIT_NOFILE: 256 -> 1024
== CPython 3.13.0a0 (heads/gh-105223-dirty:19fbfa8752, Aug 29 2023, 22:15:44) [Clang 14.0.0 (clang-1400.0.29.202)]
== macOS-12.5.1-arm64-arm-64bit little-endian
== Python build: release
== cwd: /Users/user/oss/cpython/build/test_python_98543æ
== CPU count: 10
== encodings: locale=UTF-8, FS=utf-8
0:00:00 load avg: 6.06 Run tests sequentially
0:00:00 load avg: 6.06 [1/1] test_readline
readline version: 0x402
readline runtime version: 0x402
readline library version: 'EditLine wrapper'
use libedit emulation? True
testHistoryUpdates (test.test_readline.TestHistoryManipulation.testHistoryUpdates) ... ok
test_nonascii_history (test.test_readline.TestHistoryManipulation.test_nonascii_history) ... ok
test_write_read_append (test.test_readline.TestHistoryManipulation.test_write_read_append) ... skipped 'append_history not available'
test_auto_history_disabled (test.test_readline.TestReadline.test_auto_history_disabled) ... ok
test_auto_history_enabled (test.test_readline.TestReadline.test_auto_history_enabled) ... ok
test_history_size (test.test_readline.TestReadline.test_history_size) ... skipped 'this readline version does not support history-size'
test_init (test.test_readline.TestReadline.test_init) ... ok
test_nonascii (test.test_readline.TestReadline.test_nonascii) ... ok

----------------------------------------------------------------------
Ran 8 tests in 0.057s

OK (skipped=2)

== Tests result: SUCCESS ==

1 test OK.

Total duration: 123 ms
Tests result: SUCCESS

Modules/readline.c

ned-deily · 2023-08-29T15:48:08Z

Modules/readline.c

@@ -1019,10 +1019,16 @@ on_hook(PyObject *func)
 static int
 #if defined(_RL_FUNCTION_TYPEDEF)
 on_startup_hook(void)
+{
+#elif defined(__APPLE__) && defined(WITH_EDITLINE)


Note that other distributors and users of Python on macOS may be linking to a libedit that is not the Apple-supplied libedit on macOS. For example, MacPorts supplies their own newer version of libedit. So tests like this might not be appropriate. It needs to be tested (see my comments on the issue).

Ned, then how about defining the new macro WITH_APPLE_READLINE and using it?
(It will only enabled if the user does not designate the readline option through ./configure`)

Have you established that there aren't also warnings when compiling with versions of editline other than the macOS system one? I literally don't have time to delve into this now as I'm traveling but I did try to do a quick compile with the current MacPorts version of editline I have installed with me and that also produced compile warnings though not exactly the same ones.

./configure: Follow apple readline

./configure --with-readline=editline: Follow custom editline

./configure --with-readline=readline: Follow custom readline

Or we can add a version check also: RL_READLINE_VERSION==0x0402, all macOS are using identical versions for built-in readline.

cc @ronaldoussoren

I'm traveling but I did try to do a quick compile with the current MacPorts version of editline I have installed with me and that also produced compile warnings though not exactly the same ones.

~~Well, it's up to which commits of Python you are using.~~
~~After we reverted Irit's patch, there is no compiler warning for system one too.~~
But it does not mean that the issue is solved because we don't use proper parameters for apple system built-in library and in the future clang compiler will not skip such a mistake.

Please see my comment: #108588 (comment)

Well, it's up to which commits of Python you are using.
After we reverted Irit's patch, there is no compiler warning for system one too.

Umm, sorry, it was not reverted yet.

Have you established that there aren't also warnings when compiling with versions of editline other than the macOS system one?

I will test them and share soon.

Okay with b89dd4c
I detected the following warnings with homebrew libedit. (./configure --with-readline=editline)

./Modules/readline.c:1269:21: warning: incompatible function pointer types assigning to 'rl_hook_func_t *' (aka 'int (*)(void)') from 'int (const char *, int)' [-Wincompatible-function-pointer-types] rl_startup_hook = on_startup_hook; ^ ~~~~~~~~~~~~~~~ ./Modules/readline.c:1271:23: warning: incompatible function pointer types assigning to 'rl_hook_func_t *' (aka 'int (*)(void)') from 'int (const char *, int)' [-Wincompatible-function-pointer-types] rl_pre_input_hook = on_pre_input_hook;

And with 67e3a54
there were no compiler warnings.

So I would like to propose updating the policy as I proposed from #108633 (comment)
If people want to custom readline library passing the explicit option looks reasonable and if people want to use the builtin readline library ./configure will be enough.

erlend-aasland · 2023-08-29T19:52:02Z

configure.ac

@@ -5822,13 +5823,20 @@ AC_ARG_WITH(
    AS_CASE([$with_readline],
      [editline|edit], [with_readline=edit],
      [yes|readline], [with_readline=readline],
-      [no], [],
+      [no], [with_readline=default],


This does not feel right; if --with-readline=no we ... default to system readline?

Ah it was my mistake.

erlend-aasland · 2023-08-29T19:53:16Z

configure.ac

+    [Darwin/*], [AC_DEFINE([WITH_APPLE_READLINE], [1]) with_readline=readline],
+    [with_readline=readline])


You set with_readline=readline no matter which branch; perhaps you can put that assignment outside the switch.

corona10 · 2023-08-30T03:02:00Z

@ned-deily @ronaldoussoren @erlend-aasland
OTAH, Introducing a new configuration option for the Apple built-in editline library can be a more proper fix.
I submitted another PR for this approach: #108665
It will be no side-effect at all.

corona10 · 2023-08-30T04:36:30Z

I gave up this PR and promote #108665, which has no side effect for every case.

pythongh-105223: Add special workround for macOS default editline

aaf7bf0

corona10 requested a review from ned-deily August 29, 2023 12:50

corona10 requested a review from erlend-aasland as a code owner August 29, 2023 12:50

bedevere-bot added the awaiting core review label Aug 29, 2023

Add NEWS.d

19fbfa8

corona10 changed the title ~~gh-105223: Add special workround for macOS default editline~~ gh-105323: Add special workround for macOS default editline Aug 29, 2023

bedevere-bot mentioned this pull request Aug 29, 2023

Incompatible function pointer types assigning to 'Function *' at readline module #105323

Closed

erlend-aasland reviewed Aug 29, 2023

View reviewed changes

Misc/NEWS.d/next/Tests/2023-08-29-21-51-04.gh-issue-105223.lqw_5B.rst Outdated Show resolved Hide resolved

erlend-aasland reviewed Aug 29, 2023

View reviewed changes

configure.ac Outdated Show resolved Hide resolved

corona10 added the skip news label Aug 29, 2023

This comment was marked as resolved.

Sign in to view

corona10 added DO-NOT-MERGE and removed DO-NOT-MERGE labels Aug 29, 2023

Address code review

da9156f

corona10 mentioned this pull request Aug 29, 2023

Module/readline.c has suspicious macro #108588

Closed

erlend-aasland reviewed Aug 29, 2023

View reviewed changes

Modules/readline.c Outdated Show resolved Hide resolved

Add comment

b89dd4c

ned-deily reviewed Aug 29, 2023

View reviewed changes

Address code review

67e3a54

corona10 requested review from ned-deily and ronaldoussoren August 29, 2023 16:44

erlend-aasland reviewed Aug 29, 2023

View reviewed changes

Address code review

2e2377f

corona10 closed this Aug 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-105323: Add special workround for macOS default editline #108633

gh-105323: Add special workround for macOS default editline #108633

corona10 commented Aug 29, 2023 •

edited by bedevere-bot

Loading

corona10 commented Aug 29, 2023

This comment was marked as resolved.

corona10 commented Aug 29, 2023 •

edited

Loading

ned-deily Aug 29, 2023

corona10 Aug 29, 2023 •

edited

Loading

ned-deily Aug 29, 2023

corona10 Aug 29, 2023 •

edited

Loading

corona10 Aug 29, 2023

corona10 Aug 29, 2023

corona10 Aug 29, 2023 •

edited

Loading

corona10 Aug 29, 2023

corona10 Aug 29, 2023

corona10 Aug 29, 2023 •

edited

Loading

erlend-aasland Aug 29, 2023

corona10 Aug 30, 2023

erlend-aasland Aug 29, 2023

corona10 commented Aug 30, 2023

corona10 commented Aug 30, 2023 •

edited

Loading

		[Darwin/*], [AC_DEFINE([WITH_APPLE_READLINE], [1]) with_readline=readline],
		[with_readline=readline])

gh-105323: Add special workround for macOS default editline #108633

gh-105323: Add special workround for macOS default editline #108633

Conversation

corona10 commented Aug 29, 2023 • edited by bedevere-bot Loading

corona10 commented Aug 29, 2023

This comment was marked as resolved.

corona10 commented Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

corona10 Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corona10 Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corona10 Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corona10 Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corona10 commented Aug 30, 2023

corona10 commented Aug 30, 2023 • edited Loading

corona10 commented Aug 29, 2023 •

edited by bedevere-bot

Loading

corona10 commented Aug 29, 2023 •

edited

Loading

corona10 Aug 29, 2023 •

edited

Loading

corona10 Aug 29, 2023 •

edited

Loading

corona10 Aug 29, 2023 •

edited

Loading

corona10 Aug 29, 2023 •

edited

Loading

corona10 commented Aug 30, 2023 •

edited

Loading