-
-
Notifications
You must be signed in to change notification settings - Fork 205
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
Redirect output across streams #423
Conversation
stream.write(output) | ||
def redirect(stream, pipes:) | ||
Tee.open(*pipes, stdout: nil) do |pipe| | ||
while line = stream.gets do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not use do
with multi-line while
.
369c37c
to
52cbe75
Compare
stream.write(output) | ||
def redirect(stream, pipes:) | ||
Tee.open(*pipes, stdout: nil) do |pipe| | ||
while line = stream.gets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the best way to redirect STDERR
/ STDOUT
to multiple output streams? The use case is to redirect STDOUT
to both a log file and STDOUT
(which explains the dependence on a tee
Ruby implementation, given the trouble we've had with using a shell tee
implementation)
@thoughtbot/shell I'm unsure if this while
loop (within a Ruby block) will block other output streams (imagine a case where a command is writing to STDOUT
, encounters an error, wants to write to STDERR
, then continue to write to STDOUT
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know the tee
gem or too much about the nuances Ruby's process-running features, so it's hard to weigh in on the general approach.
It's not generally a problem with blocking, as STDOUT
and STDERR
have pretty large buffers which protect writers from being blocked. But it is possible.
This will also stream all STDOUT
messages first, then all STDERR
messages, which may be unexpected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bernerdschaefer this PR is a follow up to #420.
The idea here is that streaming up-to-date information is better than waiting until the subprocess completes, then dumping all output.
Do you have suggestions for a better approach that also handles STDOUT
/ STDERR
redirection from Ruby-land?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In capybara-webkit, we used IO.copy_stream in a background thread.
https://github.com/thoughtbot/capybara-webkit/blob/master/lib/capybara/webkit/connection.rb#L103
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jferris thanks for the suggestion.
I've tried copy_stream
before, but not from a background thread -- is the Thread necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe copy_stream
is blocking, so it won't ever return for an infinite stream.
6e0306e
to
6eba11b
Compare
"echo '#{out}'", | ||
"echo '#{err}' > /dev/stderr", | ||
"exit 1", | ||
].join('; ') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
Thread.current.abort_on_exception = true | ||
|
||
IO.copy_stream(stream, pipe) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jferris thanks for the tip about IO.copy_stream
.
Does this implementation look ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jferris on top of that, now that we've introduce asynchrony, what's a good way to test this?
I've manually verified that the expected behavior happens outside of the test suite, but we're currently failing erratically, with different failures each time:
1) EmberCli::Runner#run! when the command fails writes errors to `err` streams
Failure/Error: expect(error.split).to eq(%w[err err])
expected: ["err", "err"]
got: []
(compared using ==)
# ./spec/lib/ember_cli/runner_spec.rb:36:in `block (5 levels) in <top (required)>'
# ./spec/lib/ember_cli/runner_spec.rb:35:in `each'
# ./spec/lib/ember_cli/runner_spec.rb:35:in `block (4 levels) in <top (required)>'
2) EmberCli::Runner#run! when the command fails writes output to `out` streams
Failure/Error: expect(output.split).to eq(%w[out out])
expected: ["out", "out"]
got: ["out"]
(compared using ==)
# ./spec/lib/ember_cli/runner_spec.rb:19:in `block (5 levels) in <top (required)>'
# ./spec/lib/ember_cli/runner_spec.rb:18:in `each'
# ./spec/lib/ember_cli/runner_spec.rb:18:in `block (4 levels) in <top (required)>'
1) EmberCli::Runner#run! when the command fails writes output to `out` streams
Failure/Error: expect(output.split).to eq(%w[out out])
expected: ["out", "out"]
got: []
(compared using ==)
# ./spec/lib/ember_cli/runner_spec.rb:19:in `block (5 levels) in <top (required)>'
# ./spec/lib/ember_cli/runner_spec.rb:18:in `each'
# ./spec/lib/ember_cli/runner_spec.rb:18:in `block (4 levels) in <top (required)>'
2) EmberCli::Runner#run! when the command fails writes errors to `err` streams
Failure/Error: expect(error.split).to eq(%w[err err])
expected: ["err", "err"]
got: ["err"]
(compared using ==)
# ./spec/lib/ember_cli/runner_spec.rb:36:in `block (5 levels) in <top (required)>'
# ./spec/lib/ember_cli/runner_spec.rb:35:in `each'
# ./spec/lib/ember_cli/runner_spec.rb:35:in `block (4 levels) in <top (required)>'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you close the input stream, copy_stream
will end. If you want to wait until the thread is finished, you'll have to store the thread somewhere and call wait
.
`Runner` will "stream" output to `STDOUT`, `STDERR`, and each of the associated streams instead of waiting until the subprocesses complete.
tee
gem to redirect output across streams
Runner
will "stream" output toSTDOUT
,STDERR
, and each of theassociated streams instead of waiting until the subprocesses complete.