Skip to content
This repository has been archived by the owner on Dec 9, 2023. It is now read-only.

Commit

Permalink
Fix: "ProcessException: %1 is not a valid Win32 application. (#38)
Browse files Browse the repository at this point in the history
  • Loading branch information
Emmanuel Garcia authored and tvolkert committed Aug 6, 2019
1 parent 0a000ae commit 8e9b03b
Show file tree
Hide file tree
Showing 5 changed files with 61 additions and 4 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
#### 3.0.10

* Added quoted strings to indicate where the command name ends and the arguments
begin otherwise, the file name is ambiguous on Windows.

#### 3.0.9

Expand Down
18 changes: 18 additions & 0 deletions lib/src/interface/common.dart
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,24 @@ const Map<String, String> _osToPathStyle = const <String, String>{
'windows': 'windows',
};

/// Sanatizes the executable path on Windows.
/// https://github.com/dart-lang/sdk/issues/37751
String sanitizeExecutablePath(String executable,
{Platform platform = const LocalPlatform()}) {
if (executable.isEmpty) {
return executable;
}
if (!platform.isWindows) {
return executable;
}
if (executable.contains(' ') && !executable.startsWith('"')) {
// Use quoted strings to indicate where the file name ends and the arguments begin;
// otherwise, the file name is ambiguous.
return '"$executable"';
}
return executable;
}

/// Searches the `PATH` for the executable that [command] is supposed to launch.
///
/// This first builds a list of candidate paths where the executable may reside.
Expand Down
20 changes: 17 additions & 3 deletions lib/src/interface/local_process_manager.dart
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ import 'dart:io'
ProcessStartMode,
systemEncoding;

import 'package:platform/platform.dart';

import 'common.dart';
import 'process_manager.dart';

Expand All @@ -38,7 +40,11 @@ class LocalProcessManager implements ProcessManager {
ProcessStartMode mode: ProcessStartMode.normal,
}) {
return Process.start(
_getExecutable(command, workingDirectory, runInShell),
sanitizeExecutablePath(_getExecutable(
command,
workingDirectory,
runInShell,
)),
_getArguments(command),
workingDirectory: workingDirectory,
environment: environment,
Expand All @@ -59,7 +65,11 @@ class LocalProcessManager implements ProcessManager {
Encoding stderrEncoding: systemEncoding,
}) {
return Process.run(
_getExecutable(command, workingDirectory, runInShell),
sanitizeExecutablePath(_getExecutable(
command,
workingDirectory,
runInShell,
)),
_getArguments(command),
workingDirectory: workingDirectory,
environment: environment,
Expand All @@ -81,7 +91,11 @@ class LocalProcessManager implements ProcessManager {
Encoding stderrEncoding: systemEncoding,
}) {
return Process.runSync(
_getExecutable(command, workingDirectory, runInShell),
sanitizeExecutablePath(_getExecutable(
command,
workingDirectory,
runInShell,
)),
_getArguments(command),
workingDirectory: workingDirectory,
environment: environment,
Expand Down
2 changes: 1 addition & 1 deletion pubspec.yaml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
name: process
version: 3.0.9
version: 3.0.10
authors:
- Todd Volkert <tvolkert@google.com>
- Michael Goderbauer <goderbauer@google.com>
Expand Down
21 changes: 21 additions & 0 deletions test/src/interface/common_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,20 @@ void main() {
);
expect(executablePath, isNull);
});

test('when path has spaces', () {
expect(
sanitizeExecutablePath('Program Files\\bla.exe',
platform: platform),
'"Program Files\\bla.exe"');
expect(
sanitizeExecutablePath('ProgramFiles\\bla.exe', platform: platform),
'ProgramFiles\\bla.exe');
expect(
sanitizeExecutablePath('"Program Files\\bla.exe"',
platform: platform),
'"Program Files\\bla.exe"');
});
});

group('on Linux', () {
Expand Down Expand Up @@ -247,6 +261,13 @@ void main() {
);
expect(executablePath, isNull);
});

test('when path has spaces', () {
expect(
sanitizeExecutablePath('/usr/local/bin/foo bar',
platform: platform),
'/usr/local/bin/foo bar');
});
});
});
}
Expand Down

8 comments on commit 8e9b03b

@flar
Copy link
Member

@flar flar commented on 8e9b03b Aug 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is leading to odd quoting issues that cause errors for the engine roller. For example:

Command: "\"C:\Program Files\Git\cmd\git.EXE\"" log -n 1 --pretty=format:%H

@tvolkert
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tvolkert
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@flar is the engine roller blindly quoting the executables? Can it check to see if it's already quoted?

@tvolkert
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or is it vice versa - that the engine roller is making it \"C:\Program Files\Git\cmd\git.EXE\", and package:process is then quoting that string? If so, why the backslashes?

@flar
Copy link
Member

@flar flar commented on 8e9b03b Aug 9, 2019 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zanderso
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did this fix 57a88cd solve this issue? It looks like there was also a Dart VM change that could have been involved: https://dart-review.googlesource.com/c/sdk/+/112267

@blasten
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn’t realize this was fixed in Dart SDK. Sorry about that. Could you revert 57a88cd and 8e9b03b?

@zanderso
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix in the Dart SDK is still being worked on. Let's wait to revert (if needed) until that's landed and sticks.

Please sign in to comment.