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-109617: fix ncurses incompatibility on macOS with Xcode 15 #111258

Merged
merged 11 commits into from May 4, 2024

Conversation

sorcio
Copy link
Contributor

@sorcio sorcio commented Oct 24, 2023

macOS 14 ships with a new version of ncurses1. But their new SDK tries to support both the old and the new version at build time in a way that CPython's configure script could not correctly detect.

  1. Default to using NCURSES_OPAQUE = 0 which, when using ncurses, avoids the use of some exported functions, such as is_pad2. The macro versions are used instead.
  2. Since the same build can dynamically link against significantly different ncurses implementations, query the version information at runtime instead of hardcoding it.

Question: do we want to limit the change to macOS, or possibly make it conditional to MACOSX_DEPLOYMENT_TARGET < 14.0?

Footnotes

  1. for a long time, macOS has shipped a libncurses version based on ncurses 5.7.20081102. Since macOS 14.0, the version seems to be 6.0.20150808 plus some patches.

  2. see https://invisible-island.net/ncurses/man/curs_opaque.3x.html. Opaque mode would be necessary to use the alternative libncursest (ncurses thread-safer ABI) but Python does not use it.

Modules/_cursesmodule.c Outdated Show resolved Hide resolved
Modules/_cursesmodule.c Outdated Show resolved Hide resolved
Modules/_cursesmodule.c Outdated Show resolved Hide resolved
Modules/_cursesmodule.c Show resolved Hide resolved
@@ -39,7 +39,8 @@
#ifdef HAVE_NCURSES_H
/* configure was checking <curses.h>, but we will
use <ncurses.h>, which has some or all these features. */
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain the version check in the comment?

Include/py_curses.h Show resolved Hide resolved
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

The current code do not use pechochar(), pnoutrefresh(), prefresh() and subpad() if neither HAVE_CURSES_IS_PAD nor WINDOW_HAS_FLAGS are defined. It allows to build with the curses implementation that does not support pads and does not have these function.

This PR makes the above functions required.

Modules/_cursesmodule.c Show resolved Hide resolved
@sorcio
Copy link
Contributor Author

sorcio commented Oct 31, 2023

The current code do not use pechochar(), pnoutrefresh(), prefresh() and subpad() if neither HAVE_CURSES_IS_PAD nor WINDOW_HAS_FLAGS are defined. It allows to build with the curses implementation that does not support pads and does not have these function.

This PR makes the above functions required.

Thanks, I missed that. I will revert to the macro version. Do you have a suggestion on how to test for those implementations?

@hugovk
Copy link
Member

hugovk commented Feb 13, 2024

Updated from main to trigger the CI and see if this fixes the new #115383.

configure.ac Outdated Show resolved Hide resolved
Modules/_cursesmodule.c Outdated Show resolved Hide resolved
Modules/_cursesmodule.c Outdated Show resolved Hide resolved
@hugovk
Copy link
Member

hugovk commented Feb 13, 2024

We got lucky with GitHub Actions runner image allocation.

The free-threaded build ran on the older 20240114.1 with Xcode 14 (which has recently started failing):

And the regular build ran on the newer 20240204.1 with Xcode 15:

And both passed 👍

@sorcio
Copy link
Contributor Author

sorcio commented Feb 13, 2024

I updated the patch according to feedback:

  • reverted to the py_is_pad definition as a macro, so that it can build with curses implementation that don't have pechochar(), pnoutrefresh(), prefresh() or subpad()
  • added some comments to clarify context

I left in the code that parses ncurses version. It addresses a problem that only arises once this fix is available (that is, static version could report 6 but the interpreter could be loading 5 if running on older macOS). I can remove if this feels too controversial, or split to a new issue.

@ambv ambv merged commit 08d169f into python:main May 4, 2024
36 checks passed
SonicField pushed a commit to SonicField/cpython that referenced this pull request May 8, 2024
…ython#111258)

Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
Co-authored-by: Łukasz Langa <lukasz@langa.pl>
@sorcio sorcio deleted the curses-mac-fix branch May 12, 2024 14:45
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.

None yet

7 participants