GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — v8: fix build errors with g++ 7 by bnoordhuis · Pull Request #12392 · nodejs/node

Conversation

@bnoordhuis

@bnoordhuis bnoordhuis commented Apr 13, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

This is a local patch because upstream fixed it differently by moving
large chunks of code out of objects.h. We cannot easily back-port
those changes due to their size and invasiveness.

Node CI: https://ci.nodejs.org/job/node-test-pull-request/7376/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/645/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Fishrock123 and BlackYoup reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@nodejs-github-bot nodejs-github-bot added v7.x v8 engine Issues and PRs related to the V8 dependency. labels Apr 13, 2017
@bnoordhuis bnoordhuis mentioned this pull request Apr 13, 2017
deps/v8/src/objects.h:3263:46: error: invalid use of incomplete type ‘class v8::internal::Heap #10388
Closed
@benjamingr

benjamingr commented Apr 13, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

test failure is weird and does not happen on my box with this commit (although, with newer clang).

Found this catchorg/Catch2#334

Related issue #12388 , I'm fine with landing as is.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

benjamingr
benjamingr approved these changes Apr 13, 2017
View reviewed changes
addaleax
addaleax approved these changes Apr 13, 2017
View reviewed changes
@bnoordhuis

bnoordhuis commented Apr 13, 2017

Copy link
Copy Markdown
Member Author

The test failure:

In file included from ../deps/v8/src/base/functional.cc:11:
In file included from ../deps/v8/src/base/functional.h:11:
/usr/bin/../lib/gcc/x86_64-linux-gnu/4.9/../../../../include/c++/4.9/cstddef:51:11: error: no member named 'max_align_t' in the global namespace
  using ::max_align_t;
        ~~^
1 error generated.

It's not related to this pull request, it happens intermittently on the clang 3.4.1 buildbot for reasons beyond my ken.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mscdex mscdex added the build Issues and PRs related to Node.js builds or CI infrastructure. label Apr 13, 2017
jasnell
jasnell approved these changes Apr 13, 2017
View reviewed changes
danbev
danbev approved these changes Apr 14, 2017
View reviewed changes
@danbev

danbev commented Apr 14, 2017

Copy link
Copy Markdown
Contributor

Sorry if this is the wrong place and might be a stupid question...
When reviewing this I wanted to run it locally with g++7, so I installed it (on Mac):

$ g++ -v
gcc version 7.0.1 20170409 (experimental) (GCC)
$ gcc -v
gcc version 7.0.1 20170409 (experimental) (GCC)

Running make I get the following error:

make -C out BUILDTYPE=Release V=1
  c++ '-D_DARWIN_USE_64_BIT_INODE=1' -I../deps/gtest -I../deps/gtest/include  -Os -gdwarf-2 -mmacosx-version-min=10.7 -arch x86_64 -Wall -Wendif-labels -W -Wno-unused-parameter -std=gnu++0x -stdlib=libc++ -fno-rtti -fno-exceptions -fno-threadsafe-statics -fno-strict-aliasing -MMD -MF /Users/danielbevenius/work/nodejs/node/out/Release/.deps//Users/danielbevenius/work/nodejs/node/out/Release/obj.target/gtest/deps/gtest/src/gtest-death-test.o.d.raw   -c -o /Users/danielbevenius/work/nodejs/node/out/Release/obj.target/gtest/deps/gtest/src/gtest-death-test.o ../deps/gtest/src/gtest-death-test.cc
c++: error: unrecognized command line option ‘-stdlib=libc++’
make[1]: *** [/Users/danielbevenius/work/nodejs/node/out/Release/obj.target/gtest/deps/gtest/src/gtest-death-test.o] Error 1
make: *** [node] Error 2

I'm able to force this compile and run the test by setting clang to 0 in common.gypi:

      ['OS=="mac"', {
        'clang%': 0,
      }, {
        'clang%': 0,
      }],

I get the feeling that I'm missing something. If you have any advice I'd appreciated it. I'd like to be able to switch between gcc/clang for future reviews/issues/troubleshooting. Thanks

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bnoordhuis

bnoordhuis commented Apr 14, 2017

Copy link
Copy Markdown
Member Author

I believe -stdlib=... is an apple-clang extension. We assume clang on OS X and set CLANG_CXX_LIBRARY accordingly. You should be able to override it with export CXX=g++ CXX.host=g++ && ./configure -- -Dclang=0.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@danbev

danbev commented Apr 14, 2017

Copy link
Copy Markdown
Contributor

You should be able to override it with export CXX=g++ CXX.host=g++ && ./configure -- -Dclang=0.

Worked like a charm, thanks!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@thefourtheye thefourtheye mentioned this pull request Apr 18, 2017
deps: cherry-pick 79aee39 from upstream v8 #12412
Merged
@bnoordhuis
v8: fix build errors with g++ 7
2bbee49
This is a local patch because upstream fixed it differently by moving
large chunks of code out of objects.h.  We cannot easily back-port
those changes due to their size and invasiveness.

Fixes: nodejs#10388
PR-URL: nodejs#12392
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@bnoordhuis
bnoordhuis force-pushed the fix-v8-gcc-7 branch from 26dfbd6 to 2bbee49 Compare April 19, 2017 09:03
@bnoordhuis
bnoordhuis merged commit 2bbee49 into nodejs:v7.x-staging Apr 19, 2017
@bnoordhuis
bnoordhuis deleted the fix-v8-gcc-7 branch April 19, 2017 09:03
@gibfahn gibfahn mentioned this pull request Apr 24, 2017
ubuntu1204-clang341 has been disabled in CI nodejs/build#685
Closed
@kasicka

kasicka commented Apr 26, 2017

Copy link
Copy Markdown

Can you merge this into v8 branch too?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bnoordhuis

bnoordhuis commented Apr 26, 2017

Copy link
Copy Markdown
Member Author

Ambiguity alert: do you mean the node.js v8.0 branch or the upstream V8 project?

If it's the former, I don't remember build issues with g++ 7 but if you have them, file a pull request against the master branch.

If it's V8, you'll have to send the changes to them - although here too I don't remember issues last time I tried.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@kasicka

kasicka commented Apr 26, 2017

Copy link
Copy Markdown

I meant node.js v8 branch, latest nightly fails to build on rawhide.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@kasicka kasicka mentioned this pull request Apr 26, 2017
v8: fix gcc 7 build errors #12676
Closed
4 tasks
addaleax pushed a commit that referenced this pull request Apr 29, 2017
Zuzana Svetlikova @addaleax
v8: fix gcc 7 build errors
b07e1a8
Porting #12392 to master

Ref: #12392
Fixes: #10388
PR-URL: #12676
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@evanlucas evanlucas mentioned this pull request May 1, 2017
v7.10.0 proposal #12775
Merged
evanlucas pushed a commit that referenced this pull request May 1, 2017
@bnoordhuis @evanlucas
v8: fix build errors with g++ 7
25b851b
This is a local patch because upstream fixed it differently by moving
large chunks of code out of objects.h.  We cannot easily back-port
those changes due to their size and invasiveness.

Fixes: #10388
PR-URL: #12392
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
evanlucas added a commit that referenced this pull request May 2, 2017
@evanlucas
2017-05-02, Version 7.10.0 (Current)
e0dd758
Notable changes:

* **crypto**:
  - add randomFill and randomFillSync (Evan Lucas)
    #10209
* **meta**: Added new collaborators
  - add lucamaraschi to collaborators (Luca Maraschi)
    #12538
  - add DavidCai1993 to collaborators (David Cai)
    #12435
  - add jkrems to collaborators (Jan Krems)
    #12427
  - add AnnaMag to collaborators (AnnaMag)
    #12414
* **process**:
  - fix crash when Promise rejection is a Symbol (Cameron Little)
    #11640
* **url**:
  - make WHATWG URL more spec compliant (Timothy Gu)
    #12507
* **v8**:
  - fix stack overflow in recursive method (Ben Noordhuis)
    #12460
  - fix build errors with g++ 7 (Ben Noordhuis)
    #12392

PR-URL: #12775
@gibfahn

gibfahn commented Jun 9, 2017

Copy link
Copy Markdown
Member

v6.x backport here: #13574

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

addaleax pushed a commit that referenced this pull request Jun 17, 2017
@targos @addaleax
v8: fix gcc 7 build errors
04da748
Porting #12392 to V8 5.9

Ref: #12392
Fixes: #10388
PR-URL: #13515
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
gibfahn pushed a commit to gibfahn/node that referenced this pull request Jun 17, 2017
Zuzana Svetlikova @gibfahn
v8: fix build errors with g++ 7
11c7e01
This is a local patch because upstream fixed it differently by moving
large chunks of code out of objects.h.  We cannot easily back-port
those changes due to their size and invasiveness.

Fixes: nodejs#10388
PR-URL: nodejs#12392
Backport-PR-URL: nodejs#13574
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
Zuzana Svetlikova @gibfahn
v8: fix build errors with g++ 7
66c1197
This is a local patch because upstream fixed it differently by moving
large chunks of code out of objects.h.  We cannot easily back-port
those changes due to their size and invasiveness.

Fixes: #10388
PR-URL: #12392
Backport-PR-URL: #13574
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Jun 21, 2017
@targos @addaleax
v8: fix gcc 7 build errors
41f5120
Porting #12392 to V8 5.9

Ref: #12392
Fixes: #10388
PR-URL: #13515
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos added a commit to targos/node that referenced this pull request Jun 23, 2017
@targos
deps: backport c0f1ff2 from upstream V8
0ba74db
Original commit message:

    Fix GCC 7 build errors

    BUG=chromium:691681
    [email protected]

    Change-Id: Id7e5698487f16dc217a804f6d3f24da7213c72b9
    Reviewed-on: https://chromium-review.googlesource.com/530227
    Commit-Queue: Toon Verwaest <[email protected]>
    Reviewed-by: Toon Verwaest <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#46045}

PR-URL: nodejs#13517
Fixes: nodejs#10388
Refs: nodejs#12392
Reviewed-By: Ben Noordhuis <[email protected]>
addaleax pushed a commit that referenced this pull request Jun 24, 2017
@targos @addaleax
v8: fix gcc 7 build errors
547ca9a
Porting #12392 to V8 5.9

Ref: #12392
Fixes: #10388
PR-URL: #13515
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
rvagg pushed a commit that referenced this pull request Jun 29, 2017
@targos @rvagg
v8: fix gcc 7 build errors
950b97c
Porting #12392 to V8 5.9

Ref: #12392
Fixes: #10388
PR-URL: #13515
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
Zuzana Svetlikova @MylesBorins
v8: fix build errors with g++ 7
d9a8f80
This is a local patch because upstream fixed it differently by moving
large chunks of code out of objects.h.  We cannot easily back-port
those changes due to their size and invasiveness.

Fixes: #10388
PR-URL: #12392
Backport-PR-URL: #13574
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 11, 2017
@targos @addaleax
v8: fix gcc 7 build errors
dd48f5d
Porting #12392 to V8 5.9

Ref: #12392
Fixes: #10388
PR-URL: #13515
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 18, 2017
@targos @addaleax
v8: fix gcc 7 build errors
9235125
Porting #12392 to V8 5.9

Ref: #12392
Fixes: #10388
PR-URL: #13515
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
v6.11.2 proposal #14356
Merged
targos added a commit to targos/node that referenced this pull request Jul 21, 2017
@targos
v8: fix gcc 7 build errors
b47e758
Porting nodejs#12392 to V8 5.9

Ref: nodejs#12392
Fixes: nodejs#10388
addaleax pushed a commit that referenced this pull request Jul 24, 2017
@targos @addaleax
v8: fix gcc 7 build errors
0be4d17
Porting #12392 to V8 5.9

Ref: #12392
Fixes: #10388
PR-URL: #13515
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@Qantas94Heavy Qantas94Heavy mentioned this pull request Oct 11, 2017
v4.8.5 proposal #15507
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@addaleax addaleax addaleax approved these changes
@benjamingr benjamingr benjamingr approved these changes
+1 more reviewer
@danbev danbev danbev approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

build Issues and PRs related to Node.js builds or CI infrastructure. v8 engine Issues and PRs related to the V8 dependency.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

10 participants

@bnoordhuis @benjamingr @danbev @kasicka @MylesBorins @gibfahn @jasnell @addaleax @mscdex @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.