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

Update cmake_policy in vcpkg #41574

Merged
merged 17 commits into from
Nov 19, 2024
Merged

Update cmake_policy in vcpkg #41574

merged 17 commits into from
Nov 19, 2024

Conversation

gwankyun
Copy link
Contributor

Compatibility with versions of CMake older than 3.10 is deprecated.

Compatibility with versions of CMake older than 3.10 is deprecated.
scripts/buildsystems/vcpkg.cmake Outdated Show resolved Hide resolved
scripts/buildsystems/vcpkg.cmake Outdated Show resolved Hide resolved
@BillyONeal
Copy link
Member

/azp run

@BillyONeal
Copy link
Member

Rerunning due to network mistake last night.

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jimwang118 jimwang118 added the category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly label Oct 16, 2024
@jimwang118 jimwang118 added the info:reviewed Pull Request changes follow basic guidelines label Oct 17, 2024
@BillyONeal BillyONeal added requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. and removed info:reviewed Pull Request changes follow basic guidelines labels Oct 18, 2024
@BillyONeal
Copy link
Member

I think this should be set to 3.16.3
Related: microsoft/vcpkg-docs#415

Tagging vcpkg-team-review to confirm with other maintainers.

@dg0yt
Copy link
Contributor

dg0yt commented Oct 18, 2024

I think this should be set to 3.16.3

Do you want to raise the general minimum CMake version (as avaiable in Ubuntu 20.04)?
(Just asking.)

@BillyONeal
Copy link
Member

Do you want to raise the general minimum CMake version (as avaiable in Ubuntu 20.04)?
(Just asking.)

That's what I'm arguing, yes. The reason we cared about old CMake versions is "work with the CMake the downstream customer is likely to be using on our supported Linuxes", so with Debian 10 "Buster" leaving support the new oldest distro we care about is Ubuntu 20.04.

Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

This is what I concluded from the discussion.
BTW it is not necessary to resync with master all the time.

scripts/buildsystems/vcpkg.cmake Outdated Show resolved Hide resolved
scripts/buildsystems/vcpkg.cmake Outdated Show resolved Hide resolved
@BillyONeal
Copy link
Member

@vicroms @ras0219-msft and I discussed this today and affirm that we want this to say 3.16.

@BillyONeal BillyONeal removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Oct 29, 2024
@BillyONeal BillyONeal marked this pull request as draft October 29, 2024 22:41
@silverqx
Copy link
Contributor

silverqx commented Nov 12, 2024

Can we merge this? The output is really flooded with these warnings, it's hard to develop with them.

Ubuntu 20.04 LTS has this CMake version, there is no need to support versions before v3.16 as no other distro uses a lower version of CMake.

@dg0yt
Copy link
Contributor

dg0yt commented Nov 12, 2024

There is a change requested but not applied yet.

And there is a set of PRs which all trigger world rebuilds, so it might make sense to coordinate them to reduce impact on users.

@silverqx
Copy link
Contributor

silverqx commented Nov 12, 2024

Ok thats reasonable, but what about cherrypick this change only to main? It's not a big deal if not, we will have flooded output for a while.

So I patched it manually/locally, to get rid of these warnings, I'm not arguing anymore, sry 😁

@dg0yt
Copy link
Contributor

dg0yt commented Nov 12, 2024

It is not causing errors, and there is a mitigation: CMake 3.30.

Again, this PR causes a world rebuild for all users: It invalidates all binary artifacts. That's why it shouldn't occur too often. (And that's the reason why I don't simply open or change another PR. It takes two days to pass CI.)

Ping @gwankyun

@patrikhuber
Copy link
Contributor

Looking forward to getting this merged, I just ran a sudo apt update/upgrade on my WSL Ubuntu 24.04, that seemed to have updated CMake to 3.31, and the terminal is now flooded with those warnings, it's hard to find/see anything else.

@gwankyun
Copy link
Contributor Author

It is not causing errors, and there is a mitigation: CMake 3.30.

Again, this PR causes a world rebuild for all users: It invalidates all binary artifacts. That's why it shouldn't occur too often. (And that's the reason why I don't simply open or change another PR. It takes two days to pass CI.)

Ping @gwankyun

OK, I'm not familiar with the PR process.

@dg0yt
Copy link
Contributor

dg0yt commented Nov 12, 2024

OK, I'm not familiar with the PR process.

If I understand correctly, #41574 (comment) says that my suggestions in #41574 (review) should be applied. You can do this right here on this page.

If you want somebody else to continue, it is also okay. Just leave a note.

gwankyun and others added 2 commits November 13, 2024 10:29
Co-authored-by: Kai Pastor <dg0yt@darc.de>
Co-authored-by: Kai Pastor <dg0yt@darc.de>
@gwankyun gwankyun marked this pull request as ready for review November 14, 2024 02:06
@gwankyun gwankyun requested a review from dg0yt November 15, 2024 06:53
Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

LGTM if the only failing port is cmake-user.

cmake-user is a test port, located in scripts/test_ports. It tests standard find_package for the current vcpkg cmake (3.30) and for the minimum supported version. That's why it fails: It still tests with 3.7 binaries, but the toolchain wants 3.16 now.

So this port needs to be updated, too. Either you do it in this PR, or I do it separately. But I don't know if I will have the time to do so before sunday.

@dg0yt
Copy link
Contributor

dg0yt commented Nov 15, 2024

For a minimal fix, you only have to remove cmake 3.7 testing from cmake-user's default features, i.e. these lines:

},
{
"name": "cmake-user",
"default-features": false,
"features": [
"cmake-3-7"
],

This keeps testing with cmake 3.30 while disabling the outdated part.
I can complete remove cmake-3-7 and add cmake-3-16 in a separate PR.

@gwankyun
Copy link
Contributor Author

@microsoft-github-policy-service agree

@Osyotr
Copy link
Contributor

Osyotr commented Nov 16, 2024

There are a few other things (mostly comments) that can be removed after rasing minimum CMake version to 3.16.

# NOTE: to figure out what cmake versions are required for different things,
# grep for `CMake 3`. All version requirement comments should follow that format.

Not sure it's worth restting CI over this.

@jimwang118 jimwang118 added the info:reviewed Pull Request changes follow basic guidelines label Nov 18, 2024
BillyONeal
BillyONeal previously approved these changes Nov 19, 2024
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

I fixed the 3-7 test rather than deactivating it.

@BillyONeal
Copy link
Member

I'm reverting the cmake-user changes because we merged #42203

# Conflicts:
#	scripts/test_ports/cmake-user/portfile.cmake
#	scripts/test_ports/cmake-user/vcpkg.json
@BillyONeal BillyONeal merged commit db49246 into microsoft:master Nov 19, 2024
16 checks passed
msvetkin added a commit to msvetkin/injectx that referenced this pull request Dec 8, 2024
Fixes:
CMake Deprecation Warning at vcpkg/scripts/buildsystems/vcpkg.cmake:40 (cmake_policy):
  Compatibility with CMake < 3.10 will be removed from a future version of
  CMake.

Ref: microsoft/vcpkg#41574
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants