gh-113565: Improve and harden detection of curses dependencies #119816

erlend-aasland · 2024-05-31T00:06:05Z

Issue: configure prefers system install of ncurses over available pkg-config on macOS #113565

configure

erlend-aasland · 2024-05-31T00:10:35Z

If you agree on the overall strategy, I'll continue working on this.

…S_VERSION to guard ncurses features

…library checks

…heck for update_panels() independently of initscr()

erlend-aasland · 2024-06-02T12:32:12Z

I finally managed to coerce this the way I would like it. Locally on macOS, the SDK supplied curses/panel libs are detected just fine using ./configure without args. The Homebrew supplied curses installation is now correctly identified if I add it to PKG_CONFIG_PATH.

Now, this is a large PR, but the configure.ac change should be as readable as M4 and Autoconf macros get 😄

With this PR we now do:

use pkg-config to check for ncursesw/panelw¹
if 1. fails, use pkg-config to check for ncurses/panel
regardless of pkg-config output, search for curses/panel headers, so we're sure we have all defines in pyconfig.h
regardless of pkg-config output, see if libncurses or libncursesw contains the initscr symbol; if it does and pkg-config failed earlier, add the resulting -llib linker option to CURSES_LIBS
ditto for update_panels and PANEL_LIBS
wrap the rest of the curses/panel checks with WITH_SAVE_ENV and make sure we're using updated LIBS and CPPFLAGS for those checks

I believe this is now ready for an initial round of review.

I created the PY_CHECK_CURSES utility macro for this ↩

erlend-aasland · 2024-06-02T12:34:02Z

I'm conflicted regarding backporting this change. I think we should consider it for 3.13, but I'm not so sure about 3.12.

vstinner · 2024-06-06T16:46:25Z

All buildbot failures are unrelated to this PR: good!

AMD64 Arch Linux TraceRefs PR: unrelated. test_import crash, known issue, fixed in the meanwhile.
AMD64 Arch Linux Usan PR: unrelated. Objects/object.c:1019:16: runtime error: call to function long_hash through pointer to incorrect function type 'long (*)(struct _object *)'
AMD64 Ubuntu NoGIL Refleaks PR: unrelated. timeout.
AMD64 Windows10 PR: unrelated. test_launcher failed.
AMD64 Windows11 Bigmem PR: unrelated. test_launcher failed.
ARM64 Windows Non-Debug PR: unrelated. test_cext and test_launcher failed.
ARM64 Windows PR: unrelated. test_launcher failed.
x86 Debian Installed with X PR: unrelated. test_ctypes failed.
x86 Debian Non-Debug with X PR: unrelated. test_ctypes failed.

erlend-aasland · 2024-06-07T09:52:06Z

All buildbot failures are unrelated to this PR: good!

Yes, and that is expected :) The hard ting with changes like this, is we have to tediously examine all buildbots and verify that the dependency detection works as expected, and examine again that the correct compile and linker flags are used in the build phase.

corona10

LGTM! (But prefer not to backport as possible, original issue is also classified as the feature not the bug..)

About the backport, I would like to delegate the decision to @ned-deily

vstinner · 2024-06-09T08:14:57Z

LGTM! (But prefer not to backport as possible, original issue is also classified as the feature not the bug..)

Oh same here, I prefer to not backport such change just to limit the risk of regression.

erlend-aasland · 2024-06-09T19:48:57Z

Thanks for the reviews, Donghee and Victor! I still think we should backport :)

vstinner · 2024-06-10T09:50:58Z

I still think we should backport :)

It's up to you if you want to be responsible for potential regression. What are advantages of backporting the change if the current code "just works" on the stable 3.12 branch?

Buildbots only test the most common major platforms. Other platforms such as OpenBSD, NetBSD, AIX, Solaris, HP-UX, and others are not tested.

erlend-aasland · 2024-06-10T10:17:33Z

What are advantages of backporting the change if the current code "just works" on the stable 3.12 branch?

It's a bug-fix; it does not "just work" on 3.12 ;) IMO, the risk of regression is extremely low. This change improves the build system's ability to detect curses dependencies. If dependencies are not detected, the _curses module is simply not built; the build still succeeds. It does not change the behaviour of the _curses module itself.

erlend-aasland · 2024-06-27T21:16:39Z

@ned-deily, friendly ping 🏓

ned-deily

LGTM, although I limited my review to a non-exhaustive read-through and to testing before and after build configurations on macOS using the current (Sonoma 14.5) release and Command Line Tools with (1) the system-provided ncurses, (2) MacPorts ncurses, (3) Homebrew ncurses, and (4) our Python for macOS installer build. No regressions were noted with or without pkg-config use. I did not test on any other platforms.

FWIW, the PR also eliminates a previously seen compile warning when using either the MacPorts or Homebrew ncurses:

In file included from ./Include/py_curses.h:40:
/opt/homebrew/Cellar/ncurses/6.5/include/ncursesw/ncurses.h:152:9: warning: 'NCURSES_OPAQUE' macro redefined [-Wmacro-redefined]
#define NCURSES_OPAQUE       1
        ^
./Include/py_curses.h:36:9: note: previous definition is here
#define NCURSES_OPAQUE 0

ned-deily · 2024-07-01T06:23:55Z

WRT backporting, I would like to see this in 3.13; I'm somewhat ambivalent about backporting to 3.12. While it is indeed fixing some somewhat buggy build behavior, it is behavior that has been out there since 3.12.0 (and 3.11?) and presumably most downstream builders of Python 3.12 have figured out how to live with it. If it were a bug that affected all users of Python, that would be one thing, but one that only affects builders and, at that, only those who are concerned about ncurses ... that's a lot of churn in some of our favorite files (like configure.ac and header files). I guess that's a +0 from me for 3.12; I think @Yhg1s needs to call it.

erlend-aasland · 2024-07-01T07:42:30Z

Thank you so much for taking the time to review this huge PR, everyone; highly appreciated!

Regarding other operating systems, I did examine a big chunk of the buildbots and found no regressions. I'll create backports for 3.13 and 3.12, but I'll leave the 3.12 one open for Thomas to decide.

…ythonGH-119816) 1. Use pkg-config to check for ncursesw/panelw. If that fails, use pkg-config to check for ncurses/panel. 2. Regardless of pkg-config output, search for curses/panel headers, so we're sure we have all defines in pyconfig.h. 3. Regardless of pkg-config output, check if libncurses or libncursesw contains the 'initscr' symbol; if it does _and_ pkg-config failed earlier, add the resulting -llib linker option to CURSES_LIBS. Ditto for 'update_panels' and PANEL_LIBS. 4. Wrap the rest of the checks with WITH_SAVE_ENV and make sure we're using updated LIBS and CPPFLAGS for those. Add the PY_CHECK_CURSES convenience macro. (cherry picked from commit f80376b) Co-authored-by: Erlend E. Aasland <erlend@python.org>

bedevere-app · 2024-07-01T08:10:20Z

GH-121202 is a backport of this pull request to the 3.13 branch.

…GH-119816) (#121202) 1. Use pkg-config to check for ncursesw/panelw. If that fails, use pkg-config to check for ncurses/panel. 2. Regardless of pkg-config output, search for curses/panel headers, so we're sure we have all defines in pyconfig.h. 3. Regardless of pkg-config output, check if libncurses or libncursesw contains the 'initscr' symbol; if it does _and_ pkg-config failed earlier, add the resulting -llib linker option to CURSES_LIBS. Ditto for 'update_panels' and PANEL_LIBS. 4. Wrap the rest of the checks with WITH_SAVE_ENV and make sure we're using updated LIBS and CPPFLAGS for those. Add the PY_CHECK_CURSES convenience macro. (cherry picked from commit f80376b) Co-authored-by: Erlend E. Aasland <erlend@python.org>

vstinner · 2024-07-01T08:39:25Z

Well done @erlend-aasland :-)

bedevere-app · 2024-07-01T12:13:04Z

GH-121222 is a backport of this pull request to the 3.12 branch.

…ython#119816) 1. Use pkg-config to check for ncursesw/panelw. If that fails, use pkg-config to check for ncurses/panel. 2. Regardless of pkg-config output, search for curses/panel headers, so we're sure we have all defines in pyconfig.h. 3. Regardless of pkg-config output, check if libncurses or libncursesw contains the 'initscr' symbol; if it does _and_ pkg-config failed earlier, add the resulting -llib linker option to CURSES_LIBS. Ditto for 'update_panels' and PANEL_LIBS. 4. Wrap the rest of the checks with WITH_SAVE_ENV and make sure we're using updated LIBS and CPPFLAGS for those. Add the PY_CHECK_CURSES convenience macro.

pythongh-113565: Use pkg-config to detect ncurses[w] and panel[w]

71a243d

bedevere-app bot mentioned this pull request May 31, 2024

configure prefers system install of ncurses over available pkg-config on macOS #113565

Closed

This comment was marked as outdated.

Sign in to view

erlend-aasland commented May 31, 2024

View reviewed changes

configure Outdated Show resolved Hide resolved

erlend-aasland added 10 commits May 31, 2024 10:04

Pull in main

a036e9f

Clean up pkg-config variables, fix manual check

a57e18d

Pull in main

2965d4c

Correctly choose includes in configure.ac and py_curses.h; use NCURSE…

de58647

…S_VERSION to guard ncurses features

Add NEWS

f874d3f

Simplify pkg-config checks and make sure we always do our header and …

4eb1580

…library checks

Disable curses/panel if not found; don't mark them as missing

fb275e2

Enable panel only if curses is also found; harden term.h check, and c…

18b68c5

…heck for update_panels() independently of initscr()

Pull in main

6031b83

Use WITH_SAVE_ENV for all curses feature checks

bedd4b0

erlend-aasland marked this pull request as ready for review June 2, 2024 12:32

erlend-aasland requested a review from corona10 as a code owner June 2, 2024 12:32

bedevere-app bot added the awaiting core review label Jun 2, 2024

erlend-aasland requested review from ronaldoussoren and ned-deily June 2, 2024 12:32

erlend-aasland changed the title ~~gh-113565: Use pkg-config to detect ncurses[w] and panel[w]~~ gh-113565: Improve and harden detection of curses dependencies Jun 2, 2024

erlend-aasland mentioned this pull request Jun 2, 2024

Build failure for python@3.12.1 on mac with non-system version of ncurses #118873

Closed

erlend-aasland added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 2, 2024

This comment was marked as outdated.

Sign in to view

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 2, 2024

This comment was marked as outdated.

Sign in to view

erlend-aasland added 2 commits June 3, 2024 11:09

Pull in main

4990e46

Fix pkg-config failure clause

e61f55d

corona10 approved these changes Jun 9, 2024

View reviewed changes

ned-deily approved these changes Jul 1, 2024

View reviewed changes

Pull in main

487e0fd

erlend-aasland enabled auto-merge (squash) July 1, 2024 07:54

erlend-aasland merged commit f80376b into python:main Jul 1, 2024
36 checks passed

erlend-aasland deleted the autoconf/curses branch July 1, 2024 08:10

This comment has been minimized.

Sign in to view

bedevere-app bot removed the awaiting merge label Jul 1, 2024

This comment was marked as outdated.

Sign in to view

miss-islington-app bot assigned erlend-aasland Jul 1, 2024

bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jul 1, 2024

bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Jul 1, 2024

erlend-aasland mentioned this pull request Jul 1, 2024

[3.12] gh-113565: Improve and harden detection of curses dependencies (#119816) #121222

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-113565: Improve and harden detection of curses dependencies #119816

gh-113565: Improve and harden detection of curses dependencies #119816

erlend-aasland commented May 31, 2024 •

edited by bedevere-app bot

Loading

This comment was marked as outdated.

erlend-aasland commented May 31, 2024

erlend-aasland commented Jun 2, 2024

erlend-aasland commented Jun 2, 2024

This comment was marked as outdated.

This comment was marked as outdated.

vstinner commented Jun 6, 2024

erlend-aasland commented Jun 7, 2024

corona10 left a comment •

edited

Loading

vstinner commented Jun 9, 2024 •

edited

Loading

erlend-aasland commented Jun 9, 2024

vstinner commented Jun 10, 2024 •

edited

Loading

erlend-aasland commented Jun 10, 2024

erlend-aasland commented Jun 27, 2024

ned-deily left a comment

ned-deily commented Jul 1, 2024 •

edited

Loading

erlend-aasland commented Jul 1, 2024

This comment has been minimized.

This comment was marked as outdated.

bedevere-app bot commented Jul 1, 2024

vstinner commented Jul 1, 2024

bedevere-app bot commented Jul 1, 2024

gh-113565: Improve and harden detection of curses dependencies #119816

gh-113565: Improve and harden detection of curses dependencies #119816

Conversation

erlend-aasland commented May 31, 2024 • edited by bedevere-app bot Loading

This comment was marked as outdated.

erlend-aasland commented May 31, 2024

erlend-aasland commented Jun 2, 2024

Footnotes

erlend-aasland commented Jun 2, 2024

This comment was marked as outdated.

This comment was marked as outdated.

vstinner commented Jun 6, 2024

erlend-aasland commented Jun 7, 2024

corona10 left a comment • edited Loading

Choose a reason for hiding this comment

vstinner commented Jun 9, 2024 • edited Loading

erlend-aasland commented Jun 9, 2024

vstinner commented Jun 10, 2024 • edited Loading

erlend-aasland commented Jun 10, 2024

erlend-aasland commented Jun 27, 2024

ned-deily left a comment

Choose a reason for hiding this comment

ned-deily commented Jul 1, 2024 • edited Loading

erlend-aasland commented Jul 1, 2024

This comment has been minimized.

This comment was marked as outdated.

bedevere-app bot commented Jul 1, 2024

vstinner commented Jul 1, 2024

bedevere-app bot commented Jul 1, 2024

erlend-aasland commented May 31, 2024 •

edited by bedevere-app bot

Loading

corona10 left a comment •

edited

Loading

vstinner commented Jun 9, 2024 •

edited

Loading

vstinner commented Jun 10, 2024 •

edited

Loading

ned-deily commented Jul 1, 2024 •

edited

Loading