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

Fix TruffleRuby backend after #325 #328

Merged
merged 13 commits into from
Jan 9, 2025

Conversation

eregon
Copy link
Contributor

@eregon eregon commented Jan 8, 2025

See #325 (comment) and after for context.
All tests pass now, so this should fix the CI.

@eregon eregon mentioned this pull request Jan 8, 2025
Copy link
Collaborator

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Left some comments.

lib/mini_racer.rb Outdated Show resolved Hide resolved
Comment on lines +8 to +9
MARSHAL_STACKDEPTH_DEFAULT = 2**9-2
MARSHAL_STACKDEPTH_MAX_VALUE = 2**10-2
Copy link
Collaborator

Choose a reason for hiding this comment

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

FWIW, these are gone; the C code accepts marshal_stack_depth but otherwise ignores it.

@eval_thread = nil

# defined in the C class
init_unsafe(isolate, snapshot)
Copy link
Collaborator

Choose a reason for hiding this comment

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

init_unsafe no longer exists

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does, it's defined in lib/mini_racer/truffleruby.rb.

Comment on lines +137 to +141
def isolate
return @isolate if @isolate != false
# defined in the C class
@isolate = create_isolate_value
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isolates no longer exist (or rather, are not exposed in the Ruby API)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I took a quick look but it's not so easy to remove so that could be done separately and I think best to get the CI green first (e.g. to avoid regressions).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Isolates no longer exist (or rather, are not exposed in the Ruby API)

I only followed the recent changes partially, but why has the Isolates API been removed? I have never used them but they have a big section in the README (https://github.com/rubyjs/mini_racer#shared-isolates). If they are gone for good, the README needs to be cleaned up to and it's a big fat breaking change too, but not mentioned in the CHANGELOG for yesterdays 0.17.0.pre6.

Copy link
Collaborator

Choose a reason for hiding this comment

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

README update in #331. The changelog was Sam's work, I think?

@eregon eregon force-pushed the fix-truffleruby-after-pr-325 branch from 9e9ec35 to ab4c4f5 Compare January 9, 2025 11:01
Copy link
Collaborator

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

I'm probably not the most qualified person to review but LGTM at a quick glance. Thanks for doing this, @eregon.

test/mini_racer_test.rb Outdated Show resolved Hide resolved
Co-authored-by: Ben Noordhuis <info@bnoordhuis.nl>
@SamSaffron SamSaffron merged commit 080836f into rubyjs:main Jan 9, 2025
17 of 25 checks passed
@SamSaffron
Copy link
Collaborator

thank you so much @eregon for the push here, I know how much extra work out of nowhere there was but our upgrade of our v8 integration was critical for the long term survival of MiniRacer, so happy to bring back full support

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.

4 participants