FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

deps: backport c0f1ff2 from upstream V8 by targos · Pull Request #13517 · nodejs/node · GitHub

/ node Public

deps: backport c0f1ff2 from upstream V8 - #13517

Closed
targos wants to merge 1 commit into
nodejs:masterfrom
targos:fix-gcc-7-v8-59
ConversationCommits (1)ChecksFiles changed
Closed

deps: backport c0f1ff2 from upstream V8#13517
targos wants to merge 1 commit into
nodejs:masterfrom
targos:fix-gcc-7-v8-59

Conversation

targos commented Jun 7, 2017

Copy link
Copy Markdown
Member

Porting #12392 to V8 5.9

Ref: #12392
Fixes: #10388

/cc @nodejs/v8 @bnoordhuis @octoploid

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

V8

nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Jun 7, 2017
targos mentioned this pull request Jun 7, 2017
Closed

bnoordhuis commented Jun 7, 2017

Copy link
Copy Markdown
Member

If it's still an issue, it should be fixed upstream first.

octoploid commented Jun 7, 2017

Copy link
Copy Markdown

Well, the standard upstream response is: »Do we care about gcc?«

gibfahn commented Jun 7, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Well, the standard upstream response is: »Do we care about gcc?«

Isn't V8 built with gcc by default on some platforms?

cc/ @nodejs/v8

bnoordhuis commented Jun 7, 2017

Copy link
Copy Markdown
Member

Yes, there is at least one gcc buildbot, although it probably uses gcc 6, not 7.

mscdex added the build Issues and PRs related to build files or the CI. label Jun 7, 2017
gibfahn mentioned this pull request Jun 9, 2017
Closed

octoploid commented Jun 10, 2017

Copy link
Copy Markdown

Upstream bug: https://bugs.chromium.org/p/chromium/issues/detail?id=691681

targos commented Jun 12, 2017

Copy link
Copy Markdown
Member Author

Trying to upstream the change: https://chromium-review.googlesource.com/c/530227/

/cc @fhinkel

jasnell previously approved these changes Jun 13, 2017
View reviewed changes

targos commented Jun 19, 2017

Copy link
Copy Markdown
Member Author

I'm still waiting for a review upstream.

bnoordhuis commented Jun 19, 2017

Copy link
Copy Markdown
Member

Maybe pick another reviewer?

targos commented Jun 20, 2017

Copy link
Copy Markdown
Member Author

@octoploid @bnoordhuis Could you please take a look at my last patch: https://chromium-review.googlesource.com/c/530227/3
I'd like to make sure I got this hash-table-inl.h thing right and that I did not reintroduce a cycle.

octoploid commented Jun 20, 2017 •
edited
Loading

Copy link
Copy Markdown

Please post an updated patch for Node.js.

bnoordhuis commented Jun 20, 2017

Copy link
Copy Markdown
Member

@octoploid It's going upstream first before it goes into node.

@targos Left a comment on the CL.

octoploid commented Jun 20, 2017

Copy link
Copy Markdown

@bnoordhuis: Sure, it is just that I don't have v8 (or chromium) installed on my test machines.
So it would be much easier for me to test the patch for node.js.

bnoordhuis commented Jun 20, 2017

Copy link
Copy Markdown
Member

@octoploid Just git apply --directory=deps/v8 it.

targos commented Jun 20, 2017

Copy link
Copy Markdown
Member Author

The change landed on V8 master: v8/v8@c0f1ff2

I will update this PR with a backport

targos force-pushed the fix-gcc-7-v8-59 branch from d780f80 to ee63c30 Compare June 20, 2017 15:09
targos changed the title v8: fix gcc 7 build errors deps: backport c0f1ff2 from upstream V8 Jun 20, 2017
deps: backport c0f1ff2 from upstream V8
522c29e 
Original commit message:

    Fix GCC 7 build errors

    BUG=chromium:691681
    R=franzih@chromium.org

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

Fixes: nodejs#10388
targos force-pushed the fix-gcc-7-v8-59 branch from ee63c30 to 522c29e Compare June 20, 2017 15:16

targos commented Jun 20, 2017

Copy link
Copy Markdown
Member Author

Updated to a backport. PTAL.

octoploid commented Jun 20, 2017

Copy link
Copy Markdown

Works fine. Thanks.

targos commented Jun 22, 2017

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/8795/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/753/

targos added a commit to targos/node that referenced this pull request Jun 23, 2017
deps: backport c0f1ff2 from upstream V8
0ba74db 
Original commit message:

    Fix GCC 7 build errors

    BUG=chromium:691681
    R=franzih@chromium.org

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

PR-URL: nodejs#13517
Fixes: nodejs#10388
Refs: nodejs#12392
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

targos commented Jun 23, 2017

Copy link
Copy Markdown
Member Author

Landed in 0ba74db

targos closed this Jun 23, 2017
targos deleted the fix-gcc-7-v8-59 branch June 23, 2017 08:15
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Aug 1, 2017
deps: backport c0f1ff2 from upstream V8
e649b00 
Original commit message:

    Fix GCC 7 build errors

    BUG=chromium:691681
    R=franzih@chromium.org

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

PR-URL: nodejs#13517
Fixes: nodejs#10388
Refs: nodejs#12392
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Aug 1, 2017
deps: backport c0f1ff2 from upstream V8
5c285c6 
Original commit message:

    Fix GCC 7 build errors

    BUG=chromium:691681
    R=franzih@chromium.org

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

Refs: nodejs#13517
Fixes: nodejs#10388
Refs: nodejs#12392

PR-URL: nodejs#14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Aug 1, 2017
deps: backport c0f1ff2 from upstream V8
5651a6f 
Original commit message:

    Fix GCC 7 build errors

    BUG=chromium:691681
    R=franzih@chromium.org

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

Refs: #13517
Fixes: #10388
Refs: #12392

PR-URL: #14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit that referenced this pull request Aug 2, 2017
deps: backport c0f1ff2 from upstream V8
2dbf95d 
Original commit message:

    Fix GCC 7 build errors

    BUG=chromium:691681
    R=franzih@chromium.org

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

Refs: #13517
Fixes: #10388
Refs: #12392

Backport-PR-URL: #14574
Backport-Reviewed-By: Anna Henningsen <anna@addaleax.net>
Backport-Reviewed-By: Refael Ackermann <refack@gmail.com>

PR-URL: #14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jasnell mentioned this pull request Oct 30, 2017
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

bnoordhuis bnoordhuis approved these changes
jasnell jasnell left review comments

Assignees

No one assigned

Labels

build Issues and PRs related to build files or the CI. 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.

7 participants

Back | FazBrowse Home | New Git URL