Skip to content
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

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Sep 14, 2024

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).

@rwgk rwgk changed the title Factor out pybind11/compat/wrap_include_python_h.h Factor out compat_pybind11_platform_abi_id_h Sep 15, 2024
@rwgk rwgk changed the title Factor out compat_pybind11_platform_abi_id_h Factor out pybind11/compat/pybind11_platform_abi_id.h Sep 15, 2024
@rwgk
Copy link
Collaborator Author

rwgk commented Sep 15, 2024

@henryiii I want to have a README.txt in include/pybind11/compat, but I'm getting the error (see below for full context):

Documentation build test

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.

@rwgk
Copy link
Collaborator Author

rwgk commented Sep 15, 2024

@henryiii I decided it's best to add pybind11/compat/README.txt to the wheels. (commit a0eb9f3).

Currently waiting to see if that resolves the "Documentation build test" issue.

@rwgk rwgk marked this pull request as ready for review September 15, 2024 20:18
@rwgk rwgk requested a review from henryiii as a code owner September 15, 2024 20:18
@rwgk
Copy link
Collaborator Author

rwgk commented Sep 15, 2024

@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.

@henryiii
Copy link
Collaborator

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).

@rwgk
Copy link
Collaborator Author

rwgk commented Sep 16, 2024

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: 75871ef

I just see some GHA jobs aren't happy. I'll put this PR back in draft mode until I have that resolved.

@rwgk rwgk marked this pull request as draft September 16, 2024 18:21
…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.
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
```
@rwgk rwgk marked this pull request as ready for review September 16, 2024 23:11
@rwgk
Copy link
Collaborator Author

rwgk commented Sep 16, 2024

@henryiii This is as complete now as I'd want to make it.

@henryiii
Copy link
Collaborator

henryiii commented Sep 25, 2024

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).

#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.

@henryiii
Copy link
Collaborator

henryiii commented Sep 25, 2024

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.

Copy link
Collaborator

@henryiii henryiii left a 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.

@rwgk
Copy link
Collaborator Author

rwgk commented Sep 25, 2024

(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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants