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

gh-128156: Guard use of `ffi_type_complex_double` on macOS system libffi by encukou · Pull Request #128680 · python/cpython · GitHub

/ cpython Public

gh-128156: Guard use of ffi_type_complex_double on macOS system libffi - #128680

Merged
encukou merged 8 commits into
python:mainfrom
encukou:guard-complex_double
Jan 21, 2025
ConversationCommits (8)ChecksFiles changed
Merged

gh-128156: Guard use of ffi_type_complex_double on macOS system libffi#128680
encukou merged 8 commits into
python:mainfrom
encukou:guard-complex_double

Conversation

encukou commented Jan 9, 2025 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Member

formattable.fmt{C,E,F} now have a runtime guard (if USING_APPLE_OS_LIBFFI).
The list of type codes (previously SIMPLE_TYPE_CHARS) is now generated at runtime.

📚 Documentation preview 📚: https://cpython-previews--128680.org.readthedocs.build/

bedevere-app Bot mentioned this pull request Jan 9, 2025
Closed
skirpichev reviewed Jan 10, 2025
View reviewed changes

skirpichev 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

Looks good, but we need some feedback from MacOS experts.

Comment thread Modules/_ctypes/cfield.c
encukou added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 13, 2025

bedevere-bot commented Jan 13, 2025

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @encukou for commit e8b0992 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 13, 2025

ned-deily commented Jan 14, 2025

Copy link
Copy Markdown
Member

cc: @ronaldoussoren @python/macos-team

ned-deily approved these changes Jan 21, 2025
View reviewed changes

ned-deily 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

LGTM. I verified that with the PR we could now build and install cpython on the current macOS 15.2 using MACOSX_DEPLOYMENT_TARGET=10.14 and then also install and execute the same binaries on a macOS 10.14.x system. As expected, on 15.2 the three complex types were available and on 10.14, they were not. And test_ctypes ran without errors on both systems. Nice work, @encukou !

encukou commented Jan 21, 2025

Copy link
Copy Markdown
Member Author

Thank you for the thorough review!

encukou merged commit d3b1bb2 into python:main Jan 21, 2025
encukou deleted the guard-complex_double branch January 21, 2025 09:59

picnixz commented Jan 21, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Not sure if it's my system or not but I'm unable to compile :(

In file included from ./Modules/_ctypes/callproc.c:87:0:
./Modules/_ctypes/ctypes.h:12:72: error: missing binary operator before token "("
 #   if USING_APPLE_OS_LIBFFI && defined(__has_builtin) && __has_builtin(__builtin_available)

Other uses of __has_builtin are guarded by #if defined(__APPLE__):

#if defined(__APPLE__) && defined(__has_builtin)

I'm using OpenSUSE and gcc 7.5

picnixz commented Jan 21, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

I think we should do the following:

diff --git a/Modules/_ctypes/ctypes.h b/Modules/_ctypes/ctypes.h
index 1330754a7e0..dc079f2441b 100644
--- a/Modules/_ctypes/ctypes.h
+++ b/Modules/_ctypes/ctypes.h
@@ -9,8 +9,12 @@
 // For Apple's libffi, this must be determined at runtime (see gh-128156).
 #if defined(Py_HAVE_C_COMPLEX) && defined(Py_FFI_SUPPORT_C_COMPLEX)
 #   include "../_complex.h"       // complex
-#   if USING_APPLE_OS_LIBFFI && defined(__has_builtin) && __has_builtin(__builtin_available)
-#       define Py_FFI_COMPLEX_AVAILABLE __builtin_available(macOS 10.15, *)
+#   ifdef __has_builtin
+#       if USING_APPLE_OS_LIBFFI && __has_builtin(__builtin_available)
+#           define Py_FFI_COMPLEX_AVAILABLE __builtin_available(macOS 10.15, *)
+#       else
+#           define Py_FFI_COMPLEX_AVAILABLE 1
+#       endif
 #   else
 #       define Py_FFI_COMPLEX_AVAILABLE 1
 #   endif

This is also what's recommended by https://gcc.gnu.org/onlinedocs/cpp/_005f_005fhas_005fbuiltin.html.

cc @encukou @zanieb @ned-deily

encukou commented Jan 21, 2025 via email

Copy link
Copy Markdown
Member Author

picnixz commented Jan 21, 2025

Copy link
Copy Markdown
Member

Yes, but somehow this doesn't work so I don't know :( (I'll check tomorrow again but the current code doesn't compile on my side)

encukou commented Jan 21, 2025

Copy link
Copy Markdown
Member Author

I'd be interested to know what compiler, OS and configure settings you're using.

picnixz commented Jan 21, 2025

Copy link
Copy Markdown
Member

I'm using gcc 7.5.0, my OS is openSUSE 15.5 and I used ./configure --with-pydebug. I can recheck tomorrow (I temporarily fixed this by defining __has_builtin).

picnixz commented Jan 21, 2025

Copy link
Copy Markdown
Member

See #129141 for the relevant issue.

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

skirpichev skirpichev approved these changes
ned-deily ned-deily approved these changes
ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL