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.6 #10992

Merged
merged 7 commits into from Feb 22, 2017

Conversation

Projects
None yet
@targos
Member

targos commented Jan 25, 2017

Depends on #9618.

V8 5.6 should become stable next week. It stable now!
I'm opening the PR now to track eventual issues with CI.

CI: https://ci.nodejs.org/job/node-test-commit/7473/?auto_refresh=true

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

v8

@targos

This comment has been minimized.

Show comment
Hide comment
@targos
Member

targos commented Jan 25, 2017

Build fails on Windows (VS2015): https://ci.nodejs.org/job/node-compile-windows/6598/

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Jan 25, 2017

Member

It's a bit buried in the sea of warnings but the build error is this:

c1xx : fatal error C1083: Cannot open source file: '..\..\..\build\Release\obj\global_intermediate\src\inspector\protocol\Protocol.cpp': No such file or directory [c:\workspace\node-compile-windows\label\win-vs2015\deps\v8\src\v8_base_0.vcxproj]
  Debugger.cpp
c1xx : fatal error C1083: Cannot open source file: '..\..\..\build\Release\obj\global_intermediate\src\inspector\protocol\Debugger.cpp': No such file or directory [c:\workspace\node-compile-windows\label\win-vs2015\deps\v8\src\v8_base_0.vcxproj]
  Profiler.cpp
c1xx : fatal error C1083: Cannot open source file: '..\..\..\build\Release\obj\global_intermediate\src\inspector\protocol\Profiler.cpp': No such file or directory [c:\workspace\node-compile-windows\label\win-vs2015\deps\v8\src\v8_base_0.vcxproj]
  Schema.cpp
c1xx : fatal error C1083: Cannot open source file: '..\..\..\build\Release\obj\global_intermediate\src\inspector\protocol\Schema.cpp': No such file or directory [c:\workspace\node-compile-windows\label\win-vs2015\deps\v8\src\v8_base_0.vcxproj]

Seems something goes wrong in the generate_inspector_protocol_sources phase.

Member

bnoordhuis commented Jan 25, 2017

It's a bit buried in the sea of warnings but the build error is this:

c1xx : fatal error C1083: Cannot open source file: '..\..\..\build\Release\obj\global_intermediate\src\inspector\protocol\Protocol.cpp': No such file or directory [c:\workspace\node-compile-windows\label\win-vs2015\deps\v8\src\v8_base_0.vcxproj]
  Debugger.cpp
c1xx : fatal error C1083: Cannot open source file: '..\..\..\build\Release\obj\global_intermediate\src\inspector\protocol\Debugger.cpp': No such file or directory [c:\workspace\node-compile-windows\label\win-vs2015\deps\v8\src\v8_base_0.vcxproj]
  Profiler.cpp
c1xx : fatal error C1083: Cannot open source file: '..\..\..\build\Release\obj\global_intermediate\src\inspector\protocol\Profiler.cpp': No such file or directory [c:\workspace\node-compile-windows\label\win-vs2015\deps\v8\src\v8_base_0.vcxproj]
  Schema.cpp
c1xx : fatal error C1083: Cannot open source file: '..\..\..\build\Release\obj\global_intermediate\src\inspector\protocol\Schema.cpp': No such file or directory [c:\workspace\node-compile-windows\label\win-vs2015\deps\v8\src\v8_base_0.vcxproj]

Seems something goes wrong in the generate_inspector_protocol_sources phase.

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Jan 25, 2017

Member

cc @ofrobots ^
Maybe something missing in node.gyp?

Member

targos commented Jan 25, 2017

cc @ofrobots ^
Maybe something missing in node.gyp?

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Jan 26, 2017

Contributor

I ran out of day-time and couldn't look at this today. I will try again tomorrow. /cc @eugeneo.

Contributor

ofrobots commented Jan 26, 2017

I ran out of day-time and couldn't look at this today. I will try again tomorrow. /cc @eugeneo.

@ChALkeR

This comment has been minimized.

Show comment
Hide comment
@ChALkeR

ChALkeR Jan 26, 2017

Member

V8 5.6 should become stable next week

I believe that already happened =).

Member

ChALkeR commented Jan 26, 2017

V8 5.6 should become stable next week

I believe that already happened =).

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Jan 30, 2017

Member

/cc @dgozman

Maybe it's a bug in V8's gypfiles?
See nodejs/help#471 for more details on the error.

Member

targos commented Jan 30, 2017

/cc @dgozman

Maybe it's a bug in V8's gypfiles?
See nodejs/help#471 for more details on the error.

@eugeneo

This comment has been minimized.

Show comment
Hide comment
@eugeneo

eugeneo Jan 30, 2017

Contributor

@targos I am not sure, but this might be an issue @ofrobots was looking into last week.

Contributor

eugeneo commented Jan 30, 2017

@targos I am not sure, but this might be an issue @ofrobots was looking into last week.

zcbenz added a commit to electron/node that referenced this pull request Feb 2, 2017

kevinsawicki added a commit to electron/node that referenced this pull request Feb 6, 2017

@YurySolovyov

This comment has been minimized.

Show comment
Hide comment
@YurySolovyov

YurySolovyov Feb 7, 2017

5.7 has been released with faster async/await and PromiseHook API. Would it make sense to jump into it ?

YurySolovyov commented Feb 7, 2017

5.7 has been released with faster async/await and PromiseHook API. Would it make sense to jump into it ?

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Feb 7, 2017

Member

@YurySolovyov 5.7 is still in beta phase. We will update to it when Chrome 57 is stable (should be mid-March)

Member

targos commented Feb 7, 2017

@YurySolovyov 5.7 is still in beta phase. We will update to it when Chrome 57 is stable (should be mid-March)

kevinsawicki added a commit to electron/node that referenced this pull request Feb 7, 2017

@targos targos removed the build label Feb 8, 2017

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Feb 8, 2017

Member

ping @nodejs/v8 for the Windows build issue.

Basically the problem is that deps\v8\src\v8_base_0.vcxproj instructs to look for the inspector generated source files in ..\..\..\build\Release\obj\global_intermediate\src but they are in fact in ..\..\..\Release\obj\global_intermediate\src.

Member

targos commented Feb 8, 2017

ping @nodejs/v8 for the Windows build issue.

Basically the problem is that deps\v8\src\v8_base_0.vcxproj instructs to look for the inspector generated source files in ..\..\..\build\Release\obj\global_intermediate\src but they are in fact in ..\..\..\Release\obj\global_intermediate\src.

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Feb 8, 2017

Member

@targos Can you check if this patch helps?

diff --git a/deps/v8/gypfiles/toolchain.gypi b/deps/v8/gypfiles/toolchain.gypi
index 95eb1d9..5105fff 100644
--- a/deps/v8/gypfiles/toolchain.gypi
+++ b/deps/v8/gypfiles/toolchain.gypi
@@ -989,8 +989,6 @@
         #       present in VS 2003 and earlier.
         'msvs_disabled_warnings': [4351],
         'msvs_configuration_attributes': {
-          'OutputDirectory': '<(DEPTH)\\build\\$(ConfigurationName)',
-          'IntermediateDirectory': '$(OutDir)\\obj\\$(ProjectName)',
           'CharacterSet': '1',
         },
       }],

If we don't want to float a patch, we should be able to override it from common.gypi like this:

diff --git a/common.gypi b/common.gypi
index d87205e..7fd9efc 100644
--- a/common.gypi
+++ b/common.gypi
@@ -148,6 +148,10 @@
             }
           }]
         ],
+        'msvs_configuration_attributes': {
+          'IntermediateDirectory': '$(ConfigurationName)\\obj\\$(ProjectName)',
+          'OutputDirectory': '$(SolutionDir)$(ConfigurationName)',
+        },
         'msvs_settings': {
           'VCCLCompilerTool': {
             'Optimization': 3, # /Ox, full optimization

(Caveat emptor: needs to be duplicated in the Debug configuration.)

Member

bnoordhuis commented Feb 8, 2017

@targos Can you check if this patch helps?

diff --git a/deps/v8/gypfiles/toolchain.gypi b/deps/v8/gypfiles/toolchain.gypi
index 95eb1d9..5105fff 100644
--- a/deps/v8/gypfiles/toolchain.gypi
+++ b/deps/v8/gypfiles/toolchain.gypi
@@ -989,8 +989,6 @@
         #       present in VS 2003 and earlier.
         'msvs_disabled_warnings': [4351],
         'msvs_configuration_attributes': {
-          'OutputDirectory': '<(DEPTH)\\build\\$(ConfigurationName)',
-          'IntermediateDirectory': '$(OutDir)\\obj\\$(ProjectName)',
           'CharacterSet': '1',
         },
       }],

If we don't want to float a patch, we should be able to override it from common.gypi like this:

diff --git a/common.gypi b/common.gypi
index d87205e..7fd9efc 100644
--- a/common.gypi
+++ b/common.gypi
@@ -148,6 +148,10 @@
             }
           }]
         ],
+        'msvs_configuration_attributes': {
+          'IntermediateDirectory': '$(ConfigurationName)\\obj\\$(ProjectName)',
+          'OutputDirectory': '$(SolutionDir)$(ConfigurationName)',
+        },
         'msvs_settings': {
           'VCCLCompilerTool': {
             'Optimization': 3, # /Ox, full optimization

(Caveat emptor: needs to be duplicated in the Debug configuration.)

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Feb 8, 2017

Contributor

@bnoordhuis thanks for getting around to it sooner than me. I did a quick test on a windows box with that patch and it seems to work. /cc @ak239: can you think of a better upstream fix?

Contributor

ofrobots commented Feb 8, 2017

@bnoordhuis thanks for getting around to it sooner than me. I did a quick test on a windows box with that patch and it seems to work. /cc @ak239: can you think of a better upstream fix?

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Feb 8, 2017

Member

@bnoordhuis I confirm it works on my computer too. What prevents us from upstreaming this fix?

Member

targos commented Feb 8, 2017

@bnoordhuis I confirm it works on my computer too. What prevents us from upstreaming this fix?

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Feb 8, 2017

Member

Nothing, except I don't know why V8 sets them in the first place. They seem to have been introduced in v8/v8@3bf4484 but the commit log doesn't explain why.

Member

bnoordhuis commented Feb 8, 2017

Nothing, except I don't know why V8 sets them in the first place. They seem to have been introduced in v8/v8@3bf4484 but the commit log doesn't explain why.

@jasnell jasnell added this to the 8.0.0 milestone Feb 8, 2017

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Feb 9, 2017

Member

@targos Can you update the paths in LICENSE and tools/license-builder.sh? They still refer to deps/v8_inspector.

Member

bnoordhuis commented Feb 9, 2017

@targos Can you update the paths in LICENSE and tools/license-builder.sh? They still refer to deps/v8_inspector.

kevinsawicki added a commit to electron/node that referenced this pull request Feb 13, 2017

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Feb 13, 2017

Member

@targos Do you think you have time to finish this this week? If not, I'd be happy to take over. The conflict is caused by 039a813 but the fix exists in 5.6-lkgr so you can just drop/revert it if you want.

Member

bnoordhuis commented Feb 13, 2017

@targos Do you think you have time to finish this this week? If not, I'd be happy to take over. The conflict is caused by 039a813 but the fix exists in 5.6-lkgr so you can just drop/revert it if you want.

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Feb 14, 2017

Member

@bnoordhuis updated.

Now the test that you added in #11255 is failing:

=== release test-intl-no-icu-data ===
Path: parallel/test-intl-no-icu-data
assert.js:85
  throw new assert.AssertionError({
  ^
AssertionError: 'Ç' === 'ç'
    at Object.<anonymous> (/home/mzasso/git/forks/node-v8-56/test/parallel/test-intl-no-icu-data.js:8:8)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)
    at Module.runMain (module.js:605:10)
    at run (bootstrap_node.js:422:7)
    at startup (bootstrap_node.js:143:9)
    at bootstrap_node.js:537:3

Am I missing some floating patch?

Edit: the CL that fixed this bug should be in 5.6: https://bugs.chromium.org/p/v8/issues/detail?id=5681

Member

targos commented Feb 14, 2017

@bnoordhuis updated.

Now the test that you added in #11255 is failing:

=== release test-intl-no-icu-data ===
Path: parallel/test-intl-no-icu-data
assert.js:85
  throw new assert.AssertionError({
  ^
AssertionError: 'Ç' === 'ç'
    at Object.<anonymous> (/home/mzasso/git/forks/node-v8-56/test/parallel/test-intl-no-icu-data.js:8:8)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)
    at Module.runMain (module.js:605:10)
    at run (bootstrap_node.js:422:7)
    at startup (bootstrap_node.js:143:9)
    at bootstrap_node.js:537:3

Am I missing some floating patch?

Edit: the CL that fixed this bug should be in 5.6: https://bugs.chromium.org/p/v8/issues/detail?id=5681

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Feb 14, 2017

Member

Interesting. Perhaps V8's fallback path for unavailable locales changed? Case in point, with the copy of V8 in master and --icu-data-dir=test/fixtures/empty:

'ç'.toLocaleUpperCase('el')  // ç
'ç'.toLocaleUpperCase('de')  // Ç

As an alternative you could check that assert.deepStrictEqual(Intl.NumberFormat.supportedLocalesOf('en'), []).

Member

bnoordhuis commented Feb 14, 2017

Interesting. Perhaps V8's fallback path for unavailable locales changed? Case in point, with the copy of V8 in master and --icu-data-dir=test/fixtures/empty:

'ç'.toLocaleUpperCase('el')  // ç
'ç'.toLocaleUpperCase('de')  // Ç

As an alternative you could check that assert.deepStrictEqual(Intl.NumberFormat.supportedLocalesOf('en'), []).

@Trott Trott referenced this pull request Feb 16, 2017

Closed

net: avoid use of `arguments` #11411

2 of 2 tasks complete

@targos targos removed the in progress label Feb 16, 2017

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Feb 16, 2017

Member

As an alternative you could check that assert.deepStrictEqual(Intl.NumberFormat.supportedLocalesOf('en'), []).

Done. I think the PR is now ready to be reviewed.

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

Member

targos commented Feb 16, 2017

As an alternative you could check that assert.deepStrictEqual(Intl.NumberFormat.supportedLocalesOf('en'), []).

Done. I think the PR is now ready to be reviewed.

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

@bnoordhuis

Mostly LGTM but I believe I spotted a dependency bug in node.gyp.

The freebsd buildbot failure is infrastructural but the ARM failures are real.

It seems the update broke cross-compiling but you can probably fix that by adding a 'toolsets': ['host'] stanza toprotocol_generated_sources and other targets.

At any rate, mostly there!

Show outdated Hide outdated node.gyp
@@ -344,11 +344,8 @@
'src/inspector_socket_server.h',
],
'dependencies': [
'deps/v8_inspector/src/inspector/inspector.gyp:standalone_inspector',
'v8_inspector_compress_protocol_json#host',
],

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 16, 2017

Member

Can you remove the dependencies list now that it's empty?

That said, I'm curious what generates <(SHARED_INTERMEDIATE_DIR)/v8_inspector_protocol_json.h now. Does it perhaps work by accident because the cctest target also has a dependency on it?

@bnoordhuis

bnoordhuis Feb 16, 2017

Member

Can you remove the dependencies list now that it's empty?

That said, I'm curious what generates <(SHARED_INTERMEDIATE_DIR)/v8_inspector_protocol_json.h now. Does it perhaps work by accident because the cctest target also has a dependency on it?

This comment has been minimized.

@targos
@targos

This comment has been minimized.

@targos

targos Feb 21, 2017

Member

@bnoordhuis

Does it perhaps work by accident because the cctest target also has a dependency on it?

I think so. I suppose we should add it as a dependency to the node target too?
Can you explain what makes cctest to be always built? Maybe that should only happen when make cctest is actually executed?

@targos

targos Feb 21, 2017

Member

@bnoordhuis

Does it perhaps work by accident because the cctest target also has a dependency on it?

I think so. I suppose we should add it as a dependency to the node target too?
Can you explain what makes cctest to be always built? Maybe that should only happen when make cctest is actually executed?

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 21, 2017

Member

That's just how gyp works. Without an explicit target, the generated out/Makefile builds everything.

@bnoordhuis

bnoordhuis Feb 21, 2017

Member

That's just how gyp works. Without an explicit target, the generated out/Makefile builds everything.

@targos

This comment has been minimized.

Show comment
Hide comment
@jbergstroem

This comment has been minimized.

Show comment
Hide comment
@jbergstroem

jbergstroem Feb 16, 2017

Member

FreeBSD seems to be:

gyp: Dependency 'deps/v8/src/inspector/inspector.gyp:protocol_generated_sources#target' not found while trying to load target deps/v8/src/v8.gyp:v8_base#target
Member

jbergstroem commented Feb 16, 2017

FreeBSD seems to be:

gyp: Dependency 'deps/v8/src/inspector/inspector.gyp:protocol_generated_sources#target' not found while trying to load target deps/v8/src/v8.gyp:v8_base#target

@italoacasas italoacasas referenced this pull request Mar 1, 2017

Merged

7.7.1 Proposal #11638

kisg pushed a commit to paul99/v8mips that referenced this pull request Mar 2, 2017

[build] Fix gyp files for building inspector
This patch fixes compilation of V8 with inspector on Windows as well as
cross-compilation of the V8 inspector.

BUG=

Refs: nodejs/node#10992
Review-Url: https://codereview.chromium.org/2705423003
Cr-Commit-Position: refs/heads/master@{#43533}
@targos

This comment has been minimized.

Show comment
Hide comment
Member

targos commented Mar 2, 2017

@zertosh zertosh referenced this pull request Mar 7, 2017

Closed

v8: add cachedDataVersionTag #11515

4 of 4 tasks complete

targos added a commit to targos/node that referenced this pull request Mar 8, 2017

deps: fix gyp build configuration for Windows
PR-URL: nodejs#10992
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

targos added a commit to targos/node that referenced this pull request Mar 8, 2017

deps: fix gyp configuration for v8-inspector
Cross-compiled builds need different toolsets.

PR-URL: nodejs#10992
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

targos added a commit to targos/node that referenced this pull request Mar 23, 2017

deps: cherry-pick c5c570f from upstream V8
Original commit message:

    [build] Fix gyp files for building inspector

    This patch fixes compilation of V8 with inspector on Windows as well as
    cross-compilation of the V8 inspector.

    BUG=

    Refs: nodejs#10992
    Review-Url: https://codereview.chromium.org/2705423003
    Cr-Commit-Position: refs/heads/master@{#43533}
@eugeneo

This comment has been minimized.

Show comment
Hide comment
@eugeneo

eugeneo Mar 23, 2017

Contributor

Are third_party being rolled as expected? https://github.com/v8/v8/blob/5.7-lkgr/third_party/inspector_protocol/lib/Values_h.template was updated 4 months ago and looks like this CL still has an old version.

Contributor

eugeneo commented Mar 23, 2017

Are third_party being rolled as expected? https://github.com/v8/v8/blob/5.7-lkgr/third_party/inspector_protocol/lib/Values_h.template was updated 4 months ago and looks like this CL still has an old version.

@eugeneo

This comment has been minimized.

Show comment
Hide comment
@eugeneo

eugeneo Mar 23, 2017

Contributor

Sorry, I've mistaken 5.6 and 5.7.

Contributor

eugeneo commented Mar 23, 2017

Sorry, I've mistaken 5.6 and 5.7.

targos added a commit to targos/node that referenced this pull request Mar 24, 2017

deps: cherry-pick c5c570f from upstream V8
Original commit message:

    [build] Fix gyp files for building inspector

    This patch fixes compilation of V8 with inspector on Windows as well as
    cross-compilation of the V8 inspector.

    BUG=

    Refs: nodejs#10992
    Review-Url: https://codereview.chromium.org/2705423003
    Cr-Commit-Position: refs/heads/master@{#43533}

targos added a commit to targos/node that referenced this pull request Mar 25, 2017

deps: cherry-pick c5c570f from upstream V8
Original commit message:

    [build] Fix gyp files for building inspector

    This patch fixes compilation of V8 with inspector on Windows as well as
    cross-compilation of the V8 inspector.

    BUG=

    Refs: nodejs#10992
    Review-Url: https://codereview.chromium.org/2705423003
    Cr-Commit-Position: refs/heads/master@{#43533}

PR-URL: nodejs#11752
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

@jasnell jasnell referenced this pull request Apr 4, 2017

Closed

8.0.0 Release Proposal #12220

targos added a commit to targos/node that referenced this pull request Apr 10, 2017

deps: cherry-pick c5c570f from upstream V8
Original commit message:

    [build] Fix gyp files for building inspector

    This patch fixes compilation of V8 with inspector on Windows as well as
    cross-compilation of the V8 inspector.

    BUG=

    Refs: nodejs#10992
    Review-Url: https://codereview.chromium.org/2705423003
    Cr-Commit-Position: refs/heads/master@{#43533}

hashseed added a commit to v8/node that referenced this pull request Apr 10, 2017

deps: cherry-pick c5c570f from upstream V8
Original commit message:

    [build] Fix gyp files for building inspector

    This patch fixes compilation of V8 with inspector on Windows as well as
    cross-compilation of the V8 inspector.

    BUG=

    Refs: nodejs#10992
    Review-Url: https://codereview.chromium.org/2705423003
    Cr-Commit-Position: refs/heads/master@{#43533}

zcbenz added a commit to electron/node that referenced this pull request Apr 27, 2017

zcbenz added a commit to electron/node that referenced this pull request May 15, 2017

zcbenz added a commit to electron/node that referenced this pull request May 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment