-
Notifications
You must be signed in to change notification settings - Fork 27.7k
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
Add Java-Gradle-AGP validation to flutter analyze #123916
Add Java-Gradle-AGP validation to flutter analyze #123916
Conversation
…rapper or system and add a test for each situation
…dle version, update documenation, add tests for agp validation.
…ame from and corner case behavior, create function to validate java and gradle and hardcode return to false
… hardcoded java version
// Update these when new versions of AGP or Gradle come out. | ||
const String maxKnownAgpVersion = '8.1'; | ||
|
||
// TODO are these the correct values we want to support? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open to feedback here.
// Returns the version of java in the format \d(.\d)+(.\d)+ | ||
// Returns null if version not found. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't these be ///
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch fixed.
return parseJavaVersion(result.stdout); | ||
} | ||
|
||
// Extracts jdk version from the output of java --version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
Also, I prefer "JDK" to "jdk" :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed but I doubt I will get all the places updated.
return operatingSystemUtils.which(_javaExecutable)?.path; | ||
final String? pathJava = operatingSystemUtils.which(_javaExecutable)?.path; | ||
if (pathJava != null) { | ||
globals.printTrace('Using path java.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tiny nit ("path java" took me a few seconds to parse&understand than "java from PATH")
globals.printTrace('Using path java.'); | |
globals.printTrace('Using java from PATH.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good readability suggestion. Fixed.
/// Ensures Java Sdk is compatible with the project's gradle version and | ||
/// the project's gradle version is compatible with the AGP version used | ||
/// in build.gradle. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Ensures Java Sdk is compatible with the project's gradle version and | |
/// the project's gradle version is compatible with the AGP version used | |
/// in build.gradle. | |
/// Ensures Java SDK is compatible with the project's Gradle version and | |
/// the project's Gradle version is compatible with the AGP version used | |
/// in build.gradle. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
description = ''' | ||
Incompatible Gradle/AGP versions. \n | ||
Gradle Version: $gradleVersion, AGP Version: $agpVersion | ||
Update gradle to at least "${gradle.getGradleVersionFor(agpVersion!)}".\n |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update gradle to at least "${gradle.getGradleVersionFor(agpVersion!)}".\n | |
Update Gradle to at least "${gradle.getGradleVersionFor(agpVersion!)}".\n |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it'd be nice if all the test cases in this file had the same formatting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. For the life of this cl I was fighting and auto formatter and given its size and some of the edge cases I didnt want a bunch of extra whitespace formatting making the review harder. That said I like individual tests cases instead of tests with dozens of test cases. I ran my formatter on just the lines I added to make sure they are consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general the Flutter repo does not use an auto-formatter:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#in-defense-of-the-extra-work-that-hand-formatting-entails
So if you have an auto-formatter running in your editor, it's probably a good idea to turn it off so that you don't have to fight it.
I ran my formatter on just the lines I added to make sure they are consistent.
Sometimes an auto-formatter makes things less consistent with each other, rather than more consistent. I think this is one of those times. There's three different formats in this test file:
test('pre java 8 format included', () {
expect(isWithinVersionRange('1.0.0_201', min: '1.0.0', max: '1.1.3'),
isTrue);
});
test('min included by default', () {
expect(
isWithinVersionRange('1.0.0', min: '1.0.0', max: '1.1.3'), isTrue);
});
// …
test('inclusive min excluded', () {
expect(
isWithinVersionRange('1.0.0',
min: '1.0.0', max: '1.1.3', inclusiveMin: false),
isFalse);
});
and those look to me like the kind of inconsistency that is typical for an auto-formatter to produce. It'd be better to pick one consistent format for all these test cases.
I think that first format is a good one, with just the isTrue
or isFalse
on a new line. That, or putting the whole thing on one line:
test('pre java 8 format included', () {
expect(isWithinVersionRange('1.0.0_201', min: '1.0.0', max: '1.1.3'), isTrue);
});
@@ -43,25 +44,45 @@ const String minSdkVersion = '16'; | |||
const String targetSdkVersion = '31'; | |||
const String ndkVersion = '23.1.7779620'; | |||
|
|||
final RegExp _androidPluginRegExp = RegExp(r'com\.android\.tools\.build:gradle:(\d+\.\d+\.\d+)'); | |||
// Update this when new versions of Gradle come out. | |||
const String _maxKnownAndSupportedGradleVersion = '8.0'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it also be updated for minor versions like 8.0.2? Might be worth including this info in the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and comment updated.
JavaGradleCompat( | ||
javaMin: '19', | ||
javaMax: '20', | ||
gradleMin: '7.6', | ||
gradleMax: _maxKnownAndSupportedGradleVersion, | ||
), | ||
JavaGradleCompat( | ||
javaMin: '18', | ||
javaMax: '19', | ||
gradleMin: '7.5', | ||
gradleMax: _maxKnownAndSupportedGradleVersion, | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be reasonable to give gradleMax
a default value in the constructor here. Or to have the loop body say max: _maxKnownAndSupportedGradleVersion
instead of max: data.gradleMax
, and just not have a gradleMax
field in the class.
Then this list can get a lot more compact, which I think will make it easier to scan through and spot patterns or possible problems in:
JavaGradleCompat( | |
javaMin: '19', | |
javaMax: '20', | |
gradleMin: '7.6', | |
gradleMax: _maxKnownAndSupportedGradleVersion, | |
), | |
JavaGradleCompat( | |
javaMin: '18', | |
javaMax: '19', | |
gradleMin: '7.5', | |
gradleMax: _maxKnownAndSupportedGradleVersion, | |
), | |
JavaGradleCompat(javaMin: '19', javaMax: '20', gradleMin: '7.6'), | |
JavaGradleCompat(javaMin: '18', javaMax: '19', gradleMin: '7.5'), |
JavaGradleCompat( | ||
javaMin: '1.8', | ||
javaMax: '1.9', | ||
gradleMin: '2.0', | ||
gradleMax: _maxKnownAndSupportedGradleVersion, | ||
), | ||
]; | ||
for (final JavaGradleCompat data in compatList) { | ||
if (isWithinVersionRange(javaV, min: data.javaMin, max: data.javaMax, inclusiveMax: false)) { | ||
return isWithinVersionRange(gradleV, min: data.gradleMin, max: data.gradleMax); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this class exists only for the sake of this list. So you could also skip having the class definition at all, and use the spiffy new Dart "records" feature:
JavaGradleCompat( | |
javaMin: '1.8', | |
javaMax: '1.9', | |
gradleMin: '2.0', | |
gradleMax: _maxKnownAndSupportedGradleVersion, | |
), | |
]; | |
for (final JavaGradleCompat data in compatList) { | |
if (isWithinVersionRange(javaV, min: data.javaMin, max: data.javaMax, inclusiveMax: false)) { | |
return isWithinVersionRange(gradleV, min: data.gradleMin, max: data.gradleMax); | |
{javaMin: '1.8', javaMax: '1.9', gradleMin: '2.0'}, | |
]; | |
for (final {:javaMin, :javaMax, :gradleMin} in compatList) { | |
if (isWithinVersionRange(javaV, min: javaMin, max: javaMax, inclusiveMax: false)) { | |
return isWithinVersionRange(gradleV, min: gradleMin, max: _maxKnownAndSupportedGradleVersion); |
I think that would be quite clean, because it makes these records purely local to this loop — it avoids any worry about whether leaving out a gradleMax
makes the class too specific for its name, or for hypothetical other ways one might use such a class.
// Version can have 2 or 3 numbers. | ||
// 'distributionUrl=https\://services.gradle.org/distributions/gradle-7.4.2-all.zip' | ||
final RegExp distributionUrlRegex = | ||
RegExp(r'distributionUrl\s?=\s?.*\.zip'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RegExp(r'distributionUrl\s?=\s?.*\.zip'); | |
RegExp(r'^\s*distributionUrl\s?=\s?.*\.zip'); |
That way this avoids matching a commented-out line.
if (isWithinVersionRange(agpV, | ||
min: '4.0.0', max: '4.1.0', inclusiveMax: false)) { | ||
return isWithinVersionRange(gradleV, | ||
min: '6.1.1', max: _maxKnownAndSupportedGradleVersion); | ||
} | ||
if (isWithinVersionRange( | ||
agpV, | ||
min: '3.6.0', | ||
max: '3.6.4', | ||
)) { | ||
return isWithinVersionRange(gradleV, | ||
min: '5.6.4', max: _maxKnownAndSupportedGradleVersion); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could benefit from the same loop-through-a-list treatment as in getGradleVersionFor
.
Frob is having issues and the branch cut is today. Merging to get this in prior. |
@gnprice sorry I missed your comments I merged on a non refreshed tab. Will handle them in a follow up pr |
No problem. I've certainly encountered that GitHub issue myself. |
#123917 Missed feedback from #123916 - Handle commented out lines in gradle-wrapper.properties This is 1 of 2 prs to handle missed feedback. Formatting will be done in the second whereas this one captures a weakness missed in the last pr. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [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.
flutter#123917 Doc covering a broad set of issues related to android studio updating. https://docs.google.com/document/d/1hTXkjbUrBnXgu8NQsth1c3aEqo77rWoEj8CcsQ39wwQ/edit?pli=1# Specifically this pr: - Adds new functions to find a projects AGP, Gradle and java versions, and tests. - Adds new functions that take versions and parse if the versions are compatible with each other, and tests. - Adds validator for `flutter analyze --suggestions` that evaluates the java/gradle/agp versions and checks if they are compatible, and integration test. - Updates the version of gradle used by dev/integration_tests/flutter_gallery/ to the minimum supported by java 18 so that the integration tests pass (It is unknown why the java version is 18.9 instead of 11) - Moves `isWithinVersionRange` to version.dart, and tests. - Adds FakeAndroidStudio to fakes to be used in multiple tests but does not remove existing copies. Metrics will be included as part of the definition of done for this bug but not as part of this cl. It is already too big. Known work still left in this pr: * Understand why analyze integration tests are failing. Example output if Java and gradle are not compatible: ``` ┌───────────────────────────────────────────────────────────────────┐ │ General Info │ │ [✓] App Name: espresso_example │ │ [✓] Supported Platforms: android │ │ [✓] Is Flutter Package: yes │ │ [✓] Uses Material Design: yes │ │ [✓] Is Plugin: no │ │ [✗] Java/Gradle/Android Gradle Plugin: │ │ │ │ Incompatible Java/Gradle versions. │ │ │ │ Java Version: 17.0.6, Gradle Version: 7.0.2 │ │ │ │ See the link below for more information. │ │ https://docs.gradle.org/current/userguide/compatibility.html#java │ │ │ └───────────────────────────────────────────────────────────────────┘ ``` Example output if Gradle and AGP are not compatible ``` ┌─────────────────────────────────────────────────────────────────────────────┐ │ General Info │ │ [✓] App Name: espresso_example │ │ [✓] Supported Platforms: android │ │ [✓] Is Flutter Package: yes │ │ [✓] Uses Material Design: yes │ │ [✓] Is Plugin: no │ │ [✗] Java/Gradle/Android Gradle Plugin: Incompatible Gradle/AGP versions. │ │ │ │ Gradle Version: 7.0.2, AGP Version: 7.4.2 │ │ │ │ Update gradle to at least "7.5". │ │ See the link below for more information: │ │ https://developer.android.com/studio/releases/gradle-plugin#updating-gradle │ │ │ │ Incompatible Java/Gradle versions. │ │ │ │ Java Version: 17.0.6, Gradle Version: 7.0.2 │ │ │ │ See the link below for more information: │ │ https://docs.gradle.org/current/userguide/compatibility.html#java │ │ │ └─────────────────────────────────────────────────────────────────────────────┘ ``` Example output if Java/Gradle/Agp are not compatible. ``` ┌─────────────────────────────────────────────────────────────────────────────┐ │ General Info │ │ [✓] App Name: espresso_example │ │ [✓] Supported Platforms: android │ │ [✓] Is Flutter Package: yes │ │ [✓] Uses Material Design: yes │ │ [✓] Is Plugin: no │ │ [✗] Java/Gradle/Android Gradle Plugin: Incompatible Gradle/AGP versions. │ │ │ │ Gradle Version: 7.0.2, AGP Version: 7.4.2 │ │ │ │ Update gradle to at least "7.5". │ │ See the link below for more information: │ │ https://developer.android.com/studio/releases/gradle-plugin#updating-gradle │ │ │ │ Incompatible Java/Gradle versions. │ │ │ │ Java Version: 17.0.6, Gradle Version: 7.0.2 │ │ │ │ See the link below for more information: │ │ https://docs.gradle.org/current/userguide/compatibility.html#java │ │ │ └─────────────────────────────────────────────────────────────────────────────┘ ``` Commit messages - Add function to gradle_utils.dart that gets the gradle version from wrapper or system and add a test for each situation - Add method to get agp version, add method to validate agp against gradle version, update documentation, add tests for agp validation. - Update dart doc for validateGradleAndAgp to describe where the info came from and corner case behavior, create function to validate java and gradle and hardcode return to false - Fill out and test java gradle compatibility function in gradle_utils - Hook up java gradle evaluateion to hasValidJavaGradleAgpVersions with hardcoded java version - Add java --version output parsing and tests - Add getJavaBinary test - Update comment in android_sdk for mac behavior with java_home -v ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [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]. - [ ] All existing and new tests are passing.
flutter#123917 Missed feedback from flutter#123916 - Handle commented out lines in gradle-wrapper.properties This is 1 of 2 prs to handle missed feedback. Formatting will be done in the second whereas this one captures a weakness missed in the last pr. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [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.
@reidbaker Your document allows anyone as an editor, it is better to make it view only |
Thanks I think I had it set to comment access only but now it is view only. |
#123917
Doc covering a broad set of issues related to android studio updating.
https://docs.google.com/document/d/1hTXkjbUrBnXgu8NQsth1c3aEqo77rWoEj8CcsQ39wwQ/edit?pli=1#
Specifically this pr:
flutter analyze --suggestions
that evaluates the java/gradle/agp versions and checks if they are compatible, and integration test.isWithinVersionRange
to version.dart, and tests.Metrics will be included as part of the definition of done for this bug but not as part of this cl. It is already too big.
Known work still left in this pr:
Example output if Java and gradle are not compatible:
Example output if Gradle and AGP are not compatible
Example output if Java/Gradle/Agp are not compatible.
Commit messages
Pre-launch Checklist
///
).