Skip to content

[flutter_plugin_android_lifecycle] use flutter.compileSdkVersion to see what breaks #8700

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

Merged
merged 4 commits into from
Feb 28, 2025

Conversation

reidbaker
Copy link
Contributor

@reidbaker reidbaker commented Feb 25, 2025

Related to flutter/flutter/issues/149836

The entire repo uses compile sdk 34 so this a change to 35 but tooling changes are supposed to be safe. Aditionally most projects uses flutter.targetSdkVersion which means that they are using tools at api 34 but targeting 35 which is not ok.

This pr is a canary to see if any unexpected issues pop up and to validate that we agree on if a version/changelog change is required. The second part will update this for all plugins.

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] page, which explains my responsibilities.
  • I read and followed the [relevant style guides] and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the [CLA].
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I [linked to at least one issue that this PR fixes] in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes].
  • I updated CHANGELOG.md to add a description of the change, [following repository CHANGELOG style], or this PR is [exempt from CHANGELOG changes].
  • 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.

@reidbaker reidbaker added the override: no changelog needed Override the check requiring CHANGELOG updates for most changes label Feb 25, 2025
@reidbaker reidbaker marked this pull request as ready for review February 25, 2025 20:12
@reidbaker reidbaker requested review from stuartmorgan-g and gmackall and removed request for stuartmorgan-g February 25, 2025 20:12
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

@reidbaker
Copy link
Contributor Author

test-exempt: Passing builds are the only signal that this is ok to land.

@stuartmorgan-g
Copy link
Contributor

I can never remember, does this value affect plugin clients? Is this the one that is documented not to, but sometimes actually does? (And did we ever get a doc written up about this?)

If it does, this seems dangerous, because people running with a new version of Flutter would get a configuration we have never tested.

This also has the downside that if anything breaks in the future due to compileSdk changing, it will break the roller rather than break an Android-specific PR that bumps the SDKs. That seems like it'll be much more difficult to deal with (and debug the cause of).

@reidbaker
Copy link
Contributor Author

reidbaker commented Feb 25, 2025

can never remember, does this value affect plugin clients? Is this the one that is documented not to, but sometimes actually does? (And did we ever get a doc written up about this?)

@gmackall do you remember I am leaning towards yes. I dont remember writing a doc for packages.
I checked https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md and dont see any guidance.
I did write Android-API-And-Related-Versions.md very recently and this pr is following that advice.

If it does, this seems dangerous, because people running with a new version of Flutter would get a configuration we have never tested.

In general it is supposed to be safe to update the compileSdkVersion immediately. Additionally if this is unsafe we are already using flutter.targetSdkVersion which is way less safe than bumping tooling.
Assuming I am correct that the non example build.gradle files are shipped to users this is actually helpful in one situation. Plugins that have not been updated in a while have older android sdk versions that must be downloaded at build time because they are used. This change will ensure that for more plugins the android version is already downloaded.
See flutter/flutter#63533 (comment) and flutter/flutter#41462 (comment)

This also has the downside that if anything breaks in the future due to compileSdk changing, it will break the roller rather than break an Android-specific PR that bumps the SDKs. That seems like it'll be much more difficult to deal with (and debug the cause of).

I think you are correct here. The flutter.compile sdk version bump happens once per android sdk release which historically is once per year and we control when flutter.compileSdkVersions changes so it should not be an out of band change.

@stuartmorgan-g
Copy link
Contributor

that must be downloaded at build time because they are used

Ah, right. I think that's what I was thinking of when I had the suspicion that this had surprised us at some point.

So we should version and publish this, as it has client benefits.

The flutter.compile sdk version bump happens once per android sdk release which historically is once per year and we control when flutter.compileSdkVersions changes so it should not be an out of band change.

Right, but moving from "this PR that does nothing but changes X breaks things, so clearly changing X is a problem" to "the roller is broken, and now the gardener has to bisect and try to figure out which underlying PR caused it" is not great.

But since this has client benefits, I think that drawback is warranted; there are a whole lot more of them than us :)

@reidbaker
Copy link
Contributor Author

that must be downloaded at build time because they are used

Ah, right. I think that's what I was thinking of when I had the suspicion that this had surprised us at some point.

So we should version and publish this, as it has client benefits.

The flutter.compile sdk version bump happens once per android sdk release which historically is once per year and we control when flutter.compileSdkVersions changes so it should not be an out of band change.

Right, but moving from "this PR that does nothing but changes X breaks things, so clearly changing X is a problem" to "the roller is broken, and now the gardener has to bisect and try to figure out which underlying PR caused it" is not great.

But since this has client benefits, I think that drawback is warranted; there are a whole lot more of them than us :)

Ok and for future readers that hit this pain point you can do something like Math.min(35, flutter.compileSdkVersion) for any plugins that are negatively impacted which can allow for a fast resolution of the roll and then let us address the failures one at a time.

@reidbaker reidbaker removed the override: no changelog needed Override the check requiring CHANGELOG updates for most changes label Feb 25, 2025
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

LGTM

@reidbaker reidbaker added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 27, 2025
@auto-submit auto-submit bot merged commit a3d1f13 into flutter:main Feb 28, 2025
82 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 28, 2025
auto-submit bot pushed a commit that referenced this pull request Feb 28, 2025
…for 35 to support flutter versions prior to 3.27 (#8758)

Fixes #flutter/flutter/issues/164362
Functionally a revert of #8700 which found out what would break.
github-merge-queue bot pushed a commit to flutter/flutter that referenced this pull request Feb 28, 2025
flutter/packages@01d3d5c...70b41e1

2025-02-28 [email protected] [camera_avfoundation] Migrate
tests to Swift - part 3 (flutter/packages#8661)
2025-02-28 [email protected] Do not update patch
versions for `dependabot/github-actions`. (flutter/packages#8697)
2025-02-28 [email protected] [flutter_plugin_android_lifecycle] use
flutter.compileSdkVersion to see what breaks (flutter/packages#8700)
2025-02-28 [email protected]
[webview_flutter_wkwebview] Fixes crash where the native
`AuthenticationChallengeResponse` could not be found for auth requests
(flutter/packages#8707)
2025-02-27 [email protected] Roll Flutter from
1659206 to 2e570ca (75 revisions) (flutter/packages#8731)

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
auto-submit bot pushed a commit that referenced this pull request Mar 3, 2025
…mps minimum flutter version to 3.27 (#8760)

- **[flutter_plugin_android_lifecycle] Uses flutter.compileSdkVersion, bumps minimum flutter version to 3.27 (first version to have support)**
related to flutter/flutter/issues/149836 
Functionally a reland of #8700 after the "revert" in #8758
auto-submit bot pushed a commit that referenced this pull request Mar 6, 2025
- **Updates compileSdk 34 to flutter.compileSdkVersion.**

Related to flutter/flutter/issues/149836
Want to wait a couple of days after 
#8700 lands before landing.
zhouyuanbo pushed a commit to zhouyuanbo/video_player_android_2.8.2 that referenced this pull request Mar 14, 2025
- **Updates compileSdk 34 to flutter.compileSdkVersion.**

Related to flutter/flutter/issues/149836
Want to wait a couple of days after 
flutter/packages#8700 lands before landing.
dko5ki23t pushed a commit to dko5ki23t/google_maps_flutter_improved that referenced this pull request May 24, 2025
- **Updates compileSdk 34 to flutter.compileSdkVersion.**

Related to flutter/flutter/issues/149836
Want to wait a couple of days after 
flutter/packages#8700 lands before landing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: flutter_plugin_android_lifecycle platform-android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants