onepub-dev / dcli

An extensive library and tooling for building console/cli applications and scripts using the Dart programming language.
242 stars 28 forks source link

.start should return an exit code. #100

Closed bsutton closed 3 years ago

bsutton commented 4 years ago

Getting an exit code from a call to .start is clumsy.

You have to create a progress pass it to start and then query the progress.

We did look at having start return the passed in progress (or the auto created one). Is there a reason why we don't do this as getting to the exit code would be much easier.

I end up writting something like:


  var lines = <String>[];
  var progress = Progress((line) {
    print(line);
    lines.add(line);
  }, stderr: (line) {
    printerr(line);
    lines.add(line);
  });

  certbot.start(runInShell: true, nothrow: true, progress: progress);

  if (progress.exitCode != 0) {}
passsy commented 3 years ago

Some here. _wireStreams should capture the output internally and allow access when the process exited.

I expect this to work

final progress = Progress.print();
startFromArgs('some', ['app', 'args'], progress: progress);
final fullOutput = progress.toList();
// more output parsing after process exit here
bsutton commented 3 years ago

This is a difficult area of the API that I'm still trying to find the correct solution for.

You particular example however is problematic as you are essentially trying to consume the output twice.

I don't think this will ever work, however we need to improve the API as it currently invite users to try this sort of operation.

passsy commented 3 years ago

At least for terminal: false I was able to capture the stdout/stderr while printing it to the stream.

import 'package:dcli/dcli.dart';

/// [Progress] that allows reading [output] after the process finished. During execution stdout and stderr are printed
class ProgressWithOutput extends Progress {
  final List<String> _output = [];

  ProgressWithOutput.devNull() : super.devNull();
  ProgressWithOutput.print() : super.print();

  List<String> get output => _output.toList(growable: false);

  @override
  void addToStdout(String line) {
    _output.add(line);
    super.addToStdout(line);
  }

  @override
  void addToStderr(String line) {
    _output.add(line);
    super.addToStdout(line);
  }
}
final progress = ProgressWithOutput.print();
startFromArgs('some', ['app', 'args'], progress: progress);
final fullOutput = progress.output; // yay, got output

While checking the internals I noticed that start doesn't return exitCode correctly with terminal: true.

https://github.com/bsutton/dcli/blob/0687d4ba97791bbabc0fa4e21c2e96fb78da87c2/lib/src/util/runnable_process.dart#L151-L156

This code path is missing setting process.exitCode. With terminal: false it is always set in processUntilExit.

      if (detached == false) {
        if (terminal == false) {
          processUntilExit(progress, nothrow: nothrow);
        } else {
+          process.exitCode = _waitForExit();
        }
bsutton commented 3 years ago

Good pickup.

When detached you can't get an exit code but without a terminal you still can.

Are you able to raise a pr?

On Mon, 5 Apr 2021, 4:42 am Pascal Welsch, @.***> wrote:

At least for terminal: false I was able to capture the stdout/stderr while printing it to the stream.

import 'package:dcli/dcli.dart'; /// [Progress] that allows reading [output] after the process finished. During execution stdout and stderr are printedclass ProgressWithOutput extends Progress { final List _output = [];

ProgressWithOutput.devNull() : super.devNull(); ProgressWithOutput.print() : super.print();

List get output => _output.toList(growable: false);

@override void addToStdout(String line) { _output.add(line); super.addToStdout(line); }

@override void addToStderr(String line) { _output.add(line); super.addToStdout(line); } }

final progress = ProgressWithOutput.print();startFromArgs('some', ['app', 'args'], progress: progress);final fullOutput = progress.output; // yay, got output

While checking the internals I noticed that start doesn't return exitCode correctly with terminal: true.

https://github.com/bsutton/dcli/blob/0687d4ba97791bbabc0fa4e21c2e96fb78da87c2/lib/src/util/runnable_process.dart#L151-L156

This code path is missing setting process.exitCode. With terminal: false it is always set in processUntilExit.

  if (detached == false) {
    if (terminal == false) {
      processUntilExit(progress, nothrow: nothrow);
    } else {+          process.exitCode = _waitForExit();
    }

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bsutton/dcli/issues/100#issuecomment-813080854, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG32OF3TXH6M2TEP4Z4BZDTHCXJ5ANCNFSM4QGYA4KQ .

bsutton commented 3 years ago

I've just reviewed this. So I"m not quite certain what the right answer is.

When running at terminal you don't get any io but we do get an exit code. What I'm uncertain of is whether its the terminals exit code or the under app we spawn via the terminal.

If it's the terminals exit code then it will be meaningless.

I've add code in 0.51.6 (which will be released in the next 15min) that sets the exit code. Can you give it a test to see if returns a meaningful exit code.

passsy commented 3 years ago

I can confirm that startFromArgs now correctly returns the exitCode of the app spawned in terminal. The injected Process also has the correct exitCode set. Thanks for the fast fix!

I think this issue can now be marked as resolved.

My issue - not being able to get the output and stream it to stdout - should be considered separate. I'll create a PR to capture the output inside Process as I did it in ProgressWithOutput.

bsutton commented 3 years ago

Building on your suggestions:


/// [ProgressWithCapture] is a [Progress] that allows reading [lines]
/// after the process finished whilst allowing you to choose if stdout
/// and stderr is printed.
/// Once the process has completed you can call [lines] to obtain
/// an unmodfiable view of the captured list.
class ProgressWithCapture extends Progress {
  /// Use this ctor to capture lines output to stdout and stderr as
  /// well as being able to process the output as it occurs via the [stdout] and
  /// [stdout] [LineAction] parameters.
  /// Call [lines] to obtain the captured output.
  ProgressWithCapture(LineAction stdout, {LineAction stderr = devNull})
      : super(stdout, stderr: stderr);

  /// Use this [ProgressWithCapture] to have both stdout and stderr output
  /// suppressed whilst capturing all output generated by the process.
  /// Call [lines] to obtain the captured output.
  ProgressWithCapture.devNull() : super.devNull();

  /// Use this [ProgressWithCapture] to capture and print stdout.
  /// Call [lines] to obtain the captured output.
  ProgressWithCapture.printStdOut() : super.printStdOut();

  /// Use this [ProgressWithCapture] to capture and print stderr.
  /// Call [lines] to obtain the captured output.
  ProgressWithCapture.printStdErr() : super.printStdErr();

  /// Use this [ProgressWithCapture] to capture and print both stdout
  /// and stderr.
  /// Call [lines] to obtain the captured output.
  ProgressWithCapture.print() : super.print();

  final List<String> _lines = [];

  /// get the list of captured lines.
  List<String> get lines => UnmodifiableListView(_lines);

  @override
  void addToStdout(String line) {
    _lines.add(line);
    super.addToStdout(line);
  }

  @override
  void addToStderr(String line) {
    _lines.add(line);
    super.addToStdout(line);
  }
}

The only thing that comes to mind is whether we should allow the user to be able to split out lines that went to stdout vs stderr. Doing so would make the class much more complex and a user can still do it themselves.