search

pybind / pybind11_protobuf

Pybind11 bindings for Google's Protocol Buffers
Other
54 stars 33 forks source link

Make pyproto api optional #161

Closed StefanBruens closed 2 months ago

StefanBruens commented 2 months ago

For CMake builds (at least), PYBIND11_PROTOBUF_ENABLE_PYPROTO_API was 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_API guards.

This was tested with google-or-tools 9.9, compilation and and unit test pass.

See #127.

StefanBruens commented 2 months ago

The cmake-format check is broken, the suggested indentation is just bonkers.

rwgk commented 2 months ago

Happy to support this, we just need to get GitHub Actions clean.

The cmake-format check is broken, the suggested indentation is just bonkers.

Could you please accept the cmake-format output anyway? — Without automatic formatting we're prone to get endless whitespace diffs. — Concretely, simply run pre-commit run --all-files in your git directoy, then commit and push the diff.

This ubuntu-build / bazel error seems related:

ImportError: /home/runner/.bazel/sandbox/linux-sandbox/606/execroot/_main/bazel-out/k8-fastbuild/bin/pybind11_protobuf/tests/extension_disallow_unknown_fields_test.runfiles/_main/pybind11_protobuf/tests/extension_module.so: undefined symbol: _ZN17pybind11_protobuf20check_unknown_fields21AllowUnknownFieldsForESt17basic_string_viewIcSt11char_traitsIcEES4_
================================================================================

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.

rwgk commented 2 months ago

@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" PyProto_API and "the only reason it's not dead is because there's a few amphibian projects that still need it for python/C++ interop."

My conclusion: I'll simply remove all PyProto_API from the OSS side entirely. Concretely, in the automated step exporting from Google to GitHub I'll strip out all related code. — I'll try to do that asap.

rwgk commented 2 months ago

The alternative #165 was merged.