Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add HIDE_SYMBOLS_BY_DEFAULT option #196

Merged
merged 3 commits into from
Dec 17, 2021

Conversation

scpeters
Copy link
Member

🎉 New feature

Closes #166

Summary

Add cmake option to IgnSetCompilerFlags that will hide symbols by default if they are not explicitly specified as visible. This option is disabled by default to match the current behavior of ign-cmake.

Test it

The option is used in gazebosim/sdformat@3a37dc8 from gazebosim/sdformat#780. To confirm that it works, remove some of the target_sources cmake calls added in gazebosim/sdformat@3a37dc8 and observe the linking errors.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • 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

Add cmake option to IgnSetCompilerFlags that will
hide symbols by default if they are not explicitly
specified as visible. This option is disabled by
default to match the current behavior of ign-cmake.

Signed-off-by: Steve Peters <[email protected]>
@scpeters scpeters requested a review from azeey December 16, 2021 08:36
@github-actions github-actions bot added Gazebo 1️1️ Dependency of Gazebo classic version 11 🌱 garden Ignition Garden 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome labels Dec 16, 2021
ign_filter_valid_compiler_options(
CUSTOM_ALL_FLAGS
-Wall -Wextra -Wno-long-long -Wno-unused-value -Wfloat-equal
-Wshadow -Winit-self -Wswitch-default -Wmissing-include-dirs -pedantic
-fvisibility)
${VISIBILITY_FLAG})

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cmake has a visibility property https://cmake.org/cmake/help/v3.16/prop_tgt/LANG_VISIBILITY_PRESET.html, is there a way to make use of that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call!

should we also set VISIBILITY_INLINES_HIDDEN?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are now setting CMAKE_C_VISIBILITY_PRESET and CMAKE_CXX_VISIBILITY_PRESET as of fd38458

@@ -115,11 +115,18 @@ endmacro()
# Set up compilation flags for GCC or Clang
macro(ign_setup_gcc_or_clang)

option(USE_DEFAULT_VISIBILITY_HIDDEN "Hide symbols by default if they are not explicitly specified as visible" FALSE)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should make this on by default in the next major version. I think many of other ignition packages also assume it's hidden by default, since they are using _VISIBLE macros instead of _HIDDEN macros.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I don't think we should close #166 until it becomes the default.

Add parameter to ign_configure_build that is used to set
CMAKE_C_VISIBILITY_PRESET and CMAKE_CXX_VISIBILITY_PRESET.

Signed-off-by: Steve Peters <[email protected]>
Copy link
Contributor

@azeey azeey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great! I've used abidiff to compared the shared libraries of ign-math and sdformat created before and after this PR and they were no differences.

cmake/IgnSetCompilerFlags.cmake Outdated Show resolved Hide resolved
@@ -115,11 +115,18 @@ endmacro()
# Set up compilation flags for GCC or Clang
macro(ign_setup_gcc_or_clang)

option(USE_DEFAULT_VISIBILITY_HIDDEN "Hide symbols by default if they are not explicitly specified as visible" FALSE)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I don't think we should close #166 until it becomes the default.

Signed-off-by: Steve Peters <[email protected]>
@scpeters scpeters changed the title Add USE_DEFAULT_VISIBILITY_HIDDEN option Add HIDE_SYMBOLS_BY_DEFAULT option Dec 17, 2021
@scpeters scpeters merged commit 3457923 into gazebosim:ign-cmake2 Dec 17, 2021
@scpeters scpeters deleted the use_default_visibility_hidden branch December 17, 2021 07:58
@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-01-10/1228/1

srmainwaring pushed a commit to srmainwaring/gz-cmake that referenced this pull request Mar 1, 2022
Add HIDE_SYMBOLS_BY_DEFAULT parameter to
ign_configure_build that will hide symbols by default
if they are not explicitly specified as visible.
This parameter is used to set CMAKE_C_VISIBILITY_PRESET
and CMAKE_CXX_VISIBILITY_PRESET.

Signed-off-by: Steve Peters <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel 🔮 dome Ignition Dome 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress 🌱 garden Ignition Garden Gazebo 1️1️ Dependency of Gazebo classic version 11
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Library symbols should be hidden by default
4 participants