build: use higher version c++ #16501

refack · 2017-10-25T22:01:43Z

Use c++14 on all Windows and clang using platforms
Use c++11 on all GCC using platforms

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

build

addaleax · 2017-10-25T22:04:23Z

I guess my main question here would be, what’s the motivation for this? We’re still stuck with C++11 for now, and c++0x is essentially just a more-widely-supported alias for c++11 (unless I am mistaken about that)?

refack · 2017-10-25T22:07:32Z

https://ci.nodejs.org/job/node-test-commit/13465/

/cc @nodejs/build @nodejs/platform-freebsd
Seems like the only failing platform is freebsd10

refack · 2017-10-25T22:08:37Z

I guess my main question here would be, what’s the motivation for this?

@targos and V8 I assume - nodejs/build#945

addaleax · 2017-10-25T22:15:23Z

@targos Does bumping to c++11 fix https://chromium-review.googlesource.com/c/v8/v8/+/728026#message-a337cd5c8611b2408a8972deb3d20528b5845be1 ?

targos · 2017-10-26T07:57:54Z

@addaleax It doesn't :(

CI results:

OSX with gnu++11: fail
OSX with gnu++14: pass

I think we should align with V8 as much as possible. Luckily, their constraints are very similar to ours: they can use C++14 features if it compiles in GCC 4.8.

targos · 2017-10-26T08:06:08Z

common.gypi

-        'cflags_cc': [ '-std=c++14' ],
-      }, {
-        'cflags_cc': [ '-std=gnu++11' ],
-      }],


maybe keep that for FreeBSD since mac seems to ignore it

seishun · 2017-10-26T08:39:30Z

common.gypi

@@ -10,7 +10,9 @@
    'component%': 'static_library',   # NB. these names match with what V8 expects
    'msvs_multi_core_compile': '0',   # we do enable multicore compiles, but not using the V8 way
    'python%': 'python',
-
+    'gas_version%': 0,
+    'llvm_version%': 0,


What is this?

It's a variable set in the configure script when clang is detected.

I'm confused. The configure script wasn't modified in this PR. How come these two lines weren't necessary before?

In order to check llvm_version version (without first doing "feature detection"), we need these vars with default value (90f055f).

P.S. It not necessary in the current changeset, so I could move this to a new PR

seishun · 2017-10-26T08:41:06Z

common.gypi

@@ -211,6 +214,9 @@
        # and their sheer number drowns out other, more legitimate warnings.
        'DisableSpecificWarnings': ['4267'],
        'WarnAsError': 'false',
+        'AdditionalOptions': [ 
+          '/std:c++14'


Unnecessary, both VS2015 and VS2015 default to /std:c++14.

I'd rather have this (1) for being explicit (2) for future proofing, in case the default changes to C++17 or higher

seishun · 2017-10-26T08:42:53Z

Use c++11 on all GCC using platforms

Why C++11 if C++14 is necessary for V8?

targos · 2017-10-26T08:44:01Z

because GCC 4.8 does not support C++14 and our build infra is still on this version.

seishun · 2017-10-26T08:49:17Z

If this doesn't solve the V8 problem, then what's the point?

targos · 2017-10-26T08:52:22Z

The V8 problem is with Clang. To fix it we need -std=gnu++14 in this compiler.
We can keep -std=gnu++0x in GCC if the change feels unnecessary.

seishun · 2017-10-26T08:57:35Z

I'm -1 on using different C++ versions on clang and gcc. Since gcc supports -std=c++14 since 5.2 (source), I suggest using either gnu++1y or c++1y for both.

bnoordhuis · 2017-10-26T09:01:16Z

because GCC 4.8 does not support C++14 and our build infra is still on this version.

Then wouldn't it make sense to upgrade to 4.9 and stop using 4.8?

targos · 2017-10-26T09:10:10Z

@bnoordhuis I agree, but that seems like a non-trivial thing to do now: nodejs/build#945 (comment)

seishun · 2017-10-26T09:11:28Z

Then wouldn't it make sense to upgrade to 4.9 and stop using 4.8?

Sure would. I've been trying to help with that, but it's been stuck on Ubuntu 14.04 and CentOS 6 for the last 3 months: nodejs/build#797, nodejs/build#809

targos · 2017-10-26T09:16:43Z

Canary build with -std=gnu++1y: https://ci.nodejs.org/job/node-test-commit/13482/

addaleax · 2017-10-26T09:16:56Z

Just to make sure I’m understanding this correctly: The purpose of this patch is to fix a compile error in V8 that happens only with clang and for which bumping to -std=c++14 is the fix that the V8 team would prefer?

targos · 2017-10-26T09:20:48Z

@addaleax Yes. the main goal is to fix a compile error. The secondary is to build with a configuration closer to what the V8 team has to prevent errors in the future.

addaleax · 2017-10-26T09:24:28Z

@targos Thanks, that is the information I was looking for.

I kind of agree with @seishun’s sentiment that it’s a bad idea to have some people running C++14 and some people running C++11. I think upgrading to a requirement of C++14 (as gnu++1y?) for everyone is a good idea if we can get away with it, but I’d also consider that a semver-major change.

targos · 2017-10-26T09:32:29Z

I'm OK with semver-major. I can float the commit in canary until we need it.

jasnell · 2017-10-26T14:15:56Z

I'm a bit reluctant on this but as long as it's semver-major then ok

seishun · 2017-10-26T14:37:43Z

I fail to see how this is semver-major.

bnoordhuis · 2017-10-26T14:45:40Z

It is unless we take care to ensure that the build keeps working with older libstdc++ versions (ref.)

More precisely, we need to ensure that it doesn't stop working with missing foo@GLIBCXX_3.4.20 errors in a patch or minor release.

edit: https://gcc.gnu.org/onlinedocs/libstdc++/manual/abi.html for details.

seishun · 2017-10-26T16:09:21Z

I'm pretty sure none of that has anything to do with the -std= option.

bnoordhuis · 2017-10-26T16:52:34Z

You'd be surprised. The language level and the standard library go hand in hand. At least, on Linux they do.

seishun · 2017-10-26T18:14:19Z

I'd love to see some reference for that.

refack · 2017-10-26T18:53:35Z

IMHO semver-ness could be patch since we already landed

node/configure

Line 657 in 75f1087

warn('C++ compiler too old, need g++ 4.9.4 or clang++ 3.4.2 (CXX=%s)' % CXX)

node/BUILDING.md

Line 74 in 75f1087

* GCC 4.9.4 or newer
I'm blocking this till we have C++14 enabled compilers on the build infra

targos · 2018-01-15T11:52:37Z

@refack may I suggest we close this? There is a commit in #17489 to update the config to -std=gnu++1y and that PR is semver-major.

BridgeAR · 2018-01-31T01:56:01Z

Closing as suggested. @refack please reopen if you feel differently.

refack added 2 commits October 25, 2017 16:49

build: use higher version c++

90f055f

[sqwsh] only for C++ files

076af6b

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Oct 25, 2017

refack requested review from bnoordhuis, addaleax, seishun and targos October 25, 2017 22:02

[try] macOS plays hard to get

198ae69

targos reviewed Oct 26, 2017

View reviewed changes

seishun reviewed Oct 26, 2017

View reviewed changes

refack self-assigned this Oct 26, 2017

refack added the blocked PRs that are blocked by other issues or PRs. label Oct 26, 2017

targos mentioned this pull request Nov 16, 2017

building against latest version of v8 requires C++14 #17065

Closed

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

BridgeAR closed this Jan 31, 2018

xzyfer mentioned this pull request Apr 25, 2018

Node 10 compilation failure on Travis CI nodejs/node-gyp#1420

Closed

lovell mentioned this pull request Jul 1, 2018

[Centos 6.9] Error during installation (unrecognized command line option "-std=gnu++1y") lovell/sharp#1283

Closed

refack deleted the higher-c++-ver-by-default branch September 5, 2018 22:29

refack removed their assignment Oct 12, 2018

build: use higher version c++ #16501

build: use higher version c++ #16501

Conversation

refack commented Oct 25, 2017

Checklist

Affected core subsystem(s)

addaleax commented Oct 25, 2017

refack commented Oct 25, 2017

refack commented Oct 25, 2017

addaleax commented Oct 25, 2017

targos commented Oct 26, 2017

targos Oct 26, 2017

Choose a reason for hiding this comment

seishun Oct 26, 2017

Choose a reason for hiding this comment

targos Oct 26, 2017

Choose a reason for hiding this comment

seishun Oct 26, 2017

Choose a reason for hiding this comment

refack Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

refack Oct 26, 2017

Choose a reason for hiding this comment

seishun Oct 26, 2017

Choose a reason for hiding this comment

refack Oct 26, 2017

Choose a reason for hiding this comment

seishun commented Oct 26, 2017

targos commented Oct 26, 2017

seishun commented Oct 26, 2017

targos commented Oct 26, 2017

seishun commented Oct 26, 2017

bnoordhuis commented Oct 26, 2017

targos commented Oct 26, 2017

seishun commented Oct 26, 2017

targos commented Oct 26, 2017

addaleax commented Oct 26, 2017

targos commented Oct 26, 2017 • edited Loading

addaleax commented Oct 26, 2017

targos commented Oct 26, 2017

jasnell commented Oct 26, 2017

seishun commented Oct 26, 2017

bnoordhuis commented Oct 26, 2017 • edited Loading

seishun commented Oct 26, 2017

bnoordhuis commented Oct 26, 2017

seishun commented Oct 26, 2017

refack commented Oct 26, 2017

targos commented Jan 15, 2018

BridgeAR commented Jan 31, 2018

refack Oct 26, 2017 •

edited

Loading

targos commented Oct 26, 2017 •

edited

Loading

bnoordhuis commented Oct 26, 2017 •

edited

Loading