Skip to content

Implement use_static_cpp flag for Linux #1747

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

Merged
merged 1 commit into from
Apr 2, 2025

Conversation

unvermuthet
Copy link
Contributor

Without statically linking libstdc++ you run into backward-compatibility issues when trying to run a GDExtension linked against a newer version of libstdc++, on a machine that runs an older version. This will also let us upgrade CI runner images to the latest version on godot-cpp-template.

Godot's buildsystem already does this. https://github.com/godotengine/godot/blob/fc827bbe256e47e1d7ec6945deb9c1e79724dac9/platform/linuxbsd/detect.py#L499-L506

I'm fumbling in the dark with CMake. Open to letting someone else handle that part.

@unvermuthet
Copy link
Contributor Author

As mentioned in the meeting, this PR will add another use_static_cpp flag. Two will now show up when running scons --help. One with the description for behavior on Windows and one describing the behavior on Linux (new). This is already happening with use_llvm. Adding these options conditionally like Godot's build system does, would require larger changes. For this PR I'd consider adding a note to the new flag description.

@unvermuthet unvermuthet marked this pull request as ready for review March 19, 2025 14:23
@unvermuthet unvermuthet requested a review from a team as a code owner March 19, 2025 14:23
Comment on lines 7 to +8
opts.Add(BoolVariable("use_llvm", "Use the LLVM compiler - only effective when targeting Linux", False))
opts.Add(BoolVariable("use_static_cpp", "Link libgcc and libstdc++ statically for better portability", True))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps also adding - only effective when targeting Linux here makes sense?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure this is worth noting. It's weird that we get the option twice, but I don't think having an extra note in the option makes that any clearer.

Also, I think this is basically a GCC option, and doesn't really have anything to do with Linux. If we were to try to consolidate the options (which I'm not necessarily saying we should), it would probably be to add this option when the compiler is GCC

Honestly, I think I'm personally fine merging this without solving the duplicate option situation. I think we could tackle that in a follow-up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was considering only adding the option once and consolidating the messaging. But there is a difference in behavior. When targeting Windows, it also adds -static in addition to the other two. The help text there is "Link MinGW/MSVC C++ runtime libraries statically".

I agree that it's okay to have them twice.

Copy link
Collaborator

Choose a reason for hiding this comment

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

given that, then have no problems with the changes, just trying to figure out how to approve them now.

@unvermuthet
Copy link
Contributor Author

@enetheru Could you take a look at the CMake part, please?

@enetheru enetheru self-requested a review March 20, 2025 01:21
Copy link
Collaborator

@enetheru enetheru left a comment

Choose a reason for hiding this comment

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

Even though there is duplication of the option, the changes are good.
A follow up consoidating the option and generator expression would be nice.

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!

@unvermuthet unvermuthet force-pushed the linux-use-static-cpp branch from e29e6bc to 58fa69b Compare April 1, 2025 20:22
@unvermuthet unvermuthet force-pushed the linux-use-static-cpp branch from 58fa69b to b0f4190 Compare April 1, 2025 20:35
@dsnopek dsnopek merged commit c2d688b into godotengine:master Apr 2, 2025
11 checks passed
@unvermuthet
Copy link
Contributor Author

This should be considered for the next round of godot-cpp 4.4 cherry picks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants