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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[馃殌 Feature]: Use Process.spawn instead of the child_process gem #11251

Closed
eregon opened this issue Nov 10, 2022 · 8 comments
Closed

[馃殌 Feature]: Use Process.spawn instead of the child_process gem #11251

eregon opened this issue Nov 10, 2022 · 8 comments
Assignees

Comments

@eregon
Copy link

eregon commented Nov 10, 2022

Feature and motivation

Since selenium is Ruby 2.7+ it can use Process.spawn which exists since 2.4.
Process.spawn covers the whole API of child_process, so there is no need for that dependency and it should be fairly easy to remove it.
child_process is unfortunately not very actively maintained, and that causes issues, for instance oracle/truffleruby#1525 / enkessler/childprocess#172.

Usage example

As a result, selenium would have 1 less dependency, one less piece of highly platform-dependent code, and runs better on more platforms.

@github-actions
Copy link

@eregon, thank you for creating this issue. We will troubleshoot it as soon as we can.


Info for maintainers

Triage this issue by using labels.

If information is missing, add a helpful comment and then I-issue-template label.

If the issue is a question, add the I-question label.

If the issue is valid but there is no time to troubleshoot it, consider adding the help wanted label.

If the issue requires changes or fixes from an external project (e.g., ChromeDriver, GeckoDriver, MSEdgeDriver, W3C), add the applicable G-* label, and it will provide the correct link and auto-close the issue.

After troubleshooting the issue, please add the R-awaiting answer label.

Thank you!

@p0deje p0deje self-assigned this Nov 11, 2022
@eregon
Copy link
Author

eregon commented Nov 11, 2022

Another advantage is Process.spawn is faster than fork+exec s used by child_process, because that can use vfork() or posix_spawn for instance.

p0deje added a commit that referenced this issue Nov 11, 2022
Process.spawn is cross-platform in Ruby 2.7+ and ChildProcess is not
actively maintained at the moment. Fixes #11251.
@p0deje p0deje closed this as completed in 969b734 Nov 11, 2022
@p0deje
Copy link
Member

p0deje commented Nov 11, 2022

Let's see how it goes, I've also added TruffleRuby to the list of Rubies for unit tests on CI 04ede9e

@eregon
Copy link
Author

eregon commented Nov 14, 2022

Thank you, that was fast!

I see the CI failed at https://github.com/SeleniumHQ/selenium/actions/runs/3448105944/jobs/5754819891
It seems LANG is not set or set to C, and HOME is not set either. Are those variables unset on purpose?

@p0deje
Copy link
Member

p0deje commented Nov 14, 2022

I've fixed it in a2d7632. Those variables are unset by default by Bazel, so I had to explicitly propagate them. I suppose TruffleRuby also suffers from jruby/jruby#5661

@eregon
Copy link
Author

eregon commented Nov 14, 2022

Thank you, I've filed oracle/truffleruby#2784 to account for that.
I'm rather surprised Bazel changes the ENV like this, but that's how it is.

And with your change it seems like TruffleRuby passes the test suite, nice! https://github.com/SeleniumHQ/selenium/actions/runs/3460208424/jobs/5776525182

@p0deje
Copy link
Member

p0deje commented Nov 14, 2022

Bazel unsets everything in ENV by default to make builds more hermetic.

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants