Reland "[ CLI ] Improved consistency of -D and --define across tools and commands"

- Added support for --define to the VM and dart2js
- Added support for -D and --define for `dart run` and `dart compile js`

Remaining improvements:
- Add support for providing multiple comma separated values for `dart
  run`, `dart`, and `dart2js`

Related issue: https://github.com/dart-lang/sdk/issues/44562

TEST=Updated CLI tests and added new dart2js tests.

This reverts commit e49937769f.

Change-Id: I5f9275b829665eb5e8695403d67f230e752ab0e6
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/183180
Commit-Queue: Ben Konyi <bkonyi@google.com>
Reviewed-by: Sigmund Cherem <sigmund@google.com>

pkg/compiler/lib/src/dart2js.dart

@@ -234,9 +234,22 @@ Future<api.CompilationResult> compile(List<String> argv,
     passThrough(argument);
   }
 
-  void addInEnvironment(String argument) {
+  void addInEnvironment(Iterator<String> arguments) {
+    final isDefine = arguments.current.startsWith('--define');
+    String argument;
+    if (arguments.current == '--define') {
+      arguments.moveNext();
+      argument = arguments.current;
+    } else {
+      argument = arguments.current.substring(isDefine ? '--define='.length : 2);
+    }
+    // Allow for ' ' or '=' after --define
     int eqIndex = argument.indexOf('=');
-    String name = argument.substring(2, eqIndex);
+    if (eqIndex <= 0) {
+      helpAndFail('Invalid value for --define: $argument');
+      return;
+    }
+    String name = argument.substring(0, eqIndex);
     String value = argument.substring(eqIndex + 1);
     environment[name] = value;
   }
@@ -564,7 +577,8 @@ Future<api.CompilationResult> compile(List<String> argv,
     new OptionHandler('${Flags.mergeFragmentsThreshold}=.+', passThrough),
 
     // The following three options must come last.
-    new OptionHandler('-D.+=.*', addInEnvironment),
+    new OptionHandler('-D.+=.*|--define=.+=.*|--define', addInEnvironment,
+        multipleArguments: true),
     new OptionHandler('-.*', (String argument) {
       helpAndFail("Unknown option '$argument'.");
     }),
@@ -1030,7 +1044,7 @@ Supported options:
   -v, --verbose
     Display verbose information.
 
-  -D<name>=<value>
+  -D<name>=<value>, --define=<name>=<value>
     Define an environment declaration.
 
   --version

pkg/dartdev/lib/src/commands/compile.dart

@@ -88,7 +88,11 @@ class CompileJSCommand extends CompileSubcommandCommand {
         help: 'Generate minified output.',
         abbr: 'm',
         negatable: false,
-      );
+      )
+      ..addMultiOption('define', abbr: 'D', valueHelp: 'key=value', help: '''
+Define an environment declaration. To specify multiple declarations, use multiple options or use commas to separate key-value pairs.
+For example: dart compile $cmdName -Da=1,b=2 main.dart''');
+
     addExperimentalFlags(argParser, verbose);
   }
 
@@ -120,6 +124,24 @@ class CompileJSCommand extends CompileSubcommandCommand {
     if (!checkFile(sourcePath)) {
       return 1;
     }
+    final args = <String>[
+      '--libraries-spec=$librariesPath',
+      if (argResults.enabledExperiments.isNotEmpty)
+        "--enable-experiment=${argResults.enabledExperiments.join(',')}",
+      if (argResults.wasParsed(commonOptions['outputFile'].flag))
+        "-o${argResults[commonOptions['outputFile'].flag]}",
+      if (argResults.wasParsed('minified')) '-m',
+    ];
+
+    if (argResults.wasParsed('define')) {
+      for (final define in argResults['define']) {
+        args.add('-D$define');
+      }
+    }
+
+    // Add any args that weren't parsed to the end. This will likely only ever
+    // be the script name.
+    args.addAll(argResults.rest);
 
     VmInteropHandler.run(
         sdk.dart2jsSnapshot,

pkg/dartdev/lib/src/commands/run.dart

@@ -109,8 +109,12 @@ class RunCommand extends DartdevCommand {
         );
     }
     argParser
-      ..addMultiOption('define',
-          abbr: 'D', help: 'Defines an environment variable', hide: true)
+      ..addMultiOption(
+        'define',
+        abbr: 'D',
+        valueHelp: 'key=value',
+        help: 'Define an environment declaration.',
+      )
       ..addFlag(
         'disable-service-auth-codes',
         hide: !verbose,

pkg/dartdev/test/commands/compile_test.dart

@@ -187,7 +187,11 @@ void defineCompileTests() {
   });
 
   test('Compile JS', () {
-    final p = project(mainSrc: "void main() { print('Hello from JS'); }");
+    final p = project(mainSrc: '''
+        void main() {
+          print('1: ' + const String.fromEnvironment('foo'));
+          print('2: ' + const String.fromEnvironment('bar'));
+        }''');
     final inFile = path.canonicalize(path.join(p.dirPath, p.relativeFilePath));
     final outFile = path.canonicalize(path.join(p.dirPath, 'main.js'));
 
@@ -195,6 +199,8 @@ void defineCompileTests() {
       'compile',
       'js',
       '-m',
+      '-Dfoo=bar',
+      '--define=bar=foo',
       '-o',
       outFile,
       '-v',
@@ -202,8 +208,14 @@ void defineCompileTests() {
     ]);
     expect(result.stderr, isEmpty);
     expect(result.exitCode, 0);
-    expect(File(outFile).existsSync(), true,
-        reason: 'File not found: $outFile');
+    final file = File(outFile);
+    expect(file.existsSync(), true, reason: 'File not found: $outFile');
+
+    // Ensure the -D and --define arguments were processed correctly.
+    final contents = file.readAsStringSync();
+    print(contents);
+    expect(contents.contains('1: bar'), true);
+    expect(contents.contains('2: foo'), true);
   });
 
   test('Compile exe with error', () {

pkg/dartdev/test/commands/run_test.dart

@@ -181,6 +181,7 @@ void main(List<String> args) => print("$b $args");
       '--no-pause-isolates-on-exit',
       '--no-pause-isolates-on-unhandled-exceptions',
       '-Dfoo=bar',
+      '--define=bar=foo',
       p.relativeFilePath,
     ]);
     expect(
@@ -202,6 +203,7 @@ void main(List<String> args) => print("$b $args");
       '--no-pause-isolates-on-unhandled-exceptions',
       '--disable-service-auth-codes',
       '-Dfoo=bar',
+      '--define=bar=foo',
       p.relativeFilePath,
     ]);
 

pkg/dev_compiler/lib/src/kernel/command.dart

@@ -730,8 +730,19 @@ Map<String, String> parseAndRemoveDeclaredVariables(List<String> args) {
   var declaredVariables = <String, String>{};
   for (var i = 0; i < args.length;) {
     var arg = args[i];
+    String rest;
+    const defineFlag = '--define';
     if (arg.startsWith('-D') && arg.length > 2) {
-      var rest = arg.substring(2);
+      rest = arg.substring(2);
+    } else if (arg.startsWith('$defineFlag=') &&
+        arg.length > defineFlag.length + 1) {
+      rest = arg.substring(defineFlag.length + 1);
+    } else if (arg == defineFlag) {
+      i++;
+      rest = args[i];
+    }
+
+    if (rest != null) {
       var eq = rest.indexOf('=');
       if (eq <= 0) {
         var kind = eq == 0 ? 'name' : 'value';

pkg/front_end/tool/_fasta/command_line.dart

@@ -203,6 +203,7 @@ const Map<String, ValueSpecification> optionSpecification =
   Flags.noDeps: const BoolValue(false),
   Flags.invocationModes: const StringValue(),
   "-D": const DefineValue(),
+  "--define": const AliasValue("-D"),
   "-h": const AliasValue(Flags.help),
   "--out": const AliasValue(Flags.output),
   "-o": const AliasValue(Flags.output),

pkg/test_runner/lib/src/compiler_configuration.dart

@@ -1197,6 +1197,7 @@ abstract class VMKernelCompilerMixin {
       arguments.where((name) => name.endsWith('.dart')).single,
       ...arguments.where((name) =>
           name.startsWith('-D') ||
+          name.startsWith('--define') ||
           name.startsWith('--packages=') ||
           name.startsWith('--enable-experiment=')),
       '-Ddart.vm.product=$isProductMode',

runtime/bin/main_options.cc

@@ -147,6 +147,9 @@ void Options::PrintUsage() {
 "  all VM options).\n"
 "--packages=<path>\n"
 "  Where to find a package spec file.\n"
+"--define=<key>=<value> or -D<key>=<value>\n"
+"  Define an environment declaration. To specify multiple declarations,\n"
+"  use multiple instances of this option.\n"
 "--observe[=<port>[/<bind-address>]]\n"
 "  The observe flag is a convenience flag used to run a program with a\n"
 "  set of options which are often useful for debugging under Observatory.\n"
@@ -180,6 +183,9 @@ void Options::PrintUsage() {
 "  all VM options).\n"
 "--packages=<path>\n"
 "  Where to find a package spec file.\n"
+"--define=<key>=<value> or -D<key>=<value>\n"
+"  Define an environment declaration. To specify multiple declarations,\n"
+"  use multiple instances of this option.\n"
 "--observe[=<port>[/<bind-address>]]\n"
 "  The observe flag is a convenience flag used to run a program with a\n"
 "  set of options which are often useful for debugging under Observatory.\n"
@@ -546,7 +552,8 @@ bool Options::ParseArguments(int argc,
     while (tmp_i < argc) {
       // Check if this flag is a potentially valid VM flag. If not, we've likely
       // hit a script name and are done parsing VM flags.
-      if (!OptionProcessor::IsValidFlag(argv[tmp_i])) {
+      if (!OptionProcessor::IsValidFlag(argv[tmp_i]) &&
+          !OptionProcessor::IsValidShortFlag(argv[tmp_i])) {
         break;
       }
       OptionProcessor::TryProcess(argv[tmp_i], vm_options);

runtime/bin/options.cc

@@ -41,22 +41,35 @@ bool OptionProcessor::TryProcess(const char* option,
   return false;
 }
 
+static bool IsPrefix(const char* prefix, size_t prefix_len, const char* str) {
+  ASSERT(prefix != nullptr);
+  ASSERT(str != nullptr);
+  const size_t str_len = strlen(str);
+  if (str_len < prefix_len) {
+    return false;
+  }
+  return strncmp(prefix, str, prefix_len) == 0;
+}
+
 bool OptionProcessor::ProcessEnvironmentOption(
     const char* arg,
     CommandLineOptions* vm_options,
     dart::SimpleHashMap** environment) {
   ASSERT(arg != NULL);
   ASSERT(environment != NULL);
-  if (*arg == '\0') {
+  const char* kShortPrefix = "-D";
+  const char* kLongPrefix = "--define=";
+  const int kShortPrefixLen = strlen(kShortPrefix);
+  const int kLongPrefixLen = strlen(kLongPrefix);
+  const bool is_short_form = IsPrefix(kShortPrefix, kShortPrefixLen, arg);
+  const bool is_long_form = IsPrefix(kLongPrefix, kLongPrefixLen, arg);
+  if (is_short_form) {
+    arg = arg + kShortPrefixLen;
+  } else if (is_long_form) {
+    arg = arg + kLongPrefixLen;
+  } else {
     return false;
   }
-  if (*arg != '-') {
-    return false;
-  }
-  if (*(arg + 1) != 'D') {
-    return false;
-  }
-  arg = arg + 2;
   if (*arg == '\0') {
     return true;
   }
@@ -69,12 +82,20 @@ bool OptionProcessor::ProcessEnvironmentOption(
   const char* equals_pos = strchr(arg, '=');
   if (equals_pos == NULL) {
     // No equal sign (name without value) currently not supported.
-    Syslog::PrintErr("No value given to -D option\n");
+    if (is_short_form) {
+      Syslog::PrintErr("No value given to -D option\n");
+    } else {
+      Syslog::PrintErr("No value given to --define option\n");
+    }
     return true;
   }
   int name_len = equals_pos - arg;
   if (name_len == 0) {
-    Syslog::PrintErr("No name given to -D option\n");
+    if (is_short_form) {
+      Syslog::PrintErr("No name given to -D option\n");
+    } else {
+      Syslog::PrintErr("No name given to --define option\n");
+    }
     return true;
   }
   // Split name=value into name and value.

tests/corelib/string_from_environment_test.dart

@@ -1,7 +1,7 @@
 // Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
-// SharedOptions=-Da=a -Db=bb -Dc=ccc -Dd=
+// SharedOptions=-Da=a -Db=bb -Dc=ccc -Dd= --define=e=eeee
 
 import "package:expect/expect.dart";
 
@@ -10,4 +10,5 @@ main() {
   Expect.equals('bb', const String.fromEnvironment('b'));
   Expect.equals('ccc', const String.fromEnvironment('c'));
   Expect.equals('', const String.fromEnvironment('d'));
+  Expect.equals('eeee', const String.fromEnvironment('e'));
 }

tests/corelib_2/string_from_environment_test.dart

@@ -1,7 +1,7 @@
 // Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
-// SharedOptions=-Da=a -Db=bb -Dc=ccc -Dd=
+// SharedOptions=-Da=a -Db=bb -Dc=ccc -Dd= --define=e=eeee
 
 import "package:expect/expect.dart";
 
@@ -10,4 +10,5 @@ main() {
   Expect.equals('bb', const String.fromEnvironment('b'));
   Expect.equals('ccc', const String.fromEnvironment('c'));
   Expect.equals('', const String.fromEnvironment('d'));
+  Expect.equals('eeee', const String.fromEnvironment('e'));
 }