Add `ProcessManager` and `sharedStdIn` #6

matanlurey · 2017-07-09T22:09:38Z

See the examples in README.md for a better overview of these features.

kevmoo

Just some general questions.

What's your use-case for this?

kevmoo · 2017-07-09T22:51:33Z

README.md

+A higher-level service for spawning and communicating with processes.
+
+##### Use `spawn` to create a process similar to how a bash/shell script would:


Instead of similar to how a bash/shell script would I'd just say what it does – forwards stdout/err – and...what else?

Fixed! Thanks.

kevmoo · 2017-07-09T22:52:04Z

README.md

+
+  // Closes stdin for the entire program.
+  await sharedStdIn.terminate();


What does this do, exactly? Practically speaking?

Closes the underlying stdin stream (allowing the isolate to exit).

kevmoo · 2017-07-09T22:53:04Z

lib/src/process_manager.dart

+        );
+}
+
+class _WindowsProcessManager extends ProcessManager {


Is the idea that this would do something different at some point?

matanlurey · 2017-07-09T22:58:50Z

@kevmoo:

What's your use-case for this?

Good question. A few things really:

I'd like to encourage package authors to use Dart instead of shell/bash scripts for simple tooling, but one reason that's really hard is that the default behavior of shell/bash is to spawn processes and auto-forward std[in|out|err], which is nice if you are running interactive processes.
For CLI projects or packages that have a CLI (like angular_test) you have to write brittle/repetitive code to run other processes but intercept their std[in|out|err] and use in your own program. I'd like to make this use case much easier.
If you look on github/stack overflow, using stdin in Dart is really hard because you can subscribe once, and then never again. It's easy enough to use some sort of shared global state if you are writing a single script, but much harder if you are writing modular code.

matanlurey · 2017-07-09T23:02:27Z

I'll wait for @srawlins and @grouma to take a look since both of them are running into cases where these utilities might be useful and I'd like some input.

kevmoo · 2017-07-10T00:02:20Z

SGTM From mobile. Sorry for brevity and/or spelling.

…

On Jul 9, 2017 16:02, "Matan Lurey" ***@***.***> wrote: I'll wait for @srawlins <https://github.com/srawlins> and @grouma <https://github.com/grouma> to take a look since both of them are running into cases where these utilities might be useful and I'd like some input. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABCit9UyQc01dXcu-XgxSfLBShP7a7Tks5sMVwEgaJpZM4OSNBI> .

grouma · 2017-07-10T16:54:36Z

lib/src/shared_stdin.dart

+    }
+    final controller = _getCurrent();
+    if (controller.hasListener) {
+      throw new StateError(''


Why the empty string? I assume for code readability but that's a pattern I haven't come across before.

I think Natalie is actually fixing this in dartfmt, but it is the only way to force the next lines to line up. I'm OK with removing it.

The other way to force a newline is if the string is just over 80 chars when at this indent, and under 80 chars if dartfmt is forced to drop it down to the next line.

grouma · 2017-07-10T16:57:56Z

lib/src/process_manager.dart

+    String executable, {
+    Iterable<String> arguments: const [],
+  }) async {
+    final process = io.Process.start(executable, arguments.toList());


Should we do any error handling here? Could we run into a state where io.Process.start executes but 'await process' never resolves?

I think we wouldn't run into any problems Process.start doesn't already have.

srawlins · 2017-07-10T17:30:34Z

lib/src/process_manager.dart

+  ///
+  /// This is _similar_ to [io.Process.start], but all standard input and output
+  /// is forwarded/routed between the process and the host, similar to how a
+  /// bash or shell script works.


I think "shell script" (minus bash) is adequate here. Unless there are popular shells that don't have the concept of stdin, stdout...

srawlins · 2017-07-10T17:30:45Z

lib/src/process_manager.dart

+/// A process instance created and managed through [ProcessManager].
+///
+/// Unlike one created directly by [io.Process.start] or [io.Process.run], a
+/// spawned process works more like executing a command in a shell or bash


I think "shell script" (minus bash) is adequate here. Unless there are popular shells that don't have the concept of stdin, stdout...

srawlins · 2017-07-10T17:35:03Z

test/process_manager_test.dart

+    test('should output Hello from another process [via stderr]', () async {
+      final spawn = await processManager.spawn(
+        'dart',
+        arguments: [p.join('test', '_files', 'stderr_hello.dart')],


Have you tested whether this test works when your PWD is not the package root? test/_files/stderr_hello.dart is relative to where you typically run pub run test, the repo root, right? I think it won't work if you are, e.g., sitting in the test directory, or at the root of some mega repo that has this as a third_party package :)

For more context, I think the old way to get a relative resource was resolvePackageUri, but then package:test uses isolates, so that doesn't work. package:resource is the new way, which uses configurable imports. Alternatively, mirrors work.

I'm deferring on this one until it matters :)

* Start on process manager. * Ready for review. * Pass on .24 and .25. * Add travis badge. * Actually fix travis. * Update README. * Address feedback.

matanlurey added 2 commits July 9, 2017 13:40

Start on process manager.

86d531a

Ready for review.

df2cfa2

matanlurey requested review from kevmoo, srawlins and grouma July 9, 2017 22:09

matanlurey added 3 commits July 9, 2017 15:17

Pass on .24 and .25.

b7e5c9b

Add travis badge.

3e98927

Actually fix travis.

3344d3d

kevmoo approved these changes Jul 9, 2017

View reviewed changes

Update README.

a9b46d7

grouma reviewed Jul 10, 2017

View reviewed changes

srawlins reviewed Jul 10, 2017

View reviewed changes

grouma approved these changes Jul 10, 2017

View reviewed changes

Address feedback.

c00b765

matanlurey merged commit 2edda5b into dart-archive:master Jul 12, 2017

matanlurey deleted the process-manager branch July 12, 2017 16:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `ProcessManager` and `sharedStdIn` #6

Add `ProcessManager` and `sharedStdIn` #6

matanlurey commented Jul 9, 2017

kevmoo left a comment

kevmoo Jul 9, 2017

matanlurey Jul 9, 2017

kevmoo Jul 9, 2017

matanlurey Jul 9, 2017

kevmoo Jul 9, 2017

matanlurey Jul 9, 2017

matanlurey commented Jul 9, 2017

matanlurey commented Jul 9, 2017

kevmoo commented Jul 10, 2017 via email

grouma Jul 10, 2017

matanlurey Jul 10, 2017

srawlins Jul 10, 2017

grouma Jul 10, 2017

matanlurey Jul 10, 2017

srawlins Jul 10, 2017

matanlurey Jul 12, 2017

srawlins Jul 10, 2017

matanlurey Jul 12, 2017

srawlins Jul 10, 2017

srawlins Jul 10, 2017

matanlurey Jul 12, 2017

		A higher-level service for spawning and communicating with processes.

		##### Use `spawn` to create a process similar to how a bash/shell script would:


		// Closes stdin for the entire program.
		await sharedStdIn.terminate();

Add ProcessManager and sharedStdIn #6

Add ProcessManager and sharedStdIn #6

Conversation

matanlurey commented Jul 9, 2017

kevmoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matanlurey commented Jul 9, 2017

matanlurey commented Jul 9, 2017

kevmoo commented Jul 10, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `ProcessManager` and `sharedStdIn` #6

Add `ProcessManager` and `sharedStdIn` #6