Skip to content

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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@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 <[email protected]>
@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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
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