Enable HIDE_SYMBOLS_BY_DEFAULT + patches (take II)

[j-rivero]

🎉 New feature

Part of https://github.com/gazebosim/gz-cmake/pull/392 and https://github.com/gazebosim/gz-cmake/issues/166.

Replaces #600.

Summary

The PR enables the HIDE_SYMBOLS_BY_DEFAULT option and patch the failures found during the build on Linux. Mainly by adding the default visibility behaviour for the custom define on each plugin.

Checklist

• [x] Signed all commits for DCO
• [ ] Added tests
• [ ] Added example and/or tutorial
• [ ] Updated documentation (as needed)
• [ ] Updated migration guide (as needed)
• [ ] Consider updating Python bindings (if the library has them)
• [ ] codecheck passed (See contributing)
• [ ] All tests passed (See test coverage)
• [ ] While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

[j-rivero]

I think that the failures on the doxygen checks are related to https://github.com/doxygen/doxygen/issues/9142. I'm unable to remove them if we add the following to gz-cmake:

diff --git a/doc/doxygen/api.in b/doc/doxygen/api.in
index b3d72b2..cf6b9c4 100644
--- a/doc/doxygen/api.in
+++ b/doc/doxygen/api.in
@@ -2138,7 +2138,7 @@ INCLUDE_FILE_PATTERNS  =
 # recursively expanded use the := operator instead of the = operator.
 # This tag requires that the tag ENABLE_PREPROCESSING is set to YES.

-PREDEFINED             =
+PREDEFINED             = __attribute__(x)=

 # If the MACRO_EXPANSION and EXPAND_ONLY_PREDEF tags are set to YES then this
 # tag can be used to specify a list of macro names that should be expanded. The

[azeey]

It might be quicker to change the names of the defines to be GZ_GUI_VISIBLE since those are filter out https://github.com/gazebosim/gz-cmake/blob/200de1b26edbad9ad1d7efd7c29dd4f2080edefa/doc/doxygen/api.in#L953

[j-rivero]

It might be quicker to change the names of the defines to be GZ_GUI_VISIBLE since those are filter out https://github.com/gazebosim/gz-cmake/blob/200de1b26edbad9ad1d7efd7c29dd4f2080edefa/doc/doxygen/api.in#L953

I'm not sure that we are able to do it, I tried in #600 and stopped that approach. The GZ_GUI_VISIBLE is used for gui libraries to control export/import of the symbols when building but in the PR there are plugins involved that need a different keyword to always import the library symbols.

[azeey]

It might be quicker to change the names of the defines to be GZ_GUI_VISIBLE since those are filter out https://github.com/gazebosim/gz-cmake/blob/200de1b26edbad9ad1d7efd7c29dd4f2080edefa/doc/doxygen/api.in#L953

I'm not sure that we are able to do it, I tried in #600 and stopped that approach. The GZ_GUI_VISIBLE is used for gui libraries to control export/import of the symbols when building but in the PR there are plugins involved that need a different keyword to always import the library symbols.

My suggestion is not to use the GZ_GUI_VISIBLE that is defined in Export.hh, but to use the name GZ_GUI_VISIBLE as the name of the define. Eg. instead of

#ifndef _WIN32
#  define ImageDisplay_EXPORTS_API __attribute__ ((visibility ("default")))
#else
#  if (defined(ImageDisplay_EXPORTS))
#    define ImageDisplay_EXPORTS_API __declspec(dllexport)
#  else
#    define ImageDisplay_EXPORTS_API __declspec(dllimport)
#  endif
#endif

do

#ifndef _WIN32
#  define GZ_GUI_VISIBLE __attribute__ ((visibility ("default")))
#else
#  if (defined(ImageDisplay_EXPORTS))
#    define GZ_GUI_VISIBLE __declspec(dllexport)
#  else
#    define GZ_GUI_VISIBLE __declspec(dllimport)
#  endif
#endif

Would that work?

[j-rivero]

Would that work?

Ah I see. That would not work out of the box since the buildsystem is generating the -D*_EXPORTS_API definition when building the plugins and not using GZ_GUI_VISIBLE as it is currently. We could add it but given that we are going to mix in the compilation plugins and library headers it would be better than them have different keywords to control visibility. Another reason not to reuse the name is the confusion that it can generate in the developers or users since they are going to think that plugin visibility is handle the same way than library/component visibility with the same GZ_GUI_VISIBLE keyword, which is clearly not the case.

[azeey]

Would that work?

Ah I see. That would not work out of the box since the buildsystem is generating the -D*_EXPORTS_API definition when building the plugins and not using GZ_GUI_VISIBLE as it is currently. We could add it but given that we are going to mix in the compilation plugins and library headers it would be better than them have different keywords to control visibility. Another reason not to reuse the name is the confusion that it can generate in the developers or users since they are going to think that plugin visibility is handle the same way than library/component visibility with the same GZ_GUI_VISIBLE keyword, which is clearly not the case.

Okay, let's go with the gz-cmake changes then.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (4c86af8) 70.09% compared to head (570d484) 70.09%.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #601 +/- ## ======================================= Coverage 70.09% 70.09% ======================================= Files 38 38 Lines 5350 5350 ======================================= Hits 3750 3750 Misses 1600 1600 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.