gyp: Inherit CC/CXX for CC/CXX.host by jbergstroem · Pull Request #6173 · nodejs/node

jbergstroem · 2016-04-13T02:41:24Z

Checklist

tests and code linting passes
the commit message follows commit guidelines

Affected core subsystem(s)

tools/gyp

Description of change

Gyp defaults to gcc/g++ if CC.host/CXX.host is unset. This is not suitable for environments that only uses the clang toolchain.

Since we already assume that the user will provide clang/clang++ through CC/CXX, lean against it (then drop to gcc/g++).

I don't think this will be accepted upstream but I guess I should give it a try.

Refs #6088 and fixes#6152.
/cc @srl295, @bnoordhuis.

jbergstroem · 2016-04-13T02:50:15Z

CI without custom configure flags:
❎ https://ci.nodejs.org/job/node-test-commit/2900/ (unrelated fail on arm)

CI on FreeBSD with custom configure flags:
✅ https://ci.nodejs.org/job/node-test-commit-freebsd/2043/

bnoordhuis · 2016-04-13T07:38:26Z

Can you try upstreaming it first? I don't want us to carry out-of-tree patches unless absolutely necessary.

jbergstroem · 2016-04-13T07:43:45Z

@bnoordhuis: That was the plan -- just want to have a better look through their bug trackers first.

jbergstroem · 2016-04-13T07:56:32Z

Upstream: https://bugs.chromium.org/p/gyp/issues/detail?id=515

bnoordhuis · 2016-04-13T08:06:02Z

Can I suggest you upload the patch with git-cl upload? You'll get an answer quicker that way. CC gyp-developer@googlegroups.com if that isn't filled out automatically by https://codereview.chromium.org/.

jbergstroem · 2016-04-13T08:36:20Z

Done: https://codereview.chromium.org/1887593002

bnoordhuis · 2016-04-13T09:33:27Z

Maybe assign a reviewer as well. git shortlog -ns for that file suggests that thakis@ is a good choice. git-cl owners can be helpful, too.

joaocgreis · 2016-04-13T14:02:19Z

LGTM here if it's rejected or gets stalled upstream

jbergstroem · 2016-04-19T01:41:59Z

@bnoordhuis silent upstream -- I'd be keen on getting this in 6.x. There's still time but I'd just like a LGTM from you so I can land it quickly if needed.

bnoordhuis · 2016-04-19T08:45:25Z

LGTM then.

jbergstroem · 2016-04-19T13:57:22Z

Thanks. I will hold off for a week (or whatever's closing in on 6.x) until I merge as-is.

jasnell · 2016-04-19T15:12:41Z

@jbergstroem ... the current plan is to cut the v6 release on Tuesday the 26th.

jbergstroem · 2016-04-20T06:03:44Z

@bnoordhuis do we carry this in node-gyp too?

bnoordhuis · 2016-04-20T09:09:34Z

I guess we should for consistency: nodejs/node-gyp#908

jbergstroem · 2016-04-24T00:48:40Z

Merging this. Was hoping for upstream but hopefully they'll catch up in time.

Gyp defaults to gcc/g++ if CC.host/CXX.host is unset. This is not suitable for environments that only uses the clang toolchain. Since we already assume that the user will provide clang/clang++ through CC/CXX, lean against it (then drop to gcc/g++). Also apply the same logic for link/ar for consistency although it doesn't affect us. PR-URL: nodejs#6173Fixes: nodejs#6152 Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

jbergstroem · 2016-04-24T01:02:22Z

Final CI run: https://ci.nodejs.org/job/node-test-pull-request/2375/

jbergstroem · 2016-04-24T02:59:51Z

PPC slaves unrelated.

Gyp defaults to gcc/g++ if CC.host/CXX.host is unset. This is not suitable for environments that only uses the clang toolchain. Since we already assume that the user will provide clang/clang++ through CC/CXX, lean against it (then drop to gcc/g++). Also apply the same logic for link/ar for consistency although it doesn't affect us. PR-URL: nodejs#6173Fixes: nodejs#6152 Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Gyp defaults to gcc/g++ if CC.host/CXX.host is unset. This is not suitable for environments that only uses the clang toolchain. Since we already assume that the user will provide clang/clang++ through CC/CXX, lean against it (then drop to gcc/g++). Also apply the same logic for link/ar for consistency although it doesn't affect us. PR-URL: #6173Fixes: #6152 Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins · 2016-06-01T23:35:00Z

@jbergstroem is this something we would want for lts?

rvagg · 2016-06-02T11:39:06Z

I think -1 for LTS, who knows what crazy build environments ppl have out there that this might impact

MylesBorins · 2016-06-02T23:22:18Z

thanks @rvagg labeling as don't land

Gyp defaults to gcc/g++ if CC.host/CXX.host is unset. This is not suitable for environments that only uses the clang toolchain. Since we already assume that the user will provide clang/clang++ through CC/CXX, lean against it (then drop to gcc/g++). Also apply the same logic for link/ar for consistency although it doesn't affect us. PR-URL: nodejs#6173Fixes: nodejs#6152 Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Gyp defaults to gcc/g++ if CC.host/CXX.host is unset. This is not suitable for environments that only uses the clang toolchain. Since we already assume that the user will provide clang/clang++ through CC/CXX, lean against it (then drop to gcc/g++). Also apply the same logic for link/ar for consistency although it doesn't affect us. PR-URL: #6173Fixes: #6152 Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Gyp defaults to gcc/g++ if CC.host/CXX.host is unset. This is not suitable for environments that only uses the clang toolchain. Since we already assume that the user will provide clang/clang++ through CC/CXX, lean against it (then drop to gcc/g++). PR-URL: nodejs#908 Refs: nodejs/node#6173 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Johan Bergström <bugs@bergstroem.nu>

Gyp defaults to gcc/g++ if CC.host/CXX.host is unset. This is not suitable for environments that only uses the clang toolchain. Since we already assume that the user will provide clang/clang++ through CC/CXX, lean against it (then drop to gcc/g++). Also apply the same logic for link/ar for consistency although it doesn't affect us. PR-URL: #6173Fixes: #6152 Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Gyp defaults to gcc/g++ if CC.host/CXX.host is unset. This is not suitable for environments that only uses the clang toolchain. Since we already assume that the user will provide clang/clang++ through CC/CXX, lean against it (then drop to gcc/g++). Also apply the same logic for link/ar for consistency although it doesn't affect us. PR-URL: nodejs#6173Fixes: nodejs#6152 Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Gyp defaults to gcc/g++ if CC.host/CXX.host is unset. This is not suitable for environments that only uses the clang toolchain. Since we already assume that the user will provide clang/clang++ through CC/CXX, lean against it (then drop to gcc/g++). PR-URL: nodejs#908 Refs: nodejs/node#6173 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Johan Bergström <bugs@bergstroem.nu>

Gyp defaults to gcc/g++ if CC.host/CXX.host is unset. This is not suitable for environments that only uses the clang toolchain. Since we already assume that the user will provide clang/clang++ through CC/CXX, lean against it (then drop to gcc/g++). Also apply the same logic for link/ar for consistency although it doesn't affect us. PR-URL: nodejs#6173Fixes: nodejs#6152 Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Gyp defaults to gcc/g++ if CC.host/CXX.host is unset. This is not suitable for environments that only uses the clang toolchain. Since we already assume that the user will provide clang/clang++ through CC/CXX, lean against it (then drop to gcc/g++). Also apply the same logic for link/ar for consistency although it doesn't affect us. PR-URL: nodejs/node#6173Fixes: nodejs/node#6152 Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Gyp defaults to gcc/g++ if CC.host/CXX.host is unset. This is not suitable for environments that only uses the clang toolchain. Since we already assume that the user will provide clang/clang++ through CC/CXX, lean against it (then drop to gcc/g++). Also apply the same logic for link/ar for consistency although it doesn't affect us. PR-URL: #6173Fixes: #6152 Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

mscdex added the build Issues and PRs related to build files or the CI. label Apr 13, 2016

srl295 mentioned this pull request Apr 13, 2016
FreeBSD with small-icu fails: g++ not found #6152
Closed

bnoordhuis mentioned this pull request Apr 20, 2016
gyp: inherit CC/CXX for CC/CXX.host nodejs/node-gyp#908
Merged

jbergstroem force-pushed the fix/gyp-cxx.host branch from 3961ffd to 065e155Compare April 24, 2016 00:58

jbergstroem force-pushed the fix/gyp-cxx.host branch from 065e155 to f1294f5Compare April 24, 2016 03:00

jbergstroem merged commit f1294f5 into nodejs:masterApr 24, 2016

MylesBorins mentioned this pull request Jun 2, 2016
Audit commits not found on v4.x-staging #7116
Closed

MylesBorins added the dont-land-on-v4.x label Jun 2, 2016

evanlucas mentioned this pull request May 1, 2017
v7.10.0 proposal #12775
Merged

MylesBorins mentioned this pull request Sep 10, 2017
v8.5.0 proposal #15308
Merged

Uh oh!

Uh oh!

Conversation

jbergstroem commented Apr 13, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

jbergstroem commented Apr 13, 2016

Uh oh!

bnoordhuis commented Apr 13, 2016

Uh oh!

jbergstroem commented Apr 13, 2016

Uh oh!

jbergstroem commented Apr 13, 2016

Uh oh!

bnoordhuis commented Apr 13, 2016

Uh oh!

jbergstroem commented Apr 13, 2016

Uh oh!

bnoordhuis commented Apr 13, 2016

Uh oh!

joaocgreis commented Apr 13, 2016

Uh oh!

jbergstroem commented Apr 19, 2016

Uh oh!

bnoordhuis commented Apr 19, 2016

Uh oh!

jbergstroem commented Apr 19, 2016

Uh oh!

jasnell commented Apr 19, 2016

Uh oh!

jbergstroem commented Apr 20, 2016

Uh oh!

bnoordhuis commented Apr 20, 2016

Uh oh!

jbergstroem commented Apr 24, 2016

Uh oh!

jbergstroem commented Apr 24, 2016

Uh oh!

jbergstroem commented Apr 24, 2016

Uh oh!

MylesBorins commented Jun 1, 2016

Uh oh!

rvagg commented Jun 2, 2016

Uh oh!

MylesBorins commented Jun 2, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants