FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

missing V8 6.0 patch by MylesBorins · Pull Request #14582 · nodejs/node · GitHub

/ node Public

missing V8 6.0 patch - #14582

Closed
MylesBorins wants to merge 1 commit into
nodejs:masterfrom
MylesBorins:missing-V8-patches
ConversationCommits (1)ChecksFiles changed
Closed

missing V8 6.0 patch#14582
MylesBorins wants to merge 1 commit into
nodejs:masterfrom
MylesBorins:missing-V8-patches

Conversation

MylesBorins commented Aug 1, 2017

Copy link
Copy Markdown
Contributor

There were three missing patches that were included in #13515 that were missed in #14004

It seems like these were missed because they either

a) Directly touch the V8 system and were not upstreamed

or

b) Were backports that did not follow our backport process

I'm not sure the best process for landing these... please advise

/cc @nodejs/v8

nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Aug 1, 2017
addaleax approved these changes Aug 1, 2017
View reviewed changes

addaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I don’t know the best way forward either, but I think you can feel free to just re-land these now.

MylesBorins mentioned this pull request Aug 1, 2017
Closed

MylesBorins commented Aug 1, 2017

Copy link
Copy Markdown
Contributor Author

Ci: https://ci.nodejs.org/job/node-test-pull-request/9441/
V8-CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/833/

targos commented Aug 1, 2017

Copy link
Copy Markdown
Member

I'm pretty sure those starting with "deps:" are not necessary with 6.0. That's why I didnt include them.

refack commented Aug 2, 2017

Copy link
Copy Markdown
Contributor

I just built debug on windows without v8: fix debug builds on Windows but on VS2017
Trying now on VS2015

v8: fix debug builds on Windows
6afa220 
Adds missing return which fixes debug builds on Windows

Fixes: nodejs#13392
Ref: https://codereview.chromium.org/2929993003/
Refs: nodejs#13634

PR-URL: nodejs#14582
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins commented Aug 2, 2017

Copy link
Copy Markdown
Contributor Author

I dropped the deps tests as they are unnecessary

@refack did you have a chance to find out if the debug build is working with VS2015?

MylesBorins changed the title Three missing V8 6.0 patches missing V8 6.0 patch Aug 2, 2017

refack commented Aug 2, 2017

Copy link
Copy Markdown
Contributor

@refack did you have a chance to find out if the debug build is working with VS2015?

Just completed. debug builds on VS2017 & VS2015 without that commit.

refack commented Aug 2, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Running CI forced to build debug: https://ci.nodejs.org/job/node-test-commit/11515/

refack commented Aug 2, 2017

Copy link
Copy Markdown
Contributor

(Previus CI seems like it works reasonalby well, I'm assuming the sporadic compilation failures are related to memory shortage)
Forcing Debug on Windows "harder": https://ci.nodejs.org/job/node-test-commit-windows-fanned/10808/

refack commented Aug 2, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Windows debug CI compiles (without patch).
@MylesBorins I think you can close this (without landing).

MylesBorins commented Aug 2, 2017

Copy link
Copy Markdown
Contributor Author

Closing as it appears we do not need these commits

MylesBorins closed this Aug 2, 2017
MylesBorins deleted the missing-V8-patches branch November 14, 2017 17:43
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
addaleax addaleax approved these changes
+1 more reviewer
fhinkel fhinkel approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

v8 engine Issues and PRs related to the V8 dependency.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL