-
-
Notifications
You must be signed in to change notification settings - Fork 640
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
Conversation
As mentioned in the meeting, this PR will add another |
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)) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@enetheru Could you take a look at the CMake part, please? |
There was a problem hiding this 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
e29e6bc
to
58fa69b
Compare
58fa69b
to
b0f4190
Compare
This should be considered for the next round of godot-cpp 4.4 cherry picks. |
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.