Skip to content

Conversation

@tiran
Copy link
Member

@tirantiran commented Jun 30, 2022

@tirantiran added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 30, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @tiran for commit 33d9e296124d7daeef89fca957bdab51e1e1d11b 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 30, 2022
@tirantiranforce-pushed the gh-90005-readline-curses branch from 8a61447 to dd59373CompareJune 30, 2022 19:40
@tirantiran added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 30, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @tiran for commit dd5937322e181781c03213621eddb0fb88527c48 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 30, 2022
@tirantiran marked this pull request as ready for review June 30, 2022 21:23
Copy link
Contributor

@erlend-aaslanderlend-aasland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! I left some comments.

configure.ac Outdated
Comment on lines 5848 to 5850
AC_CHECK_LIB($LIBREADLINE, rl_pre_input_hook,
AC_DEFINE(HAVE_RL_PRE_INPUT_HOOK, 1,
[Define if you have readline 4.0]))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this will create havoc for WASM/Emscripten static builds; multiple -l switches are passed to the compiler:

config.log:

29899 configure:21034: checking for rl_pre_input_hook in -lreadline 29900 configure:21059: gcc -o conftest conftest.c -lreadline -lreadline -lintl -ldl >&5 29901 configure:21059: $? = 0 29902 configure:21069: result: yes 29903 configure:21080: checking for rl_completion_display_matches_hook in -lreadline 29904 configure:21105: gcc -o conftest conftest.c -lreadline -lreadline -lintl -ldl >&5 29905 configure:21105: $? = 0 29906 configure:21115: result: yes 29907 configure:21126: checking for rl_resize_terminal in -lreadline 29908 configure:21151: gcc -o conftest conftest.c -lreadline -lreadline -lintl -ldl >&5 

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, should I place a WITH_SAVE_ENV around each check? By the way, readline is not yet supported on Emscripten.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WITH_SAVE_ENV can't be nested :( But your LIBS_SAVE workaround should work well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, it does not work. They're still there.

Copy link
Contributor

@erlend-aaslanderlend-aaslandJul 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AC_CHECK_LIB might be adding the library before running the check.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we need a different approach here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. This reminds me I've got an open PR for the exact same issue for sqlite3 dependency detection :) I've tried various approaches, but I haven't found a neat way to solve it yet. (I haven't looked at it for a week either, though.)

@tirantiranforce-pushed the gh-90005-readline-curses branch from dd59373 to a72b632CompareJuly 1, 2022 20:08
Copy link
Contributor

@erlend-aaslanderlend-aasland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These suggestions should work. You should be able to apply them as a batch.

tiranand others added 3 commits July 1, 2022 22:21
@tiran
Copy link
MemberAuthor

tiran commented Jul 5, 2022

@erlend-aasland I have refactored the code. There is a bit of repetition but IMHO it's ok for a few lookups. We could add our own variant of AC_CHECK_LIB at a later point in time.

@tirantiran requested a review from erlend-aaslandJuly 5, 2022 08:17
@tirantiran changed the title gh-90005: Port readline and curses to PY_STDLIB_MODgh-90005: Port readline and curses to PY_STDLIB_MOD (GH-94452)Jul 6, 2022
@tirantiran merged commit e925241 into python:mainJul 6, 2022
@tirantiran deleted the gh-90005-readline-curses branch July 6, 2022 09:56
@tiran
Copy link
MemberAuthor

tiran commented Jul 6, 2022

I have merged the PR. It works on all tested platforms. We can review and address the general issue with AC_CHECK_LIB in a future PR.

@vstinner
Copy link
Member

The change triggered a reference leak on Fedora: #94644

@kulikjak
Copy link
Contributor

Hi, I think I found an issue.

On Solaris, even a simple code like:

charreadline (); intmain (void){returnreadline ()}

needs to be linked with both -lreadline and -lncurses; that means that checking for readline in -lreadline fails. I thought that passing LIBREADLINE_LIBS would fix that, but the content of that variable is not used until the test passes, and LIBS="-lreadline $LIBS" is always used. (AFAICT, automatic pkg-config would also do nothing, although I didn't test that).

Is there something I am missing or something I can try?

@tiran
Copy link
MemberAuthor

The problem should be fixed by GH-94802.

@kulikjak
Copy link
Contributor

Ah, it's indeed fixed. I am sorry, I thought I am looking into latest main but it was 12 days old (when this was pushed).

Thanks for help!

Sign up for freeto 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.

5 participants

@tiran@bedevere-bot@vstinner@kulikjak@erlend-aasland