When exporting symbols, an include directive may be necessary

[mathstuf]

It'd be nice to have an option to add #include "mylib_export.h" to generated .pb.h files so that export symbols are defined before their use.

[TeBoring]

If your code depends on symbols in mylib_export.h, it's a better practice to directly include that in your code instead of indirectly included from our generated code.
Since symbols in mylib_export.h is not used by .pb.cc, I don't think .pb.h needs to import that.

[mathstuf]

Umm, but protobuf is also generating code using an export macro (dllexport_decl= on the command line or EXPORT_MACRO in CMake), so it should include the file which provides that macro.

[mathstuf]

@TeBoring Thoughts on the above comment?

[traversaro]

Since symbols in mylib_export.h is not used by .pb.cc, I don't think .pb.h needs to import that.

I faced a similar problem (see ros-industrial/abb_libegm#63), and I am bit confused by this comment. The macro passed to dllexport_decl, if they are defined in mylib_export.h, need to include by the preprocessor when generating the code for the compilation unit corresponding to .pb.cc file, otherwise the compiler will complain because the macro is not defined.

I noticed that some projects deal with this problem by defining a custom generator (see https://bitbucket.org/osrf/gazebo/src/804410860234af97c5e309896dc007e8cde04ba8/gazebo/msgs/generator/GazeboGenerator.cc#lines-72 and https://bitbucket.org/ignitionrobotics/ign-msgs/src/18640ef6c31777e25953ae44b6ad10ed68c4993e/src/Generator.cc#lines-106), but it would be nice to be able to insert a user-specified included header without implementing a custom generator. If there is an interest in merging such an option, I would be happy to provide a PR.

[mathstuf]

I think a PR with a test case would help a lot with this. I forget how I worked around it in the project that I found this with. But, getting the attention of a Googler back on this is probably a prerequisite given how I've seen other things happen on this project :( . Hopefully a PR would do that, but who knows.

[traversaro]

Interestingly, Chromium has a protoc wrapper that adds the necessary includes in the generated .pb.h via a --include option: https://github.com/chromium/chromium/blob/80.0.3974.1/tools/protoc_wrapper/protoc_wrapper.py#L90 .

The motivation for this wrapper is indeed to correctly handle export macros, see: https://github.com/chromium/chromium/blob/80.0.3974.1/third_party/protobuf/proto_library.gni#L34 .

[mathstuf]

The author of the latter doesn't have an (obvious) github username, but @gkraynov refactored the former last. Maybe they could get some attention on this to avoid having such a workaround in chromium's vendoring of protobuf in the future :) .

[traversaro]

In my specific case, the workaround that I found is to force the inclusion of the visibility header via the MSVC option /FI for all the compilation unit that are part of the library or if they link the library that contains the protobuf-generated code, see ros-industrial/abb_libegm#63 (comment) . This solution is a bit brittle, but it seems to be working.

[Anton1123]

@TeBoring
Can this issue be re-opened?
When generating protobuf code with the export macro option (dllexport_decl) passed to protoc, it would be extremely beneficial to be able to also pass in a force include header option (such as the MSVC compiler option /FI). Otherwise we have to use a protoc wrapper like the one in the previous comments.

[Anton1123]

The resolution to adding a #include "export_header.h" that is described here is to use /FIexport_header.h. But protoc (using version 3.6.1) does not recognize the /FI argument. So people have gotten around this by using protoc wrappers like the one shared in this thread.