[hooks_runner] Fix handling of executable paths and arguments containing spaces on Windows

pkgs/hooks_runner/CHANGELOG.md

@@ -1,6 +1,8 @@
 ## 1.5.1-wip
 
 - Fix record_use path changing caching issue.
+- Fix hook invocation when the Dart executable or an argument path contains a
+  space on Windows.
 
 ## 1.5.0
 

pkgs/hooks_runner/lib/src/utils/run_process.dart

@@ -4,8 +4,7 @@
 
 import 'dart:async';
 import 'dart:developer';
-import 'dart:io'
-    show Platform, Process, ProcessException, ProcessResult, systemEncoding;
+import 'dart:io' show Process, ProcessException, ProcessResult, systemEncoding;
 
 import 'package:file/file.dart';
 import 'package:logging/logging.dart';
@@ -32,11 +31,12 @@ Future<RunProcessResult> runProcess({
   final printWorkingDir =
       workingDirectory != null &&
       workingDirectory != filesystem.currentDirectory.uri;
+  String quoteIfSpaced(String s) => s.contains(' ') ? '"$s"' : s;
   final commandString = [
     if (printWorkingDir) '(cd ${workingDirectory.toFilePath()};',
     ...?environment?.entries.map((entry) => '${entry.key}=${entry.value}'),
-    executable.toFilePath(),
-    ...arguments.map((a) => a.contains(' ') ? "'$a'" : a),
+    quoteIfSpaced(executable.toFilePath()),
+    ...arguments.map(quoteIfSpaced),
     if (printWorkingDir) ')',
   ].join(' ');
   logger?.info('Running `$commandString`.');
@@ -58,9 +58,11 @@ Future<RunProcessResult> runProcess({
       workingDirectory: workingDirectory?.toFilePath(),
       environment: environment,
       includeParentEnvironment: includeParentEnvironment,
-      runInShell:
-          Platform.isWindows &&
-          (!includeParentEnvironment || workingDirectory != null),
+      // Never run through a shell. On Windows, running an executable through
+      // `cmd.exe /c` mangles the command line when more than one argument is
+      // quoted (cmd strips the outer quotes when the line contains more than
+      // two quote characters), which breaks any invocation whose executable
+      // and arguments contain spaces.
     );
 
     final stdoutSub = process.stdout.listen((List<int> data) {

pkgs/hooks_runner/test/build_runner/version_skew_test.dart

@@ -30,7 +30,7 @@ void main() async {
           )).success;
           expect(result.encodedAssets.length, 1);
         }
-      });
+      }, useSpacesInPath: false);
     },
   );
 
@@ -55,7 +55,7 @@ void main() async {
           logMessages.join('\n'),
           stringContainsInOrder(['Unhandled exception']),
         );
-      });
+      }, useSpacesInPath: false);
     },
   );
 }

pkgs/hooks_runner/test/helpers.dart

@@ -32,8 +32,13 @@ Future<void> inTempDir(
   Future<void> Function(Uri tempUri) fun, {
   String? prefix,
   bool keepTemp = false,
+  bool useSpacesInPath = true,
 }) async {
-  final tempDir = await Directory.systemTemp.createTemp(prefix);
+  final basePrefix = prefix ?? 'hooks_runner_test';
+  final effectivePrefix = (!useSpacesInPath || basePrefix.contains(' '))
+      ? basePrefix
+      : '$basePrefix with spaces ';
+  final tempDir = await Directory.systemTemp.createTemp(effectivePrefix);
   // Deal with Windows temp folder aliases.
   final tempUri = Directory(
     await tempDir.resolveSymbolicLinks(),
@@ -57,8 +62,16 @@ Future<void> inTempDir(
   }
 }
 
-Future<Uri> tempDirForTest({String? prefix, bool keepTemp = false}) async {
-  final tempDir = await Directory.systemTemp.createTemp(prefix);
+Future<Uri> tempDirForTest({
+  String? prefix,
+  bool keepTemp = false,
+  bool useSpacesInPath = true,
+}) async {
+  final basePrefix = prefix ?? 'hooks_runner_test';
+  final effectivePrefix = (!useSpacesInPath || basePrefix.contains(' '))
+      ? basePrefix
+      : '$basePrefix with spaces ';
+  final tempDir = await Directory.systemTemp.createTemp(effectivePrefix);
   // Deal with Windows temp folder aliases.
   final tempUri = Directory(
     await tempDir.resolveSymbolicLinks(),

pkgs/hooks_runner/test/utils/run_process_test.dart

@@ -0,0 +1,94 @@
+// Copyright (c) 2023, 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.
+
+// Regression test for running commands whose executable path or arguments
+// contain a space (e.g. the default pub cache under a Windows user name with
+// a space, `C:\Users\First Last\AppData\Local\Pub\Cache\...`).
+//
+// On Windows, `runProcess` runs through `cmd.exe` (`runInShell`) whenever a
+// `workingDirectory` is passed. `cmd.exe`'s `/c` quote-stripping rule mangles
+// the command line as soon as more than one token needs quoting because it
+// contains a space (the executable path and an argument, or two arguments),
+// causing errors like
+// `'C:\Program' is not recognized as an internal or external command`.
+
+import 'dart:io';
+
+import 'package:test/test.dart';
+
+import '../helpers.dart';
+
+void main() {
+  late Uri scriptUri;
+
+  setUpAll(() async {
+    final scriptDir = await tempDirForTest();
+    scriptUri = scriptDir.resolve('echo_args.dart');
+    await File.fromUri(scriptUri).writeAsString('''
+void main(List<String> args) {
+  print('ARGC:\${args.length}');
+  print('ARGV:\${args.join('|')}');
+}
+''');
+  });
+
+  test(
+    'runProcess handles an executable path containing a space',
+    timeout: const Timeout.factor(5),
+    () async {
+      final outDir = await tempDirForTest();
+      final exeUri = outDir.resolve(
+        'echo args${Platform.isWindows ? '.exe' : ''}',
+      );
+      final compileResult = await Process.run(Platform.resolvedExecutable, [
+        'compile',
+        'exe',
+        scriptUri.toFilePath(),
+        '-o',
+        exeUri.toFilePath(),
+      ]);
+      expect(compileResult.exitCode, 0, reason: '${compileResult.stderr}');
+
+      final workingDir = await tempDirForTest();
+      final result = await runProcess(
+        executable: exeUri,
+        arguments: ['an argument with spaces'],
+        workingDirectory: workingDir,
+        logger: logger,
+      );
+
+      expect(result.exitCode, 0);
+      expect(result.stdout, contains('ARGC:1'));
+      expect(result.stdout, contains('ARGV:an argument with spaces'));
+    },
+  );
+
+  test('runProcess handles arguments containing a space', () async {
+    final workingDir = await tempDirForTest();
+    final result = await runProcess(
+      executable: Uri.file(Platform.resolvedExecutable),
+      arguments: [scriptUri.toFilePath(), 'first arg', 'second arg'],
+      workingDirectory: workingDir,
+      logger: logger,
+    );
+
+    expect(result.exitCode, 0);
+    expect(result.stdout, contains('ARGC:2'));
+    expect(result.stdout, contains('ARGV:first arg|second arg'));
+  });
+
+  test('runProcess handles arguments containing a space and quotes', () async {
+    final workingDir = await tempDirForTest();
+    final result = await runProcess(
+      executable: Uri.file(Platform.resolvedExecutable),
+      arguments: [scriptUri.toFilePath(), 'fir"st arg', 'sec\'ond arg'],
+      workingDirectory: workingDir,
+      logger: logger,
+    );
+
+    expect(result.exitCode, 0);
+    expect(result.stdout, contains('ARGC:2'));
+    expect(result.stdout, contains('ARGV:fir"st arg|sec\'ond arg'));
+  });
+}