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

[many] Update example android apps to target SDK level 34. #7587

Merged
merged 17 commits into from
Sep 13, 2024

Conversation

yaakovschectman
Copy link
Contributor

@yaakovschectman yaakovschectman commented Sep 5, 2024

Bump up target SDK for each Android example app, bump version and changelog, and as necessary export activities and update permissions.

For each package, I ensured the example app ran on an emulator before and after the upgrade and compared its behavior, as well as running the integration tests to confirm they pass.

flutter/flutter#152929

Pre-launch Checklist

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

@yaakovschectman
Copy link
Contributor Author

Instrumentation tests for camera_android and camera_android_camerax are failing. I also observe that example app integration tests for these packages fail when run locally, but also fail when running on main. I am not yet able to answer with confidence if the failures on this PR are caused by upgrading the targeted SDK, and I intend to figure this out soon.
See flutter/flutter#154682

@yaakovschectman
Copy link
Contributor Author

This is currently blocked on flutter/flutter#154682

@reidbaker
Copy link
Contributor

reidbaker commented Sep 12, 2024

This is currently blocked on flutter/flutter#154682

I think this pr should land without the camera changes or with a smaller jump in target sdk veriosn and let @camsim99 handle updating the target sdk in flutter/flutter#154682.

Please put what testing, if any, did you do besides the automated tests for packages in the pr description.

For context there are a lot of subtle, unlikely to be automated, behavior changes from api 28 to 34 and we want to make sure the example apps help people using the plugin.

@reidbaker
Copy link
Contributor

This is currently blocked on flutter/flutter#154682

I think this pr should land without the camera changes or with a smaller jump in target sdk veriosn and let @camsim99 handle updating the target sdk in flutter/flutter#154682.

Please put what testing, if any, did you do besides the automated tests for packages in the pr description.

This is currently blocked on flutter/flutter#154682

I think this pr should land without the camera changes or with a smaller jump in target sdk veriosn and let @camsim99 handle updating the target sdk in flutter/flutter#154682.

Please put what testing, if any, did you do besides the automated tests for packages in the pr description.

For context there are a lot of subtle, unlikely to be automated, behavior changes from api 28 to 34 and we want to make sure the example apps help people using the plugin.

For a concrete example I don't think our examples use notifications but a new permission was added that is required to post a notification. So if we used notifications, then code would compile but not work and it is unlikely we would have an automated test for notifications since a permission was not required before.

@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.

Comment on lines 1 to 3
## 0.5.1+8

* Updates example app to target SDK 34.
Copy link
Contributor

Choose a reason for hiding this comment

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

I may be wrong about this, but I don't think we need version changes for example app updates @stuartmorgan can you confirm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we get confirmation, I'll update that. In addition, if we do not need version updates, do we also not need CHANGELOG updates?

Copy link
Contributor

Choose a reason for hiding this comment

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

We definitely don't need version bumps for this; the CI knows not to flag for that. Since this is a routine change that's not interesting to clients, it doesn't need a changelog either. I think CI will exempt example app build files, but if the CI check complains you can add the label to override the changelog check and re-run that check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure why right now, but it seems that only flutter_plugin_android_lifecycle is complaining about not having a CHANGELOG change. What/where are you referring to adding an override label?

Copy link
Contributor

Choose a reason for hiding this comment

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

He is referring to a github label for the pr named "override: no changelog needed".

Copy link
Contributor

@reidbaker reidbaker Sep 12, 2024

Choose a reason for hiding this comment

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

See

if (!state.hasChangelogChange && state.needsChangelogChange) {
for how the label is enforced in code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stuartmorgan Do you have any concerns with this PR as it is now?

@reidbaker
Copy link
Contributor

https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#version

Unpublished parts of the example apps appear to not need a version but I dont remember off hand what is published.

@matanlurey matanlurey removed their request for review September 12, 2024 18:52
@yaakovschectman yaakovschectman added the override: no changelog needed Override the check requiring CHANGELOG updates for most changes label Sep 12, 2024
Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

LGTM

@yaakovschectman yaakovschectman merged commit 08614a7 into flutter:main Sep 13, 2024
76 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 16, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 16, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Sep 16, 2024
flutter/packages@330581f...df88c81

2024-09-14 [email protected] [webview_flutter] Improve flaky scroll tests (flutter/packages#7621)
2024-09-13 [email protected] Bump deps (flutter/packages#7357)
2024-09-13 [email protected] [flutter_adaptive_scaffold] Use improved MediaQuery methods (flutter/packages#7565)
2024-09-13 [email protected] [many] Update example android apps to target SDK level 34. (flutter/packages#7587)
2024-09-13 [email protected] [pigeon] adds support for non nullable types in collections (flutter/packages#7547)
2024-09-13 [email protected] [flutter_adaptive_scaffold] Adds additional slot animation parameters (flutter/packages#7411)
2024-09-13 [email protected] Roll Flutter from 303f222 to 2d30fe4 (21 revisions) (flutter/packages#7646)

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],[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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants