pybind11 icon indicating copy to clipboard operation
pybind11 copied to clipboard

Factor out pybind11/compat/pybind11_platform_abi_id.h

Open rwgk opened this issue 1 year ago • 9 comments

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 avatar Sep 14 '24 20:09 rwgk

@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 avatar Sep 15 '24 17:09 rwgk

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

rwgk avatar Sep 15 '24 19:09 rwgk

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

rwgk avatar Sep 15 '24 20:09 rwgk

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

henryiii avatar Sep 16 '24 04:09 henryiii

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.

rwgk avatar Sep 16 '24 18:09 rwgk

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

rwgk avatar Sep 16 '24 23:09 rwgk

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.

henryiii avatar Sep 25 '24 16:09 henryiii

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.

henryiii avatar Sep 25 '24 17:09 henryiii

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

rwgk avatar Sep 25 '24 17:09 rwgk

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.

rwgk avatar Nov 10 '24 18:11 rwgk

PR #4953 was integrated with commit fdcd0eac3d174dd4f44a5d2e33d94163edd8b38e.

EDIT: GitHub Actions ran successfully:

All checks have passed

2 skipped and 79 successful checks

rwgk avatar Nov 10 '24 18:11 rwgk

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.

Done, thanks for the suggestion.

GitHub Actions ran successfully again:

All checks have passed

2 skipped and 79 successful checks

rwgk avatar Nov 10 '24 20:11 rwgk

I'll merge this now.

@cryos @wjakob for awareness.

rwgk avatar Nov 10 '24 20:11 rwgk