Skip to content
This repository has been archived by the owner on Dec 4, 2023. It is now read-only.

Update to 8.4.255 #301

Merged
merged 5 commits into from Jul 20, 2020
Merged

Update to 8.4.255 #301

merged 5 commits into from Jul 20, 2020

Conversation

nightpool
Copy link
Collaborator

@nightpool nightpool commented Jul 15, 2020

i.e. Use last good version before build error

Using bisect, I was able to find that the build error we saw in #300 and #299 was introduced in v8/v8@5bbca54, so let's use the version before that one while we wait for upstream to merge the fix I submitted (https://bugs.chromium.org/p/v8/issues/detail?id=10708)

closes #298, #299 and #281

@nightpool
Copy link
Collaborator Author

nightpool commented Jul 15, 2020

@runlevel5 @SamSaffron Builds have passed here! Can you please take a look?

@nightpool
Copy link
Collaborator Author

While testing this upgrade on my React SSR application, i figured out that Libv8 wasn't packing ICU data with chrome, which was causing any I18n operation to fail, even ones as simple as (10).toLocaleString(). I pushed up a new version of my Github package for testing, 8.4.255.1, but it increases the binary size by approximately 4.3 MB. (11.9 MB to 16.2 MB). Do you think I should include it? I haven't looked into how complicated it would be to package it separately.

@nightpool
Copy link
Collaborator Author

nightpool commented Jul 15, 2020

The other option, I think, would be to disable i18n support entirely. I'm pretty sure that this would also stop the errors, at the obvious cost of not supporting any of the Intl methods. I haven't tried this yet and have no idea how it would play with React et al or other code that is "expecting" more of a full-featured environment, but it is in some senses the "status quo" for the libv8 gem today.

@runlevel5
Copy link
Contributor

@nightpool nice work. Could you please show us the diff of your patch to address the I18n issue?

@nightpool
Copy link
Collaborator Author

nightpool commented Jul 16, 2020

@runlevel5 sure! it's just one additional argument for gn_args

diff --git a/ext/libv8/builder.rb b/ext/libv8/builder.rb
index 88716d5..7836f5e 100644
--- a/ext/libv8/builder.rb
+++ b/ext/libv8/builder.rb
@@ -23,7 +23,8 @@ module Libv8
          v8_use_external_startup_data=false
          target_cpu="#{libv8_arch}"
          v8_target_cpu="#{libv8_arch}"
-         treat_warnings_as_errors=false).join(' ')
+         treat_warnings_as_errors=false
+         icu_use_data_file=false).join(' ')
     end
 
     def generate_gn_args

This incurs a 4.3MB binary penalty because it compiles the icu i18n data as a static library in with the v8 monolith, but it's by far the simplest solution. (Almost as simple would be to turn off i18n support entirely, but that would be very regrettable for end-user applications that are starting to depend on it, as browsers gain support for the Intl api)

@runlevel5
Copy link
Contributor

@nightpool have you tested against mini_racer yet?

@nightpool
Copy link
Collaborator Author

nightpool commented Jul 16, 2020

@runlevel5 I have! I had to fix one flakey test (one of the max_memory tests had too high of a limit), but other then that everything passed.

@runlevel5
Copy link
Contributor

runlevel5 commented Jul 16, 2020

Since I do not have the full knowledge whether ICU i18n data is required or not, I would leave the call to @SamSaffron

@SamSaffron
Copy link
Contributor

@nightpool / @runlevel5 I feel that @ignisf is running incredibly low on time at the moment, would either of you be open to taking on the mantle of pushing out libv8 gems? There is a long adventure building it for all platforms and it can be quite time consuming.

I say we eat the 4.3MB penalty, simplicity should win here.

@runlevel5
Copy link
Contributor

@SamSaffron I could help with PR reviewing and pushing now and then.

@nightpool
Copy link
Collaborator Author

@SamSaffron happy to help! It looked like there was a travis-based deploy process and a vagrant-based deploy process, do you know which one is currently in use?

@nightpool
Copy link
Collaborator Author

nightpool commented Jul 17, 2020

Okay, looking at rubygems, it seems like the last versions was built for these platforms:

source

universal-darwin16
universal-darwin-16
universal-darwin17
universal-darwin-17
universal-darwin18
universal-darwin-18
universal-darwin19
x86_64-darwin16
x86_64-darwin-16
x86_64-darwin17
x86_64-darwin-17
x86_64-darwin-18
x86_64-darwin-19

x86_64-linux

While the github release had these platforms:

Source
universal-darwin-19
x86_64-darwin-17
x86_64-darwin-19
x86_64-linux

We can probably drop Mac OS Sierra (darwin-16) from that list uncontroversially, but High Sierra (darwin-17) is still supported by Apple through the end of the year and Ruby core still runs CI tests against it. Other then that, doesn't look too complicated! I can try my hand at rolling these builds this weekend if we can get this merged to master @SamSaffron

@nightpool
Copy link
Collaborator Author

honestly, at this point (with only linux and darwin platforms) we could probably just move all of these to Travis configuration files, so maybe that's what I'll do.

This requires changing our checkout strategy to check out the branch-heads branch (as documented at https://v8.dev/docs/version-numbers) instead of using the 8.4.371 branch, which never gets patches (it's always locked to 8.4.371.0).

We could also specify the v8 patch version manually, and i'm open to doing that, just wanted to do this to get it working.
Using bisect, I was able to find that the build error was introduced in
v8/v8@5bbca54,
so let's use the version before that one while we wait for upstream to
merge the patch (https://bugs.chromium.org/p/v8/issues/detail?id=10708)
Otherwise, attempts to use the i18n api (toLocaleString, Intl, etc)
raise the TypeError 'Internal error. Icu error.'
We now build against three darwin versions (Sierra, High Sierra and Catalina)
to match Ruby core. We continue to build against 3 ruby versions on Linux, but
we only deploy the ruby 2.7 builds (since they should all be identical).
@nightpool
Copy link
Collaborator Author

@SamSaffron I've updated the Travis config such that now, whenever a release is made on the Github respository, Travis will build and upload the supported .gem files to the Github release. I'm currently running a test release here:

As soon as the build finishes, we should see .gem files populate at the linked release tag. I could probably also automate pushing those files to Rubygems if I had the correct encrypted API token (https://guides.rubygems.org/rubygems-org-api, travis encrypt KEY)

@nightpool
Copy link
Collaborator Author

nightpool commented Jul 18, 2020

Pushed a bugfix for the deploy automation script: https://travis-ci.com/github/nightpool/libv8/builds/176168491 and got Travis to increase the build limit for my test repository, crossing my fingers for this one!

@nightpool
Copy link
Collaborator Author

Looks like there's an issue with the older darwin versions, v8 only supports compiling with the 10.15+ sdk. Will look into on Monday.

@SamSaffron SamSaffron merged commit 2d5250a into rubyjs:master Jul 20, 2020
@SamSaffron
Copy link
Contributor

@nightpool I just gave you "write" on libv8 so you can push fixes as needed.

Only @cowboyd and @ignisf have access at the moment to the gem. @cowboyd can you give me ownership on libv8, Petko is MEGA busy with other projects so we need to get more people involved here so we can push the project forward. Once done I can set @nightpool up with gem push if needed.

@nightpool
Copy link
Collaborator Author

Thanks Sam! looks like there's still one bug with High Sierra binaries (10.13), but that's two major versions behind, so I think we probably only need to worry if someone opens an issue about it.

@nightpool nightpool deleted the 8.4.255 branch July 20, 2020 14:47
@nightpool nightpool mentioned this pull request Jul 20, 2020
@cornu-ammonis
Copy link

cornu-ammonis commented Feb 9, 2021

@runlevel5 Thanks - I am a bit confused what that adoption would look like; does ruby-libv8-node not ultimately rely on libv8 and libv8's v8 engine? Or is the idea that the package you linked to will eventually replace this repo for distribution of new v8 versions?

@SamSaffron could you possibly provide any information on what this means for mini_racer? I am curious whether I should expect mini_racer to remain on this earlier version of v8 indefinitely, or what the upgrade process might look like going forward to keep abreast of patches and new releases.

@SamSaffron
Copy link
Contributor

could you possibly provide any information on what this means for mini_racer?

the mechanics of building the v8 libraries are not really relevant to mini_racer, as long as it has libraries to lean on it should be good.

In general people put mini_racer in gemfiles and are not bothered to put libv8 under a specific version. We can change our dependencies if we need.

I hope our next mini racer release will use the node build system of v8 cause we would get ARM support which is critical on the M1 CPUs.

So this all depends on @lloeki timelines and confidence with the new build system. I would love to release a new version of mini_racer tomorrow with official M1 support, it would make me very happy.

I am not seeing any upgrade concerns here, they all boil down to fiddle with your gemfile if you hacked it badly.

@cornu-ammonis
Copy link

Thanks, I appreciate the context on your approach!

@lloeki
Copy link
Contributor

lloeki commented Feb 15, 2021

Hey, here's a quick update WRT libv8-node:

  • so far most of libv8-node looks to be working as expected together with mini_racer and sq_mini_racer, so confidence is really high given the number of successful real-life deployments we had
  • I received a couple of small feedbacks I'd like to investigate (and address if necessary) regarding some installation issues (most I had were linked to broken or obsolete base systems, but I'd like to check those that are still pending)
  • I'm going to integrate building from source into the gem itself so that folks can use that on gem install when there's no binary gem published for their platform

After that I'm calling it good to go on my side.

@cornu-ammonis
Copy link

cornu-ammonis commented Feb 15, 2021

Hi thanks for providing more context. That last point about building from source sounds particularly useful. To be more explicit about my particular use case - it is not about using different platforms that you don't currently support, but rather about the ability to quickly update to the newest version of the v8 engine when security updates are released. This is important given that people periodically find vulnerabilities in v8 that could be used to sandbox-escape, and in my case it would not be acceptable to continue using an unpatched version of v8 once such a vulnerability is fixed.

In its current form mini_racer with libv8 is not safe to run on untrusted/client code, because it is using a version of v8 from the summer which contains known exploitable vulnerabilities. So I am evaluating whether I would need to fork mini_racer and other dependencies to achieve the necessary capability to rapidly apply patches.

If mini_racer were using a v8 gem that made it easy to specify/build from the newest v8 release, which it sounds like you just described, this would solve my problem.

@lloeki
Copy link
Contributor

lloeki commented Feb 16, 2021

This is one of the motivations behind libv8-node: being able to quickly and easily update, including for security reasons, is a must have for us (full disclosure: I work at Sqreen, a software security company).

Since from a release process PoV libv8-node obtains V8 through Node.js, it's always going to be a bit late.

It also means that we also benefit for Node.js security backports on the libv8 tree, which they do even when not bumping libv8 itself, following their release support policy.

So if you followed my reasoning: security fixes might be a bit late, but not by much, otherwise Node.js would be left vulnerable as well. And they also apply to older releases as well (as of today, that's the V8s that are in Node 10.x, 12.x, 14.x, and 15.x, see e.g. here).

@SamSaffron
Copy link
Contributor

@lloeki one big key issue we need to nail here is ideally having some GitHub action out there to build new versions of the gem. That way we can just click one button when we need to update v8. There are so many packages we need to push every release so making this as trivial as possible is key.

Another thought here is ... I wonder if mini_racer releases should embed v8 and be pre-compiled, in development the split makes sense, but in production it seems to only add complexity.

Then we would release, source build of v8 and binary builds of mini_racer. That also makes it a tiny bit easier and faster to install mini_racer.

I am not too concerned about the delays here, if we can keep up with the nodejs cadence we are in a very good state. The key is great CI, it could even auto push beta packages and then we just click a button to promote ... I don't know.

@lloeki
Copy link
Contributor

lloeki commented Feb 18, 2021

having some GitHub action out there to build new versions of the gem

@SamSaffron can you please stop reading my mind, this was my secret plan ;)

Another thought here is ... I wonder if mini_racer releases should embed v8 and be pre-compiled, in development the split makes sense, but in production it seems to only add complexity.

Agreed. This is what the do with PyMiniRacer. There are a number of issues regarding directly usable extensions as binary gems regarding platform API/ABI version (very painful with C++) and Ruby API version dependency. In theory to be guaranteed to work in 100% of cases the latter would require a binary gem to ship compiled extensions for every Ruby minor version.

PyMiniRacer has similar constraints and they were elegantly resolved by building a non-Python dynamic library with a wrapping C API exposing only the libv8 API points useful to PyMiniRacer, and a Python extension that dlopens (through ctypes) that dynamic library to implement the binding to the host language. Interestingly enough it means that this non-host-language dynamic library is reusable across languages.

Another specific of PyMiniRacer is that building the host-language independent dynamic library is made part of the libv8 building process by patching it in as a target of libv8 build system (that is also doable with Node's libv8 build system), and the compiler is able to prune unused symbols, which results in a library as small as it needs to be.

Also, when it fails (and it does, as the world has an increasingly wide array of compilers, linkers, and Linux versions), it would be quite useful to have a rubygems/bundler way to tell them to not pick a binary gem, which AFAIK currently does not exist. I think I have thought about this long enough that I'll probably propose it (and maybe PR it if I have time) to rubygems/rubygems.

In any case, this needs to wait for rubygems/rubygems#3174 and rubygems/rubygems#4082 (also a blocker for sassc, which has the exact same issue, and upstream Alpine, which removes the native platform from Gem.platforms because of it, always forcing a source build) as otherwise musl users would be left in the dust.

@SamSaffron
Copy link
Contributor

So excited about this progress @lloeki , let me know if you need a hand testing anything

@lloeki
Copy link
Contributor

lloeki commented Feb 22, 2021

As a general platform testbed for the "ruby" platform "fetch-tar-and-build-from-source" gem I did some testing on CI to build binaries for {amd64, arm64, arm7, ppc64le, s390x} x {gnu, musl} using good old qemu+binfmt+docker run --platform trickery, and it turns out most of these hit the 6h job limit of GHA xD.

But the fact that they got quite far as well as Node supporting these platforms and ppc64le actually building (less than 10min before the 6h deadline!) makes me confident that it should work just fine. s390x+musl segfaulted but I suppose that might be a bug in musl (or maybe musl+node) on that platform.

The only change needed to the build itself was detecting non-64bit arches and not enabling pointer compression, which IIRC would be something to alter on the mini_racer side as well.

@cornu-ammonis as we said we would be much faster in being able to keep up with V8+Node upstream security patches, but if that's not fast enough for you, it should not be too hard from the current code to add a config switch to fetch a tar from any GitHub repo ref/commit instead of pulling tar published from tagged releases. Internally this would be useful as even if we don't do releases from it it allows us to track Node's master for development (e.g it currently has V8 8.8, which could be interesting to work against early for rubyjs/mini_racer#170).

@SamSaffron If I did not forget anything this is now fairly complete, remaining work before I declare this major step 100% ready would be:

  • add checksum check for the tar that get fetched
  • add a few test steps to CI, like verify that the just-built gem installs properly, and build+run mini_racer against the just-built version
  • prevent a "double-build" (because libv8 is entirely independent of and built out of ruby AND rake binary depends on rake compile, luckily the second "build" is fast because all the artifacts are still there, but it's messy)
  • remove the node source+build tree when called from gem install and build was successful, to reclaim space from the build (which is quite sizeable)

I'll push the "ruby" platform libv8-node on rubygems soon, so that you folks can test it out.

@lloeki
Copy link
Contributor

lloeki commented Feb 22, 2021

All bulletpoints above are cleared, waiting for CI to push the "ruby" gem (tomorrow)

@lloeki
Copy link
Contributor

lloeki commented Feb 23, 2021

Okay, that's been pushed (the ruby one I mean, that builds from source).

... as well as aarch64-linux and aarch64-linux-musl. I figured out that I could build them from Docker on M1, as my Raspberry Pi 3B was clearly not up to the task.

@SamSaffron
Copy link
Contributor

Nice!! should we do a beta release of mini_racer as well ... I just gave you push on mini_racer so you can push a beta if you wish.

@runlevel5
Copy link
Contributor

@lloeki nice work!

If you are in need to access into a proper machine (for example M1 or PPC64LE) for development, please sign up at github.com/KernelCafe/welcome

@lloeki
Copy link
Contributor

lloeki commented Feb 25, 2021

Thanks @SamSaffron! I’ll probably do that some time next week :)

@cornu-ammonis
Copy link

@lloeki Exciting work here! Any update on the beta release for mini_racer using this? I am interested to try it out.

@SamSaffron
Copy link
Contributor

@lloeki going to echo what @cornu-ammonis said, are we good to cut a beta release of mini_racer / libv8.

People on Apple Silicon are hurting bad now due to the workarounds.

I would be happy to have the beta go for say 4 days, then make this the stable release. We are also getting way behind on v8 now, probably sensitive to some published exploits.

@lloeki
Copy link
Contributor

lloeki commented Mar 24, 2021

(Sorry for the delay, attended three funerals in 6 days, so I took a step back on things for a bit).

@SamSaffron Should the beta depend directly on libv8-node? (EDIT: clarifying, in that case we probably need to move the libv8-node repo under rubyjs at some point and give more people gem push access)

Also, FYI rubygems/rubygems#4082 had a corner case that wasn't properly agreed upon, so it had to be reverted. We agreed on a way forward in order to re-merge the fix, this time with a better handling of that corner case.

Anyway, it's not a blocker for us on musl anymore thanks to the new force_ruby_platform flag.

@SamSaffron
Copy link
Contributor

SamSaffron commented Mar 24, 2021 via email

@lloeki
Copy link
Contributor

lloeki commented Mar 26, 2021

Alright @SamSaffron!

I'm going to build libv8-node with the latest node releases for all platforms I can, update rubyjs/mini_racer#186 to use it, merge that, and proceed with a beta release of mini_racer.

@cornu-ammonis
Copy link

So sorry about your loss. Thanks for all your work here, this is very helpful.

@cornu-ammonis
Copy link

@SamSaffron @lloeki Hi all - I see it has been a while since the last update to libv8 node / mini_racer. Could we make PRs to bump to a more recent version of v8 for patch hygiene etc? Does this take much effort to do?

@lloeki
Copy link
Contributor

lloeki commented Oct 12, 2021

Hi, I'm not forgetting about this. IIRC I've had something around for a recent one (9.x? the one that introduces the single threaded mode API) for a while but had zero time to properly clean that up and push these builds to rubygems.

@SamSaffron
Copy link
Contributor

Just mentioning in case this helps @runlevel5 / @nightpool / @seanmakesgames / @ignisf

At Discourse we see the evolution of libv8 and maintenance as a very important thing and are happy to invest money into this project. Current bus factor of 1 here is very risky for the project, I very much want 1-2 more people around who can push updates. Discourse will pay for this work if you wish to take this on.

@lloeki
Copy link
Contributor

lloeki commented Oct 13, 2021

I'm seeing the finish line on a very important project, I think I'll be able to tackle this very soon (EOW~next week).

@ignisf
Copy link
Collaborator

ignisf commented Oct 13, 2021

Thank you for the ping. I'll have to decline, as I'm still focusing on personal life and trying to reduce my involvement in projects.

@seanmakesgames
Copy link

Just caught up on the thread. Appreciate and understand the sentiment. Currently I have 3 jobs: Tableau/Salesforce, Home Remodel Manager, hackmud, in that order. As I transition back to more hackmud dev in the next few years, I'll be available for more support. Esp. for engineering automated upgrades and untangling upgrade blockers.

@lloeki
Copy link
Contributor

lloeki commented Oct 13, 2021

@cornu-ammonis if you're really in a need for something right now, there's this libv8-node CI run which has gem artifacts for 16.4.2, combined with this mini_racer PR for libv8-node 16.x. I also just pushed a node 16.11.1 update for libv8-node for the CI to produce artifacts, but as of this writing am still waiting on the build results.

@lloeki
Copy link
Contributor

lloeki commented Oct 14, 2021

PR containing 16.x on the libv8-node side rubyjs/libv8-node#24

@lloeki
Copy link
Contributor

lloeki commented Oct 18, 2021

I pushed some 16.10.0 gems and updated the corresponding mini_racer PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bump V8 from v7.3 to v8.1
7 participants