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

CMake: Align MSVC runtime (MD[d], MT) options to engine #1647

Merged
merged 1 commit into from
Nov 28, 2024

Conversation

enetheru
Copy link
Contributor

SCons does not link to /MDd or /MTd
If needed in the future a flag can be introduced like the engine code at platforms/windows/detect.py:459

This solves issue #1459

@enetheru enetheru requested a review from a team as a code owner November 23, 2024 08:54
@AThousandShips
Copy link
Member

Please rename your PR to describe what your fix does

@enetheru enetheru changed the title Fix for issue #1459 Remove link options of MDd or MTd from CMake build to match SCons. Nov 23, 2024
@enetheru
Copy link
Contributor Author

Please rename your PR to describe what your fix does

sorry about that.

@enetheru enetheru changed the title Remove link options of MDd or MTd from CMake build to match SCons. CMake: Remove link options of MDd or MTd from CMake build to match SCons. Nov 23, 2024
@dsnopek
Copy link
Collaborator

dsnopek commented Nov 25, 2024

Thanks!

Per this comment from bruvzg, I think this is what we should do.

@enetheru Could you also amend the first line of the commit message to match the PR title? We prefer descriptive messages (versus issue number references) there too

@dsnopek dsnopek added cmake topic:buildsystem Related to the buildsystem or CI setup labels Nov 25, 2024
@dsnopek dsnopek added this to the 4.x milestone Nov 25, 2024
@enetheru
Copy link
Contributor Author

OK so here's the plan I am proposing:

Godot engine uses a flag debug_crt to specify linking to the debug runtime library for MD only, not MT.
link to source

 if env["debug_crt"]:
        # Always use dynamic runtime, static debug CRT breaks thread_local.
        env.AppendUnique(CCFLAGS=["/MDd"])
    else:
        if env["use_static_cpp"]:
            env.AppendUnique(CCFLAGS=["/MT"])
        else:
            env.AppendUnique(CCFLAGS=["/MD"])

godot-cpp doesn't have this option and only differentiates on static_cpp
link to source

        if env["use_static_cpp"]:
            env.Append(CCFLAGS=["/MT"])
        else:
            env.Append(CCFLAGS=["/MD"])

I ammend this PR so that both CMake and SCons reflect the options the engine has.

@enetheru enetheru marked this pull request as draft November 26, 2024 00:47
@enetheru enetheru changed the title CMake: Remove link options of MDd or MTd from CMake build to match SCons. CMake: Align MSVC runtime (MD[d], MT) options to engine Nov 26, 2024
@dsnopek
Copy link
Collaborator

dsnopek commented Nov 26, 2024

Matching Godot's build options sounds good to me!

@enetheru enetheru closed this Nov 27, 2024
@enetheru enetheru deleted the fix#1459 branch November 27, 2024 22:08
@enetheru enetheru restored the fix#1459 branch November 27, 2024 22:14
@enetheru enetheru reopened this Nov 27, 2024
@enetheru
Copy link
Contributor Author

I didnt realise renaming my branch will close this PR.

enetheru added a commit to enetheru/godot-cpp that referenced this pull request Nov 27, 2024
Engine has an option to link to MDd debug_crt
add flag to Scons options
Add flag to CMAKE options
enetheru added a commit to enetheru/godot-cpp that referenced this pull request Nov 27, 2024
Engine has an option to link to MDd debug_crt
add flag to Scons options
Add flag to CMAKE options
enetheru added a commit to enetheru/godot-cpp that referenced this pull request Nov 28, 2024
Engine has an option to link to MDd debug_crt
add flag to SCons options
Add flag to CMAKE options
enetheru added a commit to enetheru/godot-cpp that referenced this pull request Nov 28, 2024
Engine has an option to link to MDd debug_crt
add flag to SCons options
Add flag to CMAKE options
Engine has an option to link to MDd debug_crt
add flag to SCons options
Add flag to CMAKE options
@enetheru enetheru marked this pull request as ready for review November 28, 2024 04:02
@enetheru enetheru requested a review from dsnopek November 28, 2024 04:02
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! Looks good to me :-)

@dsnopek dsnopek merged commit 3a8d7a2 into godotengine:master Nov 28, 2024
11 checks passed
@enetheru enetheru deleted the fix#1459 branch November 28, 2024 15:10
enetheru added a commit to enetheru/godot-cpp that referenced this pull request Nov 28, 2024
Modernise existing cmake options

Merge pull request godotengine#1647 from enetheru/fix#1459

CMake: Align MSVC runtime (MD[d], MT) options to engine
Merge pull request godotengine#1649 from enetheru/fix-backward-break

CMake: fix break for build prior to modernisation PR
enetheru pushed a commit to enetheru/godot-cpp that referenced this pull request Dec 6, 2024
CMake: Align MSVC runtime (MD[d], MT) options to engine
enetheru added a commit to enetheru/godot-cpp that referenced this pull request Dec 30, 2024
Modernise existing cmake options

Merge pull request godotengine#1647 from enetheru/fix#1459

CMake: Align MSVC runtime (MD[d], MT) options to engine
Merge pull request godotengine#1649 from enetheru/fix-backward-break

CMake: fix break for build prior to modernisation PR
CMake: Fix selection of MSVC Runtime compile flags

My last attempt at solving this was not correct. I have left lots of comments in the source detailing the issue as it will effect consumers.

Replace empty EXCLUDE variable with EXCLUDE_FROM_ALL

This was a mistake left over from the modernise PR

CMake: Handle GODOT_DEV_BUILD flag correctly

.dev is added to output artifacts
Warn users for specifying dev_build and Release build config
Update documentation with deviations to SCons
Update debug_symbols handling, its rolled into build config
Cleanup helper variables and comments

 CMake: Enable using clang-cl on windows

detect clang with MSVC frontend using CMAKE_CXX_COMPILER_FRONTEND_VARIANT

Alleviate CMake target name clashes, visibility, and grouping.

Simplify <platform>_generate cmake function signature, as TARGET_ALIAS and TARGET_NAME are still in scope and do not need to be passed.

Add USE_FOLDERS, and FOLDER properties to group targets in VS and XCode

Guard test target with GODOT_ENABLE_TESTING

Tidy Small Issues

remove RPATH_USE_ORIGIN from static libs
add INTERFACE_POSITION_INDEPENDENT_CODE ON
reset GODOT_ARCH property to SYSTEM_ARCH for macos because universal
add MACOSX_RPATH to test target
remove redundant properties from test target
remove redundant and PUBLIC cxx_std_17 compile feature

Privatise compiler options.

CMake: Support using build_profile.json

Python:
- Redirect stdout for get_file_list to prevent stdout pollution
CMake:
- add GODOT_BUILD_PROFILE cache option
- Split file list generation into its own function
- add status and debug messages
Python Snippets in CMake:
- re-format for better readability
Build Profile:
- Add missing VisualInstance3D class to build_profile.json

Add CMake support for XML documentation

Implement generate_doc_source for cmake

Implement generate_doc_source for cmake

CMake: add lto options

add a fake target to expose results of generator expressions

move location of genex debug target so that it picks up the target details properly and allows us to generate for each type.

first attempt

Simplify Code Generation Variables
- use generator expressions
- use math for bits
- simplify if statements

Simplify SYSTEM_NAME generator expression.
previous version created a list, which is semicolon delimited and then string REPLACED the semicolon to create a single string.
New version uses string APPEND to create the string the first time.

Update Target Properties
- Using rpath for a static library is irrelevant, needs to be seet in consumers.
- Add INTERFACE_POSITION_INDEPENDENT_CODE to propogate flag to consumers.

Update macos
- Add TODO for missing options
- Fix formatting
- Set GODOT_ARCH in case of arch being 'universal'
- Simplify the test project when SYSTEM_NAME is Darwin

Refactor common_compiler_flags.cmake
- remove incorrectly propagated target_compile_features
- remove GNU_ prefix on generic compiler version genex's
- remove line breaks for simple genex's
- make most flags private
- comments

Update Comments
- Add TODO items for missing options
- Add missing toolchain managed options commented out
- Homogenise Headings and block comments

replace ad-hock name construction with new GODOT_SUFFIX
- This replicates how SCons does it.
- Modify test project to use GODOT_SUFFIX
- Add GODOT_SUFFIX to libgodot-cpp properties

set web shared lib suffix to bring web builds into line with SCons
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmake topic:buildsystem Related to the buildsystem or CI setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants