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

Support JRuby #3

Merged
merged 6 commits into from
Dec 9, 2021
Merged

Support JRuby #3

merged 6 commits into from
Dec 9, 2021

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 <olle.jonsson@gmail.com>
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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants