JoshCheek / seeing_is_believing

Displays the results of every line of code in your file
1.31k stars 54 forks source link

system prints directly to stdout #24

Closed JoshCheek closed 10 years ago

JoshCheek commented 10 years ago

Examples

This code blew up in SiB before this issue was resolved.

system "echo abc"

Example of the underlying problem:

File.open '/dev/null', 'w' do |black_hole|
  STDERR = $stderr = black_hole
  STDOUT = $stdout = black_hole
  puts "You won't see this, it goes into the black hole"
  system "echo but system gets around it!"
end

The Problem in MRI

There is what I consider a bug (tested in MRI 2.1) where numerous methods such as Kernel#system and Kernel#spawn print directly to the original stdout and stderr file descriptor.

This is because they invoke C's system function from stlib.h, which apparently talks directly to existing "real" output streams even if you override $stdout and STDOUT (same for err)

Following the code:

What this means is that you can't capture the output from commands like Kernel.system("echo a") but it's worse than that, Seeing Is Believing dumps its Result data structure on stdout which is then parsed on the other side. If system commands dump their stdout there, then that fucks up the data, and it will raise a parsing error.

Considering Solutions

Initially I began overriding Kernel#system and the others. So I went to RubySpec to make sure my override did all the same stuff as the original. Which led me to wonder how they got around this issue in their tests, which led to their test framework mspec, which used a solution far smarter than any I was considering, it reopened the stream to capture it.

Actual Solution

So, I used RubySpec's solution, but modified it slightly to use IO.pipe so that I don't have to deal with temp files.

If You Are Swapping Out The Matrix File

You will need to be aware of this and think about how it affects your use case and how to deal with it.

JoshCheek commented 10 years ago

So MSpec uses temp files, and Seeing Is Believing uses IO.pipe.

My reasoning was just that it didn't require me to deal with temp files. But I'm wondering if there's anything I'm failing to consider (ie why didn't MSpec use IO.pipe?)

cc @brixen

brixen commented 10 years ago

@JoshCheek MSpec does not use IO.pipe because it's a significantly more complex operation than writing to a file (which typically requires a pretty simple call to a system library). MSpec was built to be easy to use in a Ruby implementation that didn't have much working yet.

JoshCheek commented 10 years ago

Makes sense. I'm going to stick with IO.pipe since my considerations are different. Thanks!

JoshCheek commented 9 years ago

More things to consider, based on #49:

  1. You cannot catch every exception, because, for example, at_exit { raise "omg" } this will execute after a begin/rescue/end around the whole program.
  2. These types of exceptions are handled by Ruby, before getting to the matrix's at_exit hook, it deals with them by printing some info to stderr (the normal error stuff you see all the time)
  3. This causes a weird difference between exceptions in the code and exceptions in hooks.
  4. You cannot close stderr after the program runs, because it could be legitimate to print to stderr, e.g. at_exit { $stderr.puts "now we run your test suite" }

So, what do we do?

JoshCheek commented 9 years ago

Going to go with "ignore it". If I redefine at_exit, which I did, then it does not set $!. We do this intentionally, to prevent Ruby from printing it to stderr, but it also means the next at_exit hook can't see the error in $! I tried hanging onto it and setting it right before invoking the next at_exit hook, but turns out that is not allowed ($! is a readonly variable -- probably b/c it's threadlocal or something, idk).

I think it's better to have edge cases where output is inconsistent than where behaviour is changed, so going to ignore it.