Repository navigation
[Android] Update the CLI to reject passing engine configuration flags with a prebuilt binary in release mode - #190870
Conversation
|
/gemini review |
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 ( |
flutter/flutter@55b8f88...d649d2b 2026-09-30 43054281+camsim99@users.noreply.github.com [Android] Update the CLI to reject passing engine configuration flags with a prebuilt binary in release mode (flutter/flutter#190870) 2026-09-30 engine-flutter-autoroll@skia.org Roll Skia from af1e8b356dd1 to c7b323126bf0 (1 revision) (flutter/flutter#193569) 2026-09-30 zhongliu88889@gmail.com [web] Respect text affinity in getLineBoundary at a soft wrap (flutter/flutter#192664) 2026-09-30 bkonyi@google.com [tool] Migrate TestCommand and platform runner to modular dependency injection (flutter/flutter#190789) 2026-09-30 engine-flutter-autoroll@skia.org Roll Skia from 3a20a0464d25 to af1e8b356dd1 (1 revision) (flutter/flutter#193564) 2026-09-30 engine-flutter-autoroll@skia.org Roll Skia from 554ef62d11a9 to 3a20a0464d25 (6 revisions) (flutter/flutter#193556) 2026-09-30 116356835+AbdeMohlbi@users.noreply.github.com Use null aware elements in `platform_views.dart` (flutter/flutter#193214) 2026-09-30 116356835+AbdeMohlbi@users.noreply.github.com Replace deprecated `withOpacity` in `flutter_logo.dart` (flutter/flutter#192583) 2026-09-30 engine-flutter-autoroll@skia.org Roll Skia from f441ca223b2b to 554ef62d11a9 (72 revisions) (flutter/flutter#193548) 2026-09-30 joel.winarske@linux.com Fix StrcmpFixed matching prefixes in the Vulkan embedder tests (flutter/flutter#192999) 2026-09-30 jesswon@google.com Update Engine to test Android 17 AVD (flutter/flutter#193239) 2026-09-29 joel.winarske@linux.com Give the Vulkan test context the extensions Impeller requires (flutter/flutter#193002) 2026-09-29 55765052+MohanadAbdallah-mv@users.noreply.github.com Rename "subtext" to "supportingTextPadding" in `FormField`'s documentation (flutter/flutter#183582) 2026-09-29 markzipan@google.com Set --no-js-strongly-connected-components for DDC compiles by default (flutter/flutter#193485) 2026-09-29 jesswon@google.com Split pre-AGP 8.3 module AAR test into a Java 17 pinned target (flutter/flutter#193467) 2026-09-29 32538273+ValentinVignal@users.noreply.github.com Remove no-shuffle from gen_defaults_test (flutter/flutter#192280) 2026-09-29 katelovett@google.com Update guidance on bumping Dart (flutter/flutter#193131) 2026-09-29 dbebawy@users.noreply.github.com Remove vestigial `download_jdk` gclient var (flutter/flutter#188571) 2026-09-29 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#193522) 2026-09-29 bkonyi@google.com [flutter_tools] Lazily initialize AndroidSdk platform and build-tools discovery (flutter/flutter#191972) 2026-09-29 bkonyi@google.com [tool] Migrate Web build subcommands and toolchain to modular dependency injection (flutter/flutter#190783) 2026-09-29 matt.boetger@gmail.com Pin androidx.test dependencies in integration_test (flutter/flutter#193316) 2026-09-29 Deil.Christoph@gmail.com [flutter_tools] Include flutter.js.map in web builds (flutter/flutter#192257) 2026-09-29 Rusino@users.noreply.github.com [WebParagraph] Fixing edge cases for wrapping text (with newlines) (flutter/flutter#189858) 2026-09-29 ryjohn@google.com Bump customer testing version for flutter/devtools update (flutter/flutter#193511) 2026-09-29 engine-flutter-autoroll@skia.org Roll Packages from ba0364a to 3c6ce59 (14 revisions) (flutter/flutter#193507) 2026-09-29 bkonyi@google.com [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 quncheng@google.com,stuartmorgan@google.com 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
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.