pybind11
pybind11 copied to clipboard
Factor out pybind11/compat/pybind11_platform_abi_id.h
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:
- Parts of pybind11/detail/common.h → pybind11/compat/wrap_include_python_h.h
- Parts of include/pybind11/detail/internals.h → pybind11/compat/pybind11_platform_abi_id.h
- Parts of tests/exo_planet_c_api.cpp → pybind11/compat/pybind11_conduit_v1.h
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).
@henryiii I want to have a README.txt in include/pybind11/compat, but I'm getting the error (see below for full context):
Only in ./pybind11/compat: README.txt
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.
Run python3 -m pip install --user -U ../dist/*.tar.gz
python3 -m pip install --user -U ../dist/*.tar.gz
installed=$(python3 -c "import pybind11; print(pybind11.get_include() + '/pybind11')")
diff -rq $installed ./pybind11
shell: /usr/bin/bash -e {0}
env:
PIP_BREAK_SYSTEM_PACKAGES: 1
PIP_ONLY_BINARY: numpy
FORCE_COLOR: 3
PYTEST_TIMEOUT: 300
VERBOSE: 1
pythonLocation: /opt/hostedtoolcache/Python/3.1[2](https://github.com/pybind/pybind11/actions/runs/10869192453/job/30160061106?pr=5375#step:9:2).5/x64
PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/[3](https://github.com/pybind/pybind11/actions/runs/10869192453/job/30160061106?pr=5375#step:9:3).12.5/x64/lib/pkgconfig
Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.5/x64
Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.5/x6[4](https://github.com/pybind/pybind11/actions/runs/10869192453/job/30160061106?pr=5375#step:9:4)
Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.[5](https://github.com/pybind/pybind11/actions/runs/10869192453/job/30160061106?pr=5375#step:9:5)/x64
LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.12.5/x[6](https://github.com/pybind/pybind11/actions/runs/10869192453/job/30160061106?pr=5375#step:9:6)4/lib
Processing /home/runner/work/pybind11/pybind11/dist/pybind11-2.14.0.dev1.tar.gz
Installing build dependencies: started
Installing build dependencies: finished with status 'done'
Getting requirements to build wheel: started
Getting requirements to build wheel: finished with status 'done'
Preparing metadata (pyproject.toml): started
Preparing metadata (pyproject.toml): finished with status 'done'
Processing /home/runner/work/pybind11/pybind11/dist/pybind11_global-2.14.0.dev1.tar.gz
Installing build dependencies: started
Installing build dependencies: finished with status 'done'
Getting requirements to build wheel: started
Getting requirements to build wheel: finished with status 'done'
Preparing metadata (pyproject.toml): started
Preparing metadata (pyproject.toml): finished with status 'done'
Building wheels for collected packages: pybind11, pybind11_global
Building wheel for pybind11 (pyproject.toml): started
Building wheel for pybind11 (pyproject.toml): finished with status 'done'
Created wheel for pybind11: filename=pybind11-2.14.0.dev1-py3-none-any.whl size=24[7](https://github.com/pybind/pybind11/actions/runs/10869192453/job/30160061106?pr=5375#step:9:7)323 sha256=6c45c16d00dffe226c663b7697a99caf943c6e[8](https://github.com/pybind/pybind11/actions/runs/10869192453/job/30160061106?pr=5375#step:9:8)6d7a5f40072905fc242a3d1cb
Stored in directory: /home/runner/.cache/pip/wheels/33/71/4b/48a8068c064[9](https://github.com/pybind/pybind11/actions/runs/10869192453/job/30160061106?pr=5375#step:9:9)936366f43f4c61326da95fee8e122155830acd
Building wheel for pybind[11](https://github.com/pybind/pybind11/actions/runs/10869192453/job/30160061106?pr=5375#step:9:11)_global (pyproject.toml): started
Building wheel for pybind11_global (pyproject.toml): finished with status 'done'
Created wheel for pybind11_global: filename=pybind11_global-2.14.0.dev1-py3-none-any.whl size=448987 sha256=2f4c576fc669328b4c0df69dcaa0795c02a0f8c4a60c37f40[12](https://github.com/pybind/pybind11/actions/runs/10869192453/job/30160061106?pr=5375#step:9:12)f2035d4e8a7f3
Stored in directory: /home/runner/.cache/pip/wheels/d9/50/de/12393b3b6dd623bc8e25dde2f7665815fbd256d158981672fe
Successfully built pybind11 pybind11_global
Installing collected packages: pybind11_global, pybind11
Successfully installed pybind11-2.[14](https://github.com/pybind/pybind11/actions/runs/10869192453/job/30160061106?pr=5375#step:9:14).0.dev1 pybind11_global-2.14.0.dev1
Only in ./pybind11/compat: README.txt
Error: Process completed with exit code 1.
@henryiii I decided it's best to add pybind11/compat/README.txt to the wheels. (commit a0eb9f398d9b91283eb964b7dc29aab24289a7af).
Currently waiting to see if that resolves the "Documentation build test" issue.
@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 -fno-exceptions?
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).
You can add it only to the sdist with MANIFEST.in, if you'd rather. Though having it in the wheel seems fine.
I think having the README also in the wheels might be useful for people looking around there.
I can try the test you suggest tomorrow or so (remind me if I forget).
I figured out something simple that passes local testing at least: 75871ef1716e6f420de761fc78304d57e544983a
I just see some GHA jobs aren't happy. I'll put this PR back in draft mode until I have that resolved.
@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).
https://github.com/pybind/pybind11/pull/4779
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 pybind11/compat/*, but it's just the conduit stuff. If we added more compat stuff in the future, like something related to Victor's CAPI compat module, would it make sense to have these together, or would it be better to call this pybind11/conduit/*, and keep any future additions separate?
Also, I tend to expect a compat folder to have compiler/language compatibility shims. Most of my Python projects have a _compat folder with back port handling for older Python versions.
(I need to find a block of time to look closer at the MSVC ABI question.)
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 was ambivalent myself. Seeing pybind11/conduit/ spelled out, looks good to me.
- Pro: clearer
- Con: more subdirs
The Pro seems more important. I'll change it as soon as I get a chance (maybe this weekend).
Ah, that's a little unfortunate, that the CI doesn't run before I resolve the merge conflict. I meant to convince myself that this PR was still in working condition just before PR #4953 was merged. But oh well, I'll just move on.
PR #4953 was integrated with commit fdcd0eac3d174dd4f44a5d2e33d94163edd8b38e.
EDIT: GitHub Actions ran successfully:
All checks have passed
2 skipped and 79 successful checks
My question about the folder name,
compat->conduitsuggestion, is the only issue that might need addressing here (if that was a good idea). Otherwise, looks good.
Done, thanks for the suggestion.
GitHub Actions ran successfully again:
All checks have passed
2 skipped and 79 successful checks
I'll merge this now.
@cryos @wjakob for awareness.