build: compile with --gnu++17 #38807

richardlau · 2021-05-25T10:50:55Z

Enables C++17 features.

Let's see if anything breaks. Since this is in common.gypi it will affect addons as well and may introduce an incompatibility with any addons that have explicitly compiled with gnu++1z and an older (i.e. GCC 7 or earlier) version of GCC -- although using that option with those compilers is described as "highly experimental" in the GCC manual.

nodejs-github-bot · 2021-05-25T10:52:18Z

CI: https://ci.nodejs.org/job/node-test-pull-request/38319/

targos · 2021-05-25T10:53:03Z

Are our baseline compiler versions compatible?

richardlau · 2021-05-25T10:54:50Z

Are our baseline compiler versions compatible?

We believe so, #38788 (comment).

richardlau · 2021-05-25T11:25:18Z

Well this has been a useful exercise... turns out several hosts on the CI (so far SmartOS and anything on Ubuntu 18.04) are using GCC 7 even though we baselined on GCC 8 for Node.js 16.

targos · 2021-05-25T12:00:38Z

Are our baseline compiler versions compatible?

Based on https://en.cppreference.com/w/cpp/compiler_support#cpp17, it depends on which features we are going to use, but GCC 7 already supported many of them. MSVC seems to support everything and Apple Clang most of them.

richardlau · 2021-05-25T18:35:25Z

Opened nodejs/build#2660 for getting GCC 8 onto Ubuntu 18.04. Will need a separate PR for SmartOS.

nodejs-github-bot · 2021-05-28T13:00:45Z

CI: https://ci.nodejs.org/job/node-test-pull-request/38362/

jasnell · 2021-09-03T14:23:52Z

Definite +1 on bumping up to ++17 as soon as we can.

nodejs-github-bot · 2021-09-03T16:08:33Z

CI: https://ci.nodejs.org/job/node-test-pull-request/39758/

targos · 2021-09-03T16:23:02Z

Should we do it for Windows too?

richardlau · 2021-09-03T16:34:33Z

@targos Do you know what the gyp flag/setting would be? I don't think we have an existing Windows equivalent for C++14.

richardlau · 2021-09-03T16:57:47Z

Do you know what the gyp flag/setting would be? I don't think we have an existing Windows equivalent for C++14.

Ah nodejs/node-gyp#1662

nodejs-github-bot · 2021-09-03T17:09:08Z

CI: https://ci.nodejs.org/job/node-test-pull-request/39761/

richardlau · 2021-09-03T17:49:08Z

🤔 I think enabling -std:c++17 on Windows has run into the same issue as #39976:
e.g. https://ci.nodejs.org/job/node-compile-windows/41950/nodes=win-vs2019/console

18:29:34   embedded-file-writer.cc
18:29:42      Creating library ..\..\out\Release\mksnapshot.lib and object ..\..\out\Release\mksnapshot.exp
18:29:54 mksnapshot.obj : error LNK2001: unresolved external symbol "public: void __cdecl v8::internal::FixedArray::set(int,class v8::internal::Smi)" (?set@FixedArray@internal@v8@@QEAAXHVSmi@23@@Z) [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\mksnapshot.vcxproj]
18:29:55 platform-embedded-file-writer-aix.obj : error LNK2001: unresolved external symbol "public: void __cdecl v8::internal::FixedArray::set(int,class v8::internal::Smi)" (?set@FixedArray@internal@v8@@QEAAXHVSmi@23@@Z) [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\mksnapshot.vcxproj]
18:29:55 platform-embedded-file-writer-generic.obj : error LNK2001: unresolved external symbol "public: void __cdecl v8::internal::FixedArray::set(int,class v8::internal::Smi)" (?set@FixedArray@internal@v8@@QEAAXHVSmi@23@@Z) [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\mksnapshot.vcxproj]
18:29:55 platform-embedded-file-writer-mac.obj : error LNK2001: unresolved external symbol "public: void __cdecl v8::internal::FixedArray::set(int,class v8::internal::Smi)" (?set@FixedArray@internal@v8@@QEAAXHVSmi@23@@Z) [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\mksnapshot.vcxproj]
18:29:55 platform-embedded-file-writer-win.obj : error LNK2001: unresolved external symbol "public: void __cdecl v8::internal::FixedArray::set(int,class v8::internal::Smi)" (?set@FixedArray@internal@v8@@QEAAXHVSmi@23@@Z) [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\mksnapshot.vcxproj]
18:29:55 ..\..\out\Release\mksnapshot.exe : fatal error LNK1120: 1 unresolved externals [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\mksnapshot.vcxproj]
18:29:55

(I'm also surprised the build is linking files called -aix and -mac on Windows.)

targos · 2021-09-03T18:07:07Z

I think enabling -std:c++17 on Windows has run into the same issue as #39976

Makes sense. VS 2022 probably makes it the default.

targos · 2021-09-03T18:09:03Z

(I'm also surprised the build is linking files called -aix and -mac on Windows.)

That seems intended:

node/deps/v8/BUILD.gn

Lines 5311 to 5330 in f26c2ce

    
           v8_executable("mksnapshot") { 
        
             visibility = [ ":*" ]  # Only targets in this file can depend on this. 
        
             sources = [ 
        
               "src/snapshot/embedded/embedded-empty.cc", 
        
               "src/snapshot/embedded/embedded-file-writer.cc", 
        
               "src/snapshot/embedded/embedded-file-writer.h", 
        
               "src/snapshot/embedded/platform-embedded-file-writer-aix.cc", 
        
               "src/snapshot/embedded/platform-embedded-file-writer-aix.h", 
        
               "src/snapshot/embedded/platform-embedded-file-writer-base.cc", 
        
               "src/snapshot/embedded/platform-embedded-file-writer-base.h", 
        
               "src/snapshot/embedded/platform-embedded-file-writer-generic.cc", 
        
               "src/snapshot/embedded/platform-embedded-file-writer-generic.h", 
        
               "src/snapshot/embedded/platform-embedded-file-writer-mac.cc", 
        
               "src/snapshot/embedded/platform-embedded-file-writer-mac.h", 
        
               "src/snapshot/embedded/platform-embedded-file-writer-win.cc", 
        
               "src/snapshot/embedded/platform-embedded-file-writer-win.h", 
        
               "src/snapshot/mksnapshot.cc", 
        
               "src/snapshot/snapshot-empty.cc", 
        
             ]

targos · 2021-09-05T09:46:59Z

I think a00971f needs to be rewritten somehow (I tried to revert it but get the exact same errors). /cc @gengjiawen

PR-URL: nodejs/node#35415 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> deps: patch for v8 on windows PR-URL: nodejs/node#40010 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> deps: patch v8 for vs2019 in std17 PR-URL: nodejs/node#40060 Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> deps: workaround debug link error on Windows PR-URL: nodejs/node#38807 Refs: nodejs/node#38788 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ash Cripps <acripps@redhat.com>

PR-URL: #35415 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> deps: patch for v8 on windows PR-URL: #40010 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> deps: patch v8 for vs2019 in std17 PR-URL: #40060 Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> deps: workaround debug link error on Windows PR-URL: #38807 Refs: #38788 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ash Cripps <acripps@redhat.com>

PR-URL: nodejs/node#35415 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> deps: patch for v8 on windows PR-URL: nodejs/node#40010 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> deps: patch v8 for vs2019 in std17 PR-URL: nodejs/node#40060 Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> deps: workaround debug link error on Windows PR-URL: nodejs/node#38807 Refs: nodejs/node#38788 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ash Cripps <acripps@redhat.com>

PR-URL: #35415 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> deps: patch for v8 on windows PR-URL: #40010 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> deps: patch v8 for vs2019 in std17 PR-URL: #40060 Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> deps: workaround debug link error on Windows PR-URL: #38807 Refs: #38788 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ash Cripps <acripps@redhat.com>

PR-URL: nodejs/node#35415 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> deps: patch for v8 on windows PR-URL: nodejs/node#40010 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> deps: patch v8 for vs2019 in std17 PR-URL: nodejs/node#40060 Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> deps: workaround debug link error on Windows PR-URL: nodejs/node#38807 Refs: nodejs/node#38788 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ash Cripps <acripps@redhat.com>

PR-URL: nodejs#35415 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> deps: patch for v8 on windows PR-URL: nodejs#40010 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> deps: patch v8 for vs2019 in std17 PR-URL: nodejs#40060 Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> deps: workaround debug link error on Windows PR-URL: nodejs#38807 Refs: nodejs#38788 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ash Cripps <acripps@redhat.com>

PR-URL: nodejs/node#35415 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> deps: patch for v8 on windows PR-URL: nodejs/node#40010 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> deps: patch v8 for vs2019 in std17 PR-URL: nodejs/node#40060 Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> deps: workaround debug link error on Windows PR-URL: nodejs/node#38807 Refs: nodejs/node#38788 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ash Cripps <acripps@redhat.com>

richardlau added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 25, 2021

github-actions bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. labels May 25, 2021

richardlau mentioned this pull request May 25, 2021

worker: leave TODO comments for using std::variant when possible #38788

Closed

richardlau added the blocked PRs that are blocked by other issues or PRs. label May 25, 2021

RaisinTen mentioned this pull request Jun 14, 2021

src,url: separate some tables out of node_url.cc #38988

Closed

aduh95 approved these changes Sep 3, 2021

View reviewed changes

richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 3, 2021

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 3, 2021

richardlau force-pushed the c++17 branch from 9a724e6 to 596f7bd Compare September 3, 2021 16:58

richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 3, 2021

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 3, 2021

richardlau mentioned this pull request Sep 3, 2021

node-test-commit-arm should use select-compiler.sh nodejs/build#2746

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: compile with --gnu++17 #38807

build: compile with --gnu++17 #38807

richardlau commented May 25, 2021

nodejs-github-bot commented May 25, 2021

targos commented May 25, 2021

richardlau commented May 25, 2021

richardlau commented May 25, 2021

targos commented May 25, 2021

richardlau commented May 25, 2021

nodejs-github-bot commented May 28, 2021

jasnell commented Sep 3, 2021

nodejs-github-bot commented Sep 3, 2021

targos commented Sep 3, 2021

richardlau commented Sep 3, 2021

richardlau commented Sep 3, 2021

nodejs-github-bot commented Sep 3, 2021

richardlau commented Sep 3, 2021

targos commented Sep 3, 2021

targos commented Sep 3, 2021

targos commented Sep 5, 2021

build: compile with --gnu++17 #38807

build: compile with --gnu++17 #38807

Conversation

richardlau commented May 25, 2021

nodejs-github-bot commented May 25, 2021

targos commented May 25, 2021

richardlau commented May 25, 2021

richardlau commented May 25, 2021

targos commented May 25, 2021

richardlau commented May 25, 2021

nodejs-github-bot commented May 28, 2021

jasnell commented Sep 3, 2021

nodejs-github-bot commented Sep 3, 2021

targos commented Sep 3, 2021

richardlau commented Sep 3, 2021

richardlau commented Sep 3, 2021

nodejs-github-bot commented Sep 3, 2021

richardlau commented Sep 3, 2021

targos commented Sep 3, 2021

targos commented Sep 3, 2021

targos commented Sep 5, 2021