Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.2k
deps: update V8 to 5.5#9618
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: update V8 to 5.5 #9618
Uh oh!
There was an error while loading. Please reload this page.
Conversation
targos commented Nov 15, 2016 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Fishrock123 commented Nov 15, 2016
As brought up in nodejs/v8#2 I'm quite skeptical this is a good idea until we get better hooks for native promises. (Which seemed to be the plan to get in before node version 8, where this was originally supposed to land.) |
mhdawson commented Nov 15, 2016
CI run to validate across platforms: https://ci.nodejs.org/job/node-test-commit-v8-linux/421/ |
bnoordhuis commented Nov 15, 2016
That's a pretty cryptic comment on its own. I think the missing context is that 5.5 ships async/await without a flag? |
637c4c3 to bc00dddComparemhdawson commented Nov 17, 2016
Looks like CI failures across the board. |
jbergstroem commented Nov 17, 2016
Afaik smartos14 support is dropped for 5.5 |
targos commented Nov 26, 2016 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Updated. There are some new utf-8 fixes (1, 2) that make Example failure: test('utf-8',Buffer.from('F0B841','hex'),'\ufffd\ufffdA');// AssertionError: Expected "\ufffd\ufffd\u41", but got "\ufffd\u41"If I change the expected value, the error is inverted: test('utf-8',Buffer.from('F0B841','hex'),'\ufffdA');// AssertionError: Expected "\ufffd\u41", but got "\ufffd\ufffd\u41" |
ofrobots commented Nov 26, 2016
@targos see discussion here: v8@af842a7#commitcomment-19855022 |
04d75fa to cef1cc9Comparetargos commented Dec 3, 2016
I'm turning the PR into a clean semver-major that can land on master. I'll then open another one to backport to v7.x. |
targos commented Dec 3, 2016
This is still in progress because of v8@af842a7#commitcomment-19855022 |
targos commented Dec 3, 2016
@nodejs/platform-smartos What are we going to do with SmartOS 14? The CI cannot be green because of the incompatibility with this platform. |
jbergstroem commented Dec 4, 2016
@targos as far as I know, it is to be dropped for 55 and forward. I can look at skipping testing against smartos14 for a specific node version; for instance 7.3? |
misterdjules commented Dec 8, 2016
@targos FWIW, what @jbergstroem mentioned:
sounds good to me. P.S: sorry for the delay, I was on vacation until today. |
mhdawson commented Jan 26, 2017
CI run one more time: https://ci.nodejs.org/job/node-test-pull-request/6073/ |
PR-URL: #9618 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
V8 5.5 is not API/ABI compatible with 5.4. This commit increments NODE_MODULE_VERSION by one. Refs: https://github.com/nodejs/CTC/blob/master/meetings/2016-09-28.md PR-URL: #9618 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
With the upstream fix in V8, function declarations now work fine in the vm module and the test is no longer failing. Refs: https://codereview.chromium.org/2334733002Fixes: #548 PR-URL: #9618 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
The issue is fixed upstream in V8. Thus we do not need this workaround in REPL. Fixes: #548 PR-URL: #9618 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
V8 5.5 changed how invalid characters are handled and it now appears to follow the WHATWG Encoding standard, where all of an invalid character's bytes are replaced by a single replacement character (\ufffd) instead of replacing each invalid byte with separate replacement characters. Example: the byte sequence 0xF0,0xB8,0x41 is decoded as '\ufffdA' in V8 5.5, but is decoded as '\ufffd\ufffdA' in previous versions of V8. PR-URL: #9618 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
targos commented Jan 26, 2017
Landed in a67a04d...24ef1e6. Thanks everyone! |
pi0 commented Jan 27, 2017 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
iddan commented Jan 31, 2017
Is this means we can use async/await safely? |
kyrylkov commented Jan 31, 2017
leodutra commented Feb 3, 2017
Will it be included in version 8 without flags? |
kyrylkov commented Feb 3, 2017
@leodutra It is already in master branch, thus in Node.js 8 |
With the upstream fix in V8, function declarations now work fine in the vm module and the test is no longer failing. Fixes: nodejs#548 Refs: https://codereview.chromium.org/2334733002 Refs: nodejs#9618 PR-URL: nodejs#11029 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Myles Borins <[email protected]>
The issue is fixed upstream in V8. Thus we do not need this workaround in REPL. Fixes: nodejs#548 Refs: nodejs#9618 PR-URL: nodejs#11029 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Myles Borins <[email protected]>
With the upstream fix in V8, function declarations now work fine in the vm module and the test is no longer failing. Fixes: nodejs#548 Refs: https://codereview.chromium.org/2334733002 Refs: nodejs#9618 PR-URL: nodejs#11029 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Myles Borins <[email protected]>
The issue is fixed upstream in V8. Thus we do not need this workaround in REPL. Fixes: nodejs#548 Refs: nodejs#9618 PR-URL: nodejs#11029 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Myles Borins <[email protected]>
joaocgreis commented Mar 28, 2017
@targoshttps://github.com/nodejs/node/blob/master/BUILDING.md#unix still mentions Clang 3.4 as the required version. Do you know what the new required version is? |
targos commented Mar 29, 2017
@joaocgreis I don't know what is the actual required version but since we dropped the workaround from #8343 it must be at least 3.4.2. |
Checklist
make -j8 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
V8
Description of change
This PR updates V8 to the current
5.5-lkgrbranch./cc @nodejs/v8
Previous discussion: nodejs/v8#2
CI: https://ci.nodejs.org/job/node-test-pull-request/4849/V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/417/V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/422/