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 SYMBOL_VISIBILITY cache variable to match scons interface. #1579

Merged
merged 1 commit into from
Sep 13, 2024

Conversation

enetheru
Copy link
Contributor

Bring the 'cmake -LH' output inline with the scons output.
I dont know what the policy on adding notes in comments are.
I can ammend the commit to remove them if they are unwanted.

@enetheru enetheru requested a review from a team as a code owner September 12, 2024 04:14
Copy link
Collaborator

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks!

I dont know what the policy on adding notes in comments are.
I can ammend the commit to remove them if they are unwanted.

The comment is good.

CMakeLists.txt Outdated
# https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html
# To match the scons options we need to change the text to match the -fvisibility flag
# it is probably worth another PR which changes both to use the flag options
if( ${SYMBOL_VISIBILITY} STREQUAL "auto" OR ${SYMBOL_VISIBILITY} STREQUAL "visible" )
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need to decide if we are going to standardize on having a GODOT_ prefix for all our options (and so GODOT_SYMBOL_VISIBILITY), or not.

What do most other cmake projects do?

Copy link
Contributor

Choose a reason for hiding this comment

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

SFML also prefixes options with SFML_ (unless they're standardized, e.g. CMAKE_BUILD_TYPE).
Some macros also have it: sfml_set_option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes prefixing variables is the norm in cmake projects, would you like me to go back and change it, and then submit a PR bringing other options into line?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That would be great!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Execllent, to be clear we will be prefixing everything with GODOT_ yeah ? not GODOT_CPP_?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, let's do GODOT_ - we're already mostly using that one (with a couple exceptions), and there really isn't a world where you have Godot and godot-cpp in the same codebase, so I don't think we'll conflict

Copy link
Collaborator

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@dsnopek
Copy link
Collaborator

dsnopek commented Sep 13, 2024

Ah, sorry, I didn't notice this when I approved: you need to squash this down to a single commit before I can merge it.

This is part of Godot's development workflow: https://docs.godotengine.org/en/latest/contributing/workflow/pr_workflow.html#modifying-a-pull-request

@enetheru
Copy link
Contributor Author

Apologies, commits have been squashed and I'll remember it for next time.

@dsnopek
Copy link
Collaborator

dsnopek commented Sep 13, 2024

Thanks!

@dsnopek dsnopek merged commit 4131b7f into godotengine:master Sep 13, 2024
12 checks passed
@dsnopek dsnopek added enhancement This is an enhancement on the current functionality cmake cherrypick:4.2 cherrypick:4.3 labels Sep 13, 2024
@enetheru enetheru deleted the visibility-hidden branch September 13, 2024 23:17
@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
cmake enhancement This is an enhancement on the current functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants