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 7.9 #30020

Merged
merged 28 commits into from
Nov 8, 2019
Merged

deps: update V8 to 7.9 #30020

merged 28 commits into from
Nov 8, 2019

Conversation

targos
Copy link
Member

@targos targos commented Oct 18, 2019

Chrome release date: Dec 10th, 2019

@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 Oct 18, 2019
@targos targos added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 18, 2019
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 18, 2019

@targos
Copy link
Member Author

targos commented Oct 18, 2019

11:31:00 ../deps/v8/src/base/macros.h:236:33: error: 'is_trivially_copyable' is not a member of 'std'
11:31:00    static constexpr bool value = std::is_trivially_copyable<T>::value;
11:31:00                                  ^

This error is concerning. If I'm reading the documentation correctly, this was added in C++17.

@nodejs/v8 according to https://chromium-cpp.appspot.com/, C++17 is not yet supported in Chromium. Is it going to change soon?

@psmarshall
Copy link
Contributor

@backes Maybe you have an idea

@gengjiawen gengjiawen mentioned this pull request Oct 18, 2019
4 tasks
@backes
Copy link
Contributor

backes commented Oct 18, 2019

The documentation you link sais: (since C++11)
Note that v8 requires C++14 now, which should be supported by all toolchains we care about (including node).

@gengjiawen
Copy link
Member

on macOS, https://ci.nodejs.org/job/node-test-commit-osx/nodes=osx1011/29417/console

17:19:10 ../deps/v8/src/compiler/serializer-for-background-compilation.cc:390:12: error: constructor for 'v8::internal::compiler::Callee' must explicitly initialize the const member 'blueprint_'
17:19:10   explicit Callee(Handle<JSFunction> jsfunction) : jsfunction_(jsfunction) {}
17:19:10            ^
17:19:10 ../deps/v8/src/compiler/serializer-for-background-compilation.cc:415:43: note: 'blueprint_' declared here
17:19:10   base::Optional<FunctionBlueprint> const blueprint_;
17:19:10                                           ^
17:19:10 ../deps/v8/src/compiler/serializer-for-background-compilation.cc:391:12: error: constructor for 'v8::internal::compiler::Callee' must explicitly initialize the const member 'jsfunction_'
17:19:10   explicit Callee(FunctionBlueprint const& blueprint) : blueprint_(blueprint) {}
17:19:10            ^
17:19:10 ../deps/v8/src/compiler/serializer-for-background-compilation.cc:414:33: note: 'jsfunction_' declared here
17:19:10   MaybeHandle<JSFunction> const jsfunction_;

@targos
Copy link
Member Author

targos commented Oct 18, 2019

@backes there is this thing in the documentation:
image

but maybe it's not what is being used here, I'm not able to tell

@addaleax
Copy link
Member

@targos No, V8 is only using the C++11 variant (is_trivially_copyable, not is_trivially_copyable_v).

I think the macros.h header may just be missing an #include <type_traits>.

@backes
Copy link
Contributor

backes commented Oct 18, 2019

@addaleax Ack, but I would vote for putting that header in the file that uses base::is_trivially_copyable instead.
Can you point me to the exact error output, so I can create a CL on v8's side? Or you just create one and send it to me for review.

@targos
Copy link
Member Author

targos commented Oct 18, 2019

the error comes from this job: https://ci.nodejs.org/job/node-test-commit-v8-linux/2559/nodes=benchmark,v8test=v8test/console

11:31:00 In file included from ../deps/v8/src/base/lazy-instance.h:73:0,
11:31:00                  from ../deps/v8/src/base/platform/condition-variable.h:9,
11:31:00                  from ../deps/v8/src/libplatform/default-foreground-task-runner.h:13,
11:31:00                  from ../deps/v8/src/libplatform/default-foreground-task-runner.cc:5:
11:31:00 ../deps/v8/src/base/macros.h:236:33: error: 'is_trivially_copyable' is not a member of 'std'
11:31:00    static constexpr bool value = std::is_trivially_copyable<T>::value;
11:31:00                                  ^
11:31:00 ../deps/v8/src/base/macros.h:236:33: note: suggested alternative:
11:31:00 In file included from ../deps/v8/src/base/lazy-instance.h:73:0,
11:31:00                  from ../deps/v8/src/base/platform/condition-variable.h:9,
11:31:00                  from ../deps/v8/src/libplatform/default-foreground-task-runner.h:13,
11:31:00                  from ../deps/v8/src/libplatform/default-foreground-task-runner.cc:5:
11:31:00 ../deps/v8/src/base/macros.h:209:8: note:   'v8::base::is_trivially_copyable'
11:31:00  struct is_trivially_copyable {
11:31:00         ^
11:31:00 In file included from ../deps/v8/src/base/lazy-instance.h:73:0,
11:31:00                  from ../deps/v8/src/base/platform/condition-variable.h:9,
11:31:00                  from ../deps/v8/src/libplatform/default-foreground-task-runner.h:13,
11:31:00                  from ../deps/v8/src/libplatform/default-foreground-task-runner.cc:5:
11:31:00 ../deps/v8/src/base/macros.h:236:61: error: expected primary-expression before '>' token
11:31:00    static constexpr bool value = std::is_trivially_copyable<T>::value;
11:31:00                                                              ^
11:31:00 ../deps/v8/src/base/macros.h:236:62: error: '::value' has not been declared
11:31:00    static constexpr bool value = std::is_trivially_copyable<T>::value;
11:31:00                                                               ^
11:31:00 make[2]: *** [/home/iojs/build/workspace/node-test-commit-v8-linux/nodes/benchmark/v8test/v8test/out/Release/obj.target/v8_libplatform/deps/v8/src/libplatform/default-foreground-task-runner.o] Error 1

@backes
Copy link
Contributor

backes commented Oct 18, 2019

Ok, looking into macros.h, we really use more type traits internally there. I created a CL to include <type_traits> in macros.h: https://crrev.com/c/1868622

@targos
Copy link
Member Author

targos commented Oct 18, 2019

Thanks!

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 18, 2019

@targos
Copy link
Member Author

targos commented Oct 18, 2019

So, this isn't enough to make it work in our CI

@nodejs/build : could you please check the compiler version on the machine used here: https://ci.nodejs.org/job/node-test-commit-v8-linux/2561/nodes=benchmark,v8test=v8test/console

./configure says it's too old:

17:58:00 + ./configure
17:58:02 Node configure: Found Python 2.7.6...
17:58:02 WARNING: C++ compiler too old, need g++ 6.3.0 or clang++ 8.0.0 (CXX=g++)

@richardlau
Copy link
Member

So, this isn't enough to make it work in our CI

@nodejs/build : could you please check the compiler version on the machine used here: https://ci.nodejs.org/job/node-test-commit-v8-linux/2561/nodes=benchmark,v8test=v8test/console

./configure says it's too old:

17:58:00 + ./configure
17:58:02 Node configure: Found Python 2.7.6...
17:58:02 WARNING: C++ compiler too old, need g++ 6.3.0 or clang++ 8.0.0 (CXX=g++)

Don't know what version is on the build machine but I've PR'ed a change to configure so we don't have to work it out next time: #30028

@rvagg
Copy link
Member

rvagg commented Oct 21, 2019

Wait, configure says that clang 8.0.0 is a minimum but that's not documented in BUILDING.md, as far as I can tell, configure is the only place this is documented.

I'm trying to work out whether to knock out FreeBSD from >=13.x or >=14.x, it only comes with clang 6.0.0 but our docs say FreeBSD >=11 is supported.

I think this means we should just exclude FreeBSD from >=14.x but maybe this is going to be bnackported to 13.x in which case we should probably be prepared now. Does someone want to get BUILDING.md into better shape wrt clang and also probably FreeBSD while you're at it?

@richardlau
Copy link
Member

Wait, configure says that clang 8.0.0 is a minimum but that's not documented in BUILDING.md, as far as I can tell, configure is the only place this is documented.

It was bumped up to 8.0.0 (previously 3.4.2) for Node.js 12 to match the LLVM version in XCode 8 for macOS: #26719 (comment)

@backes
Copy link
Contributor

backes commented Oct 21, 2019

re std::is_trivially_copyable:
This standard library function was not supported in the stdlibc++ 4.8, see https://gcc.gnu.org/onlinedocs/gcc-4.8.3/libstdc++/manual/manual/status.html#status.iso.2011.
We remove support for gcc <5 in this CL: https://crrev.com/c/1786283

Since node requires GCC 6.3, this shouldn't be a problem though. Except if some bot is using an unsupported GCC version.

addaleax added a commit to addaleax/node that referenced this pull request Jun 9, 2020
addaleax added a commit that referenced this pull request Jun 11, 2020
Removed in 2bdeb88.

Refs: #30020

PR-URL: #33810
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
codebytere pushed a commit that referenced this pull request Jun 18, 2020
Removed in 2bdeb88.

Refs: #30020

PR-URL: #33810
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
codebytere pushed a commit that referenced this pull request Jun 30, 2020
Removed in 2bdeb88.

Refs: #30020

PR-URL: #33810
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
codebytere added a commit to electron/electron that referenced this pull request Sep 1, 2020
codebytere added a commit to electron/electron that referenced this pull request Sep 2, 2020
codebytere added a commit to electron/electron that referenced this pull request Sep 4, 2020
codebytere added a commit to electron/electron that referenced this pull request Sep 14, 2020
codebytere added a commit to electron/electron that referenced this pull request Sep 14, 2020
codebytere added a commit to electron/electron that referenced this pull request Sep 14, 2020
codebytere added a commit to electron/electron that referenced this pull request Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. 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.