-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
PYBIND11_PLATFORM_ABI_ID
Modernization Continued (platforms other than MSVC)
#5439
Conversation
With this PR at commit 14f8425 and logs_30720380481.zip downloaded from here:
This doesn't look right, @robertmaynard I was trying to follow your #4953 (comment) but maybe I'm misunderstanding? (I want to leave NVHPC for later.) |
This presumes that GCC will rev the major value of |
One more thing to fix would be to remove the compiler id |
I am surprised by |
It's generated in this line:
|
Ah, interesting. So for |
Yes. |
Further constrain to GXX_ABI 1002 or greater and less than 2000, hopefully future proof by summarizing that as `1` along with CXX11 on or off.
I just pushed a new commit that produces something like
It also ensures |
Here are the results extracted from the latest CI logs: (Click on any job, then the gear that appears in the top-right corner of the window with the log for that job, then "Download log archive".)
On chat @cryos and I decided to go with Looking at these:
I think we want to omit But what about Does anyone know if And what should we do about this one? Could/should we just omit these lines
and rely on the new |
After discussions with Ralf Grosse-Kunstleve we think these would make good identifiers that are concise and clear.
Within the `__GXX_ABI_VERSION` block this should always be defined, guard against unexpected defines and make the error obvious.
Here are the results extracted from the latest CI logs:
|
The current changes affect "C language object files created with the compiler are binary compatible with the GCC and C/C++ language library." AFAICT, the compiler also links against the system Upshot: I think we should also omit |
…X_USE_CXX11_ABI` (similarly to `gxx_abi` for `__GXX_ABI_VERSION`).
…`_gcc` with `_system`
I just pushed up the experimental commit fe2dbcb which changes Rationale:
I'll post a new |
Latest logs_31272968106.zip — https://github.com/pybind/pybind11/actions/runs/11999580827/job/33447666543?pr=5439
|
@robertmaynard do you have a moment to look at this PR? (The changes are small and all in just one file.)
|
Looks good to me
NVHPC should be ABI compatible with gcc. I am fine either way in how you want to manage that integration from a PR perspective. |
Thanks a lot @robertmaynard for the review!
Based on your comment, I added commit 9fc9515: Add NVHPC (__PGI) to the list of compilers (assumed to be) compatible with system compiler. |
What I learned from the work on #5439 (comment) got me to add I don't expect any extra fallout from those two changes. The |
Just to double-check, the latest logs_31527439795.zip — https://github.com/pybind/pybind11/actions/runs/12108518322/job/33756607834?pr=5439
|
This makes it possible to achieve these two goals: * Avoid the leading underscore in `PYBIND11_PLATFORM_ABI_ID` (see pybind#5439 (comment)) * Maintain backward compatibility for use cases as reported under pybind#5439 (comment) `PYBIND11_INTERNALS_KIND` is removed in this commit to ensure that `PYBIND11_COMPILER_TYPE` is the first element of the `PYBIND11_PLATFORM_ABI_ID`, so that `PYBIND11_COMPILER_TYPE_LEADING_UNDERSCORE` can meaningfully be used as a prefix for `PYBIND11_PLATFORM_ABI_ID` in pybind11/detail/internals.h.
This idea randomly crossed my mind when I woke up this morning: e071edc With that, I think it's best to go ahead and remove (Waiting for CI to finish.) |
Double-checked again, this PR @ commit e071edc:
|
Hi @isuruf, is there a chance that you could take a quick look, especially with a view on how this will work for Conda? I believe this PR is a massive simplification for the purposes of Conda: in the pybind11 CI, all builds using libstdc++ are mutually compatible, and all builds using libc++ are mutually compatible. See #5439 (comment). If it looks good to you I'll rewrite the PR description. |
# elif defined(__INTEL_COMPILER) || defined(__clang__) || defined(__GNUC__) | ||
# define PYBIND11_COMPILER_TYPE "system" // Assumed compatible with system compiler. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still feels weird.
How about?
# elif defined(__INTEL_COMPILER) || defined(__clang__) || defined(__GNUC__) | |
# define PYBIND11_COMPILER_TYPE "system" // Assumed compatible with system compiler. | |
# elif defined(__GLIBC__) && (defined(__INTEL_COMPILER) || defined(__clang__) || defined(__GNUC__)) | |
# define PYBIND11_COMPILER_TYPE "glibc" // Assumed compatible with system compiler. | |
# elif defined(__APPLE__) && (defined(__INTEL_COMPILER) || defined(__clang__) || defined(__GNUC__)) | |
# define PYBIND11_COMPILER_TYPE "macos" // Assumed compatible with system compiler. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like this. Trying it out in the CI right now: ca9e699
The code is exactly as suggested, but I revised the comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. On second thought, this would break musl, android builds etc. Let's keep the earlier one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, gee: sorry I missed your last comment before.
I don't know about musl and android builds.
But I just revert ... simpler is better here, I think.
… more general. The main motivation is to resolve these "Manylinux on 🐍 3.13t • GIL" and "Pyodide wheel" failures: ``` /__w/pybind11/pybind11/include/pybind11/conduit/pybind11_platform_abi_id.h:35:10: error: #error "Unknown PYBIND11_COMPILER_TYPE: PLEASE REVISE THIS CODE." 35 | # error "Unknown PYBIND11_COMPILER_TYPE: PLEASE REVISE THIS CODE." | ^~~~~ ``` (All other CI jobs succeeded.) Further thought: Essentially, under Linux and macOS the `PYBIND11_COMPILER_TYPE` is only for informational purposes. ABI compatibility is determined by the libstdc++ or libc++ ABI version.
This reverts commit 75da5fb.
This reverts commit e34dc8b.
…"glibc"` more general." This reverts commit 41daaa4.
…s suggested)." This reverts commit ca9e699.
# define PYBIND11_COMPILER_TYPE "gcc_cygwin" | ||
# elif defined(_MSC_VER) | ||
# define PYBIND11_COMPILER_TYPE "msvc" | ||
# elif defined(__INTEL_COMPILER) || defined(__clang__) || defined(__GNUC__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the interest of simplification, consider removing defined(__INTEL_COMPILER)
The Intel classic compilers, icc
for C and icpc
for C++, define __GNUC__
as well as __INTEL_COMPILER
.
The last version was 2021.10 and it stopped shipping in 2023.
The "Intel(R) oneAPI DPC++/C++ Compilers", icx
for C and icpx
for C++, do not define __INTEL_COMPILER
.
They do define all of these: __GNUC__
, __clang__
, __INTEL_CLANG_COMPILER
, and __INTEL_LLVM_COMPILER
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR looks great to me now, are there any other planned changes? |
Only some work on the PR description. Sadly Flu + RSV have wiped out a lot of my time. I'll try to do it tonight. |
PYBIND11_PLATFORM_ABI_ID
Modernization Continued (WIP)PYBIND11_PLATFORM_ABI_ID
Modernization Continued (platforms other than MSVC)
I updated the PR description. Thanks so much Everybody for all the input and feedback! |
Description
This PR updates
PYBIND11_PLATFORM_ABI_ID
, which determines cross-extension ABI compatibility, to improve accuracy across all platforms other than MSVC (which was modernized previously in #4953). For context on the importance ofPYBIND11_PLATFORM_ABI_ID
, please refer to PRs #5296 and #5375.The accuracy improvements can be seen by comparing the
PYBIND11_INTERNALS_ID
summaries before and after:master @ 741d86f (logs_32284154734.zip, logs_32284154873.zip):
This PR (pr5439_logs_32281771697.zip, pr5439_logs_32281771739.zip):
Note that the compiler-specific identifiers
clang
,gcc
,icc
,pgi
no longer appear. ABI compatibility is now determined primarily bylibstdcpp
(gcc) orlibcpp
(clang) ABI preprocessor macros.It is expected that overrides like those in pytorch will not be needed anymore. However, this PR ensures that existing overrides will still function, allowing for gradual community adoption.
Similar to #4953, silent fallbacks for unexpected situations have been replaced with
#error ... PLEASE REVISE THIS CODE.
directives. For such cases, command-line overrides for the affected macros can serve as temporary workarounds. To eliminate the need for such workarounds, users are encouraged to contribute updates topybind11/conduit/pybind11_platform_abi_id.h
as needed.For completeness, the counts for MSVC (unchanged):
The work on this PR was informed by:
Suggested changelog entry: