From aa90f55ccbabc0f7c0c96b34b3dff87a877cd36a Mon Sep 17 00:00:00 2001 From: Gabriel Terwesten Date: Tue, 6 Sep 2022 13:42:12 +0200 Subject: [PATCH] fix: run commands in single shell This change removes `runInShell: true` from `Process.start` when executing commands. This option is unnecessary since Melos is spawning a shell itself to execute the command. Using `runInShell: true` becomes a problem for command chains (e.g. '&&' or '||'). In this case only the first command is executed in the shell that Melos is spawning and the rest of the commands are executed in the shell spawned by `Process.start`. The change also cleans up a few other related bits of code. Fixes #367 --- .../melos/lib/src/commands/bootstrap.dart | 23 +--- packages/melos/lib/src/commands/exec.dart | 2 +- packages/melos/lib/src/commands/run.dart | 2 +- packages/melos/lib/src/common/utils.dart | 101 ++++++++++-------- packages/melos/lib/src/workspace.dart | 4 +- packages/melos/test/utils_test.dart | 29 +++++ 6 files changed, 97 insertions(+), 64 deletions(-) diff --git a/packages/melos/lib/src/commands/bootstrap.dart b/packages/melos/lib/src/commands/bootstrap.dart index 842e34cdf..1be661e65 100644 --- a/packages/melos/lib/src/commands/bootstrap.dart +++ b/packages/melos/lib/src/commands/bootstrap.dart @@ -173,37 +173,24 @@ mixin _BootstrapMixin on _CleanMixin { Package package, { bool inTemporaryProject = false, }) async { - final execArgs = [ + final command = [ ...pubCommandExecArgs( useFlutter: package.isFlutterPackage, workspace: workspace, ), 'get', if (workspace.config.commands.bootstrap.runPubGetOffline) '--offline' - ]; + ].join(' '); - final executable = currentPlatform.isWindows ? 'cmd' : '/bin/sh'; final packagePath = inTemporaryProject ? join(workspace.melosToolPath, package.pathRelativeToWorkspace) : package.path; - final process = await Process.start( - executable, - currentPlatform.isWindows ? ['/C', '%MELOS_SCRIPT%'] : [], + + final process = await startCommandRaw( + command, workingDirectory: packagePath, - environment: { - utils.envKeyMelosTerminalWidth: utils.terminalWidth.toString(), - 'MELOS_SCRIPT': execArgs.join(' '), - }, - runInShell: true, ); - if (!currentPlatform.isWindows) { - // Pipe in the arguments to trigger the script to run. - process.stdin.writeln(execArgs.join(' ')); - // Exit the process with the same exit code as the previous command. - process.stdin.writeln(r'exit $?'); - } - const logTimeout = Duration(seconds: 10); final packagePrefix = '[${AnsiStyles.blue.bold(package.name)}]: '; void Function(String) logLineTo(void Function(String) log) => diff --git a/packages/melos/lib/src/commands/exec.dart b/packages/melos/lib/src/commands/exec.dart index bb616708c..a5be12a73 100644 --- a/packages/melos/lib/src/commands/exec.dart +++ b/packages/melos/lib/src/commands/exec.dart @@ -74,7 +74,7 @@ mixin _ExecMixin on _Melos { environment.remove(envKeyMelosTerminalWidth); } - return startProcess( + return startCommand( execArgs, logger: logger, environment: environment, diff --git a/packages/melos/lib/src/commands/run.dart b/packages/melos/lib/src/commands/run.dart index 340f38942..80bca3fc1 100644 --- a/packages/melos/lib/src/commands/run.dart +++ b/packages/melos/lib/src/commands/run.dart @@ -158,7 +158,7 @@ mixin _RunMixin on _Melos { .child(runningLabel) .newLine(); - return startProcess( + return startCommand( scriptParts..addAll(extraArgs), logger: logger, environment: environment, diff --git a/packages/melos/lib/src/common/utils.dart b/packages/melos/lib/src/common/utils.dart index 516920cf3..c6910f739 100644 --- a/packages/melos/lib/src/common/utils.dart +++ b/packages/melos/lib/src/common/utils.dart @@ -277,59 +277,76 @@ bool isWorkspaceDirectory(String directory) => bool isPackageDirectory(String directory) => fileExists(pubspecPathForDirectory(directory)); -Future startProcess( - List execArgs, { - String? prefix, - Map environment = const {}, +Future startCommandRaw( + String command, { String? workingDirectory, - bool onlyOutputOnError = false, + Map environment = const {}, bool includeParentEnvironment = true, - required MelosLogger logger, -}) async { - final workingDirectoryPath = workingDirectory ?? Directory.current.path; - final executable = currentPlatform.isWindows ? 'cmd' : '/bin/sh'; - final filteredArgs = execArgs.map((arg) { - // Remove empty args. - if (arg.trim().isEmpty) { - return null; - } - - // Attempt to make line continuations Windows & Linux compatible. - if (arg.trim() == r'\') { - return currentPlatform.isWindows ? arg.replaceAll(r'\', '^') : arg; - } - if (arg.trim() == '^') { - return currentPlatform.isWindows ? arg : arg.replaceAll('^', r'\'); - } - - // Inject MELOS_* variables if any. - environment.forEach((key, value) { - if (key.startsWith('MELOS_')) { - arg = arg.replaceAll('\$$key', value); - arg = arg.replaceAll(key, value); - } - }); - - return arg; - }).where((element) => element != null); +}) { + final executable = currentPlatform.isWindows ? 'cmd.exe' : '/bin/sh'; + workingDirectory ??= Directory.current.path; - final execProcess = await Process.start( + return Process.start( executable, currentPlatform.isWindows ? ['/C', '%MELOS_SCRIPT%'] : ['-c', r'eval "$MELOS_SCRIPT"'], - workingDirectory: workingDirectoryPath, + workingDirectory: workingDirectory, environment: { ...environment, envKeyMelosTerminalWidth: terminalWidth.toString(), - 'MELOS_SCRIPT': filteredArgs.join(' '), + 'MELOS_SCRIPT': command, }, includeParentEnvironment: includeParentEnvironment, - runInShell: true, + ); +} + +Future startCommand( + List command, { + String? prefix, + Map environment = const {}, + String? workingDirectory, + bool onlyOutputOnError = false, + bool includeParentEnvironment = true, + required MelosLogger logger, +}) async { + final processedCommand = command + .map((arg) { + // Remove empty args. + if (arg.trim().isEmpty) { + return null; + } + + // Attempt to make line continuations Windows & Linux compatible. + if (arg.trim() == r'\') { + return currentPlatform.isWindows ? arg.replaceAll(r'\', '^') : arg; + } + if (arg.trim() == '^') { + return currentPlatform.isWindows ? arg : arg.replaceAll('^', r'\'); + } + + // Inject MELOS_* variables if any. + environment.forEach((key, value) { + if (key.startsWith('MELOS_')) { + arg = arg.replaceAll('\$$key', value); + arg = arg.replaceAll(key, value); + } + }); + + return arg; + }) + .where((element) => element != null) + .join(' '); + + final process = await startCommandRaw( + processedCommand, + workingDirectory: workingDirectory, + environment: environment, + includeParentEnvironment: includeParentEnvironment, ); - var stdoutStream = execProcess.stdout; - var stderrStream = execProcess.stderr; + var stdoutStream = process.stdout; + var stderrStream = process.stderr; if (prefix != null && prefix.isNotEmpty) { final pluginPrefixTransformer = @@ -344,12 +361,12 @@ Future startProcess( }, ); - stdoutStream = execProcess.stdout + stdoutStream = process.stdout .transform(utf8.decoder) .transform(pluginPrefixTransformer) .transform>(utf8.encoder); - stderrStream = execProcess.stderr + stderrStream = process.stderr .transform(utf8.decoder) .transform(pluginPrefixTransformer) .transform>(utf8.encoder); @@ -381,7 +398,7 @@ Future startProcess( await processStdoutCompleter.future; await processStderrCompleter.future; - final exitCode = await execProcess.exitCode; + final exitCode = await process.exitCode; if (onlyOutputOnError && exitCode > 0) { logger.stdout(utf8.decode(processStdout, allowMalformed: true)); diff --git a/packages/melos/lib/src/workspace.dart b/packages/melos/lib/src/workspace.dart index 085195603..78eca9ebd 100644 --- a/packages/melos/lib/src/workspace.dart +++ b/packages/melos/lib/src/workspace.dart @@ -183,7 +183,7 @@ class MelosWorkspace { if (childProcessPath != null) 'PATH': childProcessPath!, }; - return utils.startProcess( + return utils.startCommand( execArgs, logger: logger, environment: environment, @@ -203,7 +203,7 @@ class MelosWorkspace { if (childProcessPath != null) 'PATH': childProcessPath!, }; - return utils.startProcess( + return utils.startCommand( execArgs, logger: logger, environment: environment, diff --git a/packages/melos/test/utils_test.dart b/packages/melos/test/utils_test.dart index 94f507ed0..2b8577c4b 100644 --- a/packages/melos/test/utils_test.dart +++ b/packages/melos/test/utils_test.dart @@ -1,11 +1,14 @@ import 'dart:io'; +import 'package:melos/src/common/io.dart'; import 'package:melos/src/common/utils.dart'; +import 'package:melos/src/logging.dart'; import 'package:path/path.dart'; import 'package:path/path.dart' as p; import 'package:pub_semver/pub_semver.dart'; import 'package:test/test.dart'; +import 'matchers.dart'; import 'utils.dart'; void main() { @@ -72,4 +75,30 @@ void main() { ); }); }); + + group('startProcess', () { + test('runs command chain in single shell', () async { + final workspaceDir = createTemporaryWorkspaceDirectory(); + final testDir = p.join(workspaceDir.path, 'test'); + + ensureDir(testDir); + + final logger = TestLogger(); + await startCommand( + [ + 'cd', + 'test', + '&&', + if (Platform.isWindows) 'cd' else 'pwd', + ], + logger: logger.toMelosLogger(), + workingDirectory: workspaceDir.path, + ); + + expect( + logger.output.normalizeNewLines(), + ignoringAnsii('$testDir\n'), + ); + }); + }); }