Re-Structure cmake solution to be closer to the scons solution. #1595
Conversation
enetheru
force-pushed
the
restructure
branch
from
7cabe40
to
b203283
Compare
|
I guess I have to go install msvc now. |
|
I did end up being able to replicate the issue locally by copying the commands from the CI logs, and it turns out to be a fairly obscure issue. These links describe the problem, and current solutions. I did not see any information on a better way. |
|
This pull request purposefully tries to make no changes to the code, since it would be hidden by the re-structure. |
enetheru
force-pushed
the
restructure
branch
2 times, most recently
from
b197d23
to
355e16a
Compare
|
Last update I swear, I just wanted to be really responsible with the build tree. |
|
I want provide more evidence as to how these changes effect (or in this case dont effect) the build. If you perform a diff of the CI build logs minus the timestamps, you get essentially the same exact log with some slight( and i mean very slight) variability between compile order. Build times for MSVC as I couldnt see duration times in the other runners and I didnt want to manually check the log timestamps:
Other CI build times: PR#2271 PR#2270 PR#2265 |
|
There is also this to consider: cmake/GodotCompilerWarnings.cmake → cmake/common_compiler_flags.cmake |
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.
enetheru
force-pushed
the
restructure
branch
from
355e16a
to
2402a04
Compare
There was a problem hiding this comment.
Thanks!
Overall, this looks good to me! I tried to go through the old and new line-by-line, and this does seems mostly like a straight re-organization. Moving us closer to the organization of the scons config is a great idea, because it'll make it easier for contributors to add new features in both places.
I just have the couple notes I put on the docs (together with the issue when attempting to use a separate build directory).
| cmake . | ||
| cmake --build . |
There was a problem hiding this comment.
Do we really want to recommend running cmake . at the top-level like this? My understanding is that cmake folks tend to prefer making a build directory like:
mkdir build
cd build
cmake ..
Also, we have a Makefile at the top-level in the Git repo, so on Linux if you run cmake ., it ends up replacing the Makefile.
So, if we do decide to recommend running cmake like this, we should probably consider removing that Makefile?
There was a problem hiding this comment.
I agree, considering the mess CMake makes it's best to recommend using a subfolder. I commented here that we should pick a specific subfolder name such as cmake_build, put it in the gitignore, and use it in all documentation and CI stuff. #1598 (comment)
| Generate the buildfiles in a sub directory to not clutter the root directory with build files: | ||
|
|
||
| ```shell | ||
| mkdir build && cd build && cmake -G "Unix Makefiles" .. && cmake --build . |
There was a problem hiding this comment.
Ah, I see you have it down here!
Unfortunately, this doesn't seem to work? When I run it, it still generates the Makefile at the top-level, it doesn't put it in the build/ directory - it even says so in its output:
$ cmake -G "Unix Makefiles" ..
-- Configuring done
-- Generating done
-- Build files have been written to: /home/dsnopek/prj/default/godot-cpp
|
I agree, however it is out of scope for this PR. Which is to rearrange what
exists with minimal changes. I can update the modernise branch as that is a
better fit.
…On Fri, Sep 27, 2024, 11:24 PM Aaron Franke ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In doc/cmake.md
<#1595 (comment)>
:
> +cmake .
+cmake --build .
I agree, considering the mess CMake makes it's best to recommend using a
subfolder. I commented here that we should pick a specific subfolder name
such as cmake_build, put it in the gitignore, and use it in all
documentation and CI stuff. #1598 (comment)
<#1598 (comment)>
—
Reply to this email directly, view it on GitHub
<#1595 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AANEDDXWX2P4YODUNEWN6GTZYXLJBAVCNFSM6AAAAABONFGNLSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGMZUG43DCNZQGE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
There was a problem hiding this comment.
I agree, however it is out of scope for this PR. Which is to rearrange what
exists with minimal changes.
Yeah, that makes sense: if those things are broken even before this PR, then we don't need to fix them here.
We should address those things soon, though, because we're documenting a process that is problematic, and another process that seems not to work. If that can be included in PR #1598, that would be really great
|
Cherry-picked for 4.2 in PR #1631 |
|
Cherry-picked for 4.3 in PR #1632 |
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.