Cookbook discussion

[traversaro]

Describe the bug
Not really a bug, just a couple suggestion for the Cookbook available at https://github.com/ms-iot/ROSOnWindows/blob/master/Porting/Cookbook.md . If you agree with them, I would be happy to provide a PR for modifying the Cookbook.

Visibility macros

For dealing with visibility issues, the cookbook suggests to use the CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS CMake property.

However, the fallback solution of directly declaring the visibility macros LIB_EXTERN has some drawbacks in its current form:

• It does specify that the LIB_EXTERN and BUILDING_LIB macro need to be specific to each shared library (so if a package contains two shared libraries, you need one macro for each shared library). See https://bitbucket.org/osrf/gazebo/issues/2262/symbol-visibility-issues-in-windows for a Gazebo issue related to this. It may be worth to mention that CMake provides the DEFINE_SYMBOL target property to define the BUILDING_LIB-like macro, that is by default initialized to <targetName>_EXPORTS.
• In its current form, the suggested definition of LIB_EXTERN and BUILDING_LIB macro would break compilation of static libraries in Windows, see Fix static compilation on Windows ros/console_bridge#40 .

Threat warning an errors

The suggest settings for warnings ( https://github.com/ms-iot/ROSOnWindows/blob/master/Porting/Cookbook.md#all-warnings ) is to include the /WX flag, that will make every warning an error. This may be problematic because if a new version of Visual Studio that introduces a new warning is available, the compilation could fail. Furthermore, it is inconsistent with the corresponding GCC warnings -Wall -Wextra, that do not contain -Werror.

[ooeygui]

Hi @traversaro,
Thank you for the feedback. Please do submit a PR for updates.

Visibility
Please do a pull request to clarify this section. It's one of the more obscure issues when porting, so we'd like to minimize the pain.

Warning as Errors
I updated the documentation to clarify, as well as how to disable specific warnings.

Thank you!

[traversaro]

Thanks for the reply @ooeygui . PR on visibility added in #37 .

[ooeygui]

With your change and mine, I think we have resolved this issue. Please reopen if you believe we need further clarification