search

rubyjs / mini_racer

Minimal embedded v8
MIT License
585 stars 91 forks source link

Update to libv8-node 18.x #261

Closed lloeki closed 1 year ago

lloeki commented 1 year ago

I'm not too sure about the changes but a quick rake compile + rake test worked.

Note that even if this works, we may still want a node 17 based release of mini_racer as the requirements for node 18 have increased a lot from node 17 (e.g macOS 10.13 -> macOS 10.15).

For reference: https://stackoverflow.com/a/72754601

tnir commented 1 year ago

@lloeki Are you still on this?

SamSaffron commented 1 year ago

If anyone else is able to take over the process of publishing libv8 and upgrading ... let me know cc @nightpool @seanmakesgames

We are getting so behind here I am super concerned. We are missing many security fixes on libv8 now.

seanmakesgames commented 1 year ago

Looks like there are a few open threads here. I am keen on getting more up to date on everything, but am also weighed down by my own backlog of outdated things. What's the status on tests and supported platforms? Is everything passing and stable before this upgrade? How are we handling node17 vs node18 switching? Can we reduce the complexity of these upgrades by reducing our surface area? Though it already looks like truffleruby isn't in the set of these tests.

lloeki commented 1 year ago

Updated https://github.com/rubyjs/libv8-node/tree/node-18 from node 18.8.0 to latest LTS node 18.13.0.

Can we reduce the complexity of these upgrades by reducing our surface area?

It's fairly limited already:

The changes here are actually quite limited, to eschew these we would need an abstraction level that just doesn't exist in libv8 (this abstraction level is basically mini_racer's C extension...).

The only thing here is I kind of winged these PR changes: it compiles, it appears to run (tests are green), but I am not sure if I did the correct thing semantically.

seanmakesgames commented 1 year ago

@bjfish @eregon I think you two were maintaining the truffleruby stuff (those tests are failing on bundle) -- do you have any idea what's going on with those? https://github.com/rubyjs/mini_racer/actions/runs/3856556180/jobs/6572899343

seanmakesgames commented 1 year ago

@lloeki I assume we are waiting / working on the publish of libv8.

Because every version of mini_racer depends on libv8-node ~> 18.13.0.
and libv8-node ~> 18.13.0.0 could not be found in rubygems repository
eregon commented 1 year ago

That error on TruffleRuby is caused by the changes in Gemfile: https://github.com/rubyjs/mini_racer/pull/261/files#diff-d09ea66f8227784ff4393d88a19836f321c915ae10031d16c93d67e6283ab55f So basically libv8-node is tried to be compiled on TruffleRuby, but this is meaningless because we just use Graal.js so don't need to compile V8 on TruffleRuby. And the way it works when installing mini_racer is TruffleRuby downloads the precompiled version of libv8-node (to satisfy the gem dependency) and just doesn't use it.

One solution is to copy https://github.com/rubyjs/mini_racer/blob/b05d6914c55d26ddb4a59806b13b68434e7d73c1/ext/mini_racer_extension/extconf.rb#L3-L6 to https://github.com/rubyjs/libv8-node/blob/master/ext/libv8-node/extconf.rb , i.e., make it a noop to "build via extconf.rb" libv8-node on TruffleRuby.

Of course the LIBV8_NODE_GIT_BRANCH is probably temporary, so it's also fine to just ignore these failures on truffleruby, and rerun the CI once there is a libv8-node release, then it will work without any change for truffleruby.

seanmakesgames commented 1 year ago

Sweet. Makes sense. Thanks! Am assuming that's what @lloeki is up to. Let me know if you want help getting the libv8-node version out.

seanmakesgames commented 1 year ago

The only thing here is I kind of winged these PR changes: it compiles, it appears to run (tests are green), but I am not sure if I did the correct thing semantically.

If you / we don't trust the tests, I can definitely write a few more targeted ones to give some confidence here.

lloeki commented 1 year ago

Of course the LIBV8_NODE_GIT_BRANCH is probably temporary

Yeah that was an attempt at making such branches work when gems are not published.

I assume we are waiting / working on the publish of libv8.

The above was supposed to work (it did back then, although it was slow since it built from source, but now it doesn't seem to in some cases)

So it doesn't quite work as I expected so I reverted that change.

lloeki commented 1 year ago

I just published libv8-node 18.13.0.0 gems. Now we get stuck with #259 on Linux:

1) Error:
[13](https://github.com/rubyjs/mini_racer/actions/runs/3861797653/jobs/6582979730#step:9:14)
MiniRacerTest#test_locale:
[14](https://github.com/rubyjs/mini_racer/actions/runs/3861797653/jobs/6582979730#step:9:15)
MiniRacer::RuntimeError: RangeError: Invalid language tag: es-mx
[15](https://github.com/rubyjs/mini_racer/actions/runs/3861797653/jobs/6582979730#step:9:16)
    JavaScript at Date.toLocaleDateString (<anonymous>)
[16](https://github.com/rubyjs/mini_racer/actions/runs/3861797653/jobs/6582979730#step:9:17)
    JavaScript at <anonymous>:1:27
[17](https://github.com/rubyjs/mini_racer/actions/runs/3861797653/jobs/6582979730#step:9:18)
    /home/runner/work/mini_racer/mini_racer/lib/mini_racer.rb:228:in `eval_unsafe'
[18](https://github.com/rubyjs/mini_racer/actions/runs/3861797653/jobs/6582979730#step:9:19)
    /home/runner/work/mini_racer/mini_racer/lib/mini_racer.rb:228:in `block (2 levels) in eval'
[19](https://github.com/rubyjs/mini_racer/actions/runs/3861797653/jobs/6582979730#step:9:20)
    /home/runner/work/mini_racer/mini_racer/lib/mini_racer.rb:348:in `timeout'
[20](https://github.com/rubyjs/mini_racer/actions/runs/3861797653/jobs/6582979730#step:9:21)
    /home/runner/work/mini_racer/mini_racer/lib/mini_racer.rb:227:in `block in eval'
[21](https://github.com/rubyjs/mini_racer/actions/runs/3861797653/jobs/6582979730#step:9:22)
    /home/runner/work/mini_racer/mini_racer/lib/mini_racer.rb:225:in `synchronize'
[22](https://github.com/rubyjs/mini_racer/actions/runs/3861797653/jobs/6582979730#step:9:23)
    /home/runner/work/mini_racer/mini_racer/lib/mini_racer.rb:225:in `eval'
[23](https://github.com/rubyjs/mini_racer/actions/runs/3861797653/jobs/6582979730#step:9:24)
    /home/runner/work/mini_racer/mini_racer/test/mini_racer_test.rb:13:in `test_locale'

Note that as I mention in the linked issue:

seanmakesgames commented 1 year ago

this was a blocker for https://github.com/rubyjs/mini_racer/pull/232

Seems like we can / should fix this issue in the 17.x upgrade. Do we have 17.x libv8-node gem published already? Looks like it is. I'll poke at the other open PR.

lloeki commented 1 year ago

@seanmakesgames yeah I published these as well to pin the issue down. To go an even more direct route you could try 16.17.0 which I published too, and which also exhibits the crash.

rvowles commented 1 year ago

I realise this is blocked at least on the linux test fails, but is there likely going to be some movement on this? We cannot install the current version of mini-racer anymore because the python version is incompatible. We have to pin old versions of python just to install lib node 16.10

seanmakesgames commented 1 year ago

@rvowles https://github.com/rubyjs/libv8-node/pull/46 We're working on it!

HLFH commented 1 year ago

Any updates?

SamSaffron commented 1 year ago

I wish I could put a money bounty on this... to be honest. CDCK would pay 10k dolllars to get us on latest libv8 node at this point in time...

lloeki commented 1 year ago

Hey folks, apologies for the silence 🙏 We're getting out of a looong 1+yr crunch stretch at Datadog, which left me with no spare time (or really, energy). I should be able to resume working on rubyjs on my spare time and pick things up starting next week.

seanmakesgames commented 1 year ago

We should be pretty close if I recall. Point @Fayti1703 and I where we can support once you catch back up.

RaitoBezarius commented 1 year ago

Hello there, I am a NixOS developer and we are EOL-ing Node.js 16, we are noticing a lot of downstream software (Discourse notably) using Node.js 16 and not being compatible at least due to mini_racer, just wanted to inform you about https://endoflife.date/nodejs.

SamSaffron commented 1 year ago

As I mentioned earlier , happy to put a 10k dollar bounty on getting us upgraded to latest

On Sat, 20 May 2023 at 8:46 am, Ryan Lahfa @.***> wrote:

Hello there, I am a NixOS developer and we are EOL-ing Node.js 16, we are noticing a lot of downstream software (Discourse notably) using Node.js 16 and not being compatible at least due to mini_racer, just wanted to inform you about https://endoflife.date/nodejs.

— Reply to this email directly, view it on GitHub https://github.com/rubyjs/mini_racer/pull/261#issuecomment-1555348737, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAABIXI5XBSU7A3XOK76ZOTXG7Z6DANCNFSM57WT5I6A . You are receiving this because you commented.Message ID: @.***>

seanmakesgames commented 1 year ago

As I mentioned earlier , happy to put a 10k dollar bounty on getting us upgraded to latest

@SamSaffron I'd love to get on a 30m call with you if you can spare the time. I think we/I can get this moving or done. If you're up for it, I'll send you an email and we can schedule.

eregon commented 1 year ago

FYI, another possibility if it's too tricky to maintain these bindings to V8 could be to embed https://github.com/oracle/graaljs as a native image (i.e. link to libjsvm, part of GraalVM releases), and use the Value, etc API to C/C++ via JNI. It wouldn't be as elegant as the truffleruby backend (which in comparison to V8 bindings is so much easier to maintain) but would be similar logic overall, and probably the Value API could be exposed as Ruby API to limit the amount of C/C++/JNI needed to the minimum. Or in theory we could also embed truffleruby+graaljs in a single native image and then reuse the truffleruby backend. graaljs is a different JS engine than V8 of course, so there would be some differences in performance.

SamSaffron commented 1 year ago

@eregon it certainly sounds interesting but I would prefer to keep to using libv8

The sad thing here is that we did the whole move to nodejs based bundling cause it was meant to be easier - but back in the day it feels like it was a bit easier for @ignisf to push new releases and we are in total standstill here for about a year.

@seanmakesgames contact me please (samsaffron@gmail), it is not an offhand remark that CDCK are willing to fund this, its a super important project and I am extremely worried about shipping JS engine with multiple serious security vulnerabilities given V8 is old and has well known CVEs

lloeki commented 1 year ago

The sad thing here is that we did the whole move to nodejs based bundling cause it was meant to be easier - but back in the day it feels like it was a bit easier for @ignisf to push new releases and we are in total standstill here for about a year.

Agreed, the reasons to move to separate libv8-node packaging were not anecdotal. They notably make "from source" builds of mini-racer (thus independent of Ruby C ABI) able to still fetch ruby-ABI-independent binary builds of libv8-node.

lloeki commented 1 year ago

So, let me try to summarise status, please correct me if I'm wrong:

I now propose this plan:

I realise this may look like a lot of churn, but from experience it has proved extremely risky in complexity to bump to latest right away. Each intermediate version reduces risk towards a final release and provides a fallback spot to have an actual release with the widest OS and compiler support.

WDYT?

lloeki commented 1 year ago
seanmakesgames commented 1 year ago

This is great @lloeki --

Can you number the proposed plan steps so that we can reference them easier? I think status looks good. Plan items have a typo or two, but generally look great.

I agree that likely our best path for quality is to provide options / workarounds to consumers and have them provide feedback from their testing. Our test suite is not as exhaustive as our consumer's suites / usage.

I also like that we are releasing -a version- of 18 early in the plan steps, to get feedback on compat there as soon as possible.

I also remember reading some manual release process steps that we might be able to automate, improving the speed that we get this matrix of versions delivered.

I've opened an issue for discussion on the current version numbering scheme #276 to not derail this thread from the task at hand. :)

lloeki commented 1 year ago

Can you number the proposed plan steps so that we can reference them easier? I think status looks good. Plan items have a typo or two, but generally look great.

Moving that to a dedicated issue we can work off of instead of filling this PR up.

lloeki commented 1 year ago

I also remember reading some manual release process steps that we might be able to automate, improving the speed that we get this matrix of versions delivered.

There are indeed, in my mind everything should be basically doable from CI. Currently this can't be done in practice because:

a) CI is slow (I'm improving that by progressively introducing ccache support, which now works locally, even in Docker) b) aarch64-linux-musl cross-compilation is broken (I started moving towards using clang, an alternative is to build a gcc cross-compiler, which itself takes time to build, so needs to be cached as well, although it's something I've done on another project so it can be reused almost as-is) c) There's an issue with the build system behaving like it's cross-compiling even when it's on the same CPU arch, adding 30-50% time to a) in that case.

So for now I'm building+pushing locally on two different machines (one Intel and one ARM) which is immeasurably faster compared to a).

seanmakesgames commented 1 year ago

a) CI is slow (I'm improving that by progressively introducing ccache support, which now works locally, even in Docker) b) aarch64-linux-musl cross-compilation is broken (I started moving towards using clang, an alternative is to build a gcc cross-compiler, which itself takes time to build, so needs to be cached as well, although it's something I've done on another project so it can be reused almost as-is) c) There's an issue with the build system behaving like it's cross-compiling even when it's on the same CPU arch, adding 30-50% time to a) in that case.

Yep- definitely some interesting challenges here to solve to get us to more hands-off, safer releases. @SamSaffron I haven't looked into it super closely, but I think there's probably a way to throw some money at faster CI runs here. Based on our current engineering activity, it's probably not much, whatever the cost may be. :)

Taking a look at the diff on this PR now.

seanmakesgames commented 1 year ago

Looks like we are merging 18.16 as 0.8.0 which doesn't line up with the plan as written.

This allows every user of mini_racer that can to quickly move to node 18 without us being blocked by potential breakage coming from 18.13.0 -> 18.16.0

I'm probably ok with this if we get the customer bake time we were talking about.

seanmakesgames commented 1 year ago

I do love all the green checkmarks!

seanmakesgames commented 1 year ago

I know there are a lot of people watching this thread, so just a direct call to action for you:

We would love your help testing for stability in your products. Here's the main issue thread where we are tracking progress: https://github.com/rubyjs/mini_racer/issues/277

Please let us know the results of your tests in our branches so we can do the actual releases on those branches with confidence.

ignisf commented 1 year ago

:tada: impressive work

tnir commented 1 year ago

Thanks for excellent work 💞