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 visibility-hidden to CMake build #1563

Merged
merged 1 commit into from
Sep 11, 2024

Conversation

APokorny
Copy link
Contributor

This should make all symbols that are not marked otherwise have hidden visibility. There still may be exposed symbols if marked with respective attributes.

This should make all symbols that are not marked otherwise have hidden
visibility. There still may be exposed symbols if marked with respective
attributes.
@APokorny APokorny requested a review from a team as a code owner August 26, 2024 11:22
@APokorny
Copy link
Contributor Author

This is to avoid symbol clashes when multiple versions of godot-cpp are loaded through different plugins.
Scons seems to do something similar already,

@dsnopek
Copy link
Collaborator

dsnopek commented Aug 26, 2024

Thanks!

With scons, visibility hidden is the default, but there is an option to change it. Would it be possible to add an option for cmake as well?

@dsnopek dsnopek added bug This has been identified as a bug cmake topic:buildsystem Related to the buildsystem or CI setup cherrypick:4.1 cherrypick:4.2 cherrypick:4.3 labels Aug 26, 2024
@APokorny
Copy link
Contributor Author

You can change the behavior later - even in downstream projects via set_target_properties(<targets...> PROPERTIES CXX_VISIBILITY_PRESET default )

@enetheru
Copy link
Contributor

I think this should be merged. Even though we're aiming for feature and interface parity, even without the option displayed it still adds something missing from the cmake build that is in the scons one. Perfect is the enemy of the good and all. it's trivial to add a cache entry which will show up in 'cmake -LH'

@dsnopek
Copy link
Collaborator

dsnopek commented Sep 11, 2024

Ok, let's merge this and then add an option in a follow-up

@dsnopek dsnopek merged commit aed9b5c into godotengine:master Sep 11, 2024
12 checks passed
@dsnopek
Copy link
Collaborator

dsnopek commented Oct 28, 2024

Cherry-picked for 4.2 in PR #1631

@dsnopek
Copy link
Collaborator

dsnopek commented Oct 28, 2024

Cherry-picked for 4.3 in PR #1632

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This has been identified as a bug cmake topic:buildsystem Related to the buildsystem or CI setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants