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

Update to libv8-node 18.x #261

Merged
merged 16 commits into from May 27, 2023
Merged

Update to libv8-node 18.x #261

merged 16 commits into from May 27, 2023

Conversation

lloeki
Copy link
Collaborator

@lloeki lloeki commented Aug 26, 2022

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
Copy link

tnir commented Dec 31, 2022

@lloeki Are you still on this?

@SamSaffron
Copy link
Collaborator

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
Copy link
Contributor

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
Copy link
Collaborator Author

lloeki commented Jan 6, 2023

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:

  • unless there are breaking changes in the build system, updating libv8node is about bumping a couple of version numbers and a checksum.
  • unless there are breaking changes in the libv8 API, updating mini_racer is about changing the dependent version.

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
Copy link
Contributor

@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
Copy link
Contributor

@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
Copy link
Contributor

eregon commented Jan 6, 2023

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

if RUBY_ENGINE == "truffleruby"
File.write("Makefile", dummy_makefile($srcdir).join(""))
return
end
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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Collaborator Author

lloeki commented Jan 7, 2023

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
Copy link
Collaborator Author

lloeki commented Jan 7, 2023

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:

  • the test fails with es-mx
  • the test segfaults when changing the locale from es-mx to fr-fr
  • this seems to have started happening between node 16.10 and node 16.17 (can't recall if I was able to confirm that)
  • this was a blocker for Update to libv8-node 17.x #232

@lloeki lloeki changed the title Update to libv8-node 18.8.0 Update to libv8-node 18.x Jan 7, 2023
@rvowles
Copy link

rvowles commented Jan 30, 2023

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
Copy link
Contributor

@rvowles
rubyjs/libv8-node#46
We're working on it!

@HLFH
Copy link

HLFH commented Apr 26, 2023

Any updates?

@SamSaffron
Copy link
Collaborator

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
Copy link
Collaborator Author

lloeki commented Apr 27, 2023

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
Copy link
Contributor

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

@RaitoBezarius
Copy link

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
Copy link
Collaborator

SamSaffron commented May 19, 2023 via email

@seanmakesgames
Copy link
Contributor

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
Copy link
Contributor

eregon commented May 20, 2023

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
Copy link
Collaborator

@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
Copy link
Collaborator Author

lloeki commented May 23, 2023

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
Copy link
Collaborator Author

lloeki commented May 23, 2023

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

  • Jan 7, libv8-node 18.13.0.0 and 16.19.0.0 were built and pushed for all gem platforms for testing purposes
  • Both are affected by the locale bug
  • The locale bug is caused by an object file archive issue, and requires a package rebuild
  • Fixes for the locale bug have landed on node-16, node-17, and node-18 branches
  • Fixes do not need a code change on mini_racer
  • Node 17 and Node 18 require mini_racer code changes. PRs are open on #271 and #261
  • Node 18 OS requirements have increased a lot, notably dropping not-that-old macOS versions. I proposed having an intermediate mini_racer with node-17 so as not to block these users.
  • aarch64-linux cross-compiling on CI is currently disabled due to some brokenness
  • Node has had releases on 16 and 18 lines, notably Node 16 which had LTS security backports on 2023-03-28
  • Node 19 has been released, with possibly challenging changes and updates to libv8, as is customary for non-LTS node.
  • Node 20 (LTS) has been released, thus Node 19 won't have another bump

I now propose this plan:

  • build node-16, node-17, and node-18 current branch tips locally, and publish the resulting gems to rubygems as 16.19.0.1, 17.9.1.1, and 18.13.0.1.

    This fixes those broken versions WRT the locale bug, and moves us closer to the "up-to-date goal" with builds we know are working.

  • bump mini_racer dependency requirement to ~> 16.19.0.0 and release mini_racer 0.6.4

    This allows every user of current mini_racer to immediately move to a Node safer than 16.10.0 (and safest, short for one version)

  • merge #271 and release mini_racer 0.7.0.

    This allows every user of mini_racer to move to the latest node 17 even for OSes not supported by node 18.

  • merge #261 and release mini_racer 0.8.0.

    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

  • bump node-16 branch to 16.20.0.0, node-18 branch to 18.16.0.0.

  • bump mini_racer dependency requirement to ~> 16.19.0.0 and release mini_racer 0.6.5

    This should be the safest release for Node 16, applying to all users that can't or do not desire to move to a more recent major version.

  • There is no more Node 17 release, so nothing more to do here.

  • bump mini_racer dependency requirement to ~> 18.16.0.0 and release mini_racer 0.8.1.

    This should be the safest release for Node 18, but may not support some older OSes or compilers.

  • create node-19 branch from node-18, attempt update to Node 19.9.0, release once it's fine.

    This should handle most of the gnarliness towards Node 20.

  • bump mini_racer dependency requirement to ~> 19.9.0.0 and release mini_racer 0.9.0.

    An optional step, but one that could make sense depending on Node 20 changes. At least internally it does make sense to proceed with this iterative step.

  • create node-20 branch from node-19, attempt update to Node 20.2.0

    This may have additional changes compared to Node 19 but should be smaller.

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
Copy link
Collaborator Author

lloeki commented May 23, 2023

  • 16.19.0.1 has been built locally for all gem platforms and pushed to rubygems
  • 17.9.1.1 local builds are in progress

@seanmakesgames
Copy link
Contributor

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
Copy link
Collaborator Author

lloeki commented May 23, 2023

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 lloeki mentioned this pull request May 23, 2023
37 tasks
@lloeki
Copy link
Collaborator Author

lloeki commented May 23, 2023

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

I do love all the green checkmarks!

Copy link
Contributor

@seanmakesgames seanmakesgames left a comment

Choose a reason for hiding this comment

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

See comments above

@seanmakesgames
Copy link
Contributor

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:
#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.

@lloeki lloeki merged commit b45a4c7 into master May 27, 2023
21 checks passed
@lloeki lloeki deleted the libv8-node-18 branch May 27, 2023 21:50
@ignisf
Copy link
Collaborator

ignisf commented May 28, 2023

🎉 impressive work

@tnir
Copy link

tnir commented Jun 27, 2023

Thanks for excellent work 💞

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.

None yet

10 participants