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 path_provider #7822

Merged
merged 18 commits into from
Oct 30, 2024

Conversation

jesswrd
Copy link
Contributor

@jesswrd jesswrd commented Oct 8, 2024

Updated applying gradle plugins from usage of imperative apply to usage of declarative blocks {} apply. Intending on updating all android example apps under packages. Did one more as a proof of concept before doing more.

More information on Flutter Gradle Plugin Apply here

Partially addresses #152656

Pre-launch Checklist

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

@jesswrd jesswrd requested a review from stuartmorgan as a code owner October 8, 2024 17:44
@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

Looks like this change triggers a new check on the existing targetSdkVersion value?

@jesswrd
Copy link
Contributor Author

jesswrd commented Oct 8, 2024

Looks like this change triggers a new check on the existing targetSdkVersion value?

Yeah, I think the current targetSdkVersion is outdated. I'm just going to update it to 35 (most recent one)

@jesswrd jesswrd self-assigned this Oct 23, 2024
@jesswrd jesswrd changed the title [WIP] Applied Gradle Plugins Declaratively for path_provider Applied Gradle Plugins Declaratively for path_provider Oct 23, 2024
@jesswrd jesswrd requested a review from reidbaker October 23, 2024 22:23
@@ -1,5 +1,6 @@
## NEXT

* Updates example app Android compileSdkVersion to 35.
Copy link
Contributor

Choose a reason for hiding this comment

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

This changelog should also mention moving the example to include flutter as a declarative dependency.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we updating the CHANGELOG at all for this PR? Example-build-only changes are exempt; we generally only mention them if they are about something that is directly relevant to clients of the package (e.g., if a new version of Android requires a new permission to use the code in the package, we might mention adding that permission to the example).

Copy link
Contributor

Choose a reason for hiding this comment

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

Because we are changing the example app which is users facing and should be updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's user-facing, but (other than the one file from the example directory shown on pub.dev) it's not directly pub-package-client-facing; if someone flutter pub upgrades and gets a new version of path_provider, nothing in this PR is in the code they will import, and that's generally what we consider as the CHANGELOG audience.

We expect people running the example to check out the repo (in theory they could build and run it directly from their pub cache, but they really shouldn't), and the git history is the record of changes for someone interacting with the repo itself.

The things in the CHANGELOG change here are part of what our written policy explicitly exempts, and we also have not historically put this kind of thing in the CHANGELOG, so it seems odd to include it this time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I misunderstood the policy. @jesswrd sorry for leading you astray but it will make your follow up prs much easier.

@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 Oct 24, 2024
/// configuration without the artifact hub env variable.
/// GP stands for the gradle plugin method of flutter tooling inclusion.
@visibleForTesting
static String exampleSettingsWithoutArtifactHubStringGP = '''
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this defined here? It only seems to be used in tests.

Copy link
Contributor Author

@jesswrd jesswrd Oct 28, 2024

Choose a reason for hiding this comment

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

Moved it to the test file. done.

return documentationPresent &&
artifactRegistryDefined &&
artifactRegistryPluginApplied;
return validArtifactConfiguration;
Copy link
Contributor

Choose a reason for hiding this comment

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

If the only thing is missing is the documentation, before it would print an example of what needed to be included that included the documentation, whereas now it would fail without any actionable message.

Copy link
Contributor Author

@jesswrd jesswrd Oct 28, 2024

Choose a reason for hiding this comment

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

I just tried out only removing only the documentation, and it does include an error message. I have a check for documentationPresent when validArtifactConfiguration is deinfed, and check for !validArtifactConfiguration when an error should be printed. Could you further explain why it would fail?

if (!validArtifactConfiguration && !artifactRegistryPluginAppliedGP) {
printError('Failed Artifact Hub validation. Include the following in '
'example root settings.gradle:\n$exampleSettingsArtifactHubStringGP');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of these lines which I think are a response to this thread.

add documentationPresent to the check for GP method of applying artifact registry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

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 with a couple of small changes.

printError('Include the following in '
'example root settings.gradle:\n$exampleRootSettingsArtifactHubString');
}
else if (!declarativeArtifactRegistryApplied){
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this file needs dart format run on it. (CI currently doesn't check formatting for script/tool, which is a bug.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

final RepositoryPackage example = package.getExamples().first;
writeFakeExampleBuildGradleGP(example,
pluginName: packageName,
// ignore: avoid_redundant_argument_values
Copy link
Contributor

Choose a reason for hiding this comment

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

If we always want to have the tests be explicit about all of their arguments, then let's make the test helper arguments be required instead of having to annotate all of the calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

I dont see the version of the code where includeBuildArtifactHub and includeSettingsDocumentationArtifactHub are required without defaults and the // ignore: avoid_redundant_argument_values are removed. Did you need to push a change?

Copy link
Contributor Author

@jesswrd jesswrd Oct 30, 2024

Choose a reason for hiding this comment

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

I added required and removed the annotations for writeFakeExampleBuildGradleGP but not for writeFakeExampleBuildGradles. Keeping args optional for writeFakeExampleBuildGradles as it is used in unrelated tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that writeFakeExampleBuildGradles will need to be modified in the version of your pr that removes the "or" condition from the older way and the way you are replacing but for now can be left alone.

@jesswrd jesswrd merged commit 4feddff into flutter:main Oct 30, 2024
76 checks passed
@jesswrd jesswrd deleted the apply_path_prov branch October 30, 2024 19:40
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 31, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 31, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 1, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 1, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Nov 1, 2024
flutter/packages@7cc1caa...796afa3

2024-11-01 stuartmorgan@google.com [google_maps_flutter] Update Android for non-nullable generics (flutter/packages#7990)
2024-11-01 stuartmorgan@google.com [various] Minor cleanup in recently imported packages (flutter/packages#7995)
2024-11-01 stuartmorgan@google.com [go_router] Update example app to Kotlin 1.9.0 (flutter/packages#7997)
2024-10-31 engine-flutter-autoroll@skia.org Manual roll Flutter from fe71cad to 0fe6153 (18 revisions) (flutter/packages#7989)
2024-10-31 kevmoo@users.noreply.github.com [vector_garphics] fix execution on the web with WebAssembly (flutter/packages#7991)
2024-10-31 49699333+dependabot[bot]@users.noreply.github.com [url_launcher]: Bump androidx.annotation:annotation from 1.9.0 to 1.9.1 in /packages/url_launcher/url_launcher_android/android (flutter/packages#7986)
2024-10-31 stuartmorgan@google.com [flutter_svg] Initial import (flutter/packages#7944)
2024-10-31 stuartmorgan@google.com [vector_graphics] Initial import (flutter/packages#7941)
2024-10-30 stuartmorgan@google.com [ci] Ensure repo tool is autoformatted (flutter/packages#7963)
2024-10-30 zanderso@users.noreply.github.com Updates path_parsing README.md with a note about Dan (flutter/packages#7949)
2024-10-30 stuartmorgan@google.com [tool] Support third_party for --current-package (flutter/packages#7967)
2024-10-30 stuartmorgan@google.com [tool] Fix third_party dependency overrides (flutter/packages#7966)
2024-10-30 jessiewong401@gmail.com Applied Gradle Plugins Declaratively for `path_provider` (flutter/packages#7822)
2024-10-30 stuartmorgan@google.com [palette_generator] Remove unmanaged code snippet (flutter/packages#7962)
2024-10-30 engine-flutter-autoroll@skia.org Roll Flutter from 42132e8 to fe71cad (12 revisions) (flutter/packages#7961)

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 flutter-ecosystem@google.com 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
jesswrd added a commit that referenced this pull request Nov 4, 2024
…#7968)

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

Previously migrated applying `path_provider` plugin example app from
imperatively to declaratively in this PR
[here](#7822). Tests for changes
in the linked PR above.

More information on Flutter Gradle Pluggin Apply
[here](https://docs.flutter.dev/release/breaking-changes/flutter-gradle-plugin-apply)

Partially addresses
[#152656](flutter/flutter#152656)

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] page, which explains my
responsibilities.
- [x] 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`.)
- [x] I signed the [CLA].
- [x] The title of the PR starts with the name of the package surrounded
by square brackets, e.g. `[shared_preferences]`
- [x] I [linked to at least one issue that this PR fixes] in the
description above.
- [x] I updated `pubspec.yaml` with an appropriate new version according
to the [pub versioning philosophy], or this PR is [exempt from version
changes].
- [x] I updated `CHANGELOG.md` to add a description of the change,
[following repository CHANGELOG style], or this PR is [exempt from
CHANGELOG changes].
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] All existing and new tests are passing.

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

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/packages/blob/main/CONTRIBUTING.md
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md
[relevant style guides]:
https://github.com/flutter/packages/blob/main/CONTRIBUTING.md#style
[CLA]: https://cla.developers.google.com/
[Discord]:
https://github.com/flutter/flutter/blob/master/docs/contributing/Chat.md
[linked to at least one issue that this PR fixes]:
https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#overview
[pub versioning philosophy]: https://dart.dev/tools/pub/versioning
[exempt from version changes]:
https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#version
[following repository CHANGELOG style]:
https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#changelog-style
[exempt from CHANGELOG changes]:
https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#changelog
[test-exempt]:
https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#tests
jesswrd added a commit that referenced this pull request Nov 6, 2024
… (Part 2) (#8019)

Updated applying gradle plugins from usage of imperative apply to usage
of declarative blocks {} apply for 7 plugins. Intending on updating all
android example apps under packages. The first migration was completed
in this PR [here](#7968)

Previously migrated applying `path_provider` plugin example app from
imperatively to declaratively in this PR
[here](#7822). Tests for changes
in the linked PR above.

More information on Flutter Gradle Pluggin Apply
[here](https://docs.flutter.dev/release/breaking-changes/flutter-gradle-plugin-apply)

Partially addresses
[#152656](flutter/flutter#152656)

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] page, which explains my
responsibilities.
- [x] 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`.)
- [x] I signed the [CLA].
- [x] The title of the PR starts with the name of the package surrounded
by square brackets, e.g. `[shared_preferences]`
- [x] I [linked to at least one issue that this PR fixes] in the
description above.
- [x] I updated `pubspec.yaml` with an appropriate new version according
to the [pub versioning philosophy], or this PR is [exempt from version
changes].
- [x] I updated `CHANGELOG.md` to add a description of the change,
[following repository CHANGELOG style], or this PR is [exempt from
CHANGELOG changes].
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] All existing and new tests are passing.

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

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/packages/blob/main/CONTRIBUTING.md
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md
[relevant style guides]:
https://github.com/flutter/packages/blob/main/CONTRIBUTING.md#style
[CLA]: https://cla.developers.google.com/
[Discord]:
https://github.com/flutter/flutter/blob/master/docs/contributing/Chat.md
[linked to at least one issue that this PR fixes]:
https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#overview
[pub versioning philosophy]: https://dart.dev/tools/pub/versioning
[exempt from version changes]:
https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#version
[following repository CHANGELOG style]:
https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#changelog-style
[exempt from CHANGELOG changes]:
https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#changelog
[test-exempt]:
https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#tests
jesswrd added a commit that referenced this pull request Nov 18, 2024
…e Apps (Part 3) (#8037)

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](#7822). Tests for changes
in the linked PR above.

More information on Flutter Gradle Pluggin Apply
[here](https://docs.flutter.dev/release/breaking-changes/flutter-gradle-plugin-apply)

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

Partially addresses
[#152656](flutter/flutter#152656)

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] page, which explains my
responsibilities.
- [x] 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`.)
- [x] I signed the [CLA].
- [x] The title of the PR starts with the name of the package surrounded
by square brackets, e.g. `[shared_preferences]`
- [x] I [linked to at least one issue that this PR fixes] in the
description above.
- [x] I updated `pubspec.yaml` with an appropriate new version according
to the [pub versioning philosophy], or this PR is [exempt from version
changes].
- [x] I updated `CHANGELOG.md` to add a description of the change,
[following repository CHANGELOG style], or this PR is [exempt from
CHANGELOG changes].
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] All existing and new tests are passing.

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

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/packages/blob/main/CONTRIBUTING.md
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md
[relevant style guides]:
https://github.com/flutter/packages/blob/main/CONTRIBUTING.md#style
[CLA]: https://cla.developers.google.com/
[Discord]:
https://github.com/flutter/flutter/blob/master/docs/contributing/Chat.md
[linked to at least one issue that this PR fixes]:
https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#overview
[pub versioning philosophy]: https://dart.dev/tools/pub/versioning
[exempt from version changes]:
https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#version
[following repository CHANGELOG style]:
https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#changelog-style
[exempt from CHANGELOG changes]:
https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#changelog
[test-exempt]:
https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
override: no changelog needed Override the check requiring CHANGELOG updates for most changes override: no versioning needed Override the check requiring version bumps for most changes p: path_provider platform-android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants