Allow plugins to use compileSdkPreview #131901

gmackall · 2023-08-03T23:12:43Z

Based (heavily) off #104662

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.

gmackall · 2023-08-04T00:01:23Z

For some interesting context on why the fix was made the way it was for apps, see the discussion here (this PR was closed but the approach that was decided on there was what ended up being merged).

I think it is reasonable to have the same behavior for plugins (especially given android 14 will be releasing soon).

packages/flutter_tools/test/integration.shard/flutter_build_preview_sdk.dart

flutter-dashboard · 2023-08-04T16:40:02Z

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

gmackall · 2023-08-04T18:05:28Z

Fixing the failing tests wasn't hard, but unfortunately in doing so it became clear that the previews don't stick around after they become stable releases.

Meaning if we put "UpsideDownCake", once android 14 is released these tests will start failing. So this isn't a very useful test (and never would have been, even if it ran). I'm trying to find a better way to test, but it doesn't look like we have many great ways to test our gradle/groovy code.

reidbaker · 2023-08-04T18:59:25Z

Fixing the failing tests wasn't hard, but unfortunately in doing so it became clear that the previews don't stick around after they become stable releases.

Meaning if we put "UpsideDownCake", once android 14 is released these tests will start failing. So this isn't a very useful test (and never would have been, even if it ran). I'm trying to find a better way to test, but it doesn't look like we have many great ways to test our gradle/groovy code.

Did you have to update CIPD or another hosted dependency? if so then it might be ok.

gmackall · 2023-08-07T18:33:35Z

Fixing the failing tests wasn't hard, but unfortunately in doing so it became clear that the previews don't stick around after they become stable releases.
Meaning if we put "UpsideDownCake", once android 14 is released these tests will start failing. So this isn't a very useful test (and never would have been, even if it ran). I'm trying to find a better way to test, but it doesn't look like we have many great ways to test our gradle/groovy code.

Did you have to update CIPD or another hosted dependency? if so then it might be ok.

From what I can tell,

if the platform is installed it will use it.
If not, it will use the sdkmanager to install it (which is the part that would eventually fail).

But the recipes have a step where they install android dependencies on the shard running the test, which I think we could modify to include installing the preview version for this step (because it installs from CIPD). Then it should never fallback to 2., and the test should be fine for the future.

…ersions

….ci.yaml

reidbaker

Holding approval for after pr is out of draft and tests are fixed to run in ci. Otherwise looks good

packages/flutter_tools/gradle/src/main/groovy/flutter.groovy

gmackall · 2023-08-08T21:23:13Z

Holding approval for after pr is out of draft and tests are fixed to run in ci. Otherwise looks good

Thanks, and sorry for the constant notification spam - I'm trying to ensure I can get the tests to rely on the version of UDC I uploaded to CIPD, which means pushing updates here and letting tests run (unless you know a better way). Feel free to ignore or unsubscribe, I can ping when its ready for final review.

…commented for debugging purposes, and accept review nit re: comments

reidbaker · 2023-08-22T22:39:48Z

.ci.yaml

      dependencies: >-
        [
-          {"dependency": "android_sdk", "version": "version:33v6"},
+          {"dependency": "android_sdk", "version": "version:UDCv1"},


For the sake of future us can you add a comment to one of these files that UDCv1 is upsideDownCake and link to the cipd dependency directly?

Added a comment explaining

… remove -v that was used for debugging, and change all tests to UDC

gmackall · 2023-08-23T16:10:54Z

This is currently blocked on the Linux ci_yaml flutter roller check, from what I can tell you are able to upload to CIPD using a tag with capital letters, but you can't actually use it in the .ci.yaml, as there is some verification that happens: flutter_main_android_sdk_version_UDCv1" does not match "^[a-z0-9_]+$. I reached out in hackers-infra yesterday afternoon, will see if I get a reply for now

reidbaker · 2023-08-23T16:12:44Z

Consider uploading it again with a different name.

…

On Wed, Aug 23, 2023 at 12:11 PM Gray Mackall ***@***.***> wrote: This is currently blocked on the Linux ci_yaml flutter roller check, from what I can tell you are able to upload to CIPD using a tag with capital letters, but you can't actually use it in the .ci.yaml, as there is some verification that happens: flutter_main_android_sdk_version_UDCv1" does not match "^[a-z0-9_]+$. I reached out in hackers-infra yesterday afternoon, will see if I get a reply for now — Reply to this email directly, view it on GitHub <#131901 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIDVLATROYUDBZHPBTLSGTXWYTRVANCNFSM6AAAAAA3DN5J6I> . You are receiving this because your review was requested.Message ID: ***@***.***>

gmackall · 2023-08-23T16:16:36Z

Yep, I'm planning on doing that this afternoon if I don't get a reply. I'll have to request write access again, as the access I was originally granted was only temporary.

christopherfujino

LGTM

christopherfujino · 2023-12-06T23:00:27Z

I'm not sure where to see the run in ci, after I've marked it bringup? I need to go find that out.

It would be tricky to schedule a LED run (which is the only way to test this on LUCI before merging). I think it's reasonable to just land this, and watch post-submit to verify it passes (if it fails that's fine since it's bringup).

packages/flutter_tools/gradle/src/main/groovy/flutter.groovy

reidbaker · 2023-12-07T00:55:52Z

packages/flutter_tools/gradle/src/main/groovy/flutter.groovy

                Project pluginProject = project.rootProject.findProject(plugin.key)
                pluginProject.afterEvaluate {
-                    int pluginCompileSdkVersion = pluginProject.android.compileSdkVersion.substring(8) as int
+                    // Default to int min if using a preview version to skip the sdk check.


It seems weird to have a comment above setting pluginCompileSdkVersoin to max to skip sdk check and for this to be using min with a comment saying the same thing.

The comment above is setting projectCompileSdkVersion, whereas this is for pluginCompileSdkVersion (or am I misunderstanding your comment?)

reidbaker · 2023-12-07T00:58:22Z

packages/flutter_tools/test/integration.shard/flutter_build_preview_sdk.dart

Can you mark this file as moved in git history?

won't this be normalized anyway when GitHub does the "squash & merge"? by normalized, I'm not sure if the algorithm will detect a move or not, but i think we're at the mercy of that algorithm. Unless my understanding of git is wrong, which it often is :)

From my brief look at stack overflow, the way to get it recognized would be to do it in two separate commits. But this will inevitably be squashed to one on submit, so I'm not actually sure if its possible here

if you do git mv old_path new_path, git will immediately write that the object has moved from one path to another. if you do a mv old_path new_path && git add -A git will use a diffing algorithm to determine whether or not the file was moved or if old_path was deleted and new_path was added. If you change a few lines before doing the git add -A, the diff algorithm might get it wrong.

sorry, this is probably TMI for a drive-by comment :P

reidbaker · 2023-12-07T01:01:31Z

I think we talked about updating the readme for the older tests to warn about needing test files to end in _test.
Also consider adding a readme for "android_preview_integration.shard" that describes what tests should be in the directory.

Finally do you want start this new directory without the _test filtering that the other test repos have?

…id_previews' into 124748_plugin_android_previews

gmackall · 2023-12-07T01:40:51Z

I think we talked about updating the readme for the older tests to warn about needing test files to end in _test. Also consider adding a readme for "android_preview_integration.shard" that describes what tests should be in the directory.

Finally do you want start this new directory without the _test filtering that the other test repos have?

I knew there was something I was forgetting! Done on all points

Roll Flutter from 9719097 to 11a9cb7 (32 revisions) flutter/flutter@9719097...11a9cb7 2023-12-13 [email protected] Make tests more resilient to Skia gold failures and refactor flutter_goldens for extensive technical debt removal (flutter/flutter#139549) 2023-12-12 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 2.22.9 to 2.22.10 (flutter/flutter#140003) 2023-12-12 [email protected] Allow plugins to use compileSdkPreview (flutter/flutter#131901) 2023-12-12 [email protected] Roll pub packages (flutter/flutter#139995) 2023-12-12 [email protected] Select simulator runtime for tests based on Xcode's preferred runtime build (flutter/flutter#139919) 2023-12-12 [email protected] Roll Flutter Engine from 0c527aa1a215 to 9039ac78cf03 (1 revision) (flutter/flutter#139992) 2023-12-12 [email protected] [Docs] Added missing `CupertinoApp.showSemanticsDebugger` (flutter/flutter#139913) 2023-12-12 [email protected] Roll Flutter Engine from 444281eb5c7c to 0c527aa1a215 (2 revisions) (flutter/flutter#139985) 2023-12-12 [email protected] Update Gallery lockfiles for the new version of the video_player plugin (flutter/flutter#139832) 2023-12-12 [email protected] Roll Packages from cb6dbcd to 80aa46a (5 revisions) (flutter/flutter#139982) 2023-12-12 [email protected] Roll Flutter Engine from 3b77b1b7b42f to 444281eb5c7c (1 revision) (flutter/flutter#139979) 2023-12-12 [email protected] Roll Flutter Engine from f8e87ed193f5 to 3b77b1b7b42f (1 revision) (flutter/flutter#139977) 2023-12-12 [email protected] Roll pub packages (flutter/flutter#139969) 2023-12-12 [email protected] Roll Flutter Engine from 4102c7daf1d3 to f8e87ed193f5 (3 revisions) (flutter/flutter#139963) 2023-12-12 [email protected] Roll Flutter Engine from 95dfb1d4ac75 to 4102c7daf1d3 (1 revision) (flutter/flutter#139961) 2023-12-12 [email protected] Roll Flutter Engine from 40bfd2dc1519 to 95dfb1d4ac75 (1 revision) (flutter/flutter#139959) 2023-12-12 [email protected] Roll Flutter Engine from 061ae7023f10 to 40bfd2dc1519 (2 revisions) (flutter/flutter#139958) 2023-12-12 [email protected] Roll Flutter Engine from 75cfb050cd9a to 061ae7023f10 (1 revision) (flutter/flutter#139955) 2023-12-12 [email protected] Roll Flutter Engine from 362d0cb3ab27 to 75cfb050cd9a (1 revision) (flutter/flutter#139954) 2023-12-12 [email protected] Fix dayPeriodColor handling of non-MaterialStateColors (flutter/flutter#139845) 2023-12-12 [email protected] Roll Flutter Engine from ea1a3069e057 to 362d0cb3ab27 (1 revision) (flutter/flutter#139951) 2023-12-12 [email protected] [github actions] Automate Flutter Chery Picks (flutter/flutter#139524) 2023-12-12 [email protected] Roll Flutter Engine from d001419b436e to ea1a3069e057 (5 revisions) (flutter/flutter#139948) 2023-12-12 [email protected] [flutter_tools] catch SocketException writing to ios-deploy stdin (flutter/flutter#139784) 2023-12-11 [email protected] [ci.yaml] Add missing ci.yaml to runIf of android hot reload tests (flutter/flutter#139932) 2023-12-11 [email protected] make the tar c command in prepare_package.dart verbose (flutter/flutter#139687) 2023-12-11 [email protected] Roll Flutter Engine from 5c1f13e1e535 to d001419b436e (4 revisions) (flutter/flutter#139941) 2023-12-11 [email protected] fix typo of 'not' instead of 'now' for `useInheritedMediaQuery` (flutter/flutter#139940) 2023-12-11 [email protected] Roll Flutter Engine from 4c309195b79d to 5c1f13e1e535 (2 revisions) (flutter/flutter#139939) 2023-12-11 [email protected] Implement `switch` expressions in `examples/` and `animation/` (flutter/flutter#139882) 2023-12-11 [email protected] Renamed `appbar` to `app_bar` directory in API Examples Tests (flutter/flutter#139922) 2023-12-11 [email protected] Deprecate `RawKeyEvent`, `RawKeyboard`, et al. (flutter/flutter#136677) 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 Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: 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 ...

…#140099) I added a README at the end of #131901 and did not realize that it was being run as a test, [leading to test failures (of course)](https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8761789207346423409/+/u/run_test.dart_for_android_preview_tool_integration_tests_shard_and_subshard_None/test_stdout). This makes it so we only run dart files as dart tests.

Enables the check that was added in #131901. Has been passing since #140099 (30 runs). Not sure what the normal number of successful runs we wait for is before enabling, let me know if we should wait for more data.

…flutter#140099) I added a README at the end of flutter#131901 and did not realize that it was being run as a test, [leading to test failures (of course)](https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8761789207346423409/+/u/run_test.dart_for_android_preview_tool_integration_tests_shard_and_subshard_None/test_stdout). This makes it so we only run dart files as dart tests.

Enables the check that was added in flutter#131901. Has been passing since flutter#140099 (30 runs). Not sure what the normal number of successful runs we wait for is before enabling, let me know if we should wait for more data.

Gray Mackall added 3 commits March 21, 2023 00:25

lockfile fix

2397dac

fix compilesdk calculation for plugins in same way it was fixed for apps

21d610a

test

db0e14a

github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Aug 3, 2023

gmackall marked this pull request as ready for review August 3, 2023 23:52

gmackall requested a review from a team August 3, 2023 23:52

navaronbracke reviewed Aug 4, 2023

View reviewed changes

packages/flutter_tools/test/integration.shard/flutter_build_preview_sdk.dart Outdated Show resolved Hide resolved

navaronbracke reviewed Aug 4, 2023

View reviewed changes

packages/flutter_tools/test/integration.shard/flutter_build_preview_sdk.dart Outdated Show resolved Hide resolved

fix Tiramusi usages to be UpsideDownCake, and rename test to make it run

a9d0e32

gmackall marked this pull request as draft August 4, 2023 16:39

Gray Mackall added 4 commits August 8, 2023 13:18

change tests to use regexp for matching compileSdkVersion

b44c481

set tests to -v so I can read output in CI (MAKE SURE TO REVERT)

6e192dc

comment out test to see if we still get UpsideDownCake in installed v…

1236d64

…ersions

see if we can have multiple android dependencies to download both in …

674f3da

….ci.yaml

reidbaker reviewed Aug 8, 2023

View reviewed changes

packages/flutter_tools/gradle/src/main/groovy/flutter.groovy Outdated Show resolved Hide resolved

gmackall and others added 2 commits August 22, 2023 16:56

Merge branch 'flutter:master' into 124748_plugin_android_previews

7131141

only set UDC in android_sdk test dependency, uncomment test that was …

c84f31e

…commented for debugging purposes, and accept review nit re: comments

reidbaker reviewed Aug 22, 2023

View reviewed changes

make tests work on windows (path separator), add comment to .ci.yaml,…

184456e

… remove -v that was used for debugging, and change all tests to UDC

reidbaker approved these changes Aug 23, 2023

View reviewed changes

fix comment placement and content

61be6bb

christopherfujino approved these changes Dec 6, 2023

View reviewed changes

Merge branch 'master' into 124748_plugin_android_previews

c184528

reidbaker reviewed Dec 7, 2023

View reviewed changes

Gray Mackall added 2 commits December 6, 2023 17:39

review

0c60003

Merge remote-tracking branch 'refs/remotes/origin/124748_plugin_andro…

4c9970d

…id_previews' into 124748_plugin_android_previews

gmackall added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 12, 2023

auto-submit bot merged commit 9a72e1c into flutter:master Dec 12, 2023

engine-flutter-autoroll mentioned this pull request Dec 13, 2023

Roll Flutter from 97190973d6f5 to 11a9cb7029d7 (32 revisions) flutter/packages#5665

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 13, 2023

9a72e1c Allow plugins to use compileSdkPreview (flutter/flutter#131901)

e93a46e

gmackall mentioned this pull request Dec 13, 2023

Only run dart files as a test in the Android preview tools test shard #140099

Merged

8 tasks

gmackall mentioned this pull request Dec 20, 2023

Enable android_preview_tool_integration_tests check #140470

Merged

8 tasks

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 16, 2024

9a72e1c Allow plugins to use compileSdkPreview (flutter/flutter#131901)

c8c8049

Allow plugins to use compileSdkPreview #131901

Allow plugins to use compileSdkPreview #131901

Uh oh!

Conversation

gmackall commented Aug 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

gmackall commented Aug 4, 2023

Uh oh!

Uh oh!

Uh oh!

flutter-dashboard bot commented Aug 4, 2023

Uh oh!

gmackall commented Aug 4, 2023

Uh oh!

reidbaker commented Aug 4, 2023

Uh oh!

gmackall commented Aug 7, 2023

Uh oh!

reidbaker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gmackall commented Aug 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmackall commented Aug 23, 2023

Uh oh!

reidbaker commented Aug 23, 2023 via email

Uh oh!

gmackall commented Aug 23, 2023

Uh oh!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

christopherfujino commented Dec 6, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reidbaker commented Dec 7, 2023

Uh oh!

gmackall commented Dec 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gmackall commented Aug 3, 2023 •

edited

Loading

gmackall commented Aug 8, 2023 •

edited

Loading