pybind / pybind11_protobuf

Pybind11 bindings for Google's Protocol Buffers

Other

54 stars 33 forks source link

Fix using system dependencies with CMake build #128

Closed lopsided98 closed 11 months ago

lopsided98 commented 12 months ago

The CMake build system attempted to support using system dependencies, but this functionality did not work for two reasons:

find_package() was called in a subdirectory, which means that imported targets were not visible in the top-level build.
The version numbers for abseil and Protobuf didn't match the format used by their CMake modules, making find_package() always fail.

I also synchronized the version numbers with those used in the Bazel build and removed the exact version constraints. In my opinion, the most common use of system dependencies is probably by distro packagers. In this case we can't guarantee an exactly matching patch version and would most likely have to patch out the exact version check anyway. If you don't agree with this, I'm fine with reverting that part.

lopsided98 commented 12 months ago

cc @srmainwaring @Mizux

rwgk commented 12 months ago

@srmainwaring Is there a chance you could help reviewing this PR?

lopsided98 commented 12 months ago

The CI error to be this: https://github.com/protocolbuffers/protobuf/issues/12185. Maybe need to set ABSL_ENABLE_INSTALL.

rwgk commented 12 months ago

(Idk but sounds like it's worth a trial. Btw I'm trying to find the magic button to have the CI run automatically. Currently it wants an approving click from me, which is annoying.)

rwgk commented 12 months ago

I changed the GitHub setting for first-time contributors, I hope GHA will now run even without me having to click a button here.

rwgk commented 12 months ago

Please do not force push. It invalidates existing commit hashes (in comments and emails) and only makes it more difficult for yourself and others to follow the progress.

The Files changed tab in the GH web view auto-squashes, and the final commit will also be squashed.

BTW I will have to import this into the Google codebase manually when it's done here, then it will get re-exported automatically. The original author info will be preserved.

rwgk commented 12 months ago

Looks like it's all done already! Looks good to me, but I don't know a lot about cmake. Let's wait a day or so for feedback from @srmainwaring.

rwgk commented 12 months ago

Ping @srmainwaring

If there is not feedback, I'll start working on getting this merged tomorrow (as described above).

rwgk commented 12 months ago

129 is how this PR will get merged. I need to wait for a review internally from another Google engineer. Once approved, the steps to first submit internally and then merge here are automatic.

rwgk commented 11 months ago

Merged via #129. Thanks @lopsided98!