FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-109617: fix ncurses incompatibility on macOS with Xcode 15 by sorcio · Pull Request #111258 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

gh-109617: fix ncurses incompatibility on macOS with Xcode 15 - #111258

Merged
ambv merged 11 commits into
python:mainfrom
sorcio:curses-mac-fix
May 4, 2024
ConversationCommits (11)ChecksFiles changed
Merged

gh-109617: fix ncurses incompatibility on macOS with Xcode 15#111258
ambv merged 11 commits into
python:mainfrom
sorcio:curses-mac-fix

Conversation

sorcio commented Oct 24, 2023 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Contributor

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

bedevere-app Bot mentioned this pull request Oct 24, 2023
Closed
corona10 requested a review from a team October 25, 2023 02:21
serhiy-storchaka self-requested a review October 25, 2023 08:29
vstinner reviewed Oct 27, 2023
View reviewed changes
Comment thread Modules/_cursesmodule.c Outdated
Comment thread Modules/_cursesmodule.c Outdated
Comment thread Modules/_cursesmodule.c Outdated
Comment thread Modules/_cursesmodule.c
Comment thread Include/py_curses.h
@@ -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. */

vstinner Oct 27, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can you explain the version check in the comment?

Comment thread Include/py_curses.h
serhiy-storchaka reviewed Oct 31, 2023
View reviewed changes

serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread Modules/_cursesmodule.c

sorcio commented Oct 31, 2023

Copy link
Copy Markdown
Contributor Author

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?

sorcio mentioned this pull request Nov 9, 2023
Closed
sobolevn mentioned this pull request Feb 13, 2024
Closed

hugovk commented Feb 13, 2024

Copy link
Copy Markdown
Member

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

erlend-aasland reviewed Feb 13, 2024
View reviewed changes
Comment thread configure.ac Outdated
Comment thread Modules/_cursesmodule.c Outdated
Comment thread Modules/_cursesmodule.c Outdated

hugovk commented Feb 13, 2024

Copy link
Copy Markdown
Member

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 commented Feb 13, 2024

Copy link
Copy Markdown
Contributor Author

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.

ned-deily mentioned this pull request Feb 13, 2024
Closed
ambv merged commit 08d169f into python:main May 4, 2024
SonicField pushed a commit to SonicField/cpython that referenced this pull request May 8, 2024
pythongh-109617: fix ncurses incompatibility on macOS with Xcode 15 (p…
027cd28 
…ython#111258)

Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
Co-authored-by: Łukasz Langa <lukasz@langa.pl>
sorcio deleted the curses-mac-fix branch May 12, 2024 14:45
ned-deily mentioned this pull request Apr 7, 2025
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

vstinner vstinner left review comments
serhiy-storchaka serhiy-storchaka left review comments
erlend-aasland erlend-aasland left review comments
corona10 Awaiting requested review from corona10 corona10 is a code owner
+1 more reviewer
ma8ma ma8ma left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL