Repository navigation
[Android] Update the CLI to reject passing engine configuration flags with a prebuilt binary in release mode - #190870
Conversation
github-actions
Bot
added
tool
camsim99
changed the title
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request centralizes Android engine command-line flags into a new AndroidEngineCliFlags class and restricts passing these flags as intent extras when running prebuilt APKs in release mode, instead requiring them to be defined in the Android manifest. The DebuggingOptions class is refactored to generate launch arguments and intent extras from a single configuration map. The review feedback suggests simplifying the loops in getAndroidLaunchArguments and getAndroidLaunchArgumentsAsIntentExtras by assigning entry.value to a local variable, which leverages Dart's type promotion and eliminates the need for explicit casts and null assertions.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Code Review
This pull request centralizes Android engine CLI flags into a new AndroidEngineCliFlags class and refactors DebuggingOptions to dynamically construct launch arguments and intent extras. It also introduces validation to prevent running prebuilt release APKs with restricted engine configuration flags, throwing a tool exit if any are detected. The review feedback suggests clarifying the documentation or adding comments in AndroidEngineCliFlags to distinguish between Flutter CLI flags and actual Android engine flags, as the class currently contains a mix of both.
| /// The command line flags that are passed directly to the Android engine. | ||
| abstract final class AndroidEngineCliFlags { |
There was a problem hiding this comment.
The class documentation states that these are flags passed directly to the Android engine, but the class actually contains a mix of flutter CLI flags (e.g., enable-hcpp, verbose-system-logs) and actual Android engine flags (e.g., verbose-logging).\n\nFor example:\n- enableHcpp ('enable-hcpp') is a flutter CLI flag, whereas the engine expects 'enable-hcpp-and-surface-control'.\n- verboseSystemLogs ('verbose-system-logs') is a flutter CLI flag, whereas the engine expects 'verbose-logging'.\n\nTo improve maintainability and prevent confusion for future developers adding new flags, please clarify this distinction in the class documentation or add comments indicating which constants represent CLI flags (used for validation in allFlags) and which represent engine-side flags.
There was a problem hiding this comment.
Well, all the flags in that class are passed to the engine but happy to rename if reviewers prefer.
There was a problem hiding this comment.
Hmm I agree partially with gemini here, for example isn't
static const String verboseSystemLogs = 'verbose-system-logs';not passed to the engine? We use it later, right
if (verboseSystemLogs) AndroidEngineCliFlags.verboseLogging: true,but we don't pass it, we use it to pass the engine shell arg format (which is also contained in the class, and is the conflation that I think gemini is getting at here - we are mixing cli args and shell args).
Similarly, enableHcpp is in this class, but the flag actually passed to the engine is enable-hcpp-and-surface-control, which isn't defined here and is instead a hardcoded string literal in device.dart.
Could we either separate them into two distinct classes (e.g. AndroidEngineCliOptionKeys for CLI flags, and AndroidEngineShellArgs for the engine shell flags that Kotlin/Java will eventually share), or,
If keeping them together, clearly distinguish them with naming/comments, and add a constant for enable-hcpp-and-surface-control so we don't leave it as a magic string in device.dart?
There was a problem hiding this comment.
Fair enough. For now, kept the flags in this one classes and added docs!
|
Major changes since last review for reviewers: Most of my commits since the last review were fixing a bad merge. I also addressed the feedback by deleting a test ( |
camsim99
added
the
autosubmit
flutter-dashboard
Bot
removed
the
autosubmit
flutter/flutter@55b8f88...d649d2b 2026-09-30 [email protected] [Android] Update the CLI to reject passing engine configuration flags with a prebuilt binary in release mode (flutter/flutter#190870) 2026-09-30 [email protected] Roll Skia from af1e8b356dd1 to c7b323126bf0 (1 revision) (flutter/flutter#193569) 2026-09-30 [email protected] [web] Respect text affinity in getLineBoundary at a soft wrap (flutter/flutter#192664) 2026-09-30 [email protected] [tool] Migrate TestCommand and platform runner to modular dependency injection (flutter/flutter#190789) 2026-09-30 [email protected] Roll Skia from 3a20a0464d25 to af1e8b356dd1 (1 revision) (flutter/flutter#193564) 2026-09-30 [email protected] Roll Skia from 554ef62d11a9 to 3a20a0464d25 (6 revisions) (flutter/flutter#193556) 2026-09-30 [email protected] Use null aware elements in `platform_views.dart` (flutter/flutter#193214) 2026-09-30 [email protected] Replace deprecated `withOpacity` in `flutter_logo.dart` (flutter/flutter#192583) 2026-09-30 [email protected] Roll Skia from f441ca223b2b to 554ef62d11a9 (72 revisions) (flutter/flutter#193548) 2026-09-30 [email protected] Fix StrcmpFixed matching prefixes in the Vulkan embedder tests (flutter/flutter#192999) 2026-09-30 [email protected] Update Engine to test Android 17 AVD (flutter/flutter#193239) 2026-09-29 [email protected] Give the Vulkan test context the extensions Impeller requires (flutter/flutter#193002) 2026-09-29 [email protected] Rename "subtext" to "supportingTextPadding" in `FormField`'s documentation (flutter/flutter#183582) 2026-09-29 [email protected] Set --no-js-strongly-connected-components for DDC compiles by default (flutter/flutter#193485) 2026-09-29 [email protected] Split pre-AGP 8.3 module AAR test into a Java 17 pinned target (flutter/flutter#193467) 2026-09-29 [email protected] Remove no-shuffle from gen_defaults_test (flutter/flutter#192280) 2026-09-29 [email protected] Update guidance on bumping Dart (flutter/flutter#193131) 2026-09-29 [email protected] Remove vestigial `download_jdk` gclient var (flutter/flutter#188571) 2026-09-29 [email protected] Roll pub packages (flutter/flutter#193522) 2026-09-29 [email protected] [flutter_tools] Lazily initialize AndroidSdk platform and build-tools discovery (flutter/flutter#191972) 2026-09-29 [email protected] [tool] Migrate Web build subcommands and toolchain to modular dependency injection (flutter/flutter#190783) 2026-09-29 [email protected] Pin androidx.test dependencies in integration_test (flutter/flutter#193316) 2026-09-29 [email protected] [flutter_tools] Include flutter.js.map in web builds (flutter/flutter#192257) 2026-09-29 [email protected] [WebParagraph] Fixing edge cases for wrapping text (with newlines) (flutter/flutter#189858) 2026-09-29 [email protected] Bump customer testing version for flutter/devtools update (flutter/flutter#193511) 2026-09-29 [email protected] Roll Packages from ba0364a to 3c6ce59 (14 revisions) (flutter/flutter#193507) 2026-09-29 [email protected] [flutter_tools] Guard socket streams and done futures against socket reset errors (flutter/flutter#192941) If this roll has caused a breakage, revert this CL and set the roller to dry run mode 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
3 participants
Caution
This is a breaking change. Accompanying breaking change docs: flutter/website#13796.
For security purposes*, modifies the Flutter CLI to throw a fatal error on Android only when
flutter run --release --use-application-binaryis invoked in addition to passing any other Android engine configuration flags. So, for example, the following release-mode invocations for a Flutter Android app will be valid with this PR:flutter run --releaseflutter run --release --verbose --enable-hcpp --any-other-engine-config-flagflutter run --release --use-aplication-binaryThe following will not be valid:
flutter run --release --use-application-binary --verbose --enable-hcpp --any-other-android-engine-config-flag*See #190461 for details. This is part of that work.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.