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

deps: upgrade v8 to 4.1.0.7 #490

Merged
merged 3 commits into from
Jan 18, 2015
Merged

deps: upgrade v8 to 4.1.0.7 #490

merged 3 commits into from
Jan 18, 2015

Conversation

bnoordhuis
Copy link
Member

This commit upgrades V8 from 3.31.74.1 to 4.1.0.7. Despite the major
version bump, there are no API or ABI changes, it's a bug fix release
only.

R=@indutny, /cc @iojs/tc

https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/91/

@othiym23
Copy link
Contributor

@bnoordhuis what about https://gist.github.com/domenic/aca7774a5d94156bfcc1? The recommendations I've seen from Google have been uniformly that Node should be tracking Chromium stable when choosing V8 versions.

@domenic
Copy link
Contributor

domenic commented Jan 18, 2015

This is better than the current situation, where 3.31 is not on track for any Chrome release at all.

  • 3.30 = Chrome 40 = landing Tuesday
  • 4.1 = Chrome 41 = landing ~6 weeks from Tuesday

IMO we should go with 4.1 and then stick with it until Chrome 42 is released. (Maybe during the period after Chrome 41 is released but before 42, we can have some kind of prerelease that allows people to download and test with whatever version of V8 is packaged with Chrome 42 betas.)

@bnoordhuis
Copy link
Member Author

@othiym23 What @domenic said and I'll add that 3.31 is pretty much abandoned at this point.

@othiym23
Copy link
Contributor

@domenic 👍 Sounds good to me.

@domenic
Copy link
Contributor

domenic commented Jan 18, 2015

We should be able to revert a2751e3 and 4e58211 with this.

@bnoordhuis
Copy link
Member Author

@domenic Good point, undone in c116a0a (but kept the tests.)

@19h
Copy link
Contributor

19h commented Jan 18, 2015

👍 lgtm. Since it's a fix-only release this could land in 1.0.3, right?

@cjihrig
Copy link
Contributor

cjihrig commented Jan 18, 2015

I agree with @domenic, so rubber stamp LGTM

@rvagg
Copy link
Member

rvagg commented Jan 18, 2015

I'm unqualified to review this but if @bnoordhuis is happy then lgtm

The only thing I ask is for @bnoordhuis to confirm that he believes this to only justify a patch bump and not a minor, that's really all I care about at this stage for my responsibilities.

In another thread, the ES6 one I think, @mikeal was proposing something along the lines of tracking Chrome 41 for our own stability cycle. That would give us a nice ~6 week window to put in whatever effort we need to be able to call io.js "stable" and take off the "beta" and "unstable" caveats we put out. It also means our "stable" is V8 "stable" so we can't be criticised of being cowboys with stability.

@indutny
Copy link
Member

indutny commented Jan 18, 2015

One comment, otherwise LGTM if tests are passing.

bnoordhuis and others added 3 commits January 18, 2015 13:05
This commit upgrades V8 from 3.31.74.1 to 4.1.0.7.  Despite the major
version bump, there are no API or ABI changes, it's a bug fix release
only.

PR-URL: nodejs#490
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Kenan Sulayman <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
clang++ on FreeBSD was blaming v8 for using invalid casts from nullptr:

    reinterpret_cast from 'nullptr_t' to '...' is not allowed

Replace casts with NULL, or NULL with 0 where applicable.

Fixes: nodejs#324
PR-URL: nodejs#332
Reviewed-By: Ben Noordhuis <[email protected]>
Now that V8 has been upgraded, remove the --noharmony_classes and
--noharmony_object_literals workarounds from commits a2751e3 and
4e58211.

PR-URL: nodejs#490
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Kenan Sulayman <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
@bnoordhuis bnoordhuis merged commit e8ad773 into nodejs:v1.x Jan 18, 2015
@bnoordhuis bnoordhuis deleted the upgrade-v8 branch January 18, 2015 12:10
@bnoordhuis
Copy link
Member Author

Landed, thanks everyone. @rvagg Yes, a patch bump is all that is needed.

@rvagg
Copy link
Member

rvagg commented Jan 18, 2015

@bnoordhuis also, anything in here warrant touching NODE_MODULE_VERSION?

@bnoordhuis
Copy link
Member Author

@rvagg No, nothing. Rule of thumb: if nothing changes in deps/v8/include, the release contains no API or ABI changes.

@mgol
Copy link
Contributor

mgol commented Jan 18, 2015

@bnoordhuis what about https://gist.github.com/domenic/aca7774a5d94156bfcc1?

The 3rd step there is not needed btw, the v8 version is right there in the table.

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.

8 participants