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

Use Java ProcessBuilder for JRuby process launch #401

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

jemelyah
Copy link

References #400.

For the JRuby implementations, use Java ProcessBuilder to redirect Chrome process output to a specific file instead of stdout.

@route
Copy link
Member

route commented Sep 14, 2023

For JRuby implementation we need to add it to matrix, I wouldn't merge just one bit of the process I believe it's much harder than this. Can you add it to matrix?

@jemelyah
Copy link
Author

@route

Can you add it to matrix?

Sorry, I missed the reference. Do you mean their communications channel on Matrix?

@route
Copy link
Member

route commented Sep 18, 2023

Oh I meant to https://github.com/rubycdp/ferrum/blob/main/.github/workflows/tests.yml
Let's see how it's failing on CI

@jemelyah
Copy link
Author

Oh I meant to https://github.com/rubycdp/ferrum/blob/main/.github/workflows/tests.yml Let's see how it's failing on CI

@route I have added jruby to the tests.yml, but it seems it needs approval to run.

@route
Copy link
Member

route commented Sep 27, 2023

Looks like at some point connection to browser crashes and then after that all tests failing

@route
Copy link
Member

route commented Nov 9, 2023

@jemelyah could you please rebase against latest main branch?

@jemelyah jemelyah force-pushed the issue_400/jruby_process_launch branch from 191a9f4 to 7ac8b6d Compare November 16, 2023 08:50
@jemelyah
Copy link
Author

@route just pushed the merged changes

@jemelyah
Copy link
Author

jemelyah commented Jan 2, 2024

@route would you mind taking a look at the current PR, if it is ok, then I'll try to handle the original issue described here: Issue 402

@route
Copy link
Member

route commented Jan 15, 2024

On it

@jemelyah
Copy link
Author

@route I've updated the code to reflect latest changes in Process class; however, my local tests are still failing, so I'll need to figure out how to handle the problems.

@jemelyah
Copy link
Author

Added JRuby to the matrix once again, lost it somewhere.

@@ -33,7 +33,7 @@
expect(frame.url).to end_with("/ferrum/get_cookie")
end

it "finds main frame properly" do
it "finds main frame properly", skip: Ferrum::Utils::Platform.jruby? do
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JRuby populates browser.pages differently, hece the lookup in this test does not work. Maybe it is necessary to update the test here to fit both rubies.

@jemelyah
Copy link
Author

@route I was finally able to get the tests going locally, by redirecting the process output to the buffered reader instead of a file.
Is it possible to run the test suite once again on the matrix?

@route
Copy link
Member

route commented Mar 3, 2024

@jemelyah how long does it take to tests to pass locally?

@route
Copy link
Member

route commented Mar 3, 2024

I cannot test it on windows but I cannot even install gems on jruby 9.4.5.0 (3.1.4) 2023-11-02 1abae2700f Java HotSpot(TM) 64-Bit Server VM 19.0.1+10-21 on 19.0.1+10-21 +jit [arm64-darwin] on my mac. It's failing.

@jemelyah
Copy link
Author

jemelyah commented Jun 4, 2024

@route, sorry for the delay; we are solving some issues with our solution to this; I hope to update this PR when we are done.

@jemelyah
Copy link
Author

@route, unfortunately, it fails and hangs with Exception handling servers: #<IOError: closed stream> in the test environment for jruby-9.4. Locally, the tests pass, so I'll try to replicate and investigate the closed stream issue.

@route
Copy link
Member

route commented Jun 15, 2024

This looks very familiar to me, as I faced it long ago #15
It works much slower in github actions.

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.

2 participants