Optimized CMake Build Process #5657

AverageNerdz · 2024-11-05T00:57:55Z

Brief Description of What This PR Does

This PR refines the CMake configuration for our GDExtension, optimizing build performance across platforms. It addresses specific compilation issues on Windows, bringing improvements in speed and reliability it should mantain robust performances on Linux.

Related Issues

Progress Checklist

Note: before starting this checklist the PR should be marked as non-draft.

PR author has checked that this PR works as intended and doesn't
break existing features:
https://wiki.revolutionarygamesstudio.com/wiki/Testing_Checklist
(this is important as to not waste the time of Thrive team
members reviewing this PR)
Initial code review passed (this and further items should not be checked by the PR author)
Functionality is confirmed working by another person (see above checklist link)
Final code review is passed and code conforms to the
styleguide.

Before merging all CI jobs should finish on this PR without errors, if
there are automatically detected style issues they should be fixed by
the PR author. Merging must follow our
styleguide.

hhyyrylainen · 2024-11-05T07:55:22Z

src/extension/CMakeLists.txt

+add_library(thrive_extension SHARED)
+
+# Collect all source files recursively
+file(GLOB_RECURSE EXTENSION_SOURCES 


Again, I really dislike GLOB because it doesn't automatically re-run cmake when new files are added.

if you have another way to do it recursively i'm all ears pal, i added GLOB because it seemed the easier to implement and the one that was pretty easy on the CPUs plus it allows anyone to add anything they want in the folder and just start working on it without editing addittional code other than the interops

The only good way is to manually list each file. That way cmake is forced to re-run when a new file is defined and everything works fine. With GLOB you have to manually re-run cmake whenever you add a file so it is a major pain and you cannot easily see what all gets compiled in. So I'm totally against the use of GLOB at all for defining cmake files to compile.

hhyyrylainen · 2024-11-05T07:56:09Z

third_party/CMakeLists.txt

 set(DOUBLE_PRECISION ON)

 if(THRIVE_AVX)
-  set(USE_AVX ON)
-  set(USE_AVX2 ON)
+    set(USE_AVX ON)


Could you re-indent all the files using the existing formatting rules (two spaces per indentation level)?

hhyyrylainen · 2024-11-05T07:56:56Z

third_party/windows/godot-cpp-windows.cmake

@@ -0,0 +1,219 @@
+# Windows-specific godot-cpp configuration (temporary fix)


I'd like to not accept a temporary fix into the main repo as it'll then just be here forever...

I should have probably dvelved into more an yeah that temporary fix is a stretch but i found out that the main culprit is the cmakelist from the godot-cpp folder, and i don't know if they may release a better cmake which is also suitable for windows uses in the future that is why i put this in the comment

This is going to be really annoying to maintain as each specific godot-cpp update might break our build, and then I wouldn't notice it until someone tries using MSVC the next time and finds it broken.

If at all possible it would be better to try to modify any variables this script uses or modify the resulting targets it exports, so that any forceful customizations could still be kept outside this file (so that we can use the upstream file as-is so that we don't have to forever maintain our separate version).

Though one more alternative would be to open an issue / talk to the godot-cpp repo people and ask if they would accept changes to their file that make it MSVC compatible.

hhyyrylainen · 2024-11-05T08:01:53Z

CMakeLists.txt

+endfunction()
+
+# Source files print function
+function(print_source_files target)


Isn't this like debugging code? I'd remove this.

this helps printing the files that are being compiled in the unity instead of just having the unity being compiled it is really important for the compile imho

Long helper code like this should be moved to a separate file (probably fine to have in the scripts folder like Scripts/CMakeHelperMethods.cmake) for cleaning things up in the main files.

hhyyrylainen · 2024-11-05T08:02:46Z

CMakeLists.txt

-cmake_minimum_required(VERSION 3.10)
+cmake_minimum_required(VERSION 3.13)
+
+set(CMAKE_CONFIGURATION_TYPES "Debug;Release;RelWithDebInfo;Distribution" CACHE STRING "" FORCE)


You should copy the comment from where you moved this code (the reason why the non-standard distribution type is here).

done, i added something to it too :]

I still don't see the comment text about Jolt requiring the Distribution type...

hhyyrylainen · 2024-11-05T08:03:35Z

CMakeLists.txt

+cmake_minimum_required(VERSION 3.13)
+
+set(CMAKE_CONFIGURATION_TYPES "Debug;Release;RelWithDebInfo;Distribution" CACHE STRING "" FORCE)
+set_property(GLOBAL PROPERTY USE_FOLDERS ON)


This should be just removed as this is default behaviour and I think it is unnecessarily complication to be at the top of the file.

hhyyrylainen · 2024-11-05T08:04:17Z

CMakeLists.txt

+# Options
+# Building either the faster variant with AVX or without for older CPU support
+
+# CPU core optimization 


All of this stuff is super long, so I think it should be put into a separate file like Scripts/CMakeTargetConfiguration.cmake and included here.

you mean keeping all the compile as is for Linux and just use .cmakes for windows in the implementation you suggested?

No, I mean putting all of the long code you plan to keep into a separate file so that reading the main cmake file doesn't require you to read through a ton of specific details before getting to the options etc. actually important parts of the file.

hhyyrylainen · 2024-11-05T08:04:46Z

CMakeLists.txt

+# Windows detection function
+function(configure_windows_target target)
+    if(NOT WIN32 OR NOT MSVC)
+        return()


I think this should really be an error if this is called on a different platform.

have you tested it? if you mean the cpu part i would be really glad to know how it is done in Linux i tried to make something that works on both sides and doesn't impact the compile speed but as i don't have those platforms in hand it is difficult to make something without any feedback

This is literally just an early return that seems to not do anything. So if someone accidentally calls this method, then they silently don't get any feedback that nothing happened. That's very unintuitive which is why error conditions should print error messages.

hhyyrylainen · 2024-11-05T08:06:12Z

CMakeLists.txt

+        /wd4273  # Inconsistent dll linkage
+        /wd4141  # 'inline' used more than once
+        # Other warning disables
+        /wd4068  # Unknown pragma


If the thrive code doesn't trigger any of these warnings it would be, much much better to not include these. In general you have made a ton of changes here, whereas I'd prefer to only have the things that are truly, truly needed and not just list everything just in case at it makes it super hard to see what actually is needed.

the problem isn't in the thrive code pal, the problem is in the thousands of files from the godot library which tended to give a lot of warnings and sometimes errors making the compile task quite tedious

Then make it so that the godot library gets added these as public compile flags, that's the modern way to handle this in cmake. I see quite a lot of cmake usage where the old way to do things is used instead of the modern and more clean cmake way.

hhyyrylainen · 2024-11-05T08:07:00Z

CMakeLists.txt

+        -Wextra
+        -Wpedantic
+        -Wno-unknown-pragmas
+        -fPIC


This shouldn't be needed because we use the POSITION_INDEPENDENT_CODE from Cmake which ensures it is correctly set for all compilers cmake supports.

hhyyrylainen · 2024-11-05T08:07:43Z

CMakeLists.txt

+        configure_linux_target(${target})
+    endif()
+
+    target_compile_definitions(${target} PRIVATE


JPH stuff should all be configured in the third party cmakelists folder where that target is imported from, keeps stuff much cleaner.

you mean like putting all this code just in the third party?

Yes, in the third party cmakelists, I've already gathered quite a lot of JPH config there so having it all in one place just makes way too much sense (and when the next time JPH build breaks I'll be able to find all of the stuff related to it easily instead of having to hunt around for it).

hhyyrylainen · 2024-11-05T08:08:08Z

CMakeLists.txt

+    set(THRIVE_ARCH "x64")
+else()
+    set(THRIVE_OS "linux")
+    execute_process(COMMAND uname -m OUTPUT_VARIABLE THRIVE_ARCH OUTPUT_STRIP_TRAILING_WHITESPACE)


Did you add support for compiling as 32-bit? If you didn't I think this line is totally useless.

Did you review the constants that need tweaking for 32-bit? I'm pretty sure I put in TODOs in the precompiled header / defines file that some struct sizes have to be different for 32-bit (and there's that pointer stuffing but I guess that should also work thanks to the alignment). Also I'm not fully sure but I think the C# also needs changes as the struct sizes are different in 32-bit mode. So I think there's at least some bugs with the 32-bit mode.

added error handling on configure_windows_target

removed extra target compile options

hhyyrylainen · 2024-11-07T09:19:00Z

Scripts/CMakeConfiguration.cmake

+
+function(configure_linux_target target)
+    if(WIN32)
+        return()


This is missing an error

hhyyrylainen · 2024-11-07T09:19:48Z

Scripts/CMakeConfiguration.cmake

+    set_target_properties(${target} PROPERTIES
+        POSITION_INDEPENDENT_CODE ON
+        CXX_VISIBILITY_PRESET hidden
+        VISIBILITY_INLINES_HIDDEN ON


Why does inlines have to be forced hidden? This is a non-default option, right? So this might surprise me in the future and cause me to have to debug linker errors for hours if I forget that this flag was snuck in here.

hhyyrylainen · 2024-11-07T09:20:50Z

Scripts/CMakeConfiguration.cmake

+    endif()
+endfunction()
+
+# Function to configure godot-cpp target without modifying its CMakeLists


This stuff is just needed for the third party stuff (right?) so I would totally like to see this only be in the third_party folder cmakelists file.

hhyyrylainen · 2024-11-07T09:22:04Z

src/extension/CMakeLists.txt

-    target_compile_options(thrive_extension PRIVATE /WX)
-  endif()
+# Print files and configure build
+print_source_files(thrive_extension)


I really think it should be a special option to print the source files and not do it by default.

hhyyrylainen · 2024-11-07T09:23:05Z

src/extension/CMakeLists.txt


-  # TODO debug symbols in release for MSVC (and Distribution mode)
+# Common compile definitions for both OS


Any comments etc. referring to Thrive being just for TWO OSes is not going to be good because in the not too distant future all of this stuff also needs to be on mac. For this reason, because I think I have to do it, I don't want this huge set of cmake changes that aren't what I'm used to.

hhyyrylainen · 2024-11-07T09:24:15Z

src/extension/CMakeLists.txt

+    THRIVE_NATIVE_BUILD
+    _HAS_EXCEPTIONS=1
+    JPH_PROFILE_ENABLED=0
+    JPH_DEBUG_RENDERER=0


Why is this here? When compiling in debug mode this has to be defined to one or everything will break (well at least for the thrive native library, but it would be super weird to not have consistent options).

hhyyrylainen · 2024-11-07T09:25:59Z

third_party/CMakeLists.txt

-  set(USE_LZCNT OFF)
-  set(USE_TZCNT OFF)
+    set(DOUBLE_PRECISION OFF)
+    message(WARNING "32-bit build detected, using single precision")


Have you tested that Jolt doesn't allow double precision in 32-bit mode? This seems like an unnecessary difference to me.

Co-authored-by: Henri Hyyryläinen <[email protected]>

- Removed hardcoded version that wouldn't be updated - Reorganized options to top for better visibility - Fixed installation path structure - Proper minimum CMake version handling - Platform plain configuration - Improved debug/release configuration - Added proper dependency handling - Removed the typo in the project function

- Centralized external library handling - Fixed precision settings - Made proper runtime configuration - Build performance optimization - Platform specific handling - Installation organization - Far better warning management

- Platform independent configurations - Fixed linking issues - Debug/Release mode handling - Proper build outputs - Installation organization - Dependency management - Cross-platform support

- Proper error messages - Clear configuration structure - Build performance optimization - Debug support improvement - Maintainable organization - Future platform support

- Old CPU support maintenance - Platform specific optimizations - Proper error handling - Installation organization - Build output management - Performance configurations - Debug symbol handling

AverageNerdz · 2024-11-08T05:05:48Z

I tried to work from the advices you gave me and reworked the environment a little bit, any feedback is appreciated pal :]

AverageNerdz added 7 commits November 4, 2024 20:23

Add files via upload

2ca8153

Update godot-cpp-windows.cmake

8d8e41a

Update CMakeLists.txt

cf599f6

Update CMakeLists.txt

6efe70f

Update CMakeLists.txt

39c8d75

Update CMakeLists.txt

a816afc

Update CMakeLists.txt

16d087b

AverageNerdz changed the title ~~Optimize CMake Build Process~~ Optimized CMake Build Process Nov 5, 2024

hhyyrylainen added the review label Nov 5, 2024

hhyyrylainen added this to the Release 0.8.0 milestone Nov 5, 2024

hhyyrylainen reviewed Nov 5, 2024

View reviewed changes

AverageNerdz added 8 commits November 5, 2024 14:03

Update CMakeLists.txt

f2022e9

Update CMakeLists.txt

1c86977

Update CMakeLists.txt

b0893e0

Update CMakeLists.txt

f9c10bf

Update CMakeLists.txt

b5e3941

added error handling on configure_windows_target

Update CMakeLists.txt

eac3748

removed extra target compile options

Update CMakeLists.txt

ffc6ca7

Add files via upload

6cecb91

hhyyrylainen reviewed Nov 7, 2024

View reviewed changes

Scripts/CMakeConfiguration.cmake Outdated

function(configure_linux_target target)

if(WIN32)

return()

Copy link

Member

hhyyrylainen Nov 7, 2024

Choose a reason for hiding this comment

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

This is missing an error

hhyyrylainen reviewed Nov 7, 2024

View reviewed changes

AverageNerdz and others added 10 commits November 7, 2024 13:42

Updated message status

d82992e

Co-authored-by: Henri Hyyryläinen <[email protected]>

Update wrong project parameter

c93cfd6

Created separate MSCV configuration

bba1d30

Updated CMakeConfiguration.cmake

b50d783

Cleaned up CMakeHelperFunctions.cmake

4fd8316

remade thirdparty integration

514612b

- Centralized external library handling - Fixed precision settings - Made proper runtime configuration - Build performance optimization - Platform specific handling - Installation organization - Far better warning management

Updated extension list

8def9cd

- Platform independent configurations - Fixed linking issues - Debug/Release mode handling - Proper build outputs - Installation organization - Dependency management - Cross-platform support

pretty much explained in root

449f381

- Proper error messages - Clear configuration structure - Build performance optimization - Debug support improvement - Maintainable organization - Future platform support

Update early checks

476f5f9

- Old CPU support maintenance - Platform specific optimizations - Proper error handling - Installation organization - Build output management - Performance configurations - Debug symbol handling

AverageNerdz added 12 commits November 8, 2024 12:57

Merge branch 'master' into cmake_win_fix

2311a5c

Merge branch 'master' into cmake_win_fix

dc72d67

Deleted Scripts/MSVCConfiguration.cmake

2c4b723

optimized script to prevent failure in compile

78afd37

optimized script to prevent failure in compile

4a85d81

Updated problematic root

78657ef

fixed wrong indentation

9e405f1

fixed problematic thirdparty file

4271c83

Updated windows godot-cpp

4bcfa91

Updated problematic native file

b39f840

updated problematic early_checks file

84ab0ec

Updated problematic extension file

f2ac07c

		@@ -0,0 +1,219 @@
		# Windows-specific godot-cpp configuration (temporary fix)


		# TODO debug symbols in release for MSVC (and Distribution mode)
		# Common compile definitions for both OS

Optimized CMake Build Process #5657

Are you sure you want to change the base?

Optimized CMake Build Process #5657

Conversation

AverageNerdz commented Nov 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AverageNerdz commented Nov 8, 2024