Skip to content

Conversation

mykmelez
Copy link
Contributor

@mykmelez mykmelez commented Feb 4, 2017

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

src

Backport of the relevant commit from #11088 to v7.x-staging.

The call signature of v8_platform.StartInspector needs to be the same
whether or not NODE_USE_V8_PLATFORM, otherwise Node will fail to compile
if HAVE_INSPECTOR and !NODE_USE_V8_PLATFORM.

(cherry picked from commit e619725)

The call signature of v8_platform.StartInspector needs to be the same
whether or not NODE_USE_V8_PLATFORM, otherwise Node will fail to compile
if HAVE_INSPECTOR and !NODE_USE_V8_PLATFORM.

(cherry picked from commit e619725)
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. v7.x labels Feb 4, 2017
@mykmelez mykmelez changed the title Fix without v8 platform (backport v7.x) src: fix building --without-v8-platform (backport v7.x) Feb 4, 2017
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@italoacasas
Copy link

ping @mykmelez some test are failing here.

@bnoordhuis
Copy link
Member

@italoacasas If you click through, you'll see that the buildbots are green. The CI is somehow misreporting their status.

@jasnell
Copy link
Member

jasnell commented Feb 6, 2017

The OSX build bot did fail. It's been failing for a couple of days now. When clicking through, it shows the green result from the last successful run.

@mykmelez
Copy link
Contributor Author

mykmelez commented Feb 6, 2017

@italoacasas If you click through, you'll see that the buildbots are green. The CI is somehow misreporting their status.

Right, I see Finished: SUCCESS at the end of the log for the ARM build (https://ci.nodejs.org/job/node-test-commit-arm/7538/).

The OSX build bot did fail. It's been failing for a couple of days now. When clicking through, it shows the green result from the last successful run.

Indeed, the Mac build reports a java.lang.NullPointerException at java.util.HashSet.(HashSet.java:118) both for this PR's build and for previous ones (f.e. the previous build https://ci.nodejs.org/job/node-test-commit-osx/7642/). So that's an issue with the CI service, not this PR.

Erm, I'm unsure of next steps here (first time Node.js bug fixer). I don't see issues on the CI service itself in this repository's issue tracker. Is there another place I should file those?

@italoacasas
Copy link

italoacasas commented Feb 6, 2017

@mykmelez the next step is landing the commit in v7.x-staging, I'm going to do it later today if no one else does it before. Thanks for the backport really appreciated.

Landed 46180fa

italoacasas pushed a commit that referenced this pull request Feb 6, 2017
The call signature of v8_platform.StartInspector needs to be the same
whether or not NODE_USE_V8_PLATFORM, otherwise Node will fail to compile
if HAVE_INSPECTOR and !NODE_USE_V8_PLATFORM.

(cherry picked from commit e619725)

PR-URL: #11157
Reviewed-By: Ben Noordhuis <[email protected]>
@italoacasas italoacasas closed this Feb 6, 2017
@mykmelez mykmelez deleted the fix-without-v8-platform-v7.x branch February 6, 2017 18:00
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 14, 2017
The call signature of v8_platform.StartInspector needs to be the same
whether or not NODE_USE_V8_PLATFORM, otherwise Node will fail to compile
if HAVE_INSPECTOR and !NODE_USE_V8_PLATFORM.

(cherry picked from commit e619725)

PR-URL: nodejs#11157
Reviewed-By: Ben Noordhuis <[email protected]>
@jasnell
Copy link
Member

jasnell commented Mar 7, 2017

should this also land in v6?

@mykmelez
Copy link
Contributor Author

mykmelez commented Mar 7, 2017

should this also land in v6?

No, this fix is for a regression in #9691, which landed in v7, so v6 is unaffected.

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++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants