pybind11 icon indicating copy to clipboard operation
pybind11 copied to clipboard

Remove cmake `PROPERTIES CXX_VISIBILITY_PRESET "hidden"`

Open rwgk opened this issue 3 years ago • 3 comments

Description

  • Remove cmake PROPERTIES CXX_VISIBILITY_PRESET "hidden"
  • Add PYBIND11_NS_VISIBILITY in pybind11/detail/common.h
  • Use to resolve all "... declared with greater visibility than ..." warnings across all platforms.

TODO:

  • Decide if it is actually desirable to remove PROPERTIES CXX_VISIBILITY_PRESET "hidden" from the cmake files.
  • If not, revert the cmake changes but keep the test changes (so that the tests are compatible with default visibility).
  • Clean up the documentation including FAQ.

Background: Issue #2479

This PR was extracted from PR #4069 [smart_holder]. See commits there.

Suggested changelog entry:


rwgk avatar Jul 16 '22 16:07 rwgk

@henryiii @eacousineau

This PR proves that we do not need PROPERTIES CXX_VISIBILITY_PRESET "hidden" if we do not want to. Note that nothing is changed in the core library, except a tiny factoring to break out PYBIND11_NS_VISIBILITY(...) from PYBIND11_NAMESPACE. All other non-cmake/setup_helpers.py changes are to the tests only.

What is your opinion regarding this question (currently a TODO in the PR description):

  • Decide if it is actually desirable to remove PROPERTIES CXX_VISIBILITY_PRESET "hidden" from the cmake files.

From my perspective, pushing "hidden" as strongly as it is done currently in the documentation and cmake files is more on the harmful than helpful side. Google-internally, after significant back & forth and confusion, we removed -fvisibility=hidden from our BUILD files, for reasons very similar to what @eacousineau reported under #2479.

rwgk avatar Jul 16 '22 17:07 rwgk

I'm down for this! Any chance you were able to check binary size produced before/after this PR for one platform+Python version+impl? I would expect it might increase, but ideally it only increases due to other non-pybind11 symbols being (correctly) exposed.

EricCousineau-TRI avatar Jul 16 '22 18:07 EricCousineau-TRI

Let's wait till after 2.10.1 to decide on this. I know the original design was to do the right thing for most users, and most users are not using the ABI compat, and are not that familiar with symbol visibility. So suddenly asking everyone to start hiding their symbols or the binaries expand is not a very great thing to do. There's also a very easy workaround - a user can simply unset it, which is making the more advanced use case take more configuration, but that's nicer.

I think the corrections on the visibility are good, and we can see how much this tends to affect user code. We could also update the docs recommending users set visibility for a release before we actually pull the plug on the default.

henryiii avatar Aug 08 '22 14:08 henryiii