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: update v8_inspector #7118

Closed
wants to merge 1 commit into from
Closed

Conversation

ofrobots
Copy link
Contributor

@ofrobots ofrobots commented Jun 2, 2016

Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

deps

Description of change

Pick the latest v8_inspector [1].

  • V8 5.1 compatibility
  • Modify parse builder templates to make coverity happy
  • The whitespace differences in the jinja2 sub-depenency do exist
    upstream. I am not sure how I missed them in the original import
    (ed2eacb).

[1] pavelfeldman/v8_inspector@3b56732

/cc @targos, @pavelfeldman, @bnoordhuis
EDIT: CI: https://ci.nodejs.org/job/node-test-pull-request/2909/

Pick the latest v8_inspector [1] with:
* V8 5.1 compatibility
* Modify parse builder templates to make coverity happy
* The whitespace differences in the jinja2 sub-dependency do exist
  upstream. I am not sure how I missed them in the original import
  (ed2eac).

[1] pavelfeldman/v8_inspector@3b56732
@nodejs-github-bot nodejs-github-bot added the inspector Issues and PRs related to the V8 inspector protocol label Jun 2, 2016
@addaleax
Copy link
Member

addaleax commented Jun 3, 2016

LGTM

@MylesBorins
Copy link
Contributor

ci: https://ci.nodejs.org/job/node-test-pull-request/2910/

@ofrobots it would be awesome to get some basic tests in for the debugger. I'd be up for working on some to make the process of landing these updates clearer. Would we be able to draw on any of the v8 or chomium tests?

@cjihrig
Copy link
Contributor

cjihrig commented Jun 3, 2016

LGTM

@jasnell
Copy link
Member

jasnell commented Jun 3, 2016

LGTM if CI is green

@MylesBorins
Copy link
Contributor

arm failures appear to be infra related LGTM

@addaleax
Copy link
Member

addaleax commented Jun 3, 2016

@ofrobots’ CI is green, fwiw.

DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
Copy link
Member

@bnoordhuis bnoordhuis Jun 3, 2016

Choose a reason for hiding this comment

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

Can you undo the LF -> CRLF changes in this file?

EDIT: Or does this come verbatim from upstream?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This did come verbatim from upstream. The difference shows up, as you guessed, because we land with --whitespace=fix

@bnoordhuis
Copy link
Member

LGTM. I usually land patches with git am --whitespace=fix. That would take care of most of the changes in this PR, I think.

@targos
Copy link
Member

targos commented Jun 3, 2016

LGTM

targos pushed a commit that referenced this pull request Jun 3, 2016
Pick the latest v8_inspector [1] with:
* V8 5.1 compatibility
* Modify parse builder templates to make coverity happy
* The whitespace differences in the jinja2 sub-dependency do exist
  upstream. I am not sure how I missed them in the original import
  (ed2eac).

[1] pavelfeldman/v8_inspector@3b56732

PR-URL: #7118
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
@targos
Copy link
Member

targos commented Jun 3, 2016

Landed in 8847777

@targos targos closed this Jun 3, 2016
@MylesBorins MylesBorins added this to the 7.0.0 milestone Jun 14, 2016
Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016
Pick the latest v8_inspector [1] with:
* V8 5.1 compatibility
* Modify parse builder templates to make coverity happy
* The whitespace differences in the jinja2 sub-dependency do exist
  upstream. I am not sure how I missed them in the original import
  (ed2eac).

[1] pavelfeldman/v8_inspector@3b56732

PR-URL: #7118
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
@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.

9 participants