GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||
GeezerVison is showing github.com — deps: update V8 to 6.6 by targos · Pull Request #19201 · nodejs/node Conversation
|
|
This is probably the version that will be shipped with Node 10.0.0. I would like that we land it on /cc @nodejs/tsc @nodejs/v8 @nodejs/benchmarking @nodejs/performance |
All reactions
-
👍 10 reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
+1. Given that V8 6.6 is expected to go stable before the 10.0.0 release, it makes sense to release 10 with the latest stable. There is precedence going all the way back to Node 4.x for picking up a V8 version in anticipation that it will go stable very close to the time of a x.0.0 release. We tend to upgrade V8 in a ABI/API compatible way even after a x.0.0 release, so all this is doing is picking up V8 6.6 as the API + ABI contract for the lifetime of Node.js 10.x LTS. /cc @bmeurer @hashseed in case they see problems with using V8 6.6 API/ABI for the lifetime of LTS. |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
ofrobots
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rubberstamp LGTM.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
|
It looks like we need a new strategy to execute V8 tests: |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Yes, that's because V8 no longer supports gyp as of 6.6, and the Makefile has been removed. Please refer to these instructions. I can probably come up with some more streamlined instructions based on |
All reactions
-
👍 2 reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
MylesBorins
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rubber stamp LGTM when CI is green.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
mcollina
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
cd deps/v8 tools/node/fetch_deps.py . PATH=./_depot_tools:$PATH tools/dev/gm.py x64.release cctest/* mjsunit/* debugger/* inspector/* message/* preparser/* This should work on Mac and Linux and runs most tests. V8 unittests are not included because the gmock dependency is missing, and doesn't seem to have worked with |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Do you want to float the |
All reactions
-
👍 4 reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
+1 for floating important performance regression fixes that landed after 6.6 |
All reactions
-
👍 1 reaction
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Lets try to avoid the rush of issues when Node 10 is released next month. Node 10 is currently version [`62`][1] but is expected to be [`63`][2] before going stable. We need to pick one because of the ABI breakage betweem 62 and 63. If this bet pays off we save ourselves a bunch of time dealing with installation issues, and avoiding a new release. If we're wrong it's no different to any other major Node release. [1]: https://github.com/nodejs/node/blob/97595739/src/node_version.h#L112 [2]: nodejs/node#19201
Lets try to avoid the rush of issues when Node 10 is released next month. Node 10 is currently version [`62`][1] but is expected to be [`63`][2] before going stable. We need to pick one because of the ABI breakage betweem 62 and 63. If this bet pays off we save ourselves a bunch of time dealing with installation issues, and avoiding a new release. If we're wrong it's no different to any other major Node release. [1]: https://github.com/nodejs/node/blob/97595739/src/node_version.h#L112 [2]: nodejs/node#19201
|
@hashseed The last command gives me an error: $ PATH=./_depot_tools:$PATH tools/dev/gm.py x64.release cctest/* mjsunit/* debugger/* inspector/* message/* preparser/* Traceback (most recent call last): File "tools/dev/gm.py", line 391, in <module> sys.exit(Main(sys.argv)) File "tools/dev/gm.py", line 380, in Main return_code += configs[c].Build() File "tools/dev/gm.py", line 250, in Build build_opts = BUILD_OPTS_GOMA if self.WantsGoma() else BUILD_OPTS_DEFAULT File "tools/dev/gm.py", line 236, in WantsGoma "gn args --short --list=use_goma %s" % (GetPath(self.arch, self.mode))) File "tools/dev/gm.py", line 157, in _CallWithOutputNoTerminal return subprocess.check_output(cmd, stderr=subprocess.STDOUT, shell=True) File "/usr/lib/python2.7/subprocess.py", line 574, in check_output raise CalledProcessError(retcode, cmd, output=output) subprocess.CalledProcessError: Command 'gn args --short --list=use_goma out/x64.release' returned non-zero exit status 1 $ gn args --short --list=use_goma out/x64.release gn.py: Could not find checkout in any parent of the current path. This must be run inside a checkout. |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
hashseed
commented
Mar 13, 2018
•
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.
|
@targos could you try this instead? cd deps/v8 tools/node/fetch_deps.py . PATH=~/_depot_tools:$PATH tools/dev/v8gen.py x64.release --no-goma PATH=~/_depot_tools:$PATH ninja -C out.gn/x64.release/ d8 cctest inspector-test tools/run-tests.py --gn mjsunit cctest debugger inspector message preparser There is one test failure with Also, I would be very happy if you cherry-picked these commits from vee-eight-lkgr:
These two commits adds a I could also submit a separate PR for this if you prefer. |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
targos
commented
Mar 13, 2018
•
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.
$ PATH=~/_depot_tools:$PATH tools/dev/v8gen.py x64.release --no-goma Hint: You can raise verbosity (-vv) to see the output of failed commands. Traceback (most recent call last): File "tools/dev/v8gen.py", line 304, in <module> sys.exit(gen.main()) File "tools/dev/v8gen.py", line 298, in main return self._options.func() File "tools/dev/v8gen.py", line 166, in cmd_gen gn_outdir, File "tools/dev/v8gen.py", line 208, in _call_cmd stderr=subprocess.STDOUT, File "/usr/lib/python2.7/subprocess.py", line 574, in check_output raise CalledProcessError(retcode, cmd, output=output) subprocess.CalledProcessError: Command '['/usr/bin/python', '-u', 'tools/mb/mb.py', 'gen', '-f', 'infra/mb/mb_config.pyl', '-m', 'developer_default', '-b', 'x64.release', 'out.gn/x64.release']' returned non-zero exit status 1 Running the command manually showed me that I had to execute this: $ build/linux/sysroot_scripts/install-sysroot.py --arch=amd64 Writing """\ is_debug = false target_cpu = "x64" """ to /home/mzasso/git/nodejs/v8-6.6/deps/v8/out.gn/x64.release/args.gn. /home/mzasso/git/nodejs/v8-6.6/deps/v8/buildtools/linux64/gn gen out.gn/x64.release --check Done. Made 98 targets from 66 files in 98ms Edit: ninja is now building. Will report when it's over. |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
PR-URL: #19201 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: #19201 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Major V8 updates are usually API/ABI incompatible with previous versions. This commit adapts NODE_MODULE_VERSION for V8 6.6. Refs: https://github.com/nodejs/CTC/blob/master/meetings/2016-09-28.md PR-URL: #19201 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Synchronize source files list with upstream's BUILD.gn. PR-URL: #19201 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Refs: v8/v8@f9934aa Fixes: nodejs/node-v8#36 PR-URL: #19201 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: #19201 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Even if we only use v8_monolith build target, other targets in v8.gyp with possibly outdated file lists are parsed and could cause build to fail even with --build-v8-with-gn. PR-URL: #19201 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: #19201 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: #19201 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: #19201 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Adds `isBigInt64Array` and `isBigUint64Array`. PR-URL: #19201 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: #19201 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: #19201 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: #19201 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Major V8 updates are usually API/ABI incompatible with previous versions. This commit adapts NODE_MODULE_VERSION for V8 6.6. Refs: https://github.com/nodejs/CTC/blob/master/meetings/2016-09-28.md PR-URL: #19201 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Synchronize source files list with upstream's BUILD.gn. PR-URL: #19201 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Refs: v8/v8@f9934aa Fixes: nodejs/node-v8#36 PR-URL: #19201 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: #19201 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Even if we only use v8_monolith build target, other targets in v8.gyp with possibly outdated file lists are parsed and could cause build to fail even with --build-v8-with-gn. PR-URL: #19201 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: #19201 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: #19201 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: #19201 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Adds `isBigInt64Array` and `isBigUint64Array`. PR-URL: #19201 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: #19201 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Reviewers
+4 more reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
20 participants
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.