Use #gets rather than #read to read from pipe in System#call

[nickgrim]

I'm seeing the problem mentioned in #168, whereby calls to both Webdrivers::Chromedriver.browser_version and Webdrivers::ChromeFinder.version block indefinitely. The enclosed patch solves these for me by only reading a single line from the pipe in System.call, as opposed to calling #read, which I assume blocks until EOF(?). It looks like all of the current calling points are only expecting a single-line, but this may be breaking behaviour.

Tests all still pass, but I'm not able to test whether this works on Windows.

My environment, for posterity's sake:

$ cat /etc/lsb-release
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=20.04
DISTRIB_CODENAME=focal
DISTRIB_DESCRIPTION="Ubuntu 20.04 LTS"
$ snap list | grep chromium
chromium                 81.0.4044.138               1143  latest/stable    canonical*    -
$ ruby --version
ruby 2.6.5p114 (2019-10-01 revision 67812) [x86_64-linux]

[berkes]

It seems that after this patch, the windows version is no longer called correctly.

At least, that is what the failing test indicates:

Failures:
  1) Webdrivers::Geckodriver#update when correct binary is not found downloads binary
     Failure/Error: raise "Failed to make system call: #{cmd}" unless $CHILD_STATUS.success?
     RuntimeError:
       Failed to make system call: ["C:/Users/appveyor/.webdrivers/geckodriver.exe", "--version"]
     # ./lib/webdrivers/system.rb:161:in `call'
     # ./lib/webdrivers/common.rb:152:in `binary_version'
     # ./lib/webdrivers/geckodriver.rb:17:in `current_version'
     # ./spec/webdrivers/geckodriver_spec.rb:66:in `block in <main>'
Finished in 21.23 seconds (files took 4.47 seconds to load)

I've not verified wether this test is failing before this patch, but it seems reasonable that the patch is causing the failure.

[berkes]

I'm not familiar with appveyor as a test-runner, and neither do I have a windows machine at hand to build&run on windows.

But what you could do, is revert the patch, as a new commit, push that onto this PR, and see if appveyor then passes.

If it still fails, the test is broken (and should IMO be dealt with in a separate, unrelated ticket). If it does not fail, but passes, revert again (or rebase and force-push) onto this PR branch and see if now the appveyor is failing again.

It's a bit of a time-consuming trial-error, but unless someone with Windows on hand who can run&verify the tests pops in, it might work.

[nickgrim]

Well, I guess that's that then.

I'd be inclined to guess that the reason that this test is failing is not so much that the #gets call isn't returning something useful, but that Windows / JRuby doesn't think that the process has succeeded unless the pipe is fully drained.

Still, I don't have a windows machine to debug this, so I'm going to have to close this.

[berkes]

but that Windows / JRuby doesn't think that the process has succeeded unless the pipe is fully drained.

That was my initial thought as well.

But, rather than closing, maybe leave it open for a windows-user who can debug this or look into it? GH has the option to mark a PR as a WIP, so maybe use that?

[kapoorlakshya]

@nickgrim Thank you for this PR!

The unit tests are passing with your changes on Windows 10 build 1909 using JRuby 9.2.11.0 x64.

$ jruby -v
jruby 9.2.11.1 (2.5.7) 2020-03-25 b1f55b1a40 Java HotSpot(TM) 64-Bit Server VM 25.211-b12 on 1.8.0_211-b12 +jit [mswin32-x86_64]

$ jruby -G -S rake spec
C:/Opts/jruby-9.2.11.1/bin/jruby.exe -I'C:/Opts/jruby-9.2.11.1/lib/ruby/gems/shared/gems/rspec-core-3.9.2/lib';'C:/Opts/jruby-9.2.11.1/lib/ruby/gems/shared/gems/rspec-support-3.9.3/lib' 'C:/Opts/jruby-9.2.11.1/lib/ruby/gems/shared/gems/rspec-core-3.9.2/exe/rspec' -
-pattern 'spec/**{,/*/**}/*_spec.rb'
Coverage may be inaccurate; set the "--debug" command line option, or do JRUBY_OPTS="--debug" or set the "debug.fullTrace=true" option in your .jrubyrc
C:/Opts/jruby-9.2.11.1/lib/ruby/gems/shared/gems/simplecov-0.18.5/lib/simplecov.rb:345: warning: tracing (e.g. set_trace_func) will not capture all events without --debug flag
Run options: include {:focus=>true}

All examples were filtered out; ignoring {:focus=>true}
...................................**********************************........................................................

<-- removed ignored tests -->

Finished in 42.66 seconds (files took 13.66 seconds to load)
125 examples, 0 failures, 34 pending

I am not sure why they're failing on Appveyor with Windows Server 2016 and the same JRuby version though. Only difference is that the Appveyor is using JRuby x86.

I'll go ahead and update the Appveyor build image to Windows Server 2019 with JRuby x64 and see if that makes a difference. I'll let you know what happens.

Edit: Appveyor was already using JRuby x64. I just didn't notice it earlier.

[kapoorlakshya]

Unfortunately, that didn't help. It still fails on Windows Server 2019 with JRuby x64 - https://ci.appveyor.com/project/kapoorlakshya/webdrivers/builds/33044600

[kapoorlakshya]

$CHILD_STATUS on my Windows 10 machine is consistently exiting with 0, as expected, for that unit test:

$CHILD_STATUS: #<Process::Status: pid 4008 exit 0>

but consistently 1 on Appveyor (Windows Server 2019):

$CHILD_STATUS: #<Process::Status: pid 4232 exit 1>

It would be helpful to get confirmation on this behavior from other Windows users.

[kapoorlakshya]

@nickgrim I'm going ahead and closing this PR since the reporting issue #168 is now closed. Please see the final discussion between @berkes and I in #168 for more info.

Thank you again for your contribution!