Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
erlend-aasland merged 21 commits intopython:mainfromerlend-aasland:autoconf/curses
Jul 1, 2024

Conversation

@erlend-aasland
Copy link
Contributor

@erlend-aaslanderlend-aasland commentedMay 31, 2024•
edited by bedevere-appbot
Loading

s-sajid-ali reacted with thumbs up emoji
@erlend-aasland

This comment was marked as outdated.

@erlend-aasland
Copy link
ContributorAuthor

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

@erlend-aasland
Copy link
ContributorAuthor

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

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

With this PR we now do:

  1. use pkg-config to check for ncursesw/panelw1
  2. if 1. fails, use pkg-config to check for ncurses/panel
  3. regardless of pkg-config output, search for curses/panel headers, so we're sure we have all defines in pyconfig.h
  4. regardless of pkg-config output, see if libncurses or libncursesw contains theinitscr symbol; if it doesand pkg-config failed earlier, add the resulting-llib linker option toCURSES_LIBS
  5. ditto forupdate_panels andPANEL_LIBS
  6. wrap the rest of the curses/panel checks withWITH_SAVE_ENV and make sure we're using updatedLIBS andCPPFLAGS for those checks

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

Footnotes

  1. I created thePY_CHECK_CURSES utility macro for this↩

@erlend-aasland
Copy link
ContributorAuthor

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.

@erlend-aaslanderlend-aasland changed the titlegh-113565: Use pkg-config to detect ncurses[w] and panel[w]gh-113565: Improve and harden detection of curses dependenciesJun 2, 2024
@erlend-aaslanderlend-aasland added the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelJun 2, 2024
@bedevere-bot

This comment was marked as outdated.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelJun 2, 2024
@erlend-aasland

This comment was marked as outdated.

@vstinner
Copy link
Member

All buildbot failures areunrelated 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 reacted with thumbs up emoji

@erlend-aasland
Copy link
ContributorAuthor

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

Copy link
Member

@corona10corona10 left a comment•
edited
Loading

Choose a reason for hiding this comment

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

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
Copy link
Member

vstinner commentedJun 9, 2024•
edited
Loading

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
Copy link
ContributorAuthor

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

@vstinner
Copy link
Member

vstinner commentedJun 10, 2024•
edited
Loading

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 reacted with thumbs up emoji

@erlend-aasland
Copy link
ContributorAuthor

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 thebuild system's ability to detectcurses dependencies. If dependencies are not detected, the_curses module is simply not built; the build still succeeds. It does not change thebehaviour of the_curses module itself.

@erlend-aasland
Copy link
ContributorAuthor

@ned-deily, friendly ping 🏓

Copy link
Member

@ned-deilyned-deily left a comment

Choose a reason for hiding this comment

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

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

erlend-aasland reacted with hooray emoji
@ned-deily
Copy link
Member

ned-deily commentedJul 1, 2024•
edited
Loading

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 (likeconfigure.ac and header files). I guess that's a +0 from me for 3.12; I think@Yhg1s needs to call it.

erlend-aasland reacted with thumbs up emoji

@erlend-aasland
Copy link
ContributorAuthor

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.

@erlend-aaslanderlend-aaslandenabled auto-merge (squash)July 1, 2024 07:54
@erlend-aaslanderlend-aasland merged commitf80376b intopython:mainJul 1, 2024
@erlend-aaslanderlend-aasland deleted the autoconf/curses branchJuly 1, 2024 08:10
@miss-islington-app

This comment has been minimized.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull requestJul 1, 2024
…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 commitf80376b)Co-authored-by: Erlend E. Aasland <erlend@python.org>
@miss-islington-app

This comment was marked as outdated.

@bedevere-app
Copy link

GH-121202 is a backport of this pull request to the3.13 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.13bugs and security fixes labelJul 1, 2024
erlend-aasland added a commit that referenced this pull requestJul 1, 2024
…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 commitf80376b)Co-authored-by: Erlend E. Aasland <erlend@python.org>
@vstinner
Copy link
Member

Well done@erlend-aasland :-)

erlend-aasland reacted with heart emoji

@bedevere-app
Copy link

GH-121222 is a backport of this pull request to the3.12 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.12only security fixes labelJul 1, 2024
Akasurde pushed a commit to Akasurde/cpython that referenced this pull requestJul 3, 2024
…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.
noahbkim pushed a commit to hudson-trading/cpython that referenced this pull requestJul 11, 2024
…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.
estyxx pushed a commit to estyxx/cpython that referenced this pull requestJul 17, 2024
…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.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@vstinnervstinnervstinner approved these changes

@corona10corona10corona10 approved these changes

@ned-deilyned-deilyned-deily approved these changes

@ronaldoussorenronaldoussorenAwaiting requested review from ronaldoussoren

Assignees

@erlend-aaslanderlend-aasland

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

5 participants

@erlend-aasland@bedevere-bot@ned-deily@vstinner@corona10
[8]ページ先頭
©2009-2025 Movatter.jp