Skip to content

Conversation

@headius
Copy link
Contributor

@headius headius commented Sep 29, 2021

Tweaks and additions to support JRuby.

This adds JRuby's logic used on platforms where we do not have
native access to posix_spawn and related posix functions needed
to do fully-native subprocess launching and management. The code
here instead uses the JDK ProcessBuilder logic to simulate most
of the Open3 functionality.

This code does not pass all tests, currently, but provides most of
the key functionality on pure-Java (i.e. no native FFI) platforms.
@headius
Copy link
Contributor Author

headius commented Sep 29, 2021

@hsbt This is mergeable now, but there are some open questions:

Once this is released as a gem, JRuby will be able to eliminate its local copy of open3 and use the gem instead.

@headius headius marked this pull request as ready for review September 29, 2021 19:17
Co-authored-by: Olle Jonsson <[email protected]>
This allows the wrapper functions in the main open3 to be defined
while using our ProcessBuilder logic for the internal popen
implementation.

Note this adds logic to reject redirects from a numeric fd to a
live IO object (or not a String or to_path object) since we cannot
support direct IO redirects with ProcesBuilder.

This patch allows tests to complete with the ProcessBuilder impl.
Only three tests fail:

* test_numeric_file_descriptor2 and test_numeric_file_descriptor2
  fail due to redirecting streams to a pipe IO.
* test_pid fails expecting a real PID which we cannot provide via
  ProcessBuilder.
RUBY_PLATFORM on JRuby is always 'java' so it does not indicate
the host OS.
@headius
Copy link
Contributor Author

headius commented Sep 30, 2021

This is now complete. I did an additional pass to update and improve the JDK logic from JRuby 9.4, which now errors out for unsupported redirects and reuses the wrapper functions from open3.rb. The JDK version passes all but three tests:

  • test_numeric_file_descriptor2 and test_numeric_file_descriptor2 fail due to redirecting streams to a pipe IO, unsupportable using JDK's ProcessBuilder.
  • test_pid fails expecting a real PID which we is not exposed by ProcessBuilder.

JRuby's CI runs could add Windows, but these tests would need to be masked out.

Let me know what more I need to do to get this released!

@headius
Copy link
Contributor Author

headius commented Dec 7, 2021

@hsbt Ping! I believe this is ready to merge, modulo any recent changes to master. See also the change to jit_support.rb I detailed in #2 which is in this PR and necessary to pass tests (JRuby cannot support CRuby's JIT APIs).

@hsbt hsbt merged commit 5360d8d into ruby:master Dec 9, 2021
@headius headius deleted the jruby branch March 1, 2023 21:19
@headius headius mentioned this pull request Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants