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

Cherry-picks for the godot-cpp 4.3 branch - 2nd batch #1632

Merged
merged 13 commits into from
Oct 30, 2024

Conversation

dsnopek
Copy link
Collaborator

@dsnopek dsnopek commented Oct 28, 2024

The 2nd batch of PR's marked with cherrypick:4.3

bruvzg and others added 13 commits October 28, 2024 16:30
This should make all symbols that are not marked otherwise have hidden
visibility. There still may be exposed symbols if marked with respective
attributes.

(cherry picked from commit d18fa92)
changed cache type for api file and api dir to FILEPATH and PATH respectively.
Minor whitespace.
docstring parity

(cherry picked from commit 390a9a5)
This is just a single step, re-arranging the code without actually changing its functionality.

new docs/cmake.md
moved the block of comments from the start of the CMakeLists.txt into the cmake.md file and converted content to markdown.

new cmake/godotcpp.cmake
Moved all exposed options into a new function godotcpp_options()
Moved configuration and generation code into godotcpp_generate()

To get all the options into the godotcpp_options() I changed the logic of GODOT_USE_HOT_RELOAD which I believe is a closer match to scons, that if the options is not set, and the build type is not release, then it defaults to ON.

I msvc builds require the default flags to be modified or it will throw errors. I have added the links to articles in the commit, but its about removing the runtime error checks /RTC1 from the CMAKE_CXX_FLAGS_DEBUG variable. This needs to happen before the files are included.
https://stackoverflow.com/questions/74426638/how-to-remove-rtc1-from-specific-target-or-file-in-cmake
https://discourse.cmake.org/t/how-do-i-remove-compile-options-from-target/5965

Renamed GodotCompilerWarnings.cmake to common_compiler_flags.cmake to match scons

Included files explicitly by path, as we dont need to append to the CMAKE_MODULES_PATH which effects the whole build tree.

This prevents consumers of the library from clobbering the names of the cmake include files and breaking the build.

(cherry picked from commit 2402a04)
and also the default cmake build directory when building in clion cmake-build-*

(cherry picked from commit 9f5daa2)
Visual Studio projects are multi-config projects like Ninja-MultiConfig which means you can't set the configuration at configure time as there are multiple, it always chooses the first one by default when not specified in the build command.

Instead of this:
cmake -DCMAKE_BUILD_TYPE=Release -G"Visual Studio 17 2022" .
cmake --build . --verbose

It should be this
cmake -G"Visual Studio 17 2022" .
cmake --build . --verbose --config Release

Update ci.yml

Because the current build system doesnt use generator expressions for multi config builds, both the CMAKE_BUILD_TYPE and the build --config options need to be set

(cherry picked from commit 07704f8)
This is consistent with Godot upstream.

(cherry picked from commit 4717a78)
Required since Godot 4.3, which is also the first Godot version with
wide WASM gdnative support (previous versions were Chrome-only, and very
brittle).

(cherry picked from commit 78498da)
(cherry picked from commit 83c0f15)
Copy link
Contributor

@Ivorforce Ivorforce left a comment

Choose a reason for hiding this comment

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

It's hard to judge a big PR like this, but I pulled 94d7497 and tried it on my extension. Everything compiled and ran file!

Copy link
Contributor

@paddy-exe paddy-exe left a comment

Choose a reason for hiding this comment

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

LGTM

@dsnopek
Copy link
Collaborator Author

dsnopek commented Oct 30, 2024

@Ivorforce @paddy-exe Thanks for the review! Any chance you could take a look at the Godot 4.2 version too? #1631

It's has a couple less commits than this one, but no additional ones.

@dsnopek dsnopek merged commit 56571dc into godotengine:4.3 Oct 30, 2024
12 checks passed
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 enhancement This is an enhancement on the current functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants