Make LocalProcessManager portable #3
Conversation
goderbauer
force-pushed
the
addCanRun
branch
from
84641b6
to
5e8d31a
Compare
test/record_test.dart
Outdated
| @@ -15,6 +16,7 @@ import 'utils.dart'; | |||
|
|
|||
| void main() { | |||
| FileSystem fs = new LocalFileSystem(); | |||
| String newline = Platform.isWindows ? '\r\n' : '\n'; | |||
There was a problem hiding this comment.
FYI, I just created dart-lang/platform#1 -- maybe add a comment referencing it here so that when it's implemented, we can switch to use it here.
| /// | ||
| /// Return `null` if the executable cannot be found. | ||
| @visibleForTesting | ||
| String getExecutablePath(String commandName, String workingDirectory, |
There was a problem hiding this comment.
Put in a separate common.dart that's not exported by the library -- so the test can still import it under lib/src, but package users don't get it by accident.
| /// Return `null` if the executable cannot be found. | ||
| @visibleForTesting | ||
| String getExecutablePath(String commandName, String workingDirectory, | ||
| {bool isWindows, List<String> path, List<String> pathExt}) { |
There was a problem hiding this comment.
Rather than bool isWindows, make it Platform platform from package:platform (rather than dart:io's native Platform)
There was a problem hiding this comment.
That's a beautiful abstraction. So much nicer!
| {bool isWindows, List<String> path, List<String> pathExt}) { | ||
| workingDirectory ??= Directory.current.path; | ||
| isWindows ??= Platform.isWindows; | ||
| String pathSeparator = isWindows ? ';' : ':'; |
There was a problem hiding this comment.
Another TODO to refactor once the API is available: dart-lang/platform#2
| Iterable<String> _getCandidatePaths( | ||
| String commandName, List<String> searchPaths, List<String> extensions) { | ||
| List<String> withExtensions = extensions.isNotEmpty | ||
| ? extensions.map((String ext) => '$commandName$ext').toList() |
There was a problem hiding this comment.
Would you ever want to include the version without the extension as well?
There was a problem hiding this comment.
That wouldn't work in Windows. In flutter, we do have bin/flutter and bin/flutter.bat, but only the later is executable on Windows. As far as I can tell, windows doesn't have a concept of executables without extensions.
lib/src/record_replay/manifest.dart
Outdated
| @@ -48,7 +50,13 @@ class Manifest { | |||
| List<Map<String, dynamic>> decoded = new JsonDecoder().convert(json); | |||
| Manifest manifest = new Manifest(); | |||
| decoded.forEach((Map<String, dynamic> entry) { | |||
| manifest._entries.add(new ManifestEntry.fromJson(entry)); | |||
| if (entry['type'] == 'run') { | |||
There was a problem hiding this comment.
switch (entry['type']) { ... }
goderbauer
force-pushed
the
addCanRun
branch
2 times, most recently
from
04f5f5d
to
bb1cbc8
Compare
lib/src/interface/common.dart
Outdated
| @@ -0,0 +1,56 @@ | |||
| import 'dart:io'; | |||
goderbauer
force-pushed
the
addCanRun
branch
from
bb1cbc8
to
1f93467
Compare
|
Update |
goderbauer
force-pushed
the
addCanRun
branch
2 times, most recently
from
9bb2f57
to
84641b6
Compare
LocalProcessManager now uses the same algorithm on Windows and Posix systems to find the executable for a command. Previously, manager.run(['pub']) would work on Linux, but not on Windows because 'pub.bat' could not be located.
goderbauer
force-pushed
the
addCanRun
branch
from
84641b6
to
d027c66
Compare
LocalProcessManagernow uses the same algorithm on Windows and Posix systems to find the executable for a command.Previously,
manager.run(['pub'])would work on Linux, but not on Windows becausepub.batcould not be located.