-
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
Factor out pybind11/compat/pybind11_platform_abi_id.h #5375
base: master
Are you sure you want to change the base?
Conversation
@henryiii I want to have a README.txt in include/pybind11/compat, but I'm getting the error (see below for full context):
What's the best way to handle this situation? I believe having a README.txt right there (vs a new section in docs/) will be valuable, because this is for people who might not (want to) use pybind11.
|
@henryiii I just marked this as ready for review. A nice to have, if and only if easy: Is there an easy way to build tests/exo_planet_c_api.so with Just one build would be sufficient. E.g. there is no need to get fancy under Windows. Whatever is within easy reach. |
You can add it only to the sdist with MANIFEST.in, if you'd rather. Though having it in the wheel seems fine. I can try the test you suggest tomorrow or so (remind me if I forget). |
I think having the README also in the wheels might be useful for people looking around there.
I figured out something simple that passes local testing at least: 75871ef I just see some GHA jobs aren't happy. I'll put this PR back in draft mode until I have that resolved. |
…se the `"exo_planet_c_api"` target does not exist. 2. Add `-fno-exceptions` option also for `NVHPC`. 3. Also check for `__cpp_exceptions` in exo_planet_c_api.cpp.
…EMSCRIPTEN__ in exo_planet_c_api.cpp
There was one trouble maker (all other jobs worked): Visual Studio 15 2017: ``` cl : Command line warning D9025: overriding '/EHc' with '/EHc-' [C:\projects\pybind11\tests\exo_planet_c_api.vcxproj] ... C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.16.27023\include\xlocale(319): error C2220: warning treated as error - no 'object' file generated [C:\projects\pybind11\tests\exo_planet_c_api.vcxproj] C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.16.27023\include\xlocale(319): warning C4530: C++ exception handler used, but unwind semantics are not enabled. Specify /EHsc ```
@henryiii This is as complete now as I'd want to make it. |
One thing to keep in mind: include/pybind11/compat/pybind11_platform_abi_id.h is making our compiler ABI stuff more public, and I think it's likely too specific. We have errored on the side of being too cautious, and this is currently causing problems for conda-forge - from what I understand, conda-forge assumes different patch releases of MSVC are compatible, while we added the exact MSVC version number here a while back, so that's why pybind11 conda-forge packages are broken (and still broken after the recent releases for v2.12+). (PyPI also doesn't have any way to sync MSVC version numbers - the MSCV version was set by the Python version up till MSVC 2015, and all MSVC versions since have been ABI compatible as far as CPython has been concerned since ABI3 was introduced - though it looks like in a year or two that's going to change). If we could find where ABI changes that affect us lie, and group these at all, that would dramatically improve compatibility. The MSVC version is not something people can easily control, it gets updated for you by GHA, etc. |
A thought on the naming: we are putting this in Also, I tend to expect a |
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.
My question about the folder name, compat
-> conduit
suggestion, is the only issue that might need addressing here (if that was a good idea). Otherwise, looks good.
(I need to find a block of time to look closer at the MSVC ABI question.)
I was ambivalent myself. Seeing
The Pro seems more important. I'll change it as soon as I get a chance (maybe this weekend). |
Description
Follow-on to PR #5296:
Objective: Maximize reusability of
PYBIND11_PLATFORM_ABI_ID
(for other Python/C++ binding systems).Pure refactoring. No functional changes.
This PR also factors out pybind11/compat/pybind11_conduit_v1.h (from tests/exo_planet_c_api.cpp), but this is mainly to provide a reference implementation. See the long comment at the top of pybind11_conduit_v1.h
Birds-eye view of the refactoring steps:
Suggested changelog entry:
pybind11/compat/pybind11_platform_abi_id.h was factored out, to maximize reusability of ``PYBIND11_PLATFORM_ABI_ID`` (for other Python/C++ binding systems).