[v2.12] Backport: Enable type-safe interoperability between different independent Python/C++ bindings systems. #5368
Conversation
…n/C++ bindings systems. (pybind#5296) * `self.__cpp_transporter__()` proof of concept: Enable passing C++ pointers across extensions even if the `PYBIND11_INTERNALS_VERSION`s do not match. * Include cleanup (mainly to resolve PyPy build failures). * Fix clang-tidy errors. * Resolve `error: extra * factor out platform_abi_id.h from internals.h (no functional changes) * factor out internals_version.h from internals.h (no functional changes) * Update CMakeLists.txt, tests/extra_python_package/test_files.py * Revert "factor out internals_version.h from internals.h (no functional changes)" This reverts commit 3ccea8c. * Remove internals_version.h from CMakeLists.txt, tests/extra_python_package/test_files.py * `.__cpp_transporter__()` implementation: compare `pybind11_platform_abi_id`, `cpp_typeid_name` * Add PremiumTraveler * Rename test_cpp_transporter_traveler_type.h -> test_cpp_transporter_traveler_types.h * Expand tests: `PremiumTraveler`, `get_points()` * Shuffle order of tests (no real changes). * Move `__cpp_transporter__` lambda to `py::cpp_transporter()` regular function. * Use `type_caster_generic::load(self)` instead of `cast<T *>(self)` * Pass `const std::type_info *` via `py::capsule` (instead of `cpp_typeid_name`). * Make platform_abi_id.h completely stand-alone. * rename exo_planet.cpp -> exo_planet_pybind11.cpp * Add exo_planet_c_api.cpp (incomplete). * Fix silly oversight (wrong filename in `#include`). * Resolve clang-tidy errors: ``` /__w/pybind11/pybind11/tests/exo_planet_c_api.cpp:10:18: error: 'wrapGetLuggage' is a static definition in anonymous namespace; static is redundant here [readability-static-definition-in-anonymous-namespace,-warnings-as-errors] 10 | static PyObject *wrapGetLuggage(PyObject *, PyObject *) { return PyUnicode_FromString("TODO"); } | ~~~~~~ ^ /__w/pybind11/pybind11/tests/exo_planet_c_api.cpp:14:20: error: 'ThisMethodDef' is a static definition in anonymous namespace; static is redundant here [readability-static-definition-in-anonymous-namespace,-warnings-as-errors] 14 | static PyMethodDef ThisMethodDef[] | ~~~~~~ ^ /__w/pybind11/pybind11/tests/exo_planet_c_api.cpp:17:27: error: 'ThisModuleDef' is a static definition in anonymous namespace; static is redundant here [readability-static-definition-in-anonymous-namespace,-warnings-as-errors] 17 | static struct PyModuleDef ThisModuleDef = { | ~~~~~~ ^ ``` * Implement exo_planet_c_api GetLuggage(), GetPoints() * Move new code from test_cpp_transporter_traveler_bindings.h to pybind11/detail/type_caster_base.h, under the name `class_dunder_cpp_transporter()` * Fix oversight. * Unconditionally add `__cpp_transporter__` method to all `py::class_` objects, but do not include that magic method in docstring signatures. * Back out pybind11/detail/platform_abi_id.h for now. Maximizing reusability can be handled separately, later. * Small cleanup. * Restore and add to `test_call_cpp_transporter_*()` * Ensure pybind#3788 does not bite again. * `class_dunder_cpp_transporter()`: replace `obj.cast<std::string>()` with `std::string(obj)` * Add (simple) copyright notices in all newly added files. * Globally replace cpp_transporter with cpp_conduit * style: pre-commit fixes * IWYU fixes * Rename `class_dunder_cpp_conduit()` -> `cpp_conduit_method()` * Change `pybind11_platform_abi_id`, `pointer_kind` argument types from `str` to `bytes`. This avoids the unicode decode/encode roundtrips: * More robust (no decode/encode errors). * Minor runtime optimization. * Systematically rename `cap_cpp_type_info` -> `cpp_type_info_capsule` (no functional changes). * Systematically replace `cpp_type_info_capsule` `name`: `"const std::type_info *"` -> `typeid(std::type_info).name()` (this IS a functional change). This provides an extra layer of protection against C++ ABI mismatches: * The first and most important layer is that the `PYBIND11_PLATFORM_ABI_ID`s must match between extensions. * The second layer is that the `typeid(std::type_info).name()`s must match between extensions. * Fix sort order accident in tests/CMakeLists.txt * Apply suggestions from code review Co-authored-by: Aaron Gokaslan <[email protected]> * style: pre-commit fixes * refactor: rename to _pybind_conduit_v1_ Signed-off-by: Henry Schreiner <[email protected]> * Add test_home_planet_wrap_very_lonely_traveler(), test_exo_planet_pybind11_wrap_very_lonely_traveler() * Resolve clang-tidy errors: ``` /__w/pybind11/pybind11/tests/test_cpp_conduit_traveler_bindings.h:39:32: error: parameter 'm' is passed by value and only copied once; consider moving it to avoid unnecessary copies [performance-unnecessary-value-param,-warnings-as-errors] 10 | py::class_<LonelyTraveler>(m, "LonelyTraveler"); | ^ | std::move( ) /__w/pybind11/pybind11/tests/test_cpp_conduit_traveler_bindings.h:43:52: error: parameter 'm' is passed by value and only copied once; consider moving it to avoid unnecessary copies [performance-unnecessary-value-param,-warnings-as-errors] 43 | py::class_<VeryLonelyTraveler, LonelyTraveler>(m, "VeryLonelyTraveler"); | ^ | std::move( ) ``` --------- Signed-off-by: Henry Schreiner <[email protected]> Co-authored-by: Ralf W. Grosse-Kunstleve <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Henry Schreiner <[email protected]> Co-authored-by: Aaron Gokaslan <[email protected]>
|
@henryiii What's the best way to handle the Changelog for v2.12.1, and the version change in pybind11/detail/common.h? Should I just make all the changes in this PR? |
@henryiii — I went ahead and added all changes in this PR, with the idea to forward-port the Changelog addition to master after this PR is merged. Please let me know if there is a better way. The |
|
The existing GitHub Actions failures are: Config:
CI:
These failures are identical to those under PR #5369. I.e., they are certainly unrelated to the changes under PR #5296. |
|
I would not touch version numbers in a PR not related to version numbers. What I do is:
|
|
Ahh, this is a PR to the branch. Please put the branch name in brackets in the PR title if you do that. I'd probably not bother making these PRs, they've already been PRs and passed testing. Just cherry pick them and push. If you want, I can take a stab at doing v2.12 in a few minutes. |
|
I reverted the two commits here (Changlog addition, version number change). Could you please merge if this looks good to you? (No need to wait for GHA to finish, the code hasn't changed compared to what was tested before.) My plan from here:
|
rwgk
changed the title
I saw this only just now.
Sorry I meant to do that, but then it slipped my mind. Fixed.
Note 8b93691. I'll do a PR for v2.11 as well, to be on the safe side. |
|
I'll do it for v2.13, and record the process here as I do it:
Ahh, I'm off a version. I think I'm thinking 2.13? Current release is 2.13.6, shouldn't we get 2.13.7 out first? |
|
Okay, I'd cherry pick everything you want, make a rebase-and-merge PR, and then update the changelog with those versions to master. You might just need to make the same manual changes to the changelog in the PR, though, since otherwise you'd have to cherry pick all older changelog changes. |
|
(Sorry for the confusion, I was confused by the PR and then the version numbers. I'll try to do the next 2.13 release in a few minutes then) |
Oh, for v2.13 I was thinking of holding off until #5362 is merged. WDYT? |
|
I'd be fine to get this out now as a strict compatibility improvement, then do a 2.13.7 soon. I have a possible CMake issue I'd like to look into (with the classic search and mismatched bittiness) that we could get into the next version. The new 2.12 & 2.11 won't be very useful if they can't talk to 2.13? Regardless, I"m going to follow the same procure (that I kind of accidentally stopped you from doing): make a PR with the changelog, then make a rebase-and-merge PR with the changes to v2.13. I'll go ahead and prepare the v* addition too. |
Awesome, thanks! |
|
Squash-merging, after coordinating with @henryiii on slack. |
Description
Backport of PR #5296 to the pybind11 v2.12 series, to maximize the benefit of the new cross-version interoperability feature.
Suggested changelog entry: