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: switch to upstream v8_inspector #7302

Merged
merged 1 commit into from
Jun 17, 2016

Conversation

ofrobots
Copy link
Contributor

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

deps: v8_inspector

Description of change

This change picks up v8_inspector directly from the upstream chromium
repository 1; dropping the intermediate repository. The upstream
code has been refactored substantially to make it easy to share code
without adaptation with Node.js.

The deps/v8_inspector directory is now simply a gathering of
dependencies:

  • platform/v8_inspector: vendored from 2.
  • platform/inspector_protocol: vendored from 3.
  • deps/jinja2: vendored from 4.
  • deps/markupsafe: vendored from 5.

R=@bnoordhuis
/cc @pavelfeldman @eugeneo

@ofrobots ofrobots added the inspector Issues and PRs related to the V8 inspector protocol label Jun 15, 2016
@rvagg
Copy link
Member

rvagg commented Jun 15, 2016

sgtm

https://ci.nodejs.org/job/node-test-commit/3749/

Is @nodejs/v8 going to be good to call for review on changes here or should we consider v8_inspector a separate concern and perhaps start calling on @nodejs/diagnostics?.

@indutny
Copy link
Member

indutny commented Jun 15, 2016

Oh... this tightens our dependency on Python, though this PR SGTM.

LGTM, if CI is green and @bnoordhuis is happy.

@rvagg
Copy link
Member

rvagg commented Jun 15, 2016

Build failures on older GCC / Linux will need looking at https://ci.nodejs.org/job/node-test-commit-linux/3808/

@ofrobots
Copy link
Contributor Author

Those are due to Python 2.6 on the CentOS machines; this CL fixes things upstream: https://codereview.chromium.org/2066423002/. Once the CL lands, I will update this PR.

@ofrobots ofrobots force-pushed the inspector_upstream branch from 170b375 to b47e965 Compare June 16, 2016 17:50
@ofrobots
Copy link
Contributor Author

@ofrobots
Copy link
Contributor Author

CI is green. @bnoordhuis: ready for your review.

@bnoordhuis
Copy link
Member

LGTM at a glance.

This change picks up v8_inspector directly from the upstream chromium
repository [1]; dropping the intermediate repository. The upstream
code has been refactored substantially to make it easy to share code
without adaptation with Node.js.

The deps/v8_inspector directory is now simply a gathering of
dependencies:

* platform/v8_inspector: vendored from [2].
* platform/inspector_protocol: vendored from [3].
* deps/jinja2: vendored from [4].
* deps/markupsafe: vendored from [5].

[1]: https://chromium.googlesource.com/chromium/src/third_party/WebKit/Source/platform
[2]: https://chromium.googlesource.com/chromium/src/third_party/WebKit/Source/platform/v8_inspector
[3]: https://chromium.googlesource.com/chromium/src/third_party/WebKit/Source/platform/inspector_protocol
[4]: https://github.com/mitsuhiko/jinja2
[5]: https://github.com/mitsuhiko/markupsafe

PR-URL: nodejs#7302
Reviewed-By: indutny - Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
@ofrobots ofrobots force-pushed the inspector_upstream branch from b47e965 to 44b9154 Compare June 17, 2016 17:40
@ofrobots ofrobots merged commit 44b9154 into nodejs:master Jun 17, 2016
@ofrobots ofrobots deleted the inspector_upstream branch June 17, 2016 17:42
Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016
This change picks up v8_inspector directly from the upstream chromium
repository [1]; dropping the intermediate repository. The upstream
code has been refactored substantially to make it easy to share code
without adaptation with Node.js.

The deps/v8_inspector directory is now simply a gathering of
dependencies:

* platform/v8_inspector: vendored from [2].
* platform/inspector_protocol: vendored from [3].
* deps/jinja2: vendored from [4].
* deps/markupsafe: vendored from [5].

[1]: https://chromium.googlesource.com/chromium/src/third_party/WebKit/Source/platform
[2]: https://chromium.googlesource.com/chromium/src/third_party/WebKit/Source/platform/v8_inspector
[3]: https://chromium.googlesource.com/chromium/src/third_party/WebKit/Source/platform/inspector_protocol
[4]: https://github.com/mitsuhiko/jinja2
[5]: https://github.com/mitsuhiko/markupsafe

PR-URL: #7302
Reviewed-By: indutny - Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
@Fishrock123 Fishrock123 mentioned this pull request Jul 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants