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

Upgrade to libv8 8.4 #166

Merged
merged 3 commits into from Jul 21, 2020
Merged

Upgrade to libv8 8.4 #166

merged 3 commits into from Jul 21, 2020

Conversation

nightpool
Copy link
Contributor

@nightpool nightpool commented Jul 15, 2020

Based on #163 with a slight tweak to the gemspec and tests. Requires rubyjs/libv8#301 to be merged.

Future versions of v8 (8.4 stable, 8.5 beta, and 8.6 development) are blocked on https://bugs.chromium.org/p/v8/issues/detail?id=10041. Right now you can test locally by installing https://github.com/nightpool/libv8/packages/314620, but be aware that I've only pushed a version for x86_64-linux, since that's what I was testing on (specifically, i tested on the ruby:2.5.8-stretch docker container).

rake test passes locally, I haven't tried any of the other tests yet.

closes #153, #163

@SamSaffron
Copy link
Collaborator

This is fantastic @nightpool ... @ignisf can we merge this?

@nightpool
Copy link
Contributor Author

Unfortunately i'm still having problems with my React SSR related to locale data. I'm working on a further patch for libv8, but my current working theory is that libv8 isn't packaging locale data at all. Adding it statically to the v8 so looks pretty easy, but maybe it should do so as a separate gem or an optional configuration?

@SamSaffron
Copy link
Collaborator

I would say it depends on how much extra size would be added by adding locale data?

@nightpool
Copy link
Contributor Author

nightpool commented Jul 15, 2020

Okay, it looks like the locale data isn't too much bigger—my libv8 .gem went from 11.9 MB to 16.2 MB.

Not as bad as I was expecting, but still a pretty big increase.

@SamSaffron
Copy link
Collaborator

Will leave this for @ignisf to decide but my gut feel is that it is OK.

Is there are way to package it so it is an additional dependency eg: libv8-locale ?

@nightpool
Copy link
Contributor Author

I haven't looked into it yet. I'm sure it's possible, not sure what the additional complexity would be or how long it would take to get working.

@nightpool
Copy link
Contributor Author

nightpool commented Jul 16, 2020

I'm going to push two more PRs today to trigger travis tests with my pre-release libv8 packages, just so we can make sure tests are working on the full suite of platforms as well as locally for me.

Wait this isn't going to work because i don't have darwin binaries. Oh well.

@SamSaffron
Copy link
Collaborator

@nightpool shall I merge this now? is is backwards compat?

@nightpool
Copy link
Contributor Author

@SamSaffron I haven't investigated the c++ changes in detail, because I was mostly building on the work @cataphract and @ignisf did, but my guess is probably not. (especially since I bumped the gemfile to require libv8 8.4, which hasn't been released yet)

@SamSaffron
Copy link
Collaborator

K ... going to hold off on merging this till we get libv8 8.4 published.

@nightpool
Copy link
Contributor Author

@SamSaffron @cowboyd @ignisf built libv8 gems are ready at https://github.com/rubyjs/libv8/releases/tag/v8.4.255.0, I've tested them locally and we're just waiting on getting them pushed to rubygems before we can merge this!

@SamSaffron
Copy link
Collaborator

Amazing work @nightpool I just pushed a release to rubygems! merging this in!

@SamSaffron SamSaffron merged commit 24c4638 into rubyjs:master Jul 21, 2020
@nightpool
Copy link
Contributor Author

yes!!!!!!!! so great to get this merged.

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

3 participants