Several questions/possible improvements

[t0ny-peng]

Hi @brakmic-aleksandar. In our local integration of rosidl_typesupport_protobuf we found something that can be (possibly) improved but I want to discuss it with you.

1. In this line, rosidl_adapt_proto_interfaces.cmake was included too early. By having it here, this line will be called inside find_package where there's no IDL files yet and rosidl_write_generator_arguments will fail. This only happens when we are using rosidl_adapter_protobuf in rosidl_generator_cpp. Though I don't understand why it works for msg packages, logically a extension file should only be called when the cmake reaches the extension point.

   • Update: Regarding this problem, I kinda understand why it's included here. In the extension point of rosidl_typesupport_protobuf_c and rosidl_typesupport_protobuf_cpp, rosidl_adapter_proto is find_packaged, the adapter package was found and maybe that's where you want to generate the .proto and .pb.h/cpp files? I'll see how to suppress the error in rosidl_generator_cpp.
   • But maybe another way, instead of find_packageing rodidl_adapter_proto, could be that we execute the extension point it registers earlier? Any thoughts?

2. Relative path should be used here when registering extention

3. A follow up on item 1. If that include was removed, the following CMake code on this line can not find rosidl_adapter_proto_VISIBILITY_CONTROL_HEADER, and leaves an empty -include in the gcc call.
   I understand that there's no easy way to inject the visibility header in the protobuf header so you have to use -include here. Here's my two thoughts:

   1. Can we move the lines of adding global -include into the extension file instead of leaving them in the -extra.cmake file?
   2. Or, instead of using -include, can we modify the generated .pb.hpp file to include the visibility using sed.

[t0ny-peng]

I created a PR to demonstrate the possibility of implementing item 3.2. Please review.

https://github.com/continental/rosidl_typesupport_protobuf/pull/5

[brakmic-aleksandar]

Hi, thanks for your suggestions, i originally considered modifying generated protobuf .pb.hpp files, but i've decided against it, mainly because it seemed more cumbersome to implement, i agree that its good idea to move it to extension file.

[t0ny-peng]

@brakmic-aleksandar Cool. We've implemented 3.2 in our internal code and it worked well. Will push a final PR when it's done.

[brakmic-aleksandar]

@t0ny-peng That's awesome! Thanks!