-
- Notifications
You must be signed in to change notification settings - Fork 33.2k
gh-90005: Port readline and curses to PY_STDLIB_MOD (GH-94452) #94452
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
Conversation
🤖 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. |
8a61447
to dd59373
Compare 🤖 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. |
There was a problem hiding this 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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
dd59373
to a72b632
Compare There was a problem hiding this 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.
Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@protonmail.com>
@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 |
I have merged the PR. It works on all tested platforms. We can review and address the general issue with |
The change triggered a reference leak on Fedora: #94644 |
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 Is there something I am missing or something I can try? |
The problem should be fixed by GH-94802. |
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! |
Uh oh!
There was an error while loading. Please reload this page.