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

src: fix build break for !NODE_USE_V8_PLATFORM #8114

Closed
wants to merge 1 commit into from

Conversation

kunalspathak
Copy link
Member

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

core

Description of change

StartInspector should return bool but there was signature
mismatch if not building for v8 platform i.e.
!NODE_USE_V8_PLATFORM

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Aug 15, 2016
@addaleax addaleax added the inspector Issues and PRs related to the V8 inspector protocol label Aug 15, 2016
@addaleax
Copy link
Member

@cjihrig
Copy link
Contributor

cjihrig commented Aug 15, 2016

LGTM

@kunalspathak
Copy link
Member Author

Thanks @addaleax and @cjihrig . I have updated the commit with lint error fix.

kunalspathak added a commit to kunalspathak/node-chakracore that referenced this pull request Aug 15, 2016
`StartInspector` should return `bool` but there was signature
mismatch if not building for v8 platform i.e.
`!NODE_USE_V8_PLATFORM`.

I have submitted PR nodejs/node#8114 to fix this in
node repo.
@targos
Copy link
Member

targos commented Aug 15, 2016

LGTM

@mscdex mscdex added the lib / src Issues and PRs related to general changes in the lib or src directory. label Aug 15, 2016
@Trott
Copy link
Member

Trott commented Aug 16, 2016

@kunalspathak
Copy link
Member Author

@Trott , Thanks for triggering CI. Any idea if the failures are known or flaky? My changes shouldn't affect these test cases.

@Trott
Copy link
Member

Trott commented Aug 16, 2016

Let's try CI again: https://ci.nodejs.org/job/node-test-pull-request/3695/

`StartInspector` should return `bool` but there was signature
mismatch if not building for v8 platform i.e.
`!NODE_USE_V8_PLATFORM`

PR-URL: nodejs#8114
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@kunalspathak
Copy link
Member Author

@nodejs/collaborators , seems I can't push this change to master. Can someone merge it in master so I can pick it up in nodejs/node-chakracore?

Thanks in Advance!

kunalspathak added a commit to kunalspathak/node-chakracore that referenced this pull request Aug 17, 2016
`StartInspector` should return `bool` but there was signature
mismatch if not building for v8 platform i.e.
`!NODE_USE_V8_PLATFORM`.

I have submitted PR nodejs/node#8114 to fix this in
node repo.

PR-URL: nodejs#108
Reviewed-By: Sandeep Agarwal <agarwal.sandeep@microsoft.com>
@joshgav
Copy link
Contributor

joshgav commented Aug 17, 2016

landed in 6c7e3d1

Thanks Kunal!

@joshgav joshgav closed this Aug 17, 2016
@Trott
Copy link
Member

Trott commented Aug 17, 2016

Relanded in 3177b99 to fix metadata

Trott pushed a commit that referenced this pull request Aug 17, 2016
`StartInspector` should return `bool` but there was signature
mismatch if not building for v8 platform i.e.
`!NODE_USE_V8_PLATFORM`

PR-URL: #8114
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@Trott
Copy link
Member

Trott commented Aug 17, 2016

Should this land in LTS?

@addaleax
Copy link
Member

I was under the impression that the v8 inspector generally wouldn’t be backported to v4.x… right? (adding the dont-land label but anybody should feel free to correct me!)

evanlucas pushed a commit that referenced this pull request Aug 20, 2016
`StartInspector` should return `bool` but there was signature
mismatch if not building for v8 platform i.e.
`!NODE_USE_V8_PLATFORM`

PR-URL: #8114
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants