gh-105323: Add special workround for macOS default editline by corona10 · Pull Request #108633 · python/cpython

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

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

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

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

 #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

      [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

+    [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

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

erlend-aasland reviewed Aug 29, 2023

View reviewed changes

Comment thread configure.ac 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

Comment thread Modules/readline.c 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

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

Conversation

corona10 commented Aug 29, 2023 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

corona10 commented Aug 29, 2023

Uh oh!

This comment was marked as resolved.

corona10 commented Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corona10 Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corona10 Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corona10 Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corona10 Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corona10 commented Aug 30, 2023

Uh oh!

corona10 commented Aug 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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