[all_packages] Enforce property assignment for compile sdk over method assignment #8897

reidbaker · 2025-03-25T17:06:18Z

No issue I just noticed that we were almost consistent in our approach to compileSdk.

stuartmorgan-g

Tool changes LGTM, but I'm still seeing ~40 violations of this locally.

stuartmorgan-g · 2025-03-25T17:08:43Z

script/tool/lib/src/gradle_check_command.dart

+      if (methodAssignmentLine == null) {
+        printError('${indentation}No compileSdk or compileSdkVersion found.');
+      } else {
+        printError('${indentation}No compileSdk = found. Please use property assignment.');


How about No "compileSdk =" found to make this easier to parse if you don't already know what the issue is?

stuartmorgan-g

LGTM. All comments optional.

stuartmorgan-g · 2025-03-25T18:38:30Z

.ci/legacy_project/all_packages/android/app/build.gradle

@@ -24,7 +24,7 @@ if (flutterVersionName == null) {

 android {
    namespace = "com.example.all_packages"
-    compileSdkVersion flutter.compileSdkVersion


Do we actually want to change this one? If we somehow broke it (maybe that's not even possible, but then... gradle), we would want to know presumably. And this is what the template generated at the time.

I can see the argument for leaving it, it is code that we once generated.
That said what would break is not our code but agp code and I decided removing the last instance of compileSdkVersion was a good idea. If you feel strongly then I can revert this. I do not think gradle check runs against the legacy project so it wont cause a breakage.

That said what would break is not our code but agp code

If that happened we would want to put in a Flutter Fix or an auto-migrator though, presumably? Getting a warning about needing to do that from our CI could be nice.

I decided removing the last instance of compileSdkVersion was a good idea. If you feel strongly then I can revert this.

I would lean toward leaving it the way it was since in general the idea of legacy_project is to make absolutely minimal changes to it, but I'm fine with you making a call on it either way.

I do not think gradle check runs against the legacy project so it wont cause a breakage.

Right, the check runs on individual packages, and won't include this file.

stuartmorgan-g · 2025-03-25T18:39:20Z

packages/flutter_plugin_android_lifecycle/example/android/app/build.gradle



    defaultConfig {
        // TODO: Specify your own unique Application ID (https://developer.android.com/studio/build/application-id.html).
        applicationId "io.flutter.plugins.flutter_plugin_android_lifecycle_example"
-        minSdkVersion flutter.minSdkVersion
+        minSdkVersion = flutter.minSdkVersion
        targetSdkVersion flutter.targetSdkVersion


Why minSdkVersion but not targetSdkVersion?

Is the plan just to wait and fix the assignment type when standardizing on targetSdk?

accidental add, removing. I do want to move us to use equals and minSdk and targetSdk but only with the enforcement to keep it that way.

stuartmorgan-g · 2025-03-25T18:41:16Z

script/tool/lib/src/gradle_check_command.dart

@@ -421,12 +421,21 @@ for more details.''';

  bool _validateCompileSdkUsage(


Do we want to enforce the ndkVersion style too?

Eventually, I reverted the min change but most of the ndk versions were adjacent to the compileSdk so I went ahead and did them. I am willing to revert the ndk changes if you want to enforce it with tooling. That said I dont see a world where ndk tooling is important enough to enforce. I do think I will add enforcement for minSdk and targetSdk.

If we care about it I think we should eventually enforce it, but I don't see any reason to block incidental updates while touching surrounding code on enforcement.

auto-submit · 2025-03-25T19:49:47Z

autosubmit label was removed for flutter/packages/8897, because - The status or check suite Linux_web web_dart_unit_test_wasm_shard_2 master has failed. Please fix the issues identified (or deflake) before re-applying this label.

chunhtai

LGTM

reidbaker · 2025-03-25T21:46:22Z

Failures appear unrelated. Hopefully an updated branch will fix the issue.

auto-submit · 2025-03-25T22:06:38Z

autosubmit label was removed for flutter/packages/8897, because - The status or check suite Linux_web web_dart_unit_test_wasm_shard_2 master has failed. Please fix the issues identified (or deflake) before re-applying this label.

reidbaker · 2025-03-26T18:27:59Z

Blocked on flutter/flutter#165664

ditman · 2025-03-27T03:33:30Z

Clicked "Update branch" because web tests seem to be fixed in master after @stuartmorgan's fix.

auto-submit · 2025-03-27T16:13:52Z

autosubmit label was removed for flutter/packages/8897, because - The status or check suite Mac_arm64 ios_platform_tests_shard_2 master has failed. Please fix the issues identified (or deflake) before re-applying this label.

stuartmorgan-g · 2025-03-28T14:54:27Z

autosubmit label was removed for flutter/packages/8897, because - The status or check suite Mac_arm64 ios_platform_tests_shard_2 master has failed. Please fix the issues identified (or deflake) before re-applying this label.

I'm seeing this same issue on a roller PR. I'm wondering if it's OOB failure from a maps SDK or server change.

ditman · 2025-03-28T20:27:38Z

Looking at the build history for the suite, it seems this was the first build broken like this was on: 2025-03-25 @ 09:55:

https://ci.chromium.org/ui/p/flutter/builders/try/Mac_arm64%20ios_platform_tests_shard_2%20master/20650/overview

Associated to this commit:

347d39a (the branch name according to github was "xcode_16_test" 🔮)

stuartmorgan-g · 2025-04-01T14:16:20Z

Once #8967 lands, merging in main should unblock this.

…over method assignment (flutter/packages#8897)

flutter/packages@07496eb...4a36dc6 2025-04-03 [email protected] [all_packages] Enforce property assignment for compile sdk over method assignment (flutter/packages#8897) 2025-04-03 [email protected] [google_maps_flutter] Fix iOS info window regression (flutter/packages#8939) 2025-04-02 [email protected] Roll Flutter from 05b5e79 to a0b1b32 (37 revisions) (flutter/packages#8985) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…d assignment (flutter#8897) No issue I just noticed that we were almost consistent in our approach to compileSdk.

Enforce property assignment for compile sdk over method assignment

a771c27

reidbaker requested review from stuartmorgan-g and jesswrd March 25, 2025 17:06

reidbaker marked this pull request as ready for review March 25, 2025 17:06

stuartmorgan-g reviewed Mar 25, 2025

View reviewed changes

reidbaker and others added 2 commits March 25, 2025 13:46

Formatting and quotes around compile sdk equals

263d5f6

Merge branch 'main' into r-2025-03-25-property-assignment-compilesdk

b894db1

jesswrd approved these changes Mar 25, 2025

View reviewed changes

fix all instances of compileSdk using method set

44edef5

reidbaker requested review from bparrishMines, camsim99, matanlurey, gmackall, tarrinneal, Piinks, Hixie, gspencergoog, chunhtai, domesticmouse and ditman as code owners March 25, 2025 18:27

github-actions bot added p: extension_google_sign_in_as_googleapis_auth p: palette_generator p: pigeon p: flutter_markdown p: flutter_image p: rfw Remote Flutter Widgets p: go_router p: flutter_adaptive_scaffold p: camera p: espresso labels Mar 25, 2025

reidbaker requested a review from stuartmorgan-g March 25, 2025 18:36

stuartmorgan-g approved these changes Mar 25, 2025

View reviewed changes

reidbaker and others added 2 commits March 25, 2025 14:52

dont change lifecyle minsdkVersion

5ebeea8

leave legacy project alone.

e076a1a

reidbaker added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 25, 2025

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 25, 2025

chunhtai approved these changes Mar 25, 2025

View reviewed changes

Merge branch 'main' into r-2025-03-25-property-assignment-compilesdk

ddd8198

reidbaker added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 25, 2025

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 25, 2025

Merge branch 'main' into r-2025-03-25-property-assignment-compilesdk

87a1d52

reidbaker added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 27, 2025

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 27, 2025

matanlurey removed their request for review April 2, 2025 15:30

Merge branch 'main' into r-2025-03-25-property-assignment-compilesdk

998eb40

reidbaker added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 3, 2025

auto-submit bot merged commit 4a36dc6 into flutter:main Apr 3, 2025
82 checks passed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 3, 2025

4a36dc63f [all_packages] Enforce property assignment for compile sdk …

4404e4e

…over method assignment (flutter/packages#8897)

engine-flutter-autoroll mentioned this pull request Apr 3, 2025

Roll Packages from 07496eb03035 to 4a36dc63f719 (3 revisions) flutter/flutter#166542

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[all_packages] Enforce property assignment for compile sdk over method assignment #8897

[all_packages] Enforce property assignment for compile sdk over method assignment #8897

reidbaker commented Mar 25, 2025

stuartmorgan-g left a comment

stuartmorgan-g Mar 25, 2025

reidbaker Mar 25, 2025

stuartmorgan-g left a comment

stuartmorgan-g Mar 25, 2025

reidbaker Mar 25, 2025

stuartmorgan-g Mar 25, 2025

reidbaker Mar 25, 2025

stuartmorgan-g Mar 25, 2025

reidbaker Mar 25, 2025

stuartmorgan-g Mar 25, 2025

reidbaker Mar 25, 2025 •

edited

Loading

stuartmorgan-g Mar 25, 2025

auto-submit bot commented Mar 25, 2025

chunhtai left a comment

reidbaker commented Mar 25, 2025

auto-submit bot commented Mar 25, 2025

reidbaker commented Mar 26, 2025

ditman commented Mar 27, 2025

auto-submit bot commented Mar 27, 2025

stuartmorgan-g commented Mar 28, 2025

ditman commented Mar 28, 2025

stuartmorgan-g commented Apr 1, 2025

		@@ -421,12 +421,21 @@ for more details.''';

		bool _validateCompileSdkUsage(

[all_packages] Enforce property assignment for compile sdk over method assignment #8897

[all_packages] Enforce property assignment for compile sdk over method assignment #8897

Conversation

reidbaker commented Mar 25, 2025

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reidbaker Mar 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

auto-submit bot commented Mar 25, 2025

chunhtai left a comment

Choose a reason for hiding this comment

reidbaker commented Mar 25, 2025

auto-submit bot commented Mar 25, 2025

reidbaker commented Mar 26, 2025

ditman commented Mar 27, 2025

auto-submit bot commented Mar 27, 2025

stuartmorgan-g commented Mar 28, 2025

ditman commented Mar 28, 2025

stuartmorgan-g commented Apr 1, 2025

reidbaker Mar 25, 2025 •

edited

Loading