Revert "Improve handling of disable-dartdev-analytics"

This reverts commit 58860f4.

Reason for revert: Broke bots. Will investigate.

Original change's description:
> Improve handling of disable-dartdev-analytics
>
> This is second try of https://dart-review.googlesource.com/c/sdk/+/171284
> that was reverted to to faulty logic in main_options.
>
> Some other refactorings are piggy-backed along.
>
> TestProject.runSync no longer takes a 'command' argument. It was anyway
> often not an argument.
>
> Also stop the messy handling of pub arguments. It is no longer needed.
>
> BUG: #44135
> Change-Id: I49abf5810d9ea262409ba9d93f0471037cb8a753
> TEST=The VM change is tested via all the pkg/dartdev/test/command/* tests that invoke dart with the --no-analytics flag.
> TEST=Furthermore manual test that the --no-analytics flag is passed to dartdev.
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/174261
> Reviewed-by: Jonas Jensen <[email protected]>
> Commit-Queue: Sigurd Meldgaard <[email protected]>

[email protected],[email protected],[email protected]

Change-Id: I754bcebdcfc595158b04d431662b65bf25f5b89d
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/174466
Reviewed-by: Sigurd Meldgaard <[email protected]>
Commit-Queue: Sigurd Meldgaard <[email protected]>

Author: sigurdm

pkg/dartdev/lib/dartdev.dart

@@ -32,34 +32,129 @@ import 'src/vm_interop_handler.dart';
 /// analytics logic, it has been moved here.
 Future<void> runDartdev(List<String> args, SendPort port) async {
   VmInteropHandler.initialize(port);
-  if (args.contains('run')) {
-    // These flags have a format that can't be handled by package:args, so while
-    // they are valid flags we'll assume the VM has verified them by this point.
-    args = args
-        .where(
-          (element) => !(element.contains('--observe') ||
-              element.contains('--enable-vm-service')),
-        )
-        .toList();
+
+  int result;
+
+  // The exit code for the dartdev process; null indicates that it has not been
+  // set yet. The value is set in the catch and finally blocks below.
+  int exitCode;
+
+  // Any caught non-UsageExceptions when running the sub command.
+  Object exception;
+  StackTrace stackTrace;
+
+  // The Analytics instance used to report information back to Google Analytics;
+  // see lib/src/analytics.dart.
+  final analytics = createAnalyticsInstance(
+    args.contains('--disable-dartdev-analytics'),
+  );
+
+  // If we have not printed the analyticsNoticeOnFirstRunMessage to stdout,
+  // the user is on a terminal, and the machine is not a bot, then print the
+  // disclosure and set analytics.disclosureShownOnTerminal to true.
+  if (analytics is DartdevAnalytics &&
+      !analytics.disclosureShownOnTerminal &&
+      io.stdout.hasTerminal &&
+      !isBot()) {
+    print(analyticsNoticeOnFirstRunMessage);
+    analytics.disclosureShownOnTerminal = true;
   }
 
-  // Finally, call the runner to execute the command; see DartdevRunner.
+  // When `--disable-analytics` or `--enable-analytics` are called we perform
+  // the respective intention and print any notices to standard out and exit.
+  if (args.contains('--disable-analytics')) {
+    // This block also potentially catches the case of (disableAnalytics &&
+    // enableAnalytics), in which we favor the disabling of analytics.
+    analytics.enabled = false;
+
+    // Alert the user that analytics has been disabled.
+    print(analyticsDisabledNoticeMessage);
+    VmInteropHandler.exit(0);
+    return;
+  } else if (args.contains('--enable-analytics')) {
+    analytics.enabled = true;
+
+    // Alert the user again that anonymous data will be collected.
+    print(analyticsNoticeOnFirstRunMessage);
+    VmInteropHandler.exit(0);
+    return;
+  }
 
-  final runner = DartdevRunner(args);
-  var exitCode = 1;
   try {
-    exitCode = await runner.run(args);
-  } on UsageException catch (e) {
-    // TODO(sigurdm): It is unclear when a UsageException gets to here, and
-    // when it is in DartdevRunner.runCommand.
-    io.stderr.writeln('$e');
-    exitCode = 64;
+    final runner = DartdevRunner(args, analytics);
+
+    // Run can't be called with the '--disable-dartdev-analytics' flag; remove
+    // it if it is contained in args.
+    if (args.contains('--disable-dartdev-analytics')) {
+      args = List.from(args)..remove('--disable-dartdev-analytics');
+    }
+
+    if (args.contains('run')) {
+      // These flags have a format that can't be handled by package:args, so while
+      // they are valid flags we'll assume the VM has verified them by this point.
+      args = args
+          .where(
+            (element) => !(element.contains('--observe') ||
+                element.contains('--enable-vm-service')),
+          )
+          .toList();
+    }
+
+    // If ... help pub ... is in the args list, remove 'help', and add '--help'
+    // to the end of the list. This will make it possible to use the help
+    // command to access subcommands of pub such as `dart help pub publish`; see
+    // https://github.com/dart-lang/sdk/issues/42965.
+    if (PubUtils.shouldModifyArgs(args, runner.commands.keys.toList())) {
+      args = PubUtils.modifyArgs(args);
+    }
+
+    // Finally, call the runner to execute the command; see DartdevRunner.
+    result = await runner.run(args);
+  } catch (e, st) {
+    if (e is UsageException) {
+      io.stderr.writeln('$e');
+      exitCode = 64;
+    } else {
+      // Set the exception and stack trace only for non-UsageException cases:
+      exception = e;
+      stackTrace = st;
+      io.stderr.writeln('$e');
+      io.stderr.writeln('$st');
+      exitCode = 1;
+    }
   } finally {
+    // Set the exitCode, if it wasn't set in the catch block above.
+    exitCode ??= result ?? 0;
+
+    // Send analytics before exiting
+    if (analytics.enabled) {
+      // And now send the exceptions and events to Google Analytics:
+      if (exception != null) {
+        unawaited(
+          analytics.sendException(
+              '${exception.runtimeType}\n${sanitizeStacktrace(stackTrace)}',
+              fatal: true),
+        );
+      }
+
+      await analytics.waitForLastPing(
+          timeout: const Duration(milliseconds: 200));
+    }
+
+    // Set the enabled flag in the analytics object to true. Note: this will not
+    // enable the analytics unless the disclosure was shown (terminal detected),
+    // and the machine is not detected to be a bot.
+    if (analytics.firstRun) {
+      analytics.enabled = true;
+    }
+    analytics.close();
     VmInteropHandler.exit(exitCode);
   }
 }
 
 class DartdevRunner extends CommandRunner<int> {
+  final Analytics analytics;
+
   @override
   final ArgParser argParser = ArgParser(
     usageLineLength: dartdevUsageLineLength,
@@ -69,7 +164,8 @@ class DartdevRunner extends CommandRunner<int> {
   static const String dartdevDescription =
       'A command-line utility for Dart development';
 
-  DartdevRunner(List<String> args) : super('dart', '$dartdevDescription.') {
+  DartdevRunner(List<String> args, this.analytics)
+      : super('dart', '$dartdevDescription.') {
     final bool verbose = args.contains('-v') || args.contains('--verbose');
 
     argParser.addFlag('verbose',
@@ -84,9 +180,12 @@ class DartdevRunner extends CommandRunner<int> {
     argParser.addFlag('diagnostics',
         negatable: false, help: 'Show tool diagnostic output.', hide: !verbose);
 
+    // A hidden flag to disable analytics on this run, this constructor can be
+    // called with this flag, but should be removed before run() is called as
+    // the flag has not been added to all sub-commands.
     argParser.addFlag(
-      'analytics',
-      negatable: true,
+      'disable-dartdev-analytics',
+      negatable: false,
       help: 'Disable anonymous analytics for this `dart *` run',
       hide: true,
     );
@@ -113,38 +212,7 @@ class DartdevRunner extends CommandRunner<int> {
   @override
   Future<int> runCommand(ArgResults topLevelResults) async {
     final stopwatch = Stopwatch()..start();
-    // The Analytics instance used to report information back to Google Analytics;
-    // see lib/src/analytics.dart.
-    final analytics = createAnalyticsInstance(!topLevelResults['analytics']);
-
-    // If we have not printed the analyticsNoticeOnFirstRunMessage to stdout,
-    // the user is on a terminal, and the machine is not a bot, then print the
-    // disclosure and set analytics.disclosureShownOnTerminal to true.
-    if (analytics is DartdevAnalytics &&
-        !analytics.disclosureShownOnTerminal &&
-        io.stdout.hasTerminal &&
-        !isBot()) {
-      print(analyticsNoticeOnFirstRunMessage);
-      analytics.disclosureShownOnTerminal = true;
-    }
-
-    // When `--disable-analytics` or `--enable-analytics` are called we perform
-    // the respective intention and print any notices to standard out and exit.
-    if (topLevelResults['disable-analytics']) {
-      // This block also potentially catches the case of (disableAnalytics &&
-      // enableAnalytics), in which we favor the disabling of analytics.
-      analytics.enabled = false;
-
-      // Alert the user that analytics has been disabled.
-      print(analyticsDisabledNoticeMessage);
-      return 0;
-    } else if (topLevelResults['enable-analytics']) {
-      analytics.enabled = true;
-
-      // Alert the user again that anonymous data will be collected.
-      print(analyticsNoticeOnFirstRunMessage);
-      return 0;
-    }
+    assert(!topLevelResults.arguments.contains('--disable-dartdev-analytics'));
 
     if (topLevelResults.command == null &&
         topLevelResults.arguments.isNotEmpty) {
@@ -154,12 +222,14 @@ class DartdevRunner extends CommandRunner<int> {
         io.stderr.writeln(
             "Error when reading '$firstArg': No such file or directory.");
         // This is the exit code used by the frontend.
-        return 254;
+        VmInteropHandler.exit(254);
       }
     }
 
+    isDiagnostics = topLevelResults['diagnostics'];
+
     final Ansi ansi = Ansi(Ansi.terminalSupportsAnsi);
-    log = topLevelResults['diagnostics']
+    log = isDiagnostics
         ? Logger.verbose(ansi: ansi)
         : Logger.standard(ansi: ansi);
 
@@ -177,15 +247,8 @@ class DartdevRunner extends CommandRunner<int> {
       analytics.sendScreenView(path),
     );
 
-    // The exit code for the dartdev process; null indicates that it has not been
-    // set yet. The value is set in the catch and finally blocks below.
-    int exitCode;
-
-    // Any caught non-UsageExceptions when running the sub command.
-    Object exception;
-    StackTrace stackTrace;
     try {
-      exitCode = await super.runCommand(topLevelResults);
+      final exitCode = await super.runCommand(topLevelResults);
 
       if (path != null && analytics.enabled) {
         // Send the event to analytics
@@ -205,16 +268,8 @@ class DartdevRunner extends CommandRunner<int> {
           ),
         );
       }
-    } on UsageException catch (e) {
-      io.stderr.writeln('$e');
-      exitCode = 64;
-    } catch (e, st) {
-      // Set the exception and stack trace only for non-UsageException cases:
-      exception = e;
-      stackTrace = st;
-      io.stderr.writeln('$e');
-      io.stderr.writeln('$st');
-      exitCode = 1;
+
+      return exitCode;
     } finally {
       stopwatch.stop();
       if (analytics.enabled) {
@@ -226,32 +281,6 @@ class DartdevRunner extends CommandRunner<int> {
           ),
         );
       }
-      // Set the exitCode, if it wasn't set in the catch block above.
-      exitCode ??= 0;
-
-      // Send analytics before exiting
-      if (analytics.enabled) {
-        // And now send the exceptions and events to Google Analytics:
-        if (exception != null) {
-          unawaited(
-            analytics.sendException(
-                '${exception.runtimeType}\n${sanitizeStacktrace(stackTrace)}',
-                fatal: true),
-          );
-        }
-
-        await analytics.waitForLastPing(
-            timeout: const Duration(milliseconds: 200));
-      }
-
-      // Set the enabled flag in the analytics object to true. Note: this will not
-      // enable the analytics unless the disclosure was shown (terminal detected),
-      // and the machine is not detected to be a bot.
-      if (analytics.firstRun) {
-        analytics.enabled = true;
-      }
-      analytics.close();
-      return exitCode;
     }
   }
 }

pkg/dartdev/lib/src/analytics.dart

@@ -57,7 +57,7 @@ Analytics createAnalyticsInstance(bool disableAnalytics) {
   }
 
   if (disableAnalytics) {
-    // Dartdev tests pass a hidden 'no-analytics' flag which is
+    // Dartdev tests pass a hidden 'disable-dartdev-analytics' flag which is
     // handled here.
     //
     // Also, stdout.hasTerminal is checked; if there is no terminal we infer

pkg/dartdev/lib/src/utils.dart

@@ -38,6 +38,31 @@ String trimEnd(String s, String suffix) {
   return s;
 }
 
+/// Static util methods used in dartdev to potentially modify the order of the
+/// arguments passed into dartdev.
+class PubUtils {
+  /// If [doModifyArgs] returns true, then this method returns a modified copy
+  /// of the argument list, 'help' is removed from the interior of the list, and
+  /// '--help' is added to the end of the list of arguments. This method returns
+  /// a modified copy of the list, the list itself is not modified.
+  static List<String> modifyArgs(List<String> args) => List.from(args)
+    ..remove('help')
+    ..add('--help');
+
+  /// If ... help pub ..., and no other verb (such as 'analyze') appears before
+  /// the ... help pub ... in the argument list, then return true.
+  static bool shouldModifyArgs(List<String> args, List<String> allCmds) =>
+      args != null &&
+      allCmds != null &&
+      args.isNotEmpty &&
+      allCmds.isNotEmpty &&
+      args.firstWhere((arg) => allCmds.contains(arg), orElse: () => '') ==
+          'help' &&
+      args.contains('help') &&
+      args.contains('pub') &&
+      args.indexOf('help') + 1 == args.indexOf('pub');
+}
+
 extension FileSystemEntityExtension on FileSystemEntity {
   String get name => p.basename(path);