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

Bump AGP & Gradle versions in legacy project used for testing #5032

Closed
wants to merge 37 commits into from

Conversation

camsim99
Copy link
Contributor

@camsim99 camsim99 commented Sep 28, 2023

The legacy project used in this repo for testing has started to interact poorly with the plugins as they are using much higher AGP & Gradle versions. This PR bumps the versions just high enough to fix test failures caused by trying to upgrade the AGP versions of plugins to 8+, for example this build failure from #4781.

A follow up to @gmackall's findings: #4997 (comment). Also, just FYI I tested this on #4784.

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 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 listed 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.
  • 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.

camsim99 added 30 commits May 1, 2023 16:46
@gmackall
Copy link
Member

gmackall commented Sep 28, 2023

For some timing info (because that will be relevant to if this upgrade is deemed acceptable), these versions were released in April (Gradle) and July (AGP) of 2021.

Related-ish to flutter/flutter#125653.

Also, FYI that we probably won't be able to do anything with any of the dependabot AGP 8.1 upgrades until we land something like this.

@camsim99 camsim99 marked this pull request as ready for review September 28, 2023 22:44
@stuartmorgan
Copy link
Contributor

This PR bumps the versions just high enough to fix test failures caused by trying to upgrade the AGP versions of plugins to 8+, for example this build failure from #4781.

Unfortunately that error message:

Could not get unknown property 'android' for project ':path_provider_android' of type org.gradle.api.Project.

is extremely unhelpful to someone without a non-trivial amount of Gradle knowledge, and/or very specific context about what changed in the plugin to trigger the error (which we have in the context of a PR, but plugin clients will not when all the did is something like flutter pub upgrade, or added a new plugin)

The legacy project test is just a test of what happens if someone who ran flutter create a while ago and hasn't manually updated their Gradle and AGP versions (which many people don't know they need to do). What do we expect clients of our plugins to do when suddenly their project doesn't build and that's the only error message they have? That seems like a very poor experience.

For some timing info (because that will be relevant to if this upgrade is deemed acceptable), these versions were released in April (Gradle) and July (AGP) of 2021.

The problem is that the important thing isn't when the tools themselves were released, but the state of people's projects. If someone has never heard of AGP because it's never been a problem for them before, and suddenly they have a mysterious error message, the fact that they could in theory have updated that part of their project several years ago won't really matter.

Related-ish to flutter/flutter#125653.

I think it's much more "related" than "-ish" :) In particular my comment there:

We should probably have a soft limit, that warns prominently but continues, which we advance ahead of the hard limit, so that people's first warning that they need to update AGP isn't when they update Flutter and suddenly can't build.

In general, my view is that we should not land any change to the legacy project that is not clearly described by the error message that results from not having that change. I.e., I don't think we should land this until there is a framework change (on stable) that would mean that the error message from a PR like https://github.com/flutter/packages/pull/4781 is not just:

Could not get unknown property 'android' for project ':path_provider_android' of type org.gradle.api.Project.

but something like:

The plugin 'path_provider_android' requires a newer version of the Android Gradle Plugin that your project is currently using. Please see https://flutter.dev/some_helpful_page to learn how to update AGP and Gradle.

@camsim99
Copy link
Contributor Author

In general, my view is that we should not land any change to the legacy project that is not clearly described by the error message that results from not having that change. I.e., I don't think we should land this until there is a framework change (on stable) that would mean that the error message from a PR like #4781 is not just:

Could not get unknown property 'android' for project ':path_provider_android' of type org.gradle.api.Project.

but something like:

The plugin 'path_provider_android' requires a newer version of the Android Gradle Plugin that your project is currently using. Please see https://flutter.dev/some_helpful_page to learn how to update AGP and Gradle.

@stuartmorgan Thanks for the further context. That makes perfect sense to me, so I'll give it a go.

@stuartmorgan
Copy link
Contributor

@camsim99 Is there a tracking issue for having some kind of good error message for this case that we can reference here to know when this unblocked?

@camsim99
Copy link
Contributor Author

camsim99 commented Oct 24, 2023

@camsim99 Is there a tracking issue for having some kind of good error message for this case that we can reference here to know when this unblocked?

Ah yes this was on my todo list for today :) flutter/flutter#137181

Will change to draft for now as I'm not currently working on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants