-
Notifications
You must be signed in to change notification settings - Fork 121
Conversation
@runlevel5 @SamSaffron Builds have passed here! Can you please take a look? |
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 |
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 |
@nightpool nice work. Could you please show us the diff of your patch to address the I18n issue? |
@runlevel5 sure! it's just one additional argument for 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) |
@nightpool have you tested against mini_racer yet? |
@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. |
Since I do not have the full knowledge whether ICU i18n data is required or not, I would leave the call to @SamSaffron |
@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. |
@SamSaffron I could help with PR reviewing and pushing now and then. |
@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? |
Okay, looking at rubygems, it seems like the last versions was built for these platforms:
While the github release had these platforms:
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 |
honestly, at this point (with only |
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).
@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, |
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! |
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. |
@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. |
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. |
@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 |
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 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. |
Thanks, I appreciate the context on your approach! |
Hey, here's a quick update WRT libv8-node:
After that I'm calling it good to go on my side. |
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 If |
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). |
@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. |
@SamSaffron can you please stop reading my mind, this was my secret plan ;)
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 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 |
So excited about this progress @lloeki , let me know if you need a hand testing anything |
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:
I'll push the "ruby" platform libv8-node on rubygems soon, so that you folks can test it out. |
All bulletpoints above are cleared, waiting for CI to push the "ruby" gem (tomorrow) |
Okay, that's been pushed (the ... 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. |
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. |
@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 |
Thanks @SamSaffron! I’ll probably do that some time next week :) |
@lloeki Exciting work here! Any update on the beta release for mini_racer using this? I am interested to try it out. |
@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. |
(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 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 |
So sorry to hear about your loss, this sound terrible 😢
I am happy for mini racer to depend directly on libv8 node
…On Wed, 24 Mar 2021 at 6:13 pm, Loic Nageleisen ***@***.***> wrote:
(Sorry for the delay, attended three funerals in 6 days, so I took a step
back on things for a bit).
@SamSaffron <https://github.com/SamSaffron> Should the beta depend
directly on libv8-node?
Also, FYI rubygems/rubygems#4082
<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
<rubygems/rubygems#4425 (comment)>
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.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#301 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAABIXJWFZEHKVXU3OEH6NTTFGGLNANCNFSM4O2WFEVA>
.
|
Alright @SamSaffron! I'm going to build |
So sorry about your loss. Thanks for all your work here, this is very helpful. |
@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? |
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. |
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. |
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). |
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. |
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. |
@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. |
PR containing 16.x on the libv8-node side rubyjs/libv8-node#24 |
I pushed some 16.10.0 gems and updated the corresponding mini_racer PR. |
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