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

gh-90005: Port readline and curses to PY_STDLIB_MOD (GH-94452) #94452

Merged
merged 7 commits into from
Jul 6, 2022

Conversation

tiran
Copy link
Member

@tiran tiran commented Jun 30, 2022

@tiran tiran 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-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 30, 2022
@tiran tiran 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-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 30, 2022
@tiran tiran marked this pull request as ready for review June 30, 2022 21:23
Copy link
Contributor

@erlend-aasland erlend-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 Show resolved Hide resolved
configure.ac Outdated Show resolved Hide resolved
configure.ac Outdated Show resolved Hide resolved
configure.ac Outdated Show resolved Hide resolved
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
Member Author

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-aasland erlend-aasland Jul 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
Member Author

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.)

@tiran tiran force-pushed the gh-90005-readline-curses branch from dd59373 to a72b632 Compare July 1, 2022 20:08
Copy link
Contributor

@erlend-aasland erlend-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.

configure.ac Show resolved Hide resolved
configure.ac Outdated Show resolved Hide resolved
configure.ac Outdated Show resolved Hide resolved
configure.ac Outdated Show resolved Hide resolved
configure.ac Show resolved Hide resolved
tiran and others added 3 commits July 1, 2022 22:21
@tiran
Copy link
Member Author

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.

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

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

vstinner commented Jul 7, 2022

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:

char readline ();
int main (void) {
  return readline ();
}

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
Member Author

tiran commented Jul 18, 2022

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 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.

5 participants