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

open3.rb broken in JRuby #5018

Closed
cshupp1 opened this Issue Jan 29, 2018 · 19 comments

Comments

Projects
None yet
4 participants
@cshupp1

cshupp1 commented Jan 29, 2018

Consider the following:

irb(main):002:0* JRUBY_VERSION
=> "9.1.15.0"
irb(main):003:0> java.lang.System.getProperties['os.name']
=> "Windows 10"
irb(main):006:0> stout, stderr, status = Open3.capture3({}, "bundle exec rake routes")
IOError: Cannot run program "bundle" (in directory "C:\work\VSO\vso"): CreateProcess error=2, The system cannot find the file specified
        from org/jruby/RubyIO.java:4845:in `popen3'
        from uri:classloader:/META-INF/jruby.home/lib/ruby/stdlib/jruby/open3_windows.rb:74:in `popen3'
        from uri:classloader:/META-INF/jruby.home/lib/ruby/stdlib/jruby/open3_windows.rb:272:in `capture3'
        from (irb):6:in `<eval>'
        from org/jruby/RubyKernel.java:994:in `eval'
        from org/jruby/RubyKernel.java:1292:in `loop'
        from org/jruby/RubyKernel.java:1114:in `catch'
        from org/jruby/RubyKernel.java:1114:in `catch'
        from uri:classloader:/META-INF/jruby.home/bin/jirb:13:in `<main>'

Now consider MRI ruby

irb(main):025:0> RUBY_VERSION
=> "2.3.3"
irb(main):029:0>       sttdout, stderr, status = Open3.capture3({}, "bundle exec rake routes")
=> ["\"The version is \"\n\"Setting rails war to use production\"\n    Prefix Verb URI Pattern                      Controller#Action\n      root GET  /                                welcome#index\nfetch_text GET  /fetch_text/
fetch_text(.:format) fetch_text#fetch_text\n", "C:/work/VSO/vso/config/environment.rb:5: warning: already initialized constant WINDOWS\nC:/work/VSO/vso/config/environment.rb:7: warning: already initialized constant CATALINA_HOM
E\n", #<Process::Status: pid 11300 exit 0>]

In MRI ruby we can make a call to bundler. JRuby we cannot.

Please note the following in JRuby:

irb(main):007:0> stout, stderr, status = Open3.capture3({}, "which bundle")
=> ["/c/work/VSO/gem_home/bin/bundle\n", "", #<Process::Status: pid 7304 exit 0>]
irb(main):008:0>

So JRuby should be able to find it.

I have written up the following webpacker bug which shows a workaround, but it just could be that the root cause of the issue is with JRuby.

rails/webpacker#1175

@enebo

This comment has been minimized.

Member

enebo commented Jan 29, 2018

@cshupp1 do you know if this is a regression or likely something which has been broken for the whole 9.1.x series?

@cshupp1

This comment has been minimized.

cshupp1 commented Jan 29, 2018

@enebo It fails for me with 9.1.8 and 9.0.4.0. Can you reproduce it?

@enebo

This comment has been minimized.

Member

enebo commented Jan 29, 2018

@cshupp1 we implement capture in terms of popen3 for windows and @headius happens to have a recent PR #4964 which changes popen3 behavior. Not sure how WIP this PR is but maybe you can try that and see if it fixes anything? (I have not started up Windows to look at this yet).

We probably have a whole class of errors on windows because MRI has a bunch of native code which we have not yet made the equivalent in jnr-posix. This actually expands out to features of IO in general. We would have worked on this but it is a big job.

@cshupp1

This comment has been minimized.

cshupp1 commented Jan 29, 2018

@enebo

How do I get jruby's complete jar?
mvn -Pcomplete?

See this gist:

https://gist.github.com/cshupp1/7916b7027d0163a6b7d8ec32643743bf

@cshupp1

This comment has been minimized.

cshupp1 commented Jan 29, 2018

@enebo
I got it building... I always forget to unset JRUBY_HOME.

irb will not even come up.
https://thepasteb.in/p/WnhzYzywQWQTV

Cris

@cshupp1

This comment has been minimized.

cshupp1 commented Jan 29, 2018

On:

C:\work\jruby\jruby>git branch
* improve_windows_open3
  master

The results are unchanged.

irb(main):004:0> JRUBY_VERSION
=> "9.1.16.0-SNAPSHOT"
irb(main):005:0>
irb(main):001:0> require "open3"
=> true
irb(main):002:0> stout, stderr, status = Open3.capture3({}, "which bundle")
=> ["/c/work/VSO/gem_home/bin/bundle\n", "", #<Process::Status: pid 10692 exit 0>]
irb(main):003:0>  sttdout, stderr, status = Open3.capture3({}, "bundle exec rake routes")
IOError: Cannot run program "bundle" (in directory "C:\work\VSO\vso"): CreateProcess error=2, The system cannot find the file specified
        from org/jruby/RubyIO.java:4847:in `popen3'
        from uri:classloader:/META-INF/jruby.home/lib/ruby/stdlib/jruby/open3_windows.rb:7:in `popen3'
        from uri:classloader:/META-INF/jruby.home/lib/ruby/stdlib/open3.rb:258:in `capture3'
        from (irb):3:in `<eval>'
        from org/jruby/RubyKernel.java:995:in `eval'
        from org/jruby/RubyKernel.java:1316:in `loop'
        from org/jruby/RubyKernel.java:1138:in `catch'
        from org/jruby/RubyKernel.java:1138:in `catch'
        from uri:classloader:/META-INF/jruby.home/bin/jirb:13:in `<main>'
irb(main):004:0>
@cshupp1

This comment has been minimized.

cshupp1 commented Jan 29, 2018

FYI, this does work (Still using improve_windows_open3) (the last one)

irb(main):006:0>  sttdout, stderr, status = Open3.capture3({}, "cmd")
=> ["Microsoft Windows [Version 10.0.16299.192]\r\n(c) 2017 Microsoft Corporation. All rights reserved.\r\n\r\nC:\\work\\VSO\\vso>", "", #<Process::Status: pid 16776 exit 0>]
irb(main):007:0>  sttdout, stderr, status = Open3.capture3({}, "jruby bundle exec rake routes")
=> ["", "Error opening script file: bundle (No such file or directory)\r\n", #<Process::Status: pid 22440 exit 1>]
irb(main):008:0>  sttdout, stderr, status = Open3.capture3({}, "jruby c:/work/vso/gem_home/bin/bundle exec rake routes")
=> ["\"The version is \"\r\n\"Setting rails war to use production\"\r\n    Prefix Verb URI Pattern                      Controller#Action\r\n      root GET  /                                welcome#index\r\nfetch_text GET  /fet
ch_text/fetch_text(.:format) fetch_text#fetch_text\r\n", "C:/work/VSO/vso/config/environment.rb:5: warning: already initialized constant WINDOWS\r\nC:/work/VSO/vso/config/environment.rb:7: warning: already initialized constant
CATALINA_HOME\r\n", #<Process::Status: pid 21152 exit 0>]
@cshupp1

This comment has been minimized.

cshupp1 commented Jan 29, 2018

So for clarity:
stdout, sterr, status = Open3.capture3({}, "bundle.bat exec webpack")
That works on JRUBY
stdout, sterr, status = Open3.capture3({}, "bundle exec webpack")
That fails on jruby

Both work in MRI.

@headius

This comment has been minimized.

Member

headius commented Feb 13, 2018

Open3.capture3({}, "jruby bundle exec rake routes")

This probably should be -S bundle. Wouldn't expect it to work as you show it.

I'll boot up my Windows VM and see what I can see today.

@headius

This comment has been minimized.

Member

headius commented Feb 13, 2018

Ok, so the only problem I see here is that it's not finding bundle.bat, because the following seems to work fine:

>jruby -ropen3 -e "p Open3.capture3({}, 'bundle.bat exec gem list')"
["bundler (1.16.1)\r\n", "", #<Process::Status: pid 9764 exit 0>]

I'll look into the path searching logic, but can you confirm that providing a more specific command makes all your examples work?

headius added a commit that referenced this issue Feb 13, 2018

@headius

This comment has been minimized.

Member

headius commented Feb 13, 2018

Ok, I've pushed some tweaks to the branch that do the following:

  • Enable executable-searching for popen3. We did it for other forms of process-launching, but not this one for some reason.
  • Tweak subshell detection to use cmd.exe when the target executable is a batch or cmd script.

This appears to make your examples all work right.

@headius

This comment has been minimized.

Member

headius commented Feb 14, 2018

Ok, this looks ok in CI and it makes all your examples work, so we'll call it fixed.

@headius headius closed this Feb 14, 2018

@cngshow

This comment has been minimized.

Contributor

cngshow commented Mar 7, 2018

@headius
@enebo

Need this reopened, when trying to generate a war with warbler the dependency on webpacker still fails:

IOError: Cannot run program "bundle" (in directory "C:\Users\Sanjay\git\drc"): C
reateProcess error=2, The system cannot find the file specified
C:/Users/Sanjay/git/drc/gem_home/gems/webpacker-3.2.2/lib/webpacker/compiler.rb:
56:in `run_webpack'

The code referenced is:

sterr, stdout, status = Open3.capture3(webpack_env, "bundle exec webpack")

@enebo enebo modified the milestones: JRuby 9.1.16.0, JRuby 9.1.17.0 Mar 7, 2018

@enebo enebo reopened this Mar 7, 2018

@headius

This comment has been minimized.

Member

headius commented Mar 15, 2018

Is Warbler using JRuby 9.1.16 to generate this war file? It sure looks like it doesn't have my fix.

@cngshow

This comment has been minimized.

Contributor

cngshow commented Mar 20, 2018

Warbler does appear to be using 9.1.16.

Consider the following:

@ c:\temp\crap
$ ls
foo.bat
@ c:\temp\crap
$ foo

@ c:\temp\crap
$ echo I love you Cris!
I love you Cris!

Now consider the following irb:

$ irb
irb(main):001:0> JRUBY_VERSION
=> "9.1.16.0"
irb(main):002:0> require 'open3'
=> true
irb(main):003:0> stdout, sterr, status = Open3.capture3({}, 'foo.bat')
=> ["\r\n\e[32m\e]9;8;\"USERNAME\"\e\\@\e]9;8;\"COMPUTERNAME\"\e\\ \e[92mc:\\temp\\crap\e[90m\r\n\e[90m$\e[m echo I love you Cris! \r\nI love you Cris!\r\n", "", #<Process::Status: pid 16604 exit 0>]
irb(main):004:0> stdout, sterr, status = Open3.capture3({}, 'foo')
IOError: Cannot run program "foo" (in directory "c:\temp\crap"): CreateProcess error=2, The system cannot find the file specified
        from org/jruby/RubyIO.java:4892:in `popen3'
        from C:/languages/ruby/jruby-9.1.16.0/lib/ruby/stdlib/jruby/open3_windows.rb:74:in `popen3'
        from C:/languages/ruby/jruby-9.1.16.0/lib/ruby/stdlib/jruby/open3_windows.rb:272:in `capture3'
        from (irb):4:in `<eval>'
        from org/jruby/RubyKernel.java:995:in `eval'
        from org/jruby/RubyKernel.java:1316:in `loop'
        from org/jruby/RubyKernel.java:1138:in `catch'
        from org/jruby/RubyKernel.java:1138:in `catch'
        from C:/languages/ruby/jruby-9.1.16.0/bin/jirb:13:in `<main>'
irb(main):005:0>

That is unexpected in 9.1.16 right?

@cngshow

This comment has been minimized.

Contributor

cngshow commented Mar 20, 2018

BTW, the bomb out isn't happening in warbler. It is webpacker.

@headius

This comment has been minimized.

Member

headius commented Mar 26, 2018

Hmm...tracing through the logic for Open3.capture3 in JRuby leads me to popen3, popen_run, and then spawn which on Windows goes through our ShellLauncher logic. That logic in turn does appear to be doing the executable search.

Indicating that we should do a search:

LaunchConfig cfg = new LaunchConfig(runtime, rawArgs, true);

Proceeding to the search:

if (doExecutableSearch && shouldVerifyPathExecutable(cmdline) && !cmdBuiltin) {

The search:

public static File findPathExecutable(Ruby runtime, String fname) {

There's also conditional logic that disables the executable search if any of several shell characters (/['"<>|%\\]/) appear in it. That does not appear to be the case here.

I should be able to reproduce this on my Windows VM.

@headius

This comment has been minimized.

Member

headius commented Mar 26, 2018

Ok, I don't have a Windows VM yet on this machine. It will take some time to set up. @enebo if you are looking for something to do...

@headius

This comment has been minimized.

Member

headius commented Apr 12, 2018

There were two popen path fixes made on master that never propagated to 9.1: 1925b5d and b0f7aaf. The first uses path searching for popen, and the second fixes a regression in the first (ignoring system PATH).

I've cherry-picked them to 9.1 as 686fca1 and 85cb4e9.

Fingers crossed, I believe this should fix your issue.

@headius headius closed this Apr 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment