Skip to content
This repository has been archived by the owner on Apr 29, 2021. It is now read-only.

Add run variant with stdout and stderr separated #55

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ahippo
Copy link
Contributor

@ahippo ahippo commented May 21, 2014

The code for storing stdout and stderr separately is taken
from answer [1] by TheConstructor [3] to question [2].

[1] http://stackoverflow.com/a/18086548
[2] http://stackoverflow.com/questions/11027679/bash-store-stdout-and-stderr-in-different-variables
[3] http://stackoverflow.com/users/1266906/theconstructor

@sstephenson
Copy link
Owner

Useful functionality. I'm not a fan of adding a separate command for it, though.

How about always capturing stderr and stdout separately in addition to the existing combined output?

Here's one way to do that, using tee and temporary files: https://twitter.com/sstephenson/status/154603055030083584

I'd suggest $output_stdout, $lines_stdout, $output_stderr, and $lines_stderr for the new variable names.

@tavisrudd
Copy link

+1 for the variant @sstephenson suggests. I bumped into this limitation in the first test I wrote with bats.

@ahippo
Copy link
Contributor Author

ahippo commented Jun 27, 2014

Sorry for the late reply.

Useful functionality. I'm not a fan of adding a separate command for it, though.

How about always capturing stderr and stdout separately in addition to the existing combined output?

That's fine for me too.

Here's one way to do that, using tee and temporary files: https://twitter.com/sstephenson/status/154603055030083584

Thank you for the hint!
I'll take a look at it.
In my proposal I've tried to avoid use of temporary files but It may get too complicated now.

I'd suggest $output_stdout, $lines_stdout, $output_stderr, and $lines_stderr for the new variable names.

Ok.

How should I submit an updated version of run?

  1. push a new changeset to the run-out-err branch on top of the e9fd05c commit
  2. rebase run-out-err branch on top of the current master, then push a new changeset to the run-out-err branch dropping e9fd05c commit
  3. create a new branch and open a new pull request

@ahippo
Copy link
Contributor Author

ahippo commented Jun 27, 2014

Here's one way to do that, using tee and temporary files: https://twitter.com/sstephenson/status/154603055030083584

Thank you for the hint!
I'll take a look at it.
In my proposal I've tried to avoid use of temporary files but It may get too complicated now.

Hmm, it looks like to work unreliably.:/
rm output stdout stderr ; { { echo qqqq ;result=$?;}> >(tee stdout) 2> >(tee stderr >&2);}>output 2>&1 ; cat output stdout stderr
produces different results from one run to another, including:
1.

qqqq
qqqq
cat: stderr: No such file or directory
cat: stdout: No such file or directory
cat: stderr: No such file or directory

So I'll try to come up with another approach.

@ahippo
Copy link
Contributor Author

ahippo commented Jun 30, 2014

Hm, I'm not sure it's possible to capture stdout, stderr and a mixture of the both deterministically due to various pipe buffers.
We may try a small C helper, but I'm still afraid that it can give indeterministic results.

So we can

  1. either capture stdout and stderr only and reconstruct the mixture of them later.
  2. or stick with two different commands: run and run-outerr

I'm afraid, that option 1. will break existing users anyway, so it's no-go.

@duggan
Copy link
Contributor

duggan commented Jul 8, 2014

I'm not sure option 1. is actually going to break things for many users, since redirecting the output already forces buffering of stdout, afaik?

When output is greater than PIPE_BUF (4K on Linux) then users might start to see differences, but relying on interleaved stderr and stdout ordering in this case seems error prone regardless. As a result, it's probably ok to have stderr come before stdout.

I have an implementation of this here (that doesn't make use of tee) that gives what I expect is the same output to the tee example @sstephenson gave.

It passes existing bats tests, as well as those of a project I maintain. If there's anyone with a more complex, interleaved scenario to test, it would be appreciated.

PS, there is a wonderfully evil solution to this suggested here which unbuffers stdout, but I'm not sure it's a fit for bats :)

@ahippo
Copy link
Contributor Author

ahippo commented Jul 10, 2014

I'm not sure option 1. is actually going to break things for many users, since redirecting the output already forces buffering of stdout, afaik?

When output is greater than PIPE_BUF (4K on Linux) then users might start to see differences, but relying on interleaved stderr and stdout ordering in this case seems error prone regardless. As a result, it's probably ok to have stderr come before stdout.

I agree that relying on interleaved stdout and stderr is error-prone, but users had (and still have) no choice.

I have an implementation of this here (that doesn't make use of tee) that gives what I expect is the same output to the tee example @sstephenson gave.

It passes existing bats tests, as well as those of a project I maintain. If there's anyone with a more complex, interleaved scenario to test, it would be appreciated.

Good news, that option 1. passes bats and yours tests!
I think, I underestimated option 1. before.

PS, there is a wonderfully evil solution to this suggested here which unbuffers stdout, but I'm not sure it's a fit for bats :)

Thank you for the very interesting link and stdbuf command!
At times, I definitely miss it.
Unfortunately, it deals only with stdio buffers.

If option 1. will be accepted, we won't need any of these unbuffering tricks.

@ztombol ztombol mentioned this pull request Dec 13, 2016
18 tasks
yarikoptic pushed a commit to neurodebian/bats that referenced this pull request Aug 6, 2019
Closes sstephenson#90. Closes sstephenson#83. Closes sstephenson#56. Closes sstephenson#55. Closes sstephenson#53. Resolves
outstanding issues from sstephenson#32.

Per all these earlier issues and PRs:

- The original bin/bats symlink was causing problems on Windows.
- The attempted fix from sstephenson#32 created problems for install.sh.
- Per sstephenson#90, changing bin/bats to a one-line script in sstephenson#88 broke some Bats
  installations that rely on symlink schemes (such as when installed via
  https://github.com/basherpm/basher).

For an idea of why I wanted to keep bin/bats as a script due to how
symlinks complicate things on Windows, see the following results I
discovered during a search for "git symlink windows":

- https://github.com/git-for-windows/git/wiki/Symbolic-Links
- https://blogs.windows.com/buildingapps/2016/12/02/symlinks-windows-10/
- libgit2/libgit2#4107
- https://stackoverflow.com/q/5917249

In the course of applying these changes, I realized libexec/bats
computed some extraneous variables, so I removed them, eliminating a few
external processes and subshells. I also cleaned up other small bits of
logic.

On top of making install.sh, bin/bats, and libexec/bats more resilient
and compact, the existing test suite (before adding the new
test/installer.bats file) sped up significantly, running 0.6s-0.7s
faster on my MacBook Pro (2.9 GHz Intel Core i5, 8GB RAM):

  Bash 3.2.57(1)-release before:
  58 tests, 0 failures

  real    0m4.924s
  user    0m3.045s
  sys     0m1.798s

  Bash 3.2.57(1)-release after:
  58 tests, 0 failures

  real    0m4.341s
  user    0m2.808s
  sys     0m1.540s

  Bash 4.4.23(1)-release before:
  58 tests, 0 failures

  real    0m5.228s
  user    0m3.046s
  sys     0m1.952s

  Bash 4.4.23(1)-release after:
  58 tests, 0 failures

  real    0m4.582s
  user    0m2.791s
  sys     0m1.643s

Also tweaks the Dockerfile to update the symlink to point to bin/bats,
not libexec/bats.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants