From e092dcfa22cef503cfdceea67e45c49c0aa8062d Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Thu, 16 Apr 2020 10:56:49 -0700 Subject: [PATCH] [flutter_tools] Reland: fix multiple dart defines (#54973) --- dev/bots/test.dart | 61 ++++++++++++++++++- dev/devicelab/lib/tasks/defines_task.dart | 3 +- dev/integration_tests/ui/lib/defines.dart | 2 +- .../ui/test_driver/defines_test.dart | 2 +- .../web/lib/web_define_loading.dart | 24 ++++++++ packages/flutter_tools/bin/macos_assemble.sh | 2 +- packages/flutter_tools/bin/xcode_backend.sh | 2 +- packages/flutter_tools/gradle/flutter.gradle | 2 +- .../flutter_tools/lib/src/android/gradle.dart | 3 +- packages/flutter_tools/lib/src/aot.dart | 3 +- .../lib/src/build_system/targets/dart.dart | 16 +---- .../lib/src/build_system/targets/web.dart | 2 + packages/flutter_tools/lib/src/bundle.dart | 3 +- .../lib/src/commands/assemble.dart | 5 ++ .../flutter_tools/lib/src/ios/xcodeproj.dart | 3 +- .../flutter_tools/lib/src/web/compile.dart | 3 +- .../build_system/targets/web_test.dart | 29 ++------- .../commands/build_aot_test.dart | 3 +- 18 files changed, 113 insertions(+), 55 deletions(-) create mode 100644 dev/integration_tests/web/lib/web_define_loading.dart diff --git a/dev/bots/test.dart b/dev/bots/test.dart index 0a0713e28ff..4ab96eadafd 100644 --- a/dev/bots/test.dart +++ b/dev/bots/test.dart @@ -590,6 +590,18 @@ Future _runWebIntegrationTests() async { await _runWebDebugTest('lib/stack_trace.dart'); await _runWebDebugTest('lib/web_directory_loading.dart'); await _runWebDebugTest('test/test.dart'); + await _runWebDebugTest('lib/web_define_loading.dart', + additionalArguments: [ + '--dart-define=test.valueA=Example', + '--dart-define=test.valueB=Value', + ] + ); + await _runWebReleaseTest('lib/web_define_loading.dart', + additionalArguments: [ + '--dart-define=test.valueA=Example', + '--dart-define=test.valueB=Value', + ] + ); } Future _runWebStackTraceTest(String buildMode) async { @@ -632,10 +644,56 @@ Future _runWebStackTraceTest(String buildMode) async { } } +/// Run a web integration test in release mode. +Future _runWebReleaseTest(String target, { + List additionalArguments = const[], +}) async { + final String testAppDirectory = path.join(flutterRoot, 'dev', 'integration_tests', 'web'); + final String appBuildDirectory = path.join(testAppDirectory, 'build', 'web'); + + // Build the app. + await runCommand( + flutter, + [ 'clean' ], + workingDirectory: testAppDirectory, + ); + await runCommand( + flutter, + [ + 'build', + 'web', + '--release', + ...additionalArguments, + '-t', + target, + ], + workingDirectory: testAppDirectory, + environment: { + 'FLUTTER_WEB': 'true', + }, + ); + + // Run the app. + final String result = await evalTestAppInChrome( + appUrl: 'http://localhost:8080/index.html', + appDirectory: appBuildDirectory, + ); + + if (result.contains('--- TEST SUCCEEDED ---')) { + print('${green}Web release mode test passed.$reset'); + } else { + print(result); + print('${red}Web release mode test failed.$reset'); + exit(1); + } +} + /// Debug mode is special because `flutter build web` doesn't build in debug mode. /// /// Instead, we use `flutter run --debug` and sniff out the standard output. -Future _runWebDebugTest(String target) async { +Future _runWebDebugTest(String target, { + List additionalArguments = const[], +}) async { final String testAppDirectory = path.join(flutterRoot, 'dev', 'integration_tests', 'web'); final CapturedOutput output = CapturedOutput(); bool success = false; @@ -647,6 +705,7 @@ Future _runWebDebugTest(String target) async { '-d', 'chrome', '--web-run-headless', + ...additionalArguments, '-t', target, ], diff --git a/dev/devicelab/lib/tasks/defines_task.dart b/dev/devicelab/lib/tasks/defines_task.dart index 18ad1f040ee..afb3649cb0d 100644 --- a/dev/devicelab/lib/tasks/defines_task.dart +++ b/dev/devicelab/lib/tasks/defines_task.dart @@ -21,7 +21,8 @@ Future runDartDefinesTask() async { '--verbose', '-d', deviceId, - '--dart-define=test.value=ExampleValue', + '--dart-define=test.valueA=Example', + '--dart-define=test.valueB=Value', 'lib/defines.dart', ]); }); diff --git a/dev/integration_tests/ui/lib/defines.dart b/dev/integration_tests/ui/lib/defines.dart index e94227d1458..db3d9b09653 100644 --- a/dev/integration_tests/ui/lib/defines.dart +++ b/dev/integration_tests/ui/lib/defines.dart @@ -11,7 +11,7 @@ void main() { runApp( const Center( child: Text( - String.fromEnvironment('test.value'), + String.fromEnvironment('test.valueA') + String.fromEnvironment('test.valueB'), textDirection: TextDirection.ltr, ), ), diff --git a/dev/integration_tests/ui/test_driver/defines_test.dart b/dev/integration_tests/ui/test_driver/defines_test.dart index 4440d2f770a..af049ec746b 100644 --- a/dev/integration_tests/ui/test_driver/defines_test.dart +++ b/dev/integration_tests/ui/test_driver/defines_test.dart @@ -16,7 +16,7 @@ void main() { await driver.close(); }); - test('Can run with --dart-deinfe', () async { + test('Can run with --dart-define', () async { await driver.waitFor(find.text('ExampleValue')); }); } diff --git a/dev/integration_tests/web/lib/web_define_loading.dart b/dev/integration_tests/web/lib/web_define_loading.dart new file mode 100644 index 00000000000..32f771d4b2d --- /dev/null +++ b/dev/integration_tests/web/lib/web_define_loading.dart @@ -0,0 +1,24 @@ +// Copyright 2014 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'dart:html' as html; + +Future main() async { + final StringBuffer output = StringBuffer(); + const String combined = String.fromEnvironment('test.valueA') + + String.fromEnvironment('test.valueB'); + if (combined == 'ExampleValue') { + output.write('--- TEST SUCCEEDED ---'); + print('--- TEST SUCCEEDED ---'); + } else { + output.write('--- TEST FAILED ---'); + print('--- TEST FAILED ---'); + } + + html.HttpRequest.request( + '/test-result', + method: 'POST', + sendData: '$output', + ); +} diff --git a/packages/flutter_tools/bin/macos_assemble.sh b/packages/flutter_tools/bin/macos_assemble.sh index e09a3ddd7f4..ad90d9f6153 100755 --- a/packages/flutter_tools/bin/macos_assemble.sh +++ b/packages/flutter_tools/bin/macos_assemble.sh @@ -83,7 +83,7 @@ RunCommand "${FLUTTER_ROOT}/bin/flutter" \ -dTreeShakeIcons="${icon_tree_shaker_flag}" \ -dDartObfuscation="${dart_obfuscation_flag}" \ -dSplitDebugInfo="${SPLIT_DEBUG_INFO}" \ - -dDartDefines="${DART_DEFINES}" \ + --DartDefines="${DART_DEFINES}" \ -dExtraFrontEndOptions="${EXTRA_FRONT_END_OPTIONS}" \ --build-inputs="${build_inputs_path}" \ --build-outputs="${build_outputs_path}" \ diff --git a/packages/flutter_tools/bin/xcode_backend.sh b/packages/flutter_tools/bin/xcode_backend.sh index 47f408e6af7..37806d550b7 100755 --- a/packages/flutter_tools/bin/xcode_backend.sh +++ b/packages/flutter_tools/bin/xcode_backend.sh @@ -186,7 +186,7 @@ BuildApp() { -dTrackWidgetCreation="${track_widget_creation_flag}" \ -dDartObfuscation="${dart_obfuscation_flag}" \ -dEnableBitcode="${bitcode_flag}" \ - -dDartDefines="${DART_DEFINES}" \ + --DartDefines="${DART_DEFINES}" \ -dExtraFrontEndOptions="${EXTRA_FRONT_END_OPTIONS}" \ "${build_mode}_ios_bundle_flutter_assets" diff --git a/packages/flutter_tools/gradle/flutter.gradle b/packages/flutter_tools/gradle/flutter.gradle index fedca6842aa..67915935d50 100644 --- a/packages/flutter_tools/gradle/flutter.gradle +++ b/packages/flutter_tools/gradle/flutter.gradle @@ -916,7 +916,7 @@ abstract class BaseFlutterTask extends DefaultTask { args "-dDartObfuscation=true" } if (dartDefines != null) { - args "-dDartDefines=${dartDefines}" + args "--DartDefines=${dartDefines}" } if (extraGenSnapshotOptions != null) { args "--ExtraGenSnapshotOptions=${extraGenSnapshotOptions}" diff --git a/packages/flutter_tools/lib/src/android/gradle.dart b/packages/flutter_tools/lib/src/android/gradle.dart index 06b832c1034..61f253d52ad 100644 --- a/packages/flutter_tools/lib/src/android/gradle.dart +++ b/packages/flutter_tools/lib/src/android/gradle.dart @@ -19,7 +19,6 @@ import '../base/terminal.dart'; import '../base/utils.dart'; import '../build_info.dart'; import '../cache.dart'; -import '../convert.dart'; import '../flutter_manifest.dart'; import '../globals.dart' as globals; import '../project.dart'; @@ -331,7 +330,7 @@ Future buildGradleApp({ command.add('-Pshrink=true'); } if (androidBuildInfo.buildInfo.dartDefines?.isNotEmpty ?? false) { - command.add('-Pdart-defines=${jsonEncode(androidBuildInfo.buildInfo.dartDefines)}'); + command.add('-Pdart-defines=${androidBuildInfo.buildInfo.dartDefines.join(',')}'); } if (shouldBuildPluginAsAar) { // Pass a system flag instead of a project flag, so this flag can be diff --git a/packages/flutter_tools/lib/src/aot.dart b/packages/flutter_tools/lib/src/aot.dart index 30aa662124f..3e61d9acc7d 100644 --- a/packages/flutter_tools/lib/src/aot.dart +++ b/packages/flutter_tools/lib/src/aot.dart @@ -14,7 +14,6 @@ import 'build_system/targets/dart.dart'; import 'build_system/targets/icon_tree_shaker.dart'; import 'build_system/targets/ios.dart'; import 'cache.dart'; -import 'convert.dart'; import 'globals.dart' as globals; import 'ios/bitcode.dart'; import 'project.dart'; @@ -87,7 +86,7 @@ class AotBuilder { kBuildMode: getNameForBuildMode(buildInfo.mode), kTargetPlatform: getNameForTargetPlatform(platform), kIconTreeShakerFlag: buildInfo.treeShakeIcons.toString(), - kDartDefines: jsonEncode(buildInfo.dartDefines), + kDartDefines: buildInfo.dartDefines.join(','), kBitcodeFlag: bitcode.toString(), if (buildInfo?.extraGenSnapshotOptions?.isNotEmpty ?? false) kExtraGenSnapshotOptions: buildInfo.extraGenSnapshotOptions.join(','), diff --git a/packages/flutter_tools/lib/src/build_system/targets/dart.dart b/packages/flutter_tools/lib/src/build_system/targets/dart.dart index 0ef9c0d0873..102f63a871e 100644 --- a/packages/flutter_tools/lib/src/build_system/targets/dart.dart +++ b/packages/flutter_tools/lib/src/build_system/targets/dart.dart @@ -7,7 +7,6 @@ import '../../base/build.dart'; import '../../base/file_system.dart'; import '../../build_info.dart'; import '../../compile.dart'; -import '../../convert.dart'; import '../../globals.dart' as globals; import '../../project.dart'; import '../build_system.dart'; @@ -390,21 +389,10 @@ abstract class CopyFlutterAotBundle extends Target { } } -/// Dart defines are encoded inside [Environment] as a JSON array. +/// Dart defines are encoded inside [Environment] as a comma-separated list. List parseDartDefines(Environment environment) { if (!environment.defines.containsKey(kDartDefines) || environment.defines[kDartDefines].isEmpty) { return const []; } - - final String dartDefinesJson = environment.defines[kDartDefines]; - try { - final List parsedDefines = jsonDecode(dartDefinesJson) as List; - return parsedDefines.cast(); - } on FormatException { - throw Exception( - 'The value of -D$kDartDefines is not formatted correctly.\n' - 'The value must be a JSON-encoded list of strings but was:\n' - '$dartDefinesJson' - ); - } + return environment.defines[kDartDefines].split(','); } diff --git a/packages/flutter_tools/lib/src/build_system/targets/web.dart b/packages/flutter_tools/lib/src/build_system/targets/web.dart index 501cb3e6375..cbaa2a0f04e 100644 --- a/packages/flutter_tools/lib/src/build_system/targets/web.dart +++ b/packages/flutter_tools/lib/src/build_system/targets/web.dart @@ -168,6 +168,8 @@ class Dart2JSTarget extends Target { '--libraries-spec=$specPath', '-o', outputKernel.path, + for (final String dartDefine in dartDefines) + '-D$dartDefine', '--packages=$packageFile', '--cfe-only', environment.buildDir.childFile('main.dart').path, diff --git a/packages/flutter_tools/lib/src/bundle.dart b/packages/flutter_tools/lib/src/bundle.dart index cfdcc0fb2ab..04139530fa5 100644 --- a/packages/flutter_tools/lib/src/bundle.dart +++ b/packages/flutter_tools/lib/src/bundle.dart @@ -17,7 +17,6 @@ import 'build_system/depfile.dart'; import 'build_system/targets/dart.dart'; import 'build_system/targets/icon_tree_shaker.dart'; import 'cache.dart'; -import 'convert.dart'; import 'dart/package_map.dart'; import 'devfs.dart'; import 'globals.dart' as globals; @@ -130,7 +129,7 @@ Future buildWithAssemble({ kTrackWidgetCreation: trackWidgetCreation?.toString(), kIconTreeShakerFlag: treeShakeIcons ? 'true' : null, if (dartDefines != null && dartDefines.isNotEmpty) - kDartDefines: jsonEncode(dartDefines), + kDartDefines: dartDefines.join(','), }, artifacts: globals.artifacts, fileSystem: globals.fs, diff --git a/packages/flutter_tools/lib/src/commands/assemble.dart b/packages/flutter_tools/lib/src/commands/assemble.dart index 6c015c96c16..1723291b1cf 100644 --- a/packages/flutter_tools/lib/src/commands/assemble.dart +++ b/packages/flutter_tools/lib/src/commands/assemble.dart @@ -87,6 +87,7 @@ class AssembleCommand extends FlutterCommand { 'root of the current Flutter project.', ); argParser.addOption(kExtraGenSnapshotOptions); + argParser.addOption(kDartDefines); argParser.addOption( 'resource-pool-size', help: 'The maximum number of concurrent tasks the build system will run.', @@ -182,6 +183,10 @@ class AssembleCommand extends FlutterCommand { if (argResults.wasParsed(kExtraGenSnapshotOptions)) { results[kExtraGenSnapshotOptions] = argResults[kExtraGenSnapshotOptions] as String; } + // Workaround for dart-define formatting + if (argResults.wasParsed(kDartDefines)) { + results[kDartDefines] = argResults[kDartDefines] as String; + } return results; } diff --git a/packages/flutter_tools/lib/src/ios/xcodeproj.dart b/packages/flutter_tools/lib/src/ios/xcodeproj.dart index 12709d943d2..b100f363745 100644 --- a/packages/flutter_tools/lib/src/ios/xcodeproj.dart +++ b/packages/flutter_tools/lib/src/ios/xcodeproj.dart @@ -18,7 +18,6 @@ import '../base/terminal.dart'; import '../base/utils.dart'; import '../build_info.dart'; import '../cache.dart'; -import '../convert.dart'; import '../flutter_manifest.dart'; import '../globals.dart' as globals; import '../project.dart'; @@ -236,7 +235,7 @@ List _xcodeBuildSettingsLines({ } if (buildInfo.dartDefines?.isNotEmpty ?? false) { - xcodeBuildSettings.add('DART_DEFINES=${jsonEncode(buildInfo.dartDefines)}'); + xcodeBuildSettings.add('DART_DEFINES=${buildInfo.dartDefines.join(',')}'); } if (buildInfo.extraFrontEndOptions?.isNotEmpty ?? false) { diff --git a/packages/flutter_tools/lib/src/web/compile.dart b/packages/flutter_tools/lib/src/web/compile.dart index c80535bea8c..8ee410f5f67 100644 --- a/packages/flutter_tools/lib/src/web/compile.dart +++ b/packages/flutter_tools/lib/src/web/compile.dart @@ -13,7 +13,6 @@ import '../build_system/build_system.dart'; import '../build_system/targets/dart.dart'; import '../build_system/targets/icon_tree_shaker.dart'; import '../build_system/targets/web.dart'; -import '../convert.dart'; import '../globals.dart' as globals; import '../platform_plugins.dart'; import '../plugins.dart'; @@ -49,7 +48,7 @@ Future buildWeb( kTargetFile: target, kInitializePlatform: initializePlatform.toString(), kHasWebPlugins: hasWebPlugins.toString(), - kDartDefines: jsonEncode(buildInfo.dartDefines), + kDartDefines: buildInfo.dartDefines.join(','), kCspMode: csp.toString(), kIconTreeShakerFlag: buildInfo.treeShakeIcons.toString(), }, diff --git a/packages/flutter_tools/test/general.shard/build_system/targets/web_test.dart b/packages/flutter_tools/test/general.shard/build_system/targets/web_test.dart index 6490f196d49..6a0380d6778 100644 --- a/packages/flutter_tools/test/general.shard/build_system/targets/web_test.dart +++ b/packages/flutter_tools/test/general.shard/build_system/targets/web_test.dart @@ -360,12 +360,14 @@ void main() { test('Dart2JSTarget calls dart2js with Dart defines in release mode', () => testbed.run(() async { environment.defines[kBuildMode] = 'release'; - environment.defines[kDartDefines] = '["FOO=bar","BAZ=qux"]'; + environment.defines[kDartDefines] = 'FOO=bar,BAZ=qux'; processManager.addCommand(FakeCommand( command: [ ...kDart2jsLinuxArgs, '-o', environment.buildDir.childFile('app.dill').absolute.path, + '-DFOO=bar', + '-DBAZ=qux', '--packages=${globals.fs.path.join('foo', '.packages')}', '--cfe-only', environment.buildDir.childFile('main.dart').absolute.path, @@ -391,12 +393,14 @@ void main() { test('Dart2JSTarget calls dart2js with Dart defines in profile mode', () => testbed.run(() async { environment.defines[kBuildMode] = 'profile'; - environment.defines[kDartDefines] = '["FOO=bar","BAZ=qux"]'; + environment.defines[kDartDefines] = 'FOO=bar,BAZ=qux'; processManager.addCommand(FakeCommand( command: [ ...kDart2jsLinuxArgs, '-o', environment.buildDir.childFile('app.dill').absolute.path, + '-DFOO=bar', + '-DBAZ=qux', '--packages=${globals.fs.path.join('foo', '.packages')}', '--cfe-only', environment.buildDir.childFile('main.dart').absolute.path, @@ -421,27 +425,6 @@ void main() { ProcessManager: () => processManager, })); - test('Dart2JSTarget throws developer-friendly exception on misformatted DartDefines', () => testbed.run(() async { - environment.defines[kBuildMode] = 'profile'; - environment.defines[kDartDefines] = '[misformatted json'; - try { - await const Dart2JSTarget().build(environment); - fail('Call to build() must not have succeeded.'); - } on Exception catch(exception) { - expect( - '$exception', - 'Exception: The value of -D$kDartDefines is not formatted correctly.\n' - 'The value must be a JSON-encoded list of strings but was:\n' - '[misformatted json', - ); - } - - // Should not attempt to run any processes. - verifyNever(globals.processManager.run(any)); - }, overrides: { - ProcessManager: () => MockProcessManager(), - })); - test('Generated service worker correctly inlines file hashes', () { final String result = generateServiceWorker({'/foo': 'abcd'}); diff --git a/packages/flutter_tools/test/general.shard/commands/build_aot_test.dart b/packages/flutter_tools/test/general.shard/commands/build_aot_test.dart index 87506e5ae6a..043c9df6fd4 100644 --- a/packages/flutter_tools/test/general.shard/commands/build_aot_test.dart +++ b/packages/flutter_tools/test/general.shard/commands/build_aot_test.dart @@ -109,7 +109,8 @@ void main() { }); testUsingContext('build aot outputs timing info', () async { - globals.fs.file('.dart_tool/flutter_build/cce09742720db17ffec62331bd7e42d5/app.so') + globals.fs + .file('.dart_tool/flutter_build/0c21fd4ab3b8bde8b385ff01d08e0093/app.so') .createSync(recursive: true); when(globals.buildSystem.build(any, any)) .thenAnswer((Invocation invocation) async {