-
Notifications
You must be signed in to change notification settings - Fork 29.7k
[tool][android] Allow --target-platform work properly with --debug mode #154476
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
Conversation
84681c1 to
7b05e4b
Compare
|
FYI @reidbaker based on activity in #153359 |
3ffd017 to
83679e1
Compare
gmackall
left a 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.
This lgtm, but I'd also like to get review from a member of the tools team before merging
83679e1 to
796c687
Compare
796c687 to
46565ba
Compare
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.
LGTM with nitpicks. Thanks!
| usesTrackWidgetCreation(verboseHelp: verboseHelp); | ||
| } | ||
|
|
||
| BuildMode get buildMode { |
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.
If this is only referenced in this file, should we make this private?
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.
Makes sense. Made private this and also targetArchs
| } else if (boolArg('jit-release')) { | ||
| return BuildMode.jitRelease; | ||
| } | ||
| // The build defaults to release. |
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.
Nit: this comment feels redundant to me
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.
Deleted it.
Manual roll requested by [email protected] flutter/flutter@6bba08c...0975e61 2024-10-01 [email protected] [tool][android] Allow --target-platform work properly with --debug mode (flutter/flutter#154476) 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 Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: 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 --------- Co-authored-by: Stuart Morgan <[email protected]>
| static const List<String> _kDefaultJitArchs = <String>[ | ||
| 'android-arm', | ||
| 'android-arm64', | ||
| 'android-x86', |
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.
Hey @Sameri11, I just noticed, that with the latest Flutter Beta 3.27.0 which includes this PR, the x86 platform is by default included when running flutter build apk --debug. This was not the case, prior this PR and let our ci builds fail. Here is the issue.
Was this decision intentional or by accident?
As a workaround we now append --target-platform=android-64 and everything works as expected.
I just wanted to let you know and ask for the reasons of this change.
Thanks in advance.
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.
Actually, x86 was included into debug builds by default before these changes, as you can see here, for example: https://github.com/flutter/flutter/pull/154476/files#diff-d6576605592ef666ab7c75963cce03509e2ff46a3db735e4372754c9f9809681L37
Comment indicates that x86 was intentionally included along with x86_64, and issue was about to not include them if --target-platform specified. So, I guess in this case, this behaviour was preserved: if --target-platform is not set – apk will contain all 4 arches (3 for release mode).
In issue you linked, I can see that, despite flutter build apk --debug being called, release variant is being configured along with debug in gradle. This behaviour makes me think that there's something wrong with gradle configuration in that project.
But, I can't link these changes to the issue you provided right away, will need to investigate.
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.
Thank you @Sameri11 for the fast and detailed answer. We will investigate this in our project.
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.
@Sameri11 that's how the android gradle plugin works, even if you call assembleDebug it will still run the configuration phase for all variants (that is, release and debug by default), because it has to calculate the task graph and whatnot and is variant-agnostic at this stage of the build.
If I'm reading it right, the offending change is this line: https://github.com/flutter/flutter/pull/154476/files#diff-679494ea4187d01a5fa3d1272e537bc58c1f504cd407d99718be5e1fd49b3c93L51
It used to default to the 3 platforms when --target-platform was not specified, but now it defaults to 4 and that is what breaks.
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.
Hi @romtsn ! You're right about configuration phase, my bad. What I tried to say is that, for some reason, there is attempt to resolve dependencies during configuration time in getsentry example project, which is an error in many cases (but not always, though). You can see gradle warns about it if run build with --verbose flag.
As for "default to the 3 platforms before" – it's partially true because this line indicated that, but there was special logic in gradle build itself here, this logic added x86 in debug builds and, in the end, with flutter build apk --debug you got all 4 arches.
The line you pointing to as an offending sets default architectures for debug build. Further this file, there is also a list for aot architectures, which doesn't contain x86 – these defaults are used for release builds. So x86 was never meant to make it to release build.
What makes me think that error is the combination of Flutter build and androidNativeBundle is these lines from issue in getsentry:
> Could not resolve all dependencies for configuration ':app:releaseCompileClasspath'.
> Could not find io.flutter:x86_release:1.0.0-af0f0d559c8a87d912a20971bbd84afc80a54b0f.Question here is how x86 even get to releaseCompileClasspath.
My best guess is that in previous Flutter versions, x86 was added to desired archs later and inside actual gradle configuration, not before build.
it looked like this:
// this happens for each buildType in project, no matter which flutter build type is building now.
List<String> platforms = getTargetPlatforms().collect() // default is android-x64, android-arm, android-arm64
// Debug mode includes x86 and x64, which are commonly used in emulators.
if (flutterBuildMode == "debug" && !useLocalEngine()) {
platforms.add("android-x86")
platforms.add("android-x64")
}
// `x86` was added only in debug configuration
// If it's debug buildType, we have android-x64, android-arm, android-arm64, android-x86 – 4 deps.
// If it's release buildType, we have android-x64, android-arm, android-arm64 – 3 deps.
platforms.each { platform ->
String arch = PLATFORM_ARCH_MAP[platform].replace("-", "_")
// Add the `libflutter.so` dependency.
addApiDependencies(project, buildType.name,
// Here dependencies added, 3 or 4, depending on buildType (calculated earlier).
"io.flutter:${arch}_$flutterBuildMode:$engineVersion")
}Now, desired architectures are known and set before gradle build, the just passed to it. It looks like this:
// this happens for each buildType in project, no matter which flutter build type is building now.
List<String> platforms = getTargetPlatforms().collect() // default is android-x64, android-arm, android-arm64 AND android-x86
// No additional logic for adding x86 in debug.
// If it's debug buildType, we have android-x64, android-arm, android-arm64, android-x86 – 4 deps.
// If it's release buildType, we have android-x64, android-arm, android-arm64, android-x86 – still 4 deps.
platforms.each { platform ->
String arch = PLATFORM_ARCH_MAP[platform].replace("-", "_")
// Add the `libflutter.so` dependency.
addApiDependencies(project, buildType.name,
// always 4 dependencies added.
"io.flutter:${arch}_$flutterBuildMode:$engineVersion")
}Main thing here, is that it's not an issue in standard Flutter build, because dependencies are not resolved during configuration for each variant there. And there can only be one build type building at single moment of time.
If you build with --debug – 4 arches added for every buildType, but only resolved for debug variant, which allowed to have x86 dependency. So, io.flutter.x86-release-.... added to release, but never resolved, because it's debug build happening now, and build works.
But, if I'm right, androidNativeBundle plugin tries to resolve all configurations inside it's apply method – in Gradle's configuration phase: https://github.com/howardpang/androidNativeBundle/blob/master/buildSrc/src/main/groovy/com/yy/android/gradle/nativedepend/NativeBundleImportPlugin.groovy#L175. And that breaks the build in configuration phase.
That said, feel free to open an issue in https://github.com/flutter/flutter/issues. I'm not in the right to decide if it'll be fixed because I'm not actual maintainer (just a 3rd party contributor), and this behaviour is a part of 3rd party gradle plugin also. I guess, for minimal reproduction it will be enough to just create empty flutter project with android and add androidNativeBundle to build the way it added to build in getsentry.
Also, @gmackall and @reidbaker might be interested in this case.
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.
@Sameri11 thanks for spending your time looking into this, this all makes sense!
But, if I'm right, androidNativeBundle plugin tries to resolve all configurations inside it's apply method – in Gradle's configuration phase: howardpang/androidNativeBundle@master/buildSrc/src/main/groovy/com/yy/android/gradle/nativedepend/NativeBundleImportPlugin.groovy#L175. And that breaks the build in configuration phase.
Yep, you're right, the plugin should switch to a task that receives the configuration dependencies as a Provider to prevent eager config resolution. We'll take it from here!
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.
@Sameri11 thanks also from my side for your detailed explanation. I just wanted to mention, that the maintainer of the androidNativeBundle released a new version 1.1.5, which fixes the incompatibility.
This PR addresses an issue where the
--target-platformflag was not being respected when building APKs in debug mode. Previously, debug builds would always includex86andx64architectures, regardless of the specified target platform. This change ensures that the--target-platformflag is honored across all build modes, including debug.To achieve this,
BuildApkCommandhas been slightly changed to become responsible for list of archs that should be built in the current run,rather than just parsing arguments. Previously, this responsibility was distributed to gradle, which could be frustrating (in my opinion)Fixes #153359
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.