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.

Sign up for GitHub

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

Jump to bottom

missing V8 6.0 patch #14582

Closed
wants to merge 1 commit into from
Closed

missing V8 6.0 patch #14582

wants to merge 1 commit into from

Conversation

MylesBorins
Copy link
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 nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Aug 1, 2017
addaleax
addaleax approved these changes Aug 1, 2017
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

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

@MylesBorins MylesBorins mentioned this pull request Aug 1, 2017
Closed
@MylesBorins
Copy link
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
Copy link
Member

targos commented Aug 1, 2017

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

@refack
Copy link
Contributor

refack commented Aug 2, 2017

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

@bzoz @MylesBorins
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins
Copy link
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 MylesBorins changed the title Three missing V8 6.0 patches missing V8 6.0 patch Aug 2, 2017
@refack
Copy link
Contributor

refack commented Aug 2, 2017

@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
Copy link
Contributor

refack commented Aug 2, 2017 •
edited
Loading

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

@refack
Copy link
Contributor

refack commented Aug 2, 2017

(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
Copy link
Contributor

refack commented Aug 2, 2017 •
edited
Loading

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

@MylesBorins
Copy link
Contributor Author

Closing as it appears we do not need these commits

@MylesBorins MylesBorins closed this Aug 2, 2017
@MylesBorins MylesBorins deleted the missing-V8-patches branch November 14, 2017 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@fhinkel fhinkel fhinkel approved these changes

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
@MylesBorins @targos @refack @fhinkel @jasnell @addaleax @nodejs-github-bot @bzoz