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

Applied Gradle Plugins Declaratively for Most Remaining Plugin Example Apps (Part 3) #8037

Merged
merged 11 commits into from
Nov 18, 2024

Conversation

jesswrd
Copy link
Contributor

@jesswrd jesswrd commented Nov 8, 2024

Updated applying gradle plugins from usage of imperative apply to usage of declarative blocks {} apply for 25 remaining plugin example apps. Intending on updating all android example apps under packages.

Here are previous bulk migrations from imperative apply to declarative apply:
#7968
#8019

Previously migrated applying path_provider plugin example app from imperatively to declaratively in this PR here. Tests for changes in the linked PR above.

More information on Flutter Gradle Pluggin Apply here

All instances of minSdkVersion for example plugin apps use flutter.minSdkVersion instead of the hard-coded version #8035.

Note: Not migrating camera, camera_android, camera_android_camerax until integration tests are updated.

Partially addresses #152656

Pre-launch Checklist

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

@jesswrd jesswrd changed the title Applied Gradle Plugins Declaratively for Remaining Plugin Example Apps (Part 3) [WIP] Applied Gradle Plugins Declaratively for Remaining Plugin Example Apps (Part 3) Nov 8, 2024
@jesswrd jesswrd added override: no versioning needed Override the check requiring version bumps for most changes override: no changelog needed Override the check requiring CHANGELOG updates for most changes labels Nov 8, 2024
@jesswrd jesswrd force-pushed the i1152656-migrate-remaining-ex-plugins branch 2 times, most recently from 8dcb7ae to 4bcfa6f Compare November 8, 2024 22:39
android {
namespace 'io.flutter.plugins.cameraexample'
compileSdk flutter.compileSdkVersion


defaultConfig {
applicationId "io.flutter.plugins.cameraexample"
minSdkVersion flutter.minSdkVersion
targetSdkVersion 28
minSdkVersion 21
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this undoing what @stuartmorgan just did in #8035?

Copy link
Contributor Author

@jesswrd jesswrd Nov 8, 2024

Choose a reason for hiding this comment

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

yup nice catch. added back.

@jesswrd jesswrd changed the title [WIP] Applied Gradle Plugins Declaratively for Remaining Plugin Example Apps (Part 3) Applied Gradle Plugins Declaratively for Remaining Plugin Example Apps (Part 3) Nov 8, 2024
@jesswrd jesswrd marked this pull request as ready for review November 8, 2024 23:33
@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.

@stuartmorgan
Copy link
Contributor

test-exempt: code refactor with no semantic change

@jesswrd
Copy link
Contributor Author

jesswrd commented Nov 15, 2024

@camsim99 CI failure is unrelated to current PR. @stuartmorgan Can I land on red?

Here are potentially related issues:

flutter/flutter#154682
flutter/flutter#157556

@jesswrd jesswrd changed the title Applied Gradle Plugins Declaratively for Remaining Plugin Example Apps (Part 3) Applied Gradle Plugins Declaratively for Most Remaining Plugin Example Apps (Part 3) Nov 15, 2024
@matanlurey
Copy link
Contributor

CI failure is unrelated to current PR. Can I land on red?

Drive-by answer: In general I would not ignore CI failures and land on red. In this particular case their might not be any consequences, but it's easy to accidentally make a bad situation worse, often in hidden or complex ways.

If there is a test that is not stable enough (i.e. a re-run doesn't turn it green), as potentially mentioned in the above issues, I'd disable the test for now, and merge this PR with a truly green CI status.

(There are exceptional cases where this guidance might not hold - like a security vulnerability or severe regression where the cost of potentially breaking a test is lower than not landing code, but I wouldn't find updating example apps to be one).

I am also happy to review a PR temporarily disabling a flaky test.

@jesswrd
Copy link
Contributor Author

jesswrd commented Nov 15, 2024

@matanlurey Gotcha, I'll send that PR temporarily disabling the flaky tests. Thanks!

@stuartmorgan
Copy link
Contributor

@camsim99 CI failure is unrelated to current PR.

Is it?

Here are potentially related issues:
[...] flutter/flutter#157556

This PR changes the target SDK to a value >=31, and you are referencing an issue saying the tests break if the target SDK is >=31. So isn't this PR breaking the integration tests?

@stuartmorgan Can I land on red?

No, we don't land on red unless it's to fix the tree.

@jesswrd
Copy link
Contributor Author

jesswrd commented Nov 18, 2024

This PR changes the target SDK to a value >=31, and you are referencing an issue saying the tests break if the target SDK is >=31. So isn't this PR breaking the integration tests?

I have to bump the target sdk version to at least 33 to avoid an error. Will skip related working tests in this PR and add a note in the PR description.

# Conflicts:
#	packages/camera/camera/example/android/app/build.gradle
#	packages/camera/camera_android/example/android/app/build.gradle
#	packages/camera/camera_android_camerax/example/android/app/build.gradle
@jesswrd jesswrd force-pushed the i1152656-migrate-remaining-ex-plugins branch from 29039ca to d11f470 Compare November 18, 2024 16:31
@jesswrd jesswrd requested a review from Hixie as a code owner November 18, 2024 20:01
@github-actions github-actions bot added the p: rfw Remote Flutter Widgets label Nov 18, 2024
@jesswrd jesswrd merged commit afb1aff into flutter:main Nov 18, 2024
77 checks passed
@jesswrd jesswrd deleted the i1152656-migrate-remaining-ex-plugins branch November 18, 2024 21:51
sinyu1012 added a commit to sinyu1012/packages that referenced this pull request Nov 19, 2024
* main:
  [ci] Take screenshot when native drive test is taking longer than 10 minutes (flutter#8050)
  Applied Gradle Plugins Declaratively for Most Remaining Plugin Example Apps (Part 3) (flutter#8037)
  [pigeon]: Bump org.jetbrains.kotlin:kotlin-bom from 1.8.10 to 2.0.21 in /packages/pigeon/platform_tests/test_plugin/android (flutter#7867)
  [gradle]: Bump androidx.test.ext:junit-ktx from 1.1.5 to 1.2.1 in /packages/shared_preferences/shared_preferences_android/android (flutter#8097)
  [pigeon]: Bump org.jetbrains.kotlin:kotlin-gradle-plugin from 2.0.20 to 2.0.21 in /packages/pigeon/platform_tests/test_plugin/android (flutter#7869)
  [gradle]: Bump org.hamcrest:hamcrest from 2.2 to 3.0 in /packages/espresso/android (flutter#8092)
  [pigeon]: Bump io.mockk:mockk from 1.13.12 to 1.13.13 in /packages/pigeon/platform_tests/test_plugin/android (flutter#7868)
  [shared_pref]: Bump io.mockk:mockk from 1.13.12 to 1.13.13 in /packages/shared_preferences/shared_preferences_android/android (flutter#7866)
  Add autosubmit label to dependabot PRs (flutter#8101)
  Roll Flutter from 0e2d55e0e7b2 to b3818f6b5979 (23 revisions) (flutter#8118)
  [gradle]: Bump org.mockito.kotlin:mockito-kotlin from 4.1.0 to 5.4.0 in /packages/interactive_media_ads/android (flutter#8095)
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.

9 participants