Build platform ABI tag improvements #5437
Conversation
This commit adapts the ABI tag as follows: - It removes ``PYBIND11_INTERNALS_KIND`` that was neither used nor documented. - It adapts the MSVC ABI tag to be less stringent (see PR pybind#4953) - It adapts the GCC ABI tag to be less stringent. - It adds a check for a Clang/libc++ ABI tag that wasn't present before I plan to make a consistent set of changes to nanobind so that both libraries use interchangeable platform ABI tags.
There was a problem hiding this comment.
I didn't realize before that this is quite different from #4953.
I think it's best to handle Linux/macOS by itself in a separate PR, so that we can have a focussed discussion. — To add: Definitely, we should cover all platforms before we make a new pybind11 release.
include/pybind11/detail/internals.h
Outdated
| # if (_MSC_VER) / 100 == 19 | ||
| # define PYBIND11_BUILD_ABI NB_BUILD_LIB "_19" | ||
| # else | ||
| # define PYBIND11_BUILD_ABI NB_BUILD_LIB "_unknown" |
There was a problem hiding this comment.
I feel strongly that such a fallback will do more harm than good: people will mostly not know this is happening and will get surprised later, possibly after making releases already.
Generating an #error here means that we will know immediately that we need to update this code. It'll be under our control then to make this right.
There was a problem hiding this comment.
As was discussed in PR #4953, MSVC has been ABI-compatible for ages. There is one huge ABI break looming some years in the future (MSVC "vNext") that will mark a transition towards the next island of stability.
Based on the information that is available, this looks to be a painful and disruptive break that will in any case require recompiling all Python packages for a new Windows base platform (i.e., similar to the different "manylinux" flavors). The check here is intended to catch that transition without making pybind11 completely unusable on the vNext version.
include/pybind11/detail/internals.h
Outdated
| # define PYBIND11_BUILD_ABI NB_BUILD_LIB "_unknown" | ||
| # endif | ||
| #elif defined(_LIBCPP_ABI_VERSION) | ||
| # define PYBIND11_BUILD_ABI "_abi" NB_TOSTRING(_LIBCPP_ABI_VERSION) |
There was a problem hiding this comment.
Right :D. Should I update the PR, or will you adapt the changes in #5439?
I noticed that you changed some of the specific strings so that the generated ABI IDs are now incompatible with nanobind. (e.g. _usecxx11 vs _legacy used here)
There was a problem hiding this comment.
Ah, I didn't realize.
Could you maybe work on #5439 directly? (Maybe after git merge master; sorry I forgot to update the PR today.)
My hope was that we can work together on 5439 until everybody is happy with it.
There was a problem hiding this comment.
I forgot to add:
I believe initially it'll be best to #error for unknown situations, so that we're sure we're properly covering everything that we're testing with today. As a last step we can add in "unknown" fallbacks.
include/pybind11/detail/internals.h
Outdated
| # define PYBIND11_BUILD_LIB "" | ||
| # endif | ||
| # if (_MSC_VER) / 100 == 19 | ||
| # define PYBIND11_BUILD_ABI NB_BUILD_LIB "_19" |
There was a problem hiding this comment.
How is NB_BUILD_LIB defined?
There was a problem hiding this comment.
It was just a typo, fixed now.
|
I just merged #5375: the code changed here now lives in include/pybind11/conduit/pybind11_platform_abi_id.h |
|
I had second but inconclusive thoughts about using We could have provisions for both:
This way we'd be forced to update the code here (pybind/pybind11), but not the rest of the world. This might be convenient in some situations, but I still believe will backfire in others. I'm not sure what'll be best overall. |
|
FYI I just created #5439. It may take me a few iterations there to get all GitHub Actions to pass. I'll then post an overview of the |
With pybind11 and nanobind considering changes to the platform ABI tag, there is a nice opportunity to make them consistent with each other. This would make it potentially possible to later use that ABI tag to safely dispatch function calls between the two libraries.
This commit adapts the ABI tag as follows:
PYBIND11_INTERNALS_KINDthat was neither used nor documented.