Repository navigation
Request the user upgrade if the Android SDK version is 24 - #8462
Conversation
mit-mit
left a comment
mit-mit
left a comment
There was a problem hiding this comment.
Thanks John, one questions about the version check.
|
lgtm w/ the version check change |
|
I've updated the message and the test to be < minimumAndroidSdkVersion. PTAL. |
| String get apksignerPath => getBuildToolsPath('apksigner'); | ||
|
|
||
| List<String> validateSdkWellFormed({bool requireApkSigner = true}) { | ||
| if (buildToolsVersion.major < minimumAndroidSdkVersion) { |
There was a problem hiding this comment.
I don't think this is entirely right. The Android SDK and Android SDK Build-Tools are separate installs, are versioned independently, and can be used in different versions:
https://developer.android.com/studio/command-line/index.html
So AFAIK would be better if we had two different min variables, minimumAndroidSdkVersion and minimumAndroidBuildToolsVersion. Initially these can both be set to 25.
There was a problem hiding this comment.
If you read the code in android_sdk.dart you'll see that we only select build tools versions that match the major version number of the platform tools. I'm happy to do the version check against both versions but flutter_tools requires platform version and build tools major version match
There was a problem hiding this comment.
Makes sense for build and platform tools to be in sync. But there are AFAIK actually four artefacts in total: SDK Platform, SDK Build Tools, SDK Platform-tools, and SDK Tools:
https://developer.android.com/studio/intro/update.html#sdk-manager
Must they all four be in sync?
I thought not -- for example, prior to https://github.com/flutter/flutter/pull/8461/files our samples were using SDK 25 but build tools 24.
Or am I totally misunderstanding this?
There was a problem hiding this comment.
You are right that there are four artifacts of the Android SDK but I don't think it matters for this CL. Here is a breakdown of how things work today, what this CL does, and what we might want to do in the future:
Today:
Flutter looks at the platform and build tools versions and selects a version of build tools that has the same major version of the platform. The result of this is that flutter guarantees platformVersion.major == buildToolsVersion.major.
Flutter does not look at platform tools or SDK tools today. Maybe it should, I don't know.
This CL:
Detects if the platform/build tool major versions (they are always the same for Flutter right now) are >= 25.
Future:
In the future we may want to validate version numbers of each of these artifacts individually and may eventually need to build a blacklist / whitelist around version combinations.
There was a problem hiding this comment.
This describes how the legacy build works. We're switching to Gradle-based builds, where flutter tools don't get to select the SDK or build tools versions. Instead, Gradle uses the versions specified in the build.gradle file, so it doesn't really matter what version flutter thinks it has 'selected'.
Gradle will also automatically download the versions of build tools and SDK it needs, so there's no point in checking that they're installed before we call Gradle.
We currently still support the legacy builds, where this check (and the SDK/build tools selection) matters, but once we remove that and switch completely to Gradle (#8455), all we need to check is that the SDK/build tools licenses have been accepted (#8093).
There was a problem hiding this comment.
OK, sounds like we have some later cleanup to do. Logged #8525 for that.
|
Marking this approved now that we have a cleanup bug filed. |
|
Does this still need to make it into the |
|
A this point I think we can just add this to the list of issues to watch out for. And not bother merging it into the branch. |
7 participants
Versionclass.Fixes #7588