pybind11_protobuf icon indicating copy to clipboard operation
pybind11_protobuf copied to clipboard

How to integrate this in a CMake project?

Open maxnoe opened this issue 3 years ago • 17 comments

I am very interested in getting this to run in a CMake project and possibly contributing to make that happen.

I have this repo cloned as a submodule inside a small test project, but cannot get this to compile / run without linker errors.

This is my current setup:

Files

extern/pybind11_protobuf
CMakeLists.txt
fastproto.cpp
Person.proto

Person.proto:

syntax = "proto2";

message Person {
  required int32 id = 2;
  required string name = 1;
  optional string email = 3;
}

fastproto.cpp


#include <pybind11/pybind11.h>
#include "Person.pb.h"
#include <string>
#include "pybind11_protobuf/native_proto_caster.h"

Person get_person() {
    Person person;
    person.set_id(1);
    person.set_name(std::string{"Maximilian Nöthe"});
    person.set_email(std::string{"[email protected]"});
    return person;
}

PYBIND11_MODULE(fastproto, m) {
    pybind11_protobuf::ImportNativeProtoCasters();
    m.def("get_person", &get_person);
}

CMakeLists.txt

cmake_minimum_required(VERSION 3.17..3.22)

project(Quirc++ VERSION 0.1.0 LANGUAGES C CXX)

include(GNUInstallDirs)

find_package(Python3 REQUIRED COMPONENTS Development Interpreter)
find_package(pybind11 REQUIRED)
find_package(Protobuf REQUIRED)

protobuf_generate_cpp(PROTO_SRCS PROTO_HDRS Person.proto)

pybind11_add_module(
    fastproto
    fastproto.cpp
    ${PROTO_SRCS}
    extern/pybind11_protobuf/pybind11_protobuf/native_proto_caster.cc
    extern/pybind11_protobuf/pybind11_protobuf/proto_cast_util.cc
    extern/pybind11_protobuf/pybind11_protobuf/proto_utils.cc
)
target_include_directories(fastproto PRIVATE ${CMAKE_CURRENT_BINARY_DIR} extern/pybind11_protobuf)
target_link_libraries(fastproto PRIVATE protobuf::libprotobuf)
target_compile_features(fastproto PRIVATE cxx_std_14)
set_target_properties(fastproto PROPERTIES
    CXX_EXTENSIONS OFF
    CXX_STANDARD_REQUIRED ON
)

Or use the repo here: https://github.com/maxnoe/pybind_protobuf_test

Experimenting with this, I had several issues. However now in this version including the sources my main problem is that a protobuf header is not found:

❯ cmake --build build
-- Found pybind11: /usr/include (found version "2.9.0")
-- Configuring done
-- Generating done
-- Build files have been written to: /home/maxnoe/Projects/protobuf_pybind/build
Consolidate compiler generated dependencies of target fastproto
[ 14%] Building CXX object CMakeFiles/fastproto.dir/extern/pybind11_protobuf/pybind11_protobuf/proto_cast_util.cc.o
/home/maxnoe/Projects/protobuf_pybind/extern/pybind11_protobuf/pybind11_protobuf/proto_cast_util.cc:19:10: fatal error: python/google/protobuf/proto_api.h: No such file or directory
   19 | #include "python/google/protobuf/proto_api.h"
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
make[2]: *** [CMakeFiles/fastproto.dir/build.make:126: CMakeFiles/fastproto.dir/extern/pybind11_protobuf/pybind11_protobuf/proto_cast_util.cc.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:83: CMakeFiles/fastproto.dir/all] Error 2
make: *** [Makefile:91: all] Error 2

It seems to be this one: https://github.com/protocolbuffers/protobuf/blob/master/python/google/protobuf/proto_api.h

But that seems not to be part of an installed protobuf.

Any help on getting this to run would be appreciated.

maxnoe avatar Feb 02 '22 15:02 maxnoe

Edit: after downloading the header and placing it where it can be found, I get an undefined reference to an absl object when importing:

❯ python -c 'import fastproto; print(fastproto.get_person())'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
ImportError: /home/maxnoe/Projects/protobuf_pybind/build/fastproto.cpython-310-x86_64-linux-gnu.so: undefined symbol: _ZN4absl12lts_2021110213hash_internal15MixingHashState5kSeedE

maxnoe avatar Feb 02 '22 20:02 maxnoe

Edit: I managed to make this work by linking against linking absl::hash absl::flat_hash_map), see the updated repo.

Would you be open to adding a CMakeLists.txt for this project? I can prepare a PR

maxnoe avatar Feb 02 '22 20:02 maxnoe

Hi @maxnoe, I put together a cmake build for a project to provide bindings for some of the ignition robotics libraries. The pybind11_protobuf branch with the cmake support is here: https://github.com/srmainwaring/pybind11_protobuf/tree/feature/cmake, but it seems that you've got it sorted in the meanwhile.

I'd also be keen to see cmake support included in the repo.

srmainwaring avatar Feb 02 '22 21:02 srmainwaring

@srmainwaring Thanks! This is looking good. I see that you only include the protobuf library for the one header I also mentioned above.

I opened this issue here, to get it included in the python package: https://github.com/protocolbuffers/protobuf/issues/9464

If I understand it correctly, that header should be part of the python packge and probably some utility method to get it's location.

maxnoe avatar Feb 02 '22 21:02 maxnoe

Yes, having the headers in a place that doesn't require bringing in protobuf directly would be good.

I used submodules to include the various dependencies, but that does present some problems (i.e. conflicts) when this package is included in larger projects, so it may be better to use cmake's find_package for everything. The approach may also depend on what platform your using (I use macOS with brew for package management). Using find_package does break the hermitic build that the Basel approach offers but from the standpoint of integrating with other third-party libraries that are already including a number of the same dependencies this might be an acceptable compromise.

I don't have a strong view either way, but please use any changes in my branch if they are helpful - and I'll be happy to help with testing.

srmainwaring avatar Feb 02 '22 21:02 srmainwaring

Would you be open to adding a CMakeLists.txt for this project? I can prepare a PR

In a nutshell: open yes, staffed no.

The main problem is that we're not set up to accept external PRs. The reasons are purely technical. It's a fairly big project to enable the workflow needed.

But, if you're ok doing things manually for now, the process would be:

  • You create a PR as usual.
  • I could import it more-or-less manually into the Google environment, tweak & submit internally.
  • Then the automatic service will kick in to export the change here.

I believe this could work ok for adding cmake support (assuming there will mostly or even purely be additional files; it would probably be a lot less practical for core code changes).

Let's give it a shot and see how it goes?

rwgk avatar Feb 03 '22 08:02 rwgk

A am a bit confused now. Why is this under the pybind organisation if this is still an internal google project?

maxnoe avatar Feb 03 '22 08:02 maxnoe

This repo grew organically and was never staffed as an official project. Contributing to core pybind11 is now staffed. I'm hoping to build support for expanding the scope in the future. In the meantime I can only offer what's known as "best effort". (That's more common than one might think.)

On Thu, Feb 3, 2022 at 00:26 Maximilian Nöthe @.***> wrote:

A am a bit confused now. Why is this under the pybind organisation if this is still an internal google project?

— Reply to this email directly, view it on GitHub https://github.com/pybind/pybind11_protobuf/issues/60#issuecomment-1028720232, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFUZACYSRP5M32PYE6KZGTUZI33VANCNFSM5NMM7ZTA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you commented.Message ID: @.***>

rwgk avatar Feb 03 '22 08:02 rwgk

Would you be open to adding a CMakeLists.txt for this project? I can prepare a PR

In a nutshell: open yes, staffed no.

The main problem is that we're not set up to accept external PRs. The reasons are purely technical. It's a fairly big project to enable the workflow needed.

But, if you're ok doing things manually for now, the process would be:

  • You create a PR as usual.
  • I could import it more-or-less manually into the Google environment, tweak & submit internally.
  • Then the automatic service will kick in to export the change here.

I believe this could work ok for adding cmake support (assuming there will mostly or even purely be additional files; it would probably be a lot less practical for core code changes).

Let's give it a shot and see how it goes?

cc @joelynch

jungleraptor avatar Apr 26 '22 00:04 jungleraptor

So it appears there's no hope for pybind11 due to protocol buffer support for python taking a drastic turn away from being able to support cmake and their new UPB thing?

kyranf avatar Nov 09 '23 02:11 kyranf

So it appears there's no hope for pybind11 due to protocol buffer support for python taking a drastic turn away from being able to support cmake and their new UPB thing?

I don't understand this comment at all TBH.

Did you see that @srmainwaring contributed cmake support? Merged via #110 on Mar 9, 2023, fixes by @lopsided98 merged via #129 on Aug 25, 2023.

If that does not work for you, please provide exact details (ideally a reproducer).

rwgk avatar Nov 09 '23 08:11 rwgk

@kyranf My understanding is that if you use pybind11 to create python bindings to C++ protobuf objects, you won't use the python protobuf module at all. So them switching to upb shouldn't affect this here, correct?

maxnoe avatar Nov 09 '23 09:11 maxnoe

So it appears there's no hope for pybind11 due to protocol buffer support for python taking a drastic turn away from being able to support cmake and their new UPB thing?

I don't understand this comment at all TBH.

Did you see that @srmainwaring contributed cmake support? Merged via #110 on Mar 9, 2023, fixes by @lopsided98 merged via #129 on Aug 25, 2023.

If that does not work for you, please provide exact details (ideally a reproducer).

if I take @srmainwaring 's gz-python project which uses pybind11 and for python to use the protobufs as a dependency, it cannot compile with cmake as it cannot find one of the headers. This is a known issue right now on that project.

https://github.com/srmainwaring/gz-python/issues/24

kyranf avatar Nov 14 '23 16:11 kyranf

If you FetchContent() Protobuf, you could patch it to make the proto_api.h hdr available...

see: https://github.com/google/or-tools/blob/870edf6f7bff6b8ff0d267d936be7e331c5b8c2d/patches/protobuf-v26.1.patch#L59-L68 and https://github.com/google/or-tools/blob/870edf6f7bff6b8ff0d267d936be7e331c5b8c2d/cmake/dependencies/CMakeLists.txt#L107-L110

Mizux avatar Jun 03 '24 13:06 Mizux