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

(chore) set min version of cmake in native CmakeLists.txt to surpass Cmake CMP0000 policy warning #93437

Merged
merged 8 commits into from
Oct 14, 2023

Conversation

Neo-vortex
Copy link
Contributor

@Neo-vortex Neo-vortex commented Oct 13, 2023

This closes #93436

Setting the min version for cmake reflecting https://github.com/dotnet/runtime/blob/main/src/native/libs/CMakeLists.txt

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 13, 2023
@ghost
Copy link

ghost commented Oct 13, 2023

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

This closes #93436

Author: Neo-vortex
Assignees: -
Labels:

area-Infrastructure-libraries

Milestone: -

@Neo-vortex
Copy link
Contributor Author

@dotnet-policy-service agree

@ViktorHofer
Copy link
Member

cc @jkoritzinsky as you dealt with minimum cmake versions in our repo in the past

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thank you

Neo-vortex and others added 2 commits October 13, 2023 20:31
hange the minimum version to 3.16 to be consistent with other CMake scripts for System.Globalization.Native/CMakeLists.txt

Co-authored-by: Jeremy Koritzinsky <jkoritzinsky@gmail.com>
….txt


change the minimum version to 3.16 to be consistent with other CMake scripts 
 for System.Globalization.Native/CMakeLists.txt

Co-authored-by: Jeremy Koritzinsky <jkoritzinsky@gmail.com>
@@ -1,3 +1,6 @@
# Required for StaticICULinking
cmake_minimum_required(VERSION 3.16...3.20)
Copy link
Member

Choose a reason for hiding this comment

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

What happens when we have > 3.20 installed? Do we want to restrict on max value or can it just be set to 3.16 minimum?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right.
limiting the max version is probably not what we want to do.
we can set cmake_minimum_required(VERSION 3.16)
fixed in the latest commit

Copy link
Member

Choose a reason for hiding this comment

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

This is not setting a max version. This is setting the version of CMake to set the policy values for. Setting it to 3.20 ensures that for the dotnet/runtime build, we use a consistent set of CMake policy configurations.

@jkotas
Copy link
Member

jkotas commented Oct 13, 2023

How did you come up with 3.16?

It would be better for this version to match the min cmake version used in the rest of the repo.

@am11
Copy link
Member

am11 commented Oct 13, 2023

How did you come up with 3.16?

@jkotas, please expand the comments to see the history. It was a comment from @jkoritzinsky to use the version range 3.16...3.20, on which I commented that maybe we don't need to restrict to the max value. I think some old distros are providing older versions, so it makes sense to lower the requirements of "shipping" cmake script, which are not using any new feature.

@jkoritzinsky
Copy link
Member

3.16 is the minimum required version for the CMake scripts we use to build Apple and Android apps on customer machines (see the CMake templates in AppleAppBuilder and AndroidAppBuilder).

@Neo-vortex
Copy link
Contributor Author

@jkotas
if @jkoritzinsky is right about the min cmake version, probably when can merge this PR as is.

@jkotas
Copy link
Member

jkotas commented Oct 14, 2023

I do not see a strong reason for keeping the Apple/Android builder min cmake version to be in sync with min cmake version for this Linux-specific opt-in option.

I see the following options:

  • Use 3.20 to keep things simple.
  • Use 3.16..3.20. As @jkoritzinsky pointed out, it is important to set 3.20 as the upper bound so that the dotnet/runtime build uses consistent set of policies.
  • Match cmake version that comes with the oldest Linux distro supported by .NET runtime. If it is going Ubuntu 18.04 for .NET 9, it would make it 3.10..3.20.

@am11
Copy link
Member

am11 commented Oct 14, 2023

it is important to set 3.20 as the upper bound so that the dotnet/runtime build uses consistent set of policies.

We don't have any restriction on > 3.20 version. In fact, the minimum version is 3.20.

cmake_minimum_required(VERSION 3.20)

@jkoritzinsky
Copy link
Member

it is important to set 3.20 as the upper bound so that the dotnet/runtime build uses consistent set of policies.

We don't have any restriction on > 3.20 version. In fact, the minimum version is 3.20.

cmake_minimum_required(VERSION 3.20)

One aspect of setting the minimum required as 3.20 in that was is to set the CMake policy defaults to the 3.20 rules, no matter the version of CMake that’s actually running the build. Using the 3.10…3.20 syntax says “3.10 is the minimum required version for the features used. Use the newest version of CMake policies available until 3.20. Then use the 3.20 policy defaults”

@Neo-vortex
Copy link
Contributor Author

it is important to set 3.20 as the upper bound so that the dotnet/runtime build uses consistent set of policies.

We don't have any restriction on > 3.20 version. In fact, the minimum version is 3.20.

cmake_minimum_required(VERSION 3.20)

One aspect of setting the minimum required as 3.20 in that was is to set the CMake policy defaults to the 3.20 rules, no matter the version of CMake that’s actually running the build. Using the 3.10…3.20 syntax says “3.10 is the minimum required version for the features used. Use the newest version of CMake policies available until 3.20. Then use the 3.20 policy defaults”

so 3.10..3.20 is the the best practice here where :
1- builds fail on anything below 3.10
2- anything above 3.20 would use 3.20 policy defaults

@jkotas jkotas merged commit 5233eea into dotnet:main Oct 14, 2023
183 of 187 checks passed
@ghost ghost locked as resolved and limited conversation to collaborators Nov 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure-libraries community-contribution Indicates that the PR has been added by a community member
Projects
None yet
5 participants