deps: update v8_inspector by ofrobots · Pull Request #7796 · nodejs/node

ofrobots · 2016-07-19T19:34:41Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

deps:v8_inspector

Description of change

To add a LICENSE file along with the v8_inspector code, we need to
pick up v8_inspector from an intermediate repository:
https://github.com/pavelfeldman/v8_inspector. This repo still tracks
upstream code in Blink.

This roll also picks up the latest v8_inspector from upstream fixing
a few issues.

Pickup commit id bc60957 from pavelfeldman/v8_inspector
Update node.gyp to adapt to the new file paths
Update the DevTools hash for the devtools frontend.

Fixes: #7123
Fixes: #7736
Fixes: #7734

EDIT: CI: ~~https://ci.nodejs.org/job/node-test-pull-request/3337/~~
EDIT: CI: https://ci.nodejs.org/job/node-test-pull-request/3361/

gibfahn · 2016-07-19T21:51:42Z

@rvagg As this includes a new License file deps/v8_inspector/third_party/v8_inspector/LICENSE, presumably license-builder.sh will need to be updated and then run to update the Node license?

Not sure if this is normally done during or after landing a PR...

Ref; #7123 (comment)

MylesBorins · 2016-07-20T00:36:44Z

I think it could easily be included in this PR as a separate commit

bnoordhuis · 2016-07-20T08:27:02Z

LGTM but can you break up the long line in node.gyp? GYP supports C-style string concatenation so you can just continue on the next line.

Also, is there a reason the jinja test suite is included? Doesn't seem very necessary.

richardlau · 2016-07-21T00:31:21Z

tools/license-builder.sh

+# v8_inspector
+addlicense "v8_inspector" "deps/v8_inspector/third_party/v8_inspector" \
+           "$(cat ${rootdir}/deps/v8_inspector/third_party/v8_inspector/LICENSE)"
+# Build tooling for v8_inspector


@rvagg Is the ordering here important? Should the inspector be above the build/test tools?

ofrobots · 2016-07-21T00:33:39Z

I have updated license-builder.sh and also included a commit to update the LICENSE file.

@bnoordhuis fixed long line in node.gyp.

Also, is there a reason the jinja test suite is included? Doesn't seem very necessary.

No strong reason other than minimizing the number of local changes to a vendored dependency in order to minimize the work if the dependency needs to be updated in the future. Is there a particular reason why the jinja test files should be locally deleted?

EDIT: New CI: https://ci.nodejs.org/job/node-test-pull-request/3361/

ofrobots · 2016-07-21T21:40:39Z

The failures in the CI look like infrastructure / flakes. Will land tomorrow.

bnoordhuis · 2016-07-22T08:01:57Z

Is there a particular reason why the jinja test files should be locally deleted?

Do they end up in release tarballs? If yes, they should either be deleted from the tree or the $(TARBALL) rule in the top-level Makefile should delete them before packaging.

gibfahn · 2016-07-22T14:42:28Z

@ofrobots it sounds like an update_inspector.sh script might be useful if you're having to manually rip this out of Chromium source every time.

ofrobots · 2016-07-25T18:50:21Z

Removed tests from jinja2. New CI: https://ci.nodejs.org/job/node-test-pull-request/3401/.

@gibfahn

it sounds like an update_inspector.sh script might be useful if you're having to manually rip this out of Chromium source every time.

The next time I have to do this, I will definitely consider writing a update_inspector.sh script 😄. Hopefully these dependencies don't change very frequently. I am also hoping that v8-inspector will land in V8 at some point in the near future.

Trott · 2016-07-25T20:18:04Z

Will probably want to re-run CI after #7873 lands (hopefully within the hour). That will fix the bad build on four of the Linux hosts.

ofrobots · 2016-07-26T00:16:12Z

Reran twice: https://ci.nodejs.org/job/node-test-pull-request/3408/, https://ci.nodejs.org/job/node-test-pull-request/3410/. windows-fanned builds seems to timeout after 30 minutes. The rest is looking green.

Trott · 2016-07-26T00:21:21Z

One last time hoping the Windows stuff has passed: https://ci.nodejs.org/job/node-test-pull-request/3412/

ofrobots · 2016-07-26T01:20:33Z

That one failed too. Seems like the windows-fanned builds have infra issues. I will land this PR tomorrow morning.

ofrobots · 2016-07-26T22:13:09Z

One more build after a rebase: https://ci.nodejs.org/job/node-test-pull-request/3424/.

ofrobots · 2016-07-26T22:48:26Z

All green!!1! However, I noticed that the update licenses commit was added after the LGTM from @bnoordhuis . Can I get some LGTMs before landing?

jasnell · 2016-07-26T22:51:14Z

Lgtm

bnoordhuis · 2016-07-27T07:11:23Z

Still LGTM.

To add a LICENSE file along with the v8_inspector code, we need to pick up v8_inspector from an intermediate repository: https://github.com/pavelfeldman/v8_inspector. This repo still tracks upstream code in Blink. This roll also picks up the latest v8_inspector from upstream fixing a few issues. * Pickup commit id bc60957 from pavelfeldman/v8_inspector * Update node.gyp to adapt to the new file paths * Update the DevTools hash for the devtools frontend. Fixes: nodejs#7123 Fixes: nodejs#7736 Fixes: nodejs#7734 PR-URL: nodejs#7796 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>

jinja.el is not shipped as part of the distribution, and neither is it used in the build. While not strictly necessary, removing it simplifies life from a licensing point of view. PR-URL: nodejs#7796 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>

Start including the license from v8_inspector and its build time dependencies: jinja2 and markupsafe. PR-URL: nodejs#7796 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>

Created using `tools/license-builder.sh`. PR-URL: nodejs#7796 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>

PR-URL: nodejs#7796 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>

ofrobots · 2016-07-27T14:36:57Z

Thanks. Landed as ea725ed...2554549.

cjihrig · 2016-08-10T15:42:17Z

@ofrobots could you backport this to v6.x. It applies cleanly, but causes compile errors.

ofrobots · 2016-08-10T16:31:02Z

@cjihrig I will take a look.

To add a LICENSE file along with the v8_inspector code, we need to pick up v8_inspector from an intermediate repository: https://github.com/pavelfeldman/v8_inspector. This repo still tracks upstream code in Blink. This roll also picks up the latest v8_inspector from upstream fixing a few issues. * Pickup commit id bc60957 from pavelfeldman/v8_inspector * Update node.gyp to adapt to the new file paths * Update the DevTools hash for the devtools frontend. Fixes: #7123 Fixes: #7736 Fixes: #7734 PR-URL: #7796 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>

jinja.el is not shipped as part of the distribution, and neither is it used in the build. While not strictly necessary, removing it simplifies life from a licensing point of view. PR-URL: #7796 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>

Start including the license from v8_inspector and its build time dependencies: jinja2 and markupsafe. PR-URL: #7796 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>

Created using `tools/license-builder.sh`. PR-URL: #7796 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>

PR-URL: #7796 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>

nodejs-github-bot added the inspector Issues and PRs related to the V8 inspector protocol label Jul 19, 2016

ofrobots mentioned this pull request Jul 19, 2016

Do we need to include any new licenses when we ship the new v8 inspector? #7123

Closed

ofrobots force-pushed the roll-inspector branch from 7d128f0 to d2a8e9d Compare July 21, 2016 00:28

richardlau reviewed Jul 21, 2016
View reviewed changes

ofrobots force-pushed the roll-inspector branch from d899e29 to b038600 Compare July 26, 2016 22:11

ofrobots added 5 commits July 27, 2016 07:28

doc: update licenses

7db495a

Created using `tools/license-builder.sh`. PR-URL: nodejs#7796 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>

deps: v8_inspector: remove jinja2 tests

2554549

PR-URL: nodejs#7796 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>

ofrobots force-pushed the roll-inspector branch from b038600 to 2554549 Compare July 27, 2016 14:34

ofrobots merged commit 2554549 into nodejs:master Jul 27, 2016

cjihrig mentioned this pull request Aug 8, 2016

v6.4.0 Release Proposal #8020

Closed

cjihrig pushed a commit that referenced this pull request Aug 11, 2016

doc: update licenses

3fe122f

Created using `tools/license-builder.sh`. PR-URL: #7796 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>

cjihrig pushed a commit that referenced this pull request Aug 11, 2016

deps: v8_inspector: remove jinja2 tests

4d81362

PR-URL: #7796 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>

cjihrig mentioned this pull request Aug 11, 2016

v6.4.0 Proposal #8070

Merged

ofrobots deleted the roll-inspector branch August 17, 2016 17:58

MylesBorins added the dont-land-on-v4.x label Sep 9, 2016

Uh oh!

Conversation

ofrobots commented Jul 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

gibfahn commented Jul 19, 2016

Uh oh!

MylesBorins commented Jul 20, 2016

Uh oh!

bnoordhuis commented Jul 20, 2016

Uh oh!

richardlau Jul 21, 2016

Choose a reason for hiding this comment

Uh oh!

ofrobots commented Jul 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ofrobots commented Jul 21, 2016

Uh oh!

bnoordhuis commented Jul 22, 2016

Uh oh!

gibfahn commented Jul 22, 2016

Uh oh!

ofrobots commented Jul 25, 2016

Uh oh!

Trott commented Jul 25, 2016

Uh oh!

ofrobots commented Jul 26, 2016

Uh oh!

Trott commented Jul 26, 2016

Uh oh!

ofrobots commented Jul 26, 2016

Uh oh!

ofrobots commented Jul 26, 2016

Uh oh!

ofrobots commented Jul 26, 2016

Uh oh!

jasnell commented Jul 26, 2016

Uh oh!

bnoordhuis commented Jul 27, 2016

Uh oh!

ofrobots commented Jul 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjihrig commented Aug 10, 2016

Uh oh!

ofrobots commented Aug 10, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

ofrobots commented Jul 19, 2016 •

edited

Loading

ofrobots commented Jul 21, 2016 •

edited

Loading

ofrobots commented Jul 27, 2016 •

edited

Loading