Make pyproto api optional #161
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The |
StefanBruens
force-pushed
the
make_pyproto_api_optional
branch
from
2e83d48 to
72eaae5
Compare
|
Happy to support this, we just need to get GitHub Actions clean.
Could you please accept the cmake-format output anyway? — Without automatic formatting we're prone to get endless whitespace diffs. — Concretely, simply run This ubuntu-build / bazel error seems related: Could you please take a look? Note: Currently we're not set up for automatically importing PRs. I'll have to import manually (a little cumbersome but not a big deal), or maybe we can try to get the automatic import going for this PR. |
StefanBruens
force-pushed
the
make_pyproto_api_optional
branch
4 times, most recently
from
f732396 to
5bb8f38
Compare
In case `PYBIND11_PROTOBUF_ENABLE_PYPROTO_API` is not defined, the py_proto_api_ member of the GlobalState singleton is never changed from its default nullptr value. Any code protected by a `GlobalState::instance()->py_proto_api()` check can thus also be made dependent on the `PYPROTO_API` define. This allows to remove the dependency on the proto_api.h header file. As the call to check_unknown_fields::CheckRecursively is also protected by the `py_proto_api()` it can be stubbed out. See pybind#127.
The PYBIND11_PROTOBUF_ENABLE_PYPROTO_API define was never set in CMake builds, add a corresponding option. As the check_unknown_fields code is only called dependent on the define remove it from the build if not used.
As pybind11 does not guarantee stable ABI for its types (e.g.
pybind11::handle), these types have "hidden" visibility at least on
Linux.
This visibility is propagated to any method which has any of the pybind11
types in its parameters, which is the case for many of the public methods
in proto_cast_util.{h,cc}. Trying to link to e.g. a SHARED
pybind11_native_proto_caster library will create linker errors to to
undefined symbols for any non-trivial case.
The other approach would be to move everything which uses a pybind11
type to the headers, and only leave pybind11 agnostic methods in the
shared library. While this is trivially possible for e.g.
PyBytesAsStringView, for the majority of the methods (e.g. anything
depending on the GlobalState object) a significant refactoring would
be required.
See pybind#160.
StefanBruens
force-pushed
the
make_pyproto_api_optional
branch
from
5bb8f38 to
508257b
Compare
|
@StefanBruens Integrating this PR (into the Google codebase) turned out to be more hairy than anticipated. When I asked for advice internally, I learned that they are actively working on "burning down" My conclusion: I'll simply remove all |
copybara-service bot
pushed a commit
that referenced
this pull request
Jun 20, 2024
…le codebase to GitHub. For context see #161, in particular #161 (comment). Intentionally not stripping out the check_unknown_fields.h,cc sources: 1. For simplicity, and 2. so that it is more obvious externally what the corresponding code in pybind11_protobuf/tests/extension_test.py is about (stripping out the test code would lead to distracting clutter). PiperOrigin-RevId: 644613416
copybara-service bot
pushed a commit
that referenced
this pull request
Jun 21, 2024
…le codebase to GitHub. For context see #161, in particular #161 (comment). Intentionally not stripping out the check_unknown_fields.h,cc sources: 1. For simplicity, and 2. so that it is more obvious externally what the corresponding code in pybind11_protobuf/tests/extension_test.py is about (stripping out the test code would lead to distracting clutter). PiperOrigin-RevId: 644613416
copybara-service bot
pushed a commit
that referenced
this pull request
Jun 21, 2024
…le codebase to GitHub. For context see #161, in particular #161 (comment). Intentionally not stripping out the check_unknown_fields.h,cc sources: 1. For simplicity, and 2. so that it is more obvious externally what the corresponding code in pybind11_protobuf/tests/extension_test.py is about (stripping out the test code would lead to distracting clutter). PiperOrigin-RevId: 644613416
copybara-service bot
pushed a commit
that referenced
this pull request
Jun 21, 2024
…le codebase to GitHub. For context see #161, in particular #161 (comment). Intentionally not stripping out the check_unknown_fields.h,cc sources: 1. For simplicity, and 2. so that it is more obvious externally what the corresponding code in pybind11_protobuf/tests/extension_test.py is about (stripping out the test code would lead to distracting clutter). PiperOrigin-RevId: 645462442
|
The alternative #165 was merged. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For CMake builds (at least),
PYBIND11_PROTOBUF_ENABLE_PYPROTO_APIwas never defined, so the proto_api was not used in most cases.Though, there are some parts of the code which did depend on the (considered private/obsolete) protobuf proto_api.h header file. Make sure all uses are behind the
PYBIND11_PROTOBUF_ENABLE_PYPROTO_APIguards.This was tested with google-or-tools 9.9, compilation and and unit test pass.
See #127.