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: update V8 to 6.6 #19201

Closed
wants to merge 12 commits into from
Closed

deps: update V8 to 6.6 #19201

wants to merge 12 commits into from

Conversation

targos
Copy link
Member

@targos targos commented Mar 7, 2018

ETA: April 17th, 2018

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

CI: https://ci.nodejs.org/job/node-test-pull-request/13568/
V8: https://ci.nodejs.org/job/node-test-commit-v8-linux/1254/

@targos targos added semver-major PRs that contain breaking changes and should be released in the next major version. v8 engine Issues and PRs related to the V8 dependency. blocked PRs that are blocked by other issues or PRs. labels Mar 7, 2018
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels Mar 7, 2018
@targos targos changed the title V8 6.6 deps: update V8 to 6.6 Mar 7, 2018
@targos targos removed the build Issues and PRs related to build files or the CI. label Mar 7, 2018
@targos
Copy link
Member Author

targos commented Mar 7, 2018

This is probably the version that will be shipped with Node 10.0.0.

I would like that we land it on master before it is in Chrome stable. It would give us more time to test it with the RCs.

/cc @nodejs/tsc @nodejs/v8 @nodejs/benchmarking @nodejs/performance

@ofrobots
Copy link
Contributor

ofrobots commented Mar 7, 2018

+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.

Copy link
Contributor

@ofrobots ofrobots left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rubberstamp LGTM.

@targos
Copy link
Member Author

targos commented Mar 7, 2018

It looks like we need a new strategy to execute V8 tests:

11:39:24 make -C deps/v8 x64.release GYPFLAGS="-Dclang=0"
11:39:24 make[1]: Entering directory `/home/iojs/build/workspace/node-test-commit-v8-linux/nodes/benchmark/v8test/v8test/deps/v8'
11:39:24 make[1]: *** No rule to make target `x64.release'.  Stop.

@hashseed
Copy link
Member

hashseed commented Mar 7, 2018

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 deps/v8/tools/node/fetch_deps.py tomorrow :)

Copy link
Contributor

@MylesBorins MylesBorins left a 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.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@hashseed
Copy link
Member

hashseed commented Mar 7, 2018

I can probably come up with some more streamlined instructions based on deps/v8/tools/node/fetch_deps.py tomorrow :)

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 make either?

@bmeurer
Copy link
Member

bmeurer commented Mar 8, 2018

Do you want to float the for..of performance Bugfix on top for the issue discovered in webpack recently? It landed shortly after 6.6.

@benjamingr
Copy link
Member

+1 for floating important performance regression fixes that landed after 6.6

xzyfer added a commit to xzyfer/node-sass that referenced this pull request Mar 10, 2018
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
xzyfer added a commit to sass/node-sass that referenced this pull request Mar 10, 2018
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
@targos
Copy link
Member Author

targos commented Mar 12, 2018

@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.

@hashseed
Copy link
Member

hashseed commented Mar 13, 2018

@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 mjsunit/wasm/jsapi-harness, but I think that's because dependencies for this test have not been pulled in.

Also, I would be very happy if you cherry-picked these commits from vee-eight-lkgr:

  • 6d9eb3f
  • a454b0dd120a2ad433f0314e2b9436e567930ea1

These two commits adds a --build-v8-with-gn flag to ./configure so that V8 can be built with GN instead of gyp as described here. V8 relies on this option to run integration tests with Node.js.

I could also submit a separate PR for this if you prefer.

@targos
Copy link
Member Author

targos commented Mar 13, 2018

@hashseed

$ 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.

MylesBorins added a commit that referenced this pull request Apr 11, 2018
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]>
MylesBorins added a commit that referenced this pull request Apr 11, 2018
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]>
MylesBorins added a commit that referenced this pull request Apr 11, 2018
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]>
MylesBorins pushed a commit that referenced this pull request Apr 11, 2018
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]>
MylesBorins pushed a commit that referenced this pull request Apr 11, 2018
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]>
MylesBorins pushed a commit that referenced this pull request Apr 11, 2018
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]>
MylesBorins pushed a commit that referenced this pull request Apr 11, 2018
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]>
MylesBorins pushed a commit that referenced this pull request Apr 11, 2018
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]>
MylesBorins pushed a commit that referenced this pull request Apr 11, 2018
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]>
MylesBorins pushed a commit that referenced this pull request Apr 11, 2018
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]>
MylesBorins pushed a commit that referenced this pull request Apr 11, 2018
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]>
MylesBorins pushed a commit that referenced this pull request Apr 11, 2018
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]>
@targos targos deleted the v8-6.6 branch April 12, 2018 08:10
jasnell pushed a commit that referenced this pull request Apr 16, 2018
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]>
jasnell pushed a commit that referenced this pull request Apr 16, 2018
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]>
jasnell pushed a commit that referenced this pull request Apr 16, 2018
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]>
jasnell pushed a commit that referenced this pull request Apr 16, 2018
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]>
jasnell pushed a commit that referenced this pull request Apr 16, 2018
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]>
jasnell pushed a commit that referenced this pull request Apr 16, 2018
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]>
jasnell pushed a commit that referenced this pull request Apr 16, 2018
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]>
jasnell pushed a commit that referenced this pull request Apr 16, 2018
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]>
jasnell pushed a commit that referenced this pull request Apr 16, 2018
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]>
jasnell pushed a commit that referenced this pull request Apr 16, 2018
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]>
jasnell pushed a commit that referenced this pull request Apr 16, 2018
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]>
jasnell pushed a commit that referenced this pull request Apr 16, 2018
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major PRs that contain breaking changes and should be released in the next major version. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.