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

Remove Explicit Deletion of CMake /GR Flag as 3.20's *CMP0117* by Default Doesn't Add It #96814

Merged
merged 5 commits into from
Jan 17, 2024
Merged

Conversation

ivdiazsa
Copy link
Member

Completes #94354. CMake Version 3.20+ implements a new version of Policy CMP0117. This new version does not add the /GR flag by default to CMAKE_CXX_FLAGS. And this is the behavior we want. However, previous versions of CMake did add it, so we had to manually remove it. This PR removes that deletion as part of the efforts to simplify our CMake build in general.

@ghost
Copy link

ghost commented Jan 10, 2024

Tagging subscribers to this area: @hoyosjs
See info in area-owners.md if you want to be subscribed.

Issue Details

Completes #94354. CMake Version 3.20+ implements a new version of Policy CMP0117. This new version does not add the /GR flag by default to CMAKE_CXX_FLAGS. And this is the behavior we want. However, previous versions of CMake did add it, so we had to manually remove it. This PR removes that deletion as part of the efforts to simplify our CMake build in general.

Author: ivdiazsa
Assignees: -
Labels:

area-Infrastructure-coreclr

Milestone: 9.0.0

@@ -3,6 +3,8 @@ cmake_minimum_required(VERSION 3.20)
# Set the project name
project(CoreCLR)

cmake_policy(SET CMP0117 NEW)
Copy link
Member

Choose a reason for hiding this comment

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

Can you compare the generated build.ninja files between builds if we set the policy or just leave it at its default value?

I think they'll be the same (since the policy defaults are set based on the version in the cmake_minimum_required statement.

Copy link
Member Author

Choose a reason for hiding this comment

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

The Ninja files were the same, so indeed not specifying = set by default. I've sent a new commit with the change removing it.

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jan 16, 2024
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jan 16, 2024
@@ -783,9 +783,6 @@ if (MSVC)
add_compile_options($<$<COMPILE_LANGUAGE:C,CXX>:/GS>) # Explicitly enable the buffer security checks
add_compile_options($<$<COMPILE_LANGUAGE:C,CXX>:/fp:precise>) # Enable precise floating point

# disable C++ RTTI
Copy link
Member

Choose a reason for hiding this comment

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

Let's keep this comment

Copy link
Member

@jkoritzinsky jkoritzinsky left a comment

Choose a reason for hiding this comment

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

LGTM with one nit

@ivdiazsa
Copy link
Member Author

Failure is unrelated and is being tracked in issue #97103, so we can merge this now.

@ivdiazsa ivdiazsa merged commit 3ffa1a9 into dotnet:main Jan 17, 2024
177 of 180 checks passed
tmds pushed a commit to tmds/runtime that referenced this pull request Jan 23, 2024
…efault Doesn't Add It (dotnet#96814)

* Remove /GR flag as new CMake Policy doesn't add it by default.

* Tell CMake to use the new behavior of CMP0117.

* CMake policy is by default set when not specified, so removing the
explicit setting in CMakeLists.txt

* Flag /GR- fix.

* Restored accidentally deleted comment.
@github-actions github-actions bot locked and limited conversation to collaborators Feb 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Development

Successfully merging this pull request may close these issues.

Enable CMake policy CMP0117 and update the corresponding compile options throughout the repo.
2 participants