Print bundle install output in rails new as soon as it's available

[mxhold]

Previously, running rails new would not print any of the output from
bundle install until all the gems had finished installing. This made
it look like the generator was hanging at the bundle install step.

This is arguably a minor issue, but it can be confusing for new people.

This PR changes how we call the bundle command from using backticks
to using Open3.popen2e, allowing us to read each line of the output as
bundler produces it.

This has the added benefit of including output bundler produces on
standard error, which the previous code ignored since backticks only
capture standard out. This is not a big deal right now since bundler
does not currently print errors to standard error, but that may change
in the future (see: rubygems/bundler/issues/3353).

The one downside here is that since we're using Open3.popen2e, we are
merging standard error and standard out, and outputting them both to
standard out. This is not ideal, but it removes the possibility for a
deadlock that could occur if we used Open3.popen3 instead.

[rafaelfranca]

Does it works on windows machine?

[mxhold]

@rafaelfranca Yes. I tested it on a Windows 7 VM.

[mxhold]

I added a fix for the failing test and squashed my commit. The build is now passing for this branch.

The test was relying on $? having the status for the bundle command but that won't work anymore since it is not getting run via backticks. I changed it to just check that the bundle output contains "Bundle complete!".

[matthewd]

but it removes the possibility for a deadlock

That deadlock only happens if you try to read to EOF on one of the streams, doesn't it? So can't we just Not Do That?

Actually, looking at the "why we use backticks instead of system" comment... wouldn't it make more sense to go back to using system when we want the output, and then either do the less easy thing, or just switch to backticks, when we don't? (I'm guessing there isn't a spelling as simple as spawn(.., out: options[:quiet] ? "/dev/null" : :out) that'll work on Windows..)

[mxhold]

@matthewd honestly this whole deadlock thing was news to me so I may have misunderstood the problem and you might be right. Here's what I found after looking into this some more.

I originally thought Open3.popen3 was perfect for what we wanted and I had something like this:

Open3.popen3(full_command) do |_in, out, err, _t|
  out.each { |line| $stdout.puts line }
  err.each { |line| $stderr.puts line }
end

But then I caught the bit of the documentation that mentions deadlocks:

You should be careful to avoid deadlocks. Since pipes are fixed length buffers, ::popen3(“prog”) {|i, o, e, t| o.read } deadlocks if the program generates too much output on stderr. You should read stdout and stderr simultaneously (using threads or IO.select). However, if you don’t need stderr output, you can use ::popen2. If merged stdout and stderr output is not a problem, you can use ::popen2e. If you really need stdout and stderr output as separate strings, you can consider ::capture3.

You can get a deadlock to happen using my above implementation if you send enough to fill the pipe buffer on standard error before you send anything on standard out:

require 'open3'

command = '$stderr.puts("X" * 70_000); $stdout.puts("X" * 70_000)'
full_command = "ruby -e '#{command}'"
Open3.popen3(full_command) do |_in, out, err, _t|
  out.each { |line| $stdout.puts line }
  err.each { |line| $stderr.puts line }
end

Now, you could say that bundler is pretty unlikely to output enough to standard error to fill the buffer on a single line, which is a good point, but I don't think rails new should hang if it does. But maybe I'm overthinking this?

We can solve this problem while still using Open3.popen3 if we use threads:

require 'open3'

command = '$stderr.puts("E" * 70_000); $stdout.puts("o" * 70_000)'
full_command = "ruby -e '#{command}'"
Open3.popen3(full_command) do |_in, out, err, _t|
  threads = []
  threads << Thread.new { out.each { |line| $stdout.puts line }}
  threads << Thread.new { err.each { |line| $stderr.puts line }}
  threads.each { |t| t.join }
end

This works in Windows too.

I initially picked Open3.popen2e as a compromise, but if you're ok with the added complexity of using threads here, I think the above is the best solution.

To summarize, I think our options are:

1. Close this PR and keep using backticks
   • Good: simplest implementation
   • Good: cannot cause deadlock
   • Bad: stderr is ignored
   • Bad: makes rails new look like its hanging when running bundle install
2. Keep the PR as it is now and use Open3.popen2e
   • Good: does not make rails new look like its hanging
   • Good: stderr is not ignored
   • Good: cannot cause deadlock
   • Good: simpler than the other solutions below
   • Bad: stderr and stdout from bundle install are merged into stdout
3. Use Open3.popen3 without threads
   • Good: does not make rails new look like its hanging
   • Good: stderr is not ignored
   • Good: stderr and stdout are not merged
   • Good: simpler than the solution with threads
   • Bad: causes deadlock if bundler outputs too much to stderr (or stdout if we switch the order)
4. Use Open3.popen3 with threads
   • Good: does not make rails new look like its hanging
   • Good: stderr is not ignored
   • Good: stderr and stdout are not merged
   • Good: cannot cause deadlock
   • Bad: a bit more complex than the other solutions

@matthewd, @rafaelfranca what do you think? sorry for spending your time on something that is pretty minor!

[sivsushruth]

How about just adding a "This step might take some time. Please be patient" line ?

[matthewd]

None of those options seem to be my proposed "use system when we want the output". Am I missing something obvious that precludes it as a strategy?

If we're going to use popen3, I'd expect to use IO.select.. but so far, system (or maybe spawn) seems a much simpler option to me.

[mxhold]

Ahh no you're not missing anything obvious. I was, sorry!

So you're saying something like:

if options[:quiet]
  `#{full_command}`
else
  system(full_command)
end

Yeah that looks much better and solves all the problems.

This might make the intent more clear but I don't think would work on Windows:

if options[:quiet]
  system(full_command, [:out, :err] => '/dev/null')
else
  system(full_command)
end

I'll see if I can get something like that working on Windows and update the PR.

Thanks @matthewd. sorry for my confusion.

[mxhold]

Ok I updated and squashed. The build is passing.

[matthewd]

Looks good 👍

I'm open to being convinced otherwise, but I think we should generally let stderr through, even in "quiet" mode. If something's gone wrong, the user probably wants to know what it was. (See, e.g., grep -q . /)

Meanwhile, I'll refrain from asking (the universe in general, not you in particular) why we have to make this sort of decision at all, instead of just passing --quiet along to bundle install. 😃

[mxhold]

Ok I updated it to not redirect stderr to null in quiet mode.

Yeah I thought about just passing --quiet to bundler, but that is only an option on bundle install so it would break the other usages of bundle_command in quiet mode.

[carlosantoniodasilva]

It might be better to use %Q[] or something like that to build the string, to avoid escaping quotes.

[carlosantoniodasilva]

It probably does not affect our tests, maybe it will add extra noise to some that check the output in case they fail, that might be ok.

@matthewd do you have any other concern about this one?

[mxhold]

I updated it to use %Q[] for interpolation. Any other feedback?

[matthewd]

Sorry, I did have one other thought: can we test this? ("this" == the fact we don't buffer the output, I guess)

Nothing simple comes to mind, but I figure I might as well raise the question, in case you have any ideas.

[mxhold]

Hmm that is a good idea but I can't really think of a good way to test this either. We could test that #bundle_command calls its command with system but that is more of an implementation detail. We could try to replace $stdout with some kind of test spy and expect that generating an app writes to it a certain number of times... I'm not sure that would work and it seems a bit brittle. I'm up for other suggestions if you have any.