diff --git a/pkgs/hooks_runner/CHANGELOG.md b/pkgs/hooks_runner/CHANGELOG.md index ca6126cb19..3bb99d87d9 100644 --- a/pkgs/hooks_runner/CHANGELOG.md +++ b/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 diff --git a/pkgs/hooks_runner/lib/src/utils/run_process.dart b/pkgs/hooks_runner/lib/src/utils/run_process.dart index 6f5707c793..c949fad58a 100644 --- a/pkgs/hooks_runner/lib/src/utils/run_process.dart +++ b/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 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 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 data) { diff --git a/pkgs/hooks_runner/test/build_runner/version_skew_test.dart b/pkgs/hooks_runner/test/build_runner/version_skew_test.dart index 683407b839..bc6e313cb7 100644 --- a/pkgs/hooks_runner/test/build_runner/version_skew_test.dart +++ b/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); }, ); } diff --git a/pkgs/hooks_runner/test/helpers.dart b/pkgs/hooks_runner/test/helpers.dart index bb783a5295..1807d81c14 100644 --- a/pkgs/hooks_runner/test/helpers.dart +++ b/pkgs/hooks_runner/test/helpers.dart @@ -32,8 +32,13 @@ Future inTempDir( Future 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 inTempDir( } } -Future tempDirForTest({String? prefix, bool keepTemp = false}) async { - final tempDir = await Directory.systemTemp.createTemp(prefix); +Future 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(), diff --git a/pkgs/hooks_runner/test/utils/run_process_test.dart b/pkgs/hooks_runner/test/utils/run_process_test.dart new file mode 100644 index 0000000000..25686c6786 --- /dev/null +++ b/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 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')); + }); +}