Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deps: cherry-pick b8331cc030 from upstream V8 #16743

Closed
wants to merge 1 commit into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Nov 4, 2017

Original commit message:

I believe the paths to the V8 include headers are incorrect. The
paths to other sources seem to be relative to the parent directory.

When building Node.js I get the following warning on Windows:
Warning: Missing input files:
deps\v8\src\..\..\include\v8-inspector-protocol.h
deps\v8\src\..\..\include\v8-inspector.h

This commit updates the two include paths.

Bug:
Change-Id: I51a057abba61e294e7811ba69db03e283b0bdc3f
Reviewed-on: https://chromium-review.googlesource.com/743981
Reviewed-by: Aleksey Kozyatinskiy <[email protected]>
Commit-Queue: Aleksey Kozyatinskiy <[email protected]>
Cr-Commit-Position: refs/heads/master@{#49121}

Fixes: #16614
Refs: v8/v8@b8331cc

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

deps

Original commit message:

    I believe the paths to the V8 include headers are incorrect. The
    paths to other sources seem to be relative to the parent directory.

    When building Node.js I get the following warning on Windows:
    Warning: Missing input files:
    deps\v8\src\..\..\include\v8-inspector-protocol.h
    deps\v8\src\..\..\include\v8-inspector.h

    This commit updates the two include paths.

    Bug:
    Change-Id: I51a057abba61e294e7811ba69db03e283b0bdc3f
    Reviewed-on: https://chromium-review.googlesource.com/743981
    Reviewed-by: Aleksey Kozyatinskiy <[email protected]>
    Commit-Queue: Aleksey Kozyatinskiy <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#49121}

Fixes: nodejs#16614
Refs: v8/v8@b8331cc
@nodejs-github-bot nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Nov 4, 2017
@danbev
Copy link
Contributor Author

danbev commented Nov 4, 2017

@gibfahn
Copy link
Member

gibfahn commented Nov 4, 2017

So the base directory is deps/v8/src/, not deps/v8/src/inspector/? I can see how that would be confusing.

@danbev
Copy link
Contributor Author

danbev commented Nov 4, 2017

So the base directory is deps/v8/src/, not deps/v8/src/inspector/? I can see how that would be confusing.

Yeah, was not obvious from just looking at the file. It gets included from deps/v8/src/v8.gyp which I think is why.

@danbev
Copy link
Contributor Author

danbev commented Nov 8, 2017

@Trott
Copy link
Member

Trott commented Nov 8, 2017

CI failures on Raspberry Pi were unrelated. I've disabled the problematic host. Here's a Raspberry Pi-only rerun: https://ci.nodejs.org/job/node-test-binary-arm/11624/

danbev added a commit to danbev/node that referenced this pull request Nov 9, 2017
Original commit message:

    I believe the paths to the V8 include headers are incorrect. The
    paths to other sources seem to be relative to the parent directory.

    When building Node.js I get the following warning on Windows:
    Warning: Missing input files:
    deps\v8\src\..\..\include\v8-inspector-protocol.h
    deps\v8\src\..\..\include\v8-inspector.h

    This commit updates the two include paths.

    Bug:
    Change-Id: I51a057abba61e294e7811ba69db03e283b0bdc3f
    Reviewed-on: https://chromium-review.googlesource.com/743981
    Reviewed-by: Aleksey Kozyatinskiy <[email protected]>
    Commit-Queue: Aleksey Kozyatinskiy <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#49121}

PR-URL: nodejs#16743
Fixes: nodejs#16614
Refs: v8/v8@b8331cc
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@danbev
Copy link
Contributor Author

danbev commented Nov 9, 2017

Landed in 4e769a8

@danbev danbev closed this Nov 9, 2017
@danbev danbev deleted the v8-fix-include-paths branch November 9, 2017 07:43
evanlucas pushed a commit that referenced this pull request Nov 13, 2017
Original commit message:

    I believe the paths to the V8 include headers are incorrect. The
    paths to other sources seem to be relative to the parent directory.

    When building Node.js I get the following warning on Windows:
    Warning: Missing input files:
    deps\v8\src\..\..\include\v8-inspector-protocol.h
    deps\v8\src\..\..\include\v8-inspector.h

    This commit updates the two include paths.

    Bug:
    Change-Id: I51a057abba61e294e7811ba69db03e283b0bdc3f
    Reviewed-on: https://chromium-review.googlesource.com/743981
    Reviewed-by: Aleksey Kozyatinskiy <[email protected]>
    Commit-Queue: Aleksey Kozyatinskiy <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#49121}

PR-URL: #16743
Fixes: #16614
Refs: v8/v8@b8331cc
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@evanlucas evanlucas mentioned this pull request Nov 13, 2017
@MylesBorins
Copy link
Contributor

MylesBorins commented Nov 17, 2017

Should this be backported to v6.x-staging or v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

edit: moving backport request to the version that landed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants