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

Collect both stdout and stderr in mage/sh #356

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

coryrc
Copy link

@coryrc coryrc commented Jun 23, 2021

sh.BothOutput and sh.BothOutputWith

follows existing naming conventions.

sh.BothOutput and sh.BothOutputWith

follows existing naming conventions.
@coryrc
Copy link
Author

coryrc commented Jun 24, 2021

@DavidGamba (sorry, I can't find the comment in the UI, only e-mail!?)

CombinedOutput returns stdout and stderr in one stream, here we're returning them as separate streams. There isn't precedent in either os/exec or this package so I thought "BothOutput" but not wedded to it, of course.

@DavidGamba
Copy link

@coryrc I sent that message before reading the code more in depth and that is why I deleted it afterwards, I realized it made no sense since the use case is different.

@coryrc
Copy link
Author

coryrc commented Jun 24, 2021

@DavidGamba Thought I was losing it!

Hey, since you're here, what do you think of *Output*V() (added V) versions which simultaneously stream to os.Stdout and os.Stderr?
Or are we just encoding all permutations of exec.Cmd in variable names?

@DavidGamba
Copy link

Adding a *V version to BothOutput doesn't seem necessary since you are capturing the output, you could print it right after (though it won't be "live").

What is your use case here? I haven't found many cases where capturing stderr out of context within stdout is what I want and I would love to know how you plan to use this.

@coryrc
Copy link
Author

coryrc commented Jun 24, 2021

As part of CI tests I can popup errors to the user why it stopped. Stdout can have too much context and just Stderr is often enough to figure out what went wrong without having to page through a lot of output.

Without live streaming, you don't get any error message if the process is terminated due to timeout, which makes diagnosing hangs a real pain. Also an annoying user experience running locally, because it's hard to know if something is hung or slow.

@jufemaiz
Copy link

jufemaiz commented Nov 3, 2022

@coryrc any further thoughts on the issues you've identified?

@coryrc
Copy link
Author

coryrc commented Nov 4, 2022

@coryrc any further thoughts on the issues you've identified?

No. I just used said functions in my own library, it worked fine. I haven't used it for a while though.

@jufemaiz
Copy link

Aaaah fair enough.

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

Successfully merging this pull request may close these issues.

None yet

3 participants