From 1e1b5271f440e5f2cf26f4fc4e0c68fb0a4c90a4 Mon Sep 17 00:00:00 2001 From: David Iglesias Date: Mon, 2 Dec 2024 10:33:02 -0800 Subject: [PATCH] [tool] Removes deprecated --web-renderer parameter. (#159314) Removes the `--web-renderer` option. Flutter tool users won't be able to select their web renderer. In order to not drop test coverage, tests are still run with the HTML Renderer, until its code is fully removed from the engine. Part of https://github.com/flutter/flutter/issues/145954 ## 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] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --- dev/bots/suite_runners/run_web_tests.dart | 49 ++++++++++--- .../lib/src/commands/build_web.dart | 22 +++--- .../flutter_tools/lib/src/commands/run.dart | 12 ++-- .../flutter_tools/lib/src/commands/test.dart | 12 ++-- .../lib/src/runner/flutter_command.dart | 28 -------- .../flutter_tools/lib/src/web/compile.dart | 70 ++++++++++++------- .../hermetic/build_web_test.dart | 10 +-- .../commands.shard/hermetic/run_test.dart | 4 +- .../commands.shard/hermetic/test_test.dart | 7 +- .../test/general.shard/args_test.dart | 1 - .../targets/web_defines_test.dart | 34 +++++++-- .../build_system/targets/web_test.dart | 29 ++++++++ .../test/web_test_compiler_test.dart | 1 + .../general.shard/web/compile_web_test.dart | 28 ++++++++ .../web.shard/debugger_stepping_web_test.dart | 2 +- .../expression_evaluation_web_test.dart | 4 +- .../test/web.shard/hot_reload_web_test.dart | 12 ++-- .../test/web.shard/output_web_test.dart | 1 - .../test/web.shard/vm_service_web_test.dart | 6 +- .../test/web.shard/web_run_test.dart | 2 +- 20 files changed, 219 insertions(+), 115 deletions(-) diff --git a/dev/bots/suite_runners/run_web_tests.dart b/dev/bots/suite_runners/run_web_tests.dart index d96c0467b2..5bbbc597c3 100644 --- a/dev/bots/suite_runners/run_web_tests.dart +++ b/dev/bots/suite_runners/run_web_tests.dart @@ -105,7 +105,7 @@ class WebTestsSuite { testAppDirectory: path.join('packages', 'integration_test', 'example'), target: path.join('test_driver', 'failure.dart'), buildMode: buildMode, - renderer: 'canvaskit', + webRenderer: 'canvaskit', // This test intentionally fails and prints stack traces in the browser // logs. To avoid confusion, silence browser output. silenceBrowserOutput: true, @@ -115,7 +115,7 @@ class WebTestsSuite { target: path.join('integration_test', 'example_test.dart'), driver: path.join('test_driver', 'integration_test.dart'), buildMode: buildMode, - renderer: 'canvaskit', + webRenderer: 'canvaskit', expectWriteResponseFile: true, expectResponseFileContent: 'null', ), @@ -124,7 +124,7 @@ class WebTestsSuite { target: path.join('integration_test', 'extended_test.dart'), driver: path.join('test_driver', 'extended_integration_test.dart'), buildMode: buildMode, - renderer: 'canvaskit', + webRenderer: 'canvaskit', expectWriteResponseFile: true, expectResponseFileContent: ''' { @@ -177,7 +177,7 @@ class WebTestsSuite { testAppDirectory: path.join(flutterRoot, 'examples', 'hello_world'), target: 'test_driver/smoke_web_engine.dart', buildMode: 'profile', - renderer: 'auto', + webRenderer: 'auto', ), () => _runGalleryE2eWebTest('debug'), () => _runGalleryE2eWebTest('debug', canvasKit: true), @@ -278,7 +278,7 @@ class WebTestsSuite { await _runFlutterDriverWebTest( target: path.join('test_driver', '$name.dart'), buildMode: buildMode, - renderer: renderer, + webRenderer: renderer, testAppDirectory: path.join(flutterRoot, 'dev', 'integration_tests', 'web_e2e_tests'), ); } @@ -286,7 +286,7 @@ class WebTestsSuite { Future _runFlutterDriverWebTest({ required String target, required String buildMode, - required String renderer, + required String webRenderer, required String testAppDirectory, String? driver, bool expectFailure = false, @@ -318,7 +318,24 @@ class WebTestsSuite { '-d', 'web-server', '--$buildMode', - '--web-renderer=$renderer', + // '--web-renderer=$webRenderer', + '--dart-define=FLUTTER_WEB_AUTO_DETECT=false', + if (webRenderer == 'skwasm') ...[ + // See: WebRendererMode.dartDefines[skwasm] + '--dart-define=FLUTTER_WEB_USE_SKIA=false', + '--dart-define=FLUTTER_WEB_USE_SKWASM=true', + ], + if (webRenderer == 'canvaskit') ...[ + // See: WebRendererMode.dartDefines[canvaskit] + '--dart-define=FLUTTER_WEB_USE_SKIA=true', + '--dart-define=FLUTTER_WEB_USE_SKWASM=false', + ], + if (webRenderer == 'html') ...[ + // See: WebRendererMode.dartDefines[html] + '--dart-define=FLUTTER_WEB_USE_SKIA=false', + '--dart-define=FLUTTER_WEB_USE_SKWASM=false', + ], + ], expectNonZeroExit: expectFailure, workingDirectory: testAppDirectory, @@ -695,7 +712,23 @@ class WebTestsSuite { '-v', '--platform=chrome', if (useWasm) '--wasm', - '--web-renderer=$webRenderer', + '--dart-define=FLUTTER_WEB_AUTO_DETECT=false', + // '--web-renderer=$webRenderer', + if (webRenderer == 'skwasm') ...[ + // See: WebRendererMode.dartDefines[skwasm] + '--dart-define=FLUTTER_WEB_USE_SKIA=false', + '--dart-define=FLUTTER_WEB_USE_SKWASM=true', + ], + if (webRenderer == 'canvaskit') ...[ + // See: WebRendererMode.dartDefines[canvaskit] + '--dart-define=FLUTTER_WEB_USE_SKIA=true', + '--dart-define=FLUTTER_WEB_USE_SKWASM=false', + ], + if (webRenderer == 'html') ...[ + // See: WebRendererMode.dartDefines[html] + '--dart-define=FLUTTER_WEB_USE_SKIA=false', + '--dart-define=FLUTTER_WEB_USE_SKWASM=false', + ], '--dart-define=DART_HHH_BOT=$runningInDartHHHBot', ...flutterTestArgs, ...tests, diff --git a/packages/flutter_tools/lib/src/commands/build_web.dart b/packages/flutter_tools/lib/src/commands/build_web.dart index f0c2e4e2cc..8aba1bc81c 100644 --- a/packages/flutter_tools/lib/src/commands/build_web.dart +++ b/packages/flutter_tools/lib/src/commands/build_web.dart @@ -51,7 +51,6 @@ class BuildWebCommand extends BuildSubCommand { allowed: ServiceWorkerStrategy.values.map((ServiceWorkerStrategy e) => e.cliName), allowedHelp: CliEnum.allowedHelp(ServiceWorkerStrategy.values), ); - usesWebRendererOption(); usesWebResourcesCdnFlag(); // @@ -141,20 +140,23 @@ class BuildWebCommand extends BuildSubCommand { ? int.parse(dart2jsOptimizationLevelValue.substring(1)) : optimizationLevel; - final String? webRendererString = stringArg(FlutterOptions.kWebRendererFlag); - final WebRendererMode? webRenderer = webRendererString == null - ? null - : WebRendererMode.values.byName(webRendererString); + final List dartDefines = extractDartDefines( + defineConfigJsonMap: extractDartDefineConfigJsonMap() + ); + final bool useWasm = boolArg(FlutterOptions.kWebWasmFlag); + // See also: RunCommandBase.webRenderer and TestCommand.webRenderer. + final WebRendererMode webRenderer = WebRendererMode.fromDartDefines(dartDefines, useWasm: useWasm); final bool sourceMaps = boolArg('source-maps'); final List compilerConfigs; - if (webRenderer != null && webRenderer.isDeprecated) { + if (webRenderer.isDeprecated) { globals.logger.printWarning(webRenderer.deprecationWarning); } - if (boolArg(FlutterOptions.kWebWasmFlag)) { - if (webRenderer != null) { - throwToolExit('"--${FlutterOptions.kWebRendererFlag}" cannot be combined with "--${FlutterOptions.kWebWasmFlag}"'); + + if (useWasm) { + if (webRenderer != WebRendererMode.getDefault(useWasm: true)) { + throwToolExit('Do not attempt to set a web renderer when using "--${FlutterOptions.kWebWasmFlag}"'); } globals.logger.printBox( title: 'New feature', @@ -185,7 +187,7 @@ class BuildWebCommand extends BuildSubCommand { nativeNullAssertions: boolArg('native-null-assertions'), noFrequencyBasedMinification: boolArg('no-frequency-based-minification'), sourceMaps: sourceMaps, - renderer: webRenderer ?? WebRendererMode.defaultForJs, + renderer: webRenderer, )]; } diff --git a/packages/flutter_tools/lib/src/commands/run.dart b/packages/flutter_tools/lib/src/commands/run.dart index 03e607f26a..48bb8fa914 100644 --- a/packages/flutter_tools/lib/src/commands/run.dart +++ b/packages/flutter_tools/lib/src/commands/run.dart @@ -39,7 +39,6 @@ abstract class RunCommandBase extends FlutterCommand with DeviceBasedDevelopment addBuildModeFlags(verboseHelp: verboseHelp, defaultToRelease: false); usesDartDefineOption(); usesFlavorOption(); - usesWebRendererOption(); usesWebResourcesCdnFlag(); addNativeNullAssertions(hide: !verboseHelp); addBundleSkSLPathOption(hide: !verboseHelp); @@ -234,10 +233,13 @@ abstract class RunCommandBase extends FlutterCommand with DeviceBasedDevelopment bool get useWasm => boolArg(FlutterOptions.kWebWasmFlag); - WebRendererMode get webRenderer => WebRendererMode.fromCliOption( - stringArg(FlutterOptions.kWebRendererFlag), - useWasm: useWasm - ); + // Keep in sync with the [TestCommand.webRenderer] getter. + WebRendererMode get webRenderer { + final List dartDefines = extractDartDefines( + defineConfigJsonMap: extractDartDefineConfigJsonMap() + ); + return WebRendererMode.fromDartDefines(dartDefines, useWasm: useWasm); + } /// Create a debugging options instance for the current `run` or `drive` invocation. @visibleForTesting diff --git a/packages/flutter_tools/lib/src/commands/test.dart b/packages/flutter_tools/lib/src/commands/test.dart index fd779f5c05..ded767a9d8 100644 --- a/packages/flutter_tools/lib/src/commands/test.dart +++ b/packages/flutter_tools/lib/src/commands/test.dart @@ -76,7 +76,6 @@ class TestCommand extends FlutterCommand with DeviceBasedDevelopmentArtifacts { usesTrackWidgetCreation(verboseHelp: verboseHelp); addEnableExperimentation(hide: !verboseHelp); usesDartDefineOption(); - usesWebRendererOption(); usesDeviceUserOption(); usesFlavorOption(); addEnableImpellerFlag(verboseHelp: verboseHelp); @@ -336,10 +335,13 @@ class TestCommand extends FlutterCommand with DeviceBasedDevelopmentArtifacts { return super.verifyThenRunCommand(commandPath); } - WebRendererMode get webRenderer => WebRendererMode.fromCliOption( - stringArg(FlutterOptions.kWebRendererFlag), - useWasm: useWasm - ); + // Keep in sync with the [RunCommandBase.webRenderer] getter. + WebRendererMode get webRenderer { + final List dartDefines = extractDartDefines( + defineConfigJsonMap: extractDartDefineConfigJsonMap() + ); + return WebRendererMode.fromDartDefines(dartDefines, useWasm: useWasm); + } @override Future runCommand() async { diff --git a/packages/flutter_tools/lib/src/runner/flutter_command.dart b/packages/flutter_tools/lib/src/runner/flutter_command.dart index 102d500e38..cdf54eec84 100644 --- a/packages/flutter_tools/lib/src/runner/flutter_command.dart +++ b/packages/flutter_tools/lib/src/runner/flutter_command.dart @@ -31,7 +31,6 @@ import '../preview_device.dart'; import '../project.dart'; import '../reporting/reporting.dart'; import '../reporting/unified_analytics.dart'; -import '../web/compile.dart'; import 'flutter_command_runner.dart'; import 'target_devices.dart'; @@ -154,7 +153,6 @@ abstract final class FlutterOptions { static const String kFatalWarnings = 'fatal-warnings'; static const String kUseApplicationBinary = 'use-application-binary'; static const String kWebBrowserFlag = 'web-browser-flag'; - static const String kWebRendererFlag = 'web-renderer'; static const String kWebResourcesCdnFlag = 'web-resources-cdn'; static const String kWebWasmFlag = 'wasm'; } @@ -707,32 +705,6 @@ abstract class FlutterCommand extends Command { ); } - // This option is deprecated and is no longer publicly supported, and - // therefore is hidden. - // - // The option still exists for internal testing, and to give existing users - // time to migrate off the HTML renderer, but it is no longer advertised as a - // supported mode. - // - // See also: - // * https://github.com/flutter/flutter/issues/151786 - // * https://github.com/flutter/flutter/issues/145954 - void usesWebRendererOption() { - argParser.addOption( - hide: true, - FlutterOptions.kWebRendererFlag, - allowed: WebRendererMode.values.map((WebRendererMode e) => e.name), - help: 'This option is deprecated and will be removed in a future Flutter ' - 'release.\n' - 'Selects the renderer implementation to use when building for the ' - 'web. The supported renderers are "canvaskit" when compiling to ' - 'JavaScript, and "skwasm" when compiling to WebAssembly. Other ' - 'renderer and compiler combinations are no longer supported. ' - 'Consider migrating your app to a supported renderer.', - allowedHelp: CliEnum.allowedHelp(WebRendererMode.values) - ); - } - void usesWebResourcesCdnFlag() { argParser.addFlag( FlutterOptions.kWebResourcesCdnFlag, diff --git a/packages/flutter_tools/lib/src/web/compile.dart b/packages/flutter_tools/lib/src/web/compile.dart index 466596ce5b..f8577f2d09 100644 --- a/packages/flutter_tools/lib/src/web/compile.dart +++ b/packages/flutter_tools/lib/src/web/compile.dart @@ -11,7 +11,6 @@ import '../base/file_system.dart'; import '../base/logger.dart'; import '../base/project_migrator.dart'; import '../base/terminal.dart'; -import '../base/utils.dart'; import '../build_info.dart'; import '../build_system/build_system.dart'; import '../cache.dart'; @@ -179,7 +178,7 @@ class WebBuilder { } /// Web rendering backend mode. -enum WebRendererMode implements CliEnum { +enum WebRendererMode { /// Auto detects which rendering backend to use. auto, @@ -192,12 +191,22 @@ enum WebRendererMode implements CliEnum { /// Always use skwasm. skwasm; - factory WebRendererMode.fromCliOption(String? webRendererString, - {required bool useWasm}) { - if (webRendererString == null) { - return getDefault(useWasm: useWasm); + factory WebRendererMode.fromDartDefines(Iterable defines, { + required bool useWasm, + }) { + if (defines.contains('FLUTTER_WEB_AUTO_DETECT=true')) { + return auto; + } else if (defines.contains('FLUTTER_WEB_USE_SKIA=false') + && defines.contains('FLUTTER_WEB_USE_SKWASM=true')) { + return skwasm; + } else if (defines.contains('FLUTTER_WEB_USE_SKIA=true') + && defines.contains('FLUTTER_WEB_USE_SKWASM=false')) { + return canvaskit; + } else if (defines.contains('FLUTTER_WEB_USE_SKIA=false') + && defines.contains('FLUTTER_WEB_USE_SKWASM=false')) { + return html; // The horror! } - return WebRendererMode.values.byName(webRendererString); + return getDefault(useWasm: useWasm); } static WebRendererMode getDefault({required bool useWasm}) { @@ -219,13 +228,9 @@ enum WebRendererMode implements CliEnum { /// Returns a consistent deprecation warning for the WebRendererMode. String get deprecationWarning => - 'The HTML Renderer is deprecated. Do not use "--web-renderer=$name".' + 'The HTML Renderer is deprecated and will be removed. Please, stop using it.' '\nSee: https://docs.flutter.dev/to/web-html-renderer-deprecation'; - @override - String get cliName => kebabCase(name); - - @override String get helpText => switch (this) { auto => 'Use the HTML renderer on mobile devices, and CanvasKit on desktop devices.', @@ -236,34 +241,45 @@ enum WebRendererMode implements CliEnum { skwasm => 'Always use the experimental skwasm renderer.' }; + /// Returns [dartDefines] in a way usable from the CLI. + /// + /// This is used to start integration tests. + Iterable get toCliDartDefines => dartDefines.map( + (String define) => '--dart-define=$define'); + Iterable get dartDefines => switch (this) { - auto => [ + auto => const { 'FLUTTER_WEB_AUTO_DETECT=true', - ], - canvaskit => [ + }, + canvaskit => const { 'FLUTTER_WEB_AUTO_DETECT=false', 'FLUTTER_WEB_USE_SKIA=true', - ], - html => [ + 'FLUTTER_WEB_USE_SKWASM=false', + }, + html => const { 'FLUTTER_WEB_AUTO_DETECT=false', 'FLUTTER_WEB_USE_SKIA=false', - ], - skwasm => [ + 'FLUTTER_WEB_USE_SKWASM=false', + }, + skwasm => const { 'FLUTTER_WEB_AUTO_DETECT=false', 'FLUTTER_WEB_USE_SKIA=false', 'FLUTTER_WEB_USE_SKWASM=true', - ], + }, }; + /// Sets the dart defines for the currently selected WebRendererMode List updateDartDefines(List inputDefines) { final Set dartDefinesSet = inputDefines.toSet(); - if (!inputDefines - .any((String d) => d.startsWith('FLUTTER_WEB_AUTO_DETECT=')) && - inputDefines.any((String d) => d.startsWith('FLUTTER_WEB_USE_SKIA='))) { - dartDefinesSet - .removeWhere((String d) => d.startsWith('FLUTTER_WEB_USE_SKIA=')); - } - dartDefinesSet.addAll(dartDefines); + + dartDefinesSet + ..removeWhere((String d) { + return d.startsWith('FLUTTER_WEB_AUTO_DETECT=') || + d.startsWith('FLUTTER_WEB_USE_SKIA=') || + d.startsWith('FLUTTER_WEB_USE_SKWASM='); + }) + ..addAll(dartDefines); + return dartDefinesSet.toList(); } } diff --git a/packages/flutter_tools/test/commands.shard/hermetic/build_web_test.dart b/packages/flutter_tools/test/commands.shard/hermetic/build_web_test.dart index f65259c27b..84661ace67 100644 --- a/packages/flutter_tools/test/commands.shard/hermetic/build_web_test.dart +++ b/packages/flutter_tools/test/commands.shard/hermetic/build_web_test.dart @@ -371,7 +371,7 @@ void main() { 'build', 'web', '--no-pub', - '--web-renderer=${webRenderer.name}', + ...webRenderer.toCliDartDefines, ]); } on ToolExit catch (error) { expect(error, isA()); @@ -402,12 +402,6 @@ void main() { expect(command.usage, contains(option)); } - void expectHidden(String option) { - expect(command.argParser.options.keys, contains(option)); - expect(command.argParser.options[option]!.hide, isTrue); - expect(command.usage, isNot(contains(option))); - } - expectVisible('pwa-strategy'); expectVisible('web-resources-cdn'); expectVisible('optimization-level'); @@ -419,8 +413,6 @@ void main() { expectVisible('wasm'); expectVisible('strip-wasm'); expectVisible('base-href'); - - expectHidden('web-renderer'); }, overrides: { Platform: () => fakePlatform, FileSystem: () => fileSystem, diff --git a/packages/flutter_tools/test/commands.shard/hermetic/run_test.dart b/packages/flutter_tools/test/commands.shard/hermetic/run_test.dart index e43dd417b8..8d474b943f 100644 --- a/packages/flutter_tools/test/commands.shard/hermetic/run_test.dart +++ b/packages/flutter_tools/test/commands.shard/hermetic/run_test.dart @@ -1012,7 +1012,7 @@ void main() { () => createTestCommandRunner(command).run([ 'run', '--no-pub', - '--web-renderer=skwasm', + ...WebRendererMode.skwasm.toCliDartDefines, ]), throwsToolExit(message: 'Skwasm renderer requires --wasm')); }, overrides: { FileSystem: () => fileSystem, @@ -1030,7 +1030,7 @@ void main() { await createTestCommandRunner(RunCommand()).run([ 'run', '--no-pub', - '--web-renderer=${webRenderer.name}', + ...webRenderer.toCliDartDefines, ]); } on ToolExit catch (error) { expect(error, isA()); diff --git a/packages/flutter_tools/test/commands.shard/hermetic/test_test.dart b/packages/flutter_tools/test/commands.shard/hermetic/test_test.dart index ed8b362fb3..3e33452153 100644 --- a/packages/flutter_tools/test/commands.shard/hermetic/test_test.dart +++ b/packages/flutter_tools/test/commands.shard/hermetic/test_test.dart @@ -1395,11 +1395,11 @@ dev_dependencies: final TestCommand testCommand = TestCommand(testRunner: testRunner); final CommandRunner commandRunner = createTestCommandRunner(testCommand); - await commandRunner.run(const [ + await commandRunner.run([ 'test', '--no-pub', '--platform=chrome', - '--web-renderer=canvaskit', + ...WebRendererMode.canvaskit.toCliDartDefines, ]); expect(testRunner.lastDebuggingOptionsValue.webRenderer, WebRendererMode.canvaskit); }, overrides: { @@ -1440,7 +1440,7 @@ dev_dependencies: 'web', '--no-pub', '--platform=chrome', - '--web-renderer=${webRenderer.name}', + ...webRenderer.toCliDartDefines, ]); } on ToolExit catch (error) { expect(error, isA()); @@ -1459,7 +1459,6 @@ dev_dependencies: .where((WebRendererMode mode) => mode.isDeprecated) .forEach(testWebRendererDeprecationMessage); - testUsingContext('Can test in a pub workspace', () async { final String root = fs.path.rootPrefix(fs.currentDirectory.absolute.path); diff --git a/packages/flutter_tools/test/general.shard/args_test.dart b/packages/flutter_tools/test/general.shard/args_test.dart index 08997eae2a..65eed6102b 100644 --- a/packages/flutter_tools/test/general.shard/args_test.dart +++ b/packages/flutter_tools/test/general.shard/args_test.dart @@ -232,7 +232,6 @@ void verifyOptions(String? command, Iterable