Protobuf generates several C4251 warnings on Windows

[Blast545]

:man_farmer: Spin-off for #1175

Environment

• OS Version: Windows
• Source or binary build: Source

Description

• Expected behavior: Windows CI shouldn't have warnings
• Actual behavior: Currently there are 132 warnings for ign-gazebo5: ign-5-win

Steps to reproduce

Run a Windows CI job for ign-gazebo5

Output

Most of the warnings look like this:

'google::protobuf::internal::SerialArena::space_allocated_': struct 'std::atomic' needs to have dll-interface to be used by clients of class 'google::protobuf::internal::SerialArena' (compiling source file C:\Jenkins\workspace\ign_gazebo-ign-5-win\ws\build\ignition-gazebo5\src\gui\plugins\scene3d\moc_Scene3D.cpp)

I think the first step to clean the Windows warnings would be getting rid of the protobuf warnings. There's a list of documented warnings disabled on their code: https://github.com/protocolbuffers/protobuf/blob/master/cmake/README.md#notes-on-compiler-warnings

In particular, I'd like to get some input about disabling C4251 in the Windows farm, which currently causes 99 warnings on its own for the protobuf library. If it's ok I can open a PR to disable it, although I don't know if it's better if someone else looks at the 15 C4251 coming from ign-gazebo code. https://build.osrfoundation.org/job/ign_gazebo-ign-5-win/52/msbuild/category.63474781/ Has someone discussed about this warning in the past? @chapulina @j-rivero

[chapulina]

We just wrap the offending headers in pragmas to disable these warnings, i.e.

https://github.com/ignitionrobotics/ign-sensors/blob/26004b372497ee77616fd23e9abe1d772d79106c/include/ignition/sensors/DepthCameraSensor.hh#L29-L37

[Blast545]

We just wrap the offending headers in pragmas to disable these warnings, i.e.

@chapulina You mean for the offending headers inside the ignition stack? We don't have access to the ones internal to the protobuf library: https://build.osrfoundation.org/job/ign_gazebo-ign-5-win/52/msbuild/category.63474781/folder.140978115/

[chapulina]

Ideally we should wrap only the 3rd party headers. For example:

https://github.com/ignitionrobotics/ign-msgs/blob/5415083d5d110ab4aa3f9e9f0a3e0f952c773f3e/src/Factory.cc#L18-L37

But if I remember correctly, including some ign-msgs headers still cause these protobuf warnings. If that's the case, we either find out how that's happening or wrap the ign-msgs headers too, i.e.

https://github.com/ignitionrobotics/ign-sensors/blob/26004b372497ee77616fd23e9abe1d772d79106c/include/ignition/sensors/DepthCameraSensor.hh#L29-L37

[wilderfield]

protobuf should burn in hell

[mjcarroll]

@wilderfield generally looking for productive input, if possible...

[wilderfield]

@mjcarroll ok, I guess use the following compiler flag when you build with protobuf on Windows, /wd4251

I think that is a better solution than littering code bases with windows specific pragmas.

I don't get why protobuf automatically generated files cause the visual studio compiler to go berserk with warnings.

[Blast545]

@wilderfield We were actually able to address this issue in gz-cmake with this PR.

I'll proceed and close this issue for now, as it's no longer affecting gz-sim.