Opt into CMake policy CMP0135

[yaakovschectman]

Update the windows app template and migration to use CMP0135 when cmake version is >= 3.24.

Update app templates' and examples' CMakeLists.txt to use cmake_policy(VERSION. flutter/packages#3828 should obviate the need for a migration.

Addresses #116866

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
• I signed the CLA.
• I listed at least one issue that this PR fixes in the description above.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is test-exempt.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[loic-sharma]

Should we migrate all the affected CMakeLists.txt in this repo? Otherwise flutter run will modify these files.

[yaakovschectman]

Can you explain what modifications you expect flutter run to make to which files?

[loic-sharma]

Your change causes flutter run to migrate an app's ./windows/CMakeLists.txt file if it uses the old CMake policy. This repo has many apps that use the old CMake policy: https://github.com/search?q=repo%3Aflutter%2Fflutter+cmake_policy%28SET+CMP0063+NEW%29+path%3A**%2FCMakeLists.txt&type=code

[yaakovschectman]

That link is not showing any results for me, but if you mean to check in the migration applied to existing/example apps in our repo, then good call

[loic-sharma]

I'm a little concerned that this needle isn't conservative enough. For example, imagine I accidentally remove the whitespace before the cmake_policy in an already migrated CMake file:

if(${CMAKE_VERSION} VERSION_LESS "3.24")
- cmake_policy(SET CMP0063 NEW)
+cmake_policy(SET CMP0063 NEW)
else()
  cmake_policy(SET CMP0135 NEW)
endif()

We should make the migration conservative to reduce the likelihood of us corrupting files. I would consider only applying the migration if we get a hit on this entire expected block:

# Explicitly opt in to modern CMake behaviors to avoid warnings with recent
# versions of CMake.
cmake_policy(SET CMP0063 NEW)

Notice the empty line at the end.

[yaakovschectman]

The comments above the statement are not present in the CMakeList.txts of all first party packages. E.g. url_launcher_windows which I have been using to repro the problem does not contain those comments.

[loic-sharma]

It looks like the template changed substantially after cmake_policy(SET CMP0063 NEW) was introduced. Two potential options:

1. We update this migration to recognize two patterns: the old template used to create first party packages and the current template with comments.
2. We update this migration to recognize just the current template with comments.

I'd lean towards doing the first option if feasible.

[yaakovschectman]

I think this is no longer relevant. As of flutter/packages#3828, we will not plan for the same kind of migration

[loic-sharma]

I'm confused. Why don't we need a migration if we modify the app template? (I'd understand why we wouldn't need a migration if we were only updating the plugin template)

[yaakovschectman]

@stuartmorgan suggested updating the template going forward without needing a migration on the linked PR. Do you think you can explain the rationale here for clarity?

[stuartmorgan]

The way I thought this would work when I recommended the migrator was that we could add the new policy setting at the top level, and it would fix everything, so we needed a migrator so that everyone seeing warnings would get fixes.

Since it turns out that this apparently doesn't work (per other comments here), fixing the current set of warnings has to be done in each plugin that is triggering them, and preventing this warning in new plugins is fixed by updating the plugin template.

Even though we can't fix the original issue in the app template, we should update the app template to make it better by setting a VERSION-based cmake_policy, since that is:

• recommended by CMake docs (I missed that when doing the initial policy setting),
• more comprehensive, so that new apps will have all the new policy behaviors, not just these two.

That's not a pressing concern though, so there's no need to migrate existing projects.

[loic-sharma]

Gotcha, thanks for the additional information!

[stuartmorgan]

This logic looks wrong; why are you removing 63 for new versions? Doesn't that just trade one warning for another one?

Is there a reason not to use the simpler cmake-version-based variant that opts into all new behaviors, as I suggested in the issue?

[yaakovschectman]

Am I understanding you correctly that you would want a statement like
cmake_policy(VERSION 2.4...3.25)
as per the cmake_policy docs?

If so, the above statement still produces the CMP0135 warning message.

[stuartmorgan]

Well, this is odd. Not only does that not work in the top-level project, but setting it directly (via CMP0135) in the top level project also doesn't work. I have to set it in the plugin's CMakeLists.txt to eliminate the warning locally (VERSION also works in that context). I thought the setting would be inherited down the build hierarchy by default, and the new scopes would only allow locally overriding what was inherited.

So either I'm missing something about how CMake works here (which is certainly possible), or it's not actually possible to fix this except at the plugin level. (In which case we should add this to the plugin example template, since we use downloads there.)