Skip to content
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

ProcessBuilder#environment #5321

Merged
merged 1 commit into from Sep 24, 2018

Conversation

Projects
None yet
3 participants
@ahorek
Copy link
Contributor

ahorek commented Sep 21, 2018

this is a question.

Before this was merged #5316
shelling on windows was broken

irb(main):001:0> `dir`
ArgumentError: wrong number of arguments (1 for 0)
                          ` at uri:classloader:/jruby/kernel/jruby/process_manager.rb:28
                          ` at uri:classloader:/jruby/kernel/jruby/process_manager.rb:50

now it works fine, but according to the javadoc ProcessBuilder#environment don't accept an argument
https://docs.oracle.com/javase/7/docs/api/java/lang/ProcessBuilder.html#environment()

am I missing something?

java version "10.0.2" 2018-07-17
Java(TM) SE Runtime Environment 18.3 (build 10.0.2+13)
Java HotSpot(TM) 64-Bit Server VM 18.3 (build 10.0.2+13, mixed mode)
pavel
env
@enebo

This comment has been minimized.

Copy link
Member

enebo commented Sep 21, 2018

This is not Java 10 but 9.2.0.0 could use ``:

https://gist.github.com/enebo/b3cad2c1479c02b38a1c5b073c3720af

I admit I am confused about pb.environment(env), but I wonder what the difference is here? As I see it working in 9.2.0.0 on win10 and Java 8.

@kares

This comment has been minimized.

Copy link
Member

kares commented Sep 24, 2018

the change makes sense, although seems weird no windows users complained so far.
that line has been in since: https://github.com/jruby/jruby/blob/master/core/src/main/ruby/jruby/kernel.rb#L16
... as seen from the commit, used to only load on Java 7 which is no longer the case on 9.1/9.2

explainable if @enebo sees a warning in $VERBOSE mode: https://github.com/jruby/jruby/blob/master/core/src/main/ruby/jruby/kernel.rb#L16 ... why would the block fail on 10, unable to load ProcessBuilder?

@kares kares added this to the JRuby 9.2.1.0 milestone Sep 24, 2018

@ahorek

This comment has been minimized.

Copy link
Contributor Author

ahorek commented Sep 24, 2018

it was revealed by a different bug that was already fixed. Right now this code isn't loaded so shell works fine,
Maybe the code isn't necessary anymore and it should be removed instead?

@kares

This comment has been minimized.

Copy link
Member

kares commented Sep 24, 2018

it was revealed by a different bug that was already fixed.

ah, makes sense than - yeah Win should not run this code - was meant for GAE (not sure it still matters)
... still it might make sense for limited (constrained) environments without ProcessBuilder functionality

@kares kares merged commit 8935ab0 into jruby:master Sep 24, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.