Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

.start should return an exit code. #100

Closed
bsutton opened this issue Aug 21, 2020 · 7 comments
Closed

.start should return an exit code. #100

bsutton opened this issue Aug 21, 2020 · 7 comments

Comments

@bsutton
Copy link
Collaborator

bsutton commented Aug 21, 2020

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
Copy link
Contributor

passsy commented Apr 3, 2021

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
Copy link
Collaborator Author

bsutton commented Apr 3, 2021

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
Copy link
Contributor

passsy commented Apr 4, 2021

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
Copy link
Collaborator Author

bsutton commented Apr 4, 2021 via email

@bsutton
Copy link
Collaborator Author

bsutton commented Apr 5, 2021

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
Copy link
Contributor

passsy commented Apr 5, 2021

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.

  • .start now returns a Process after the completion which always contains a valid exitCode
  • Passing in a process object is optional to get the exitCode

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
Copy link
Collaborator Author

bsutton commented Apr 5, 2021

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.

@bsutton bsutton closed this as completed in 4110901 Apr 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants