GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — missing V8 6.0 patch by MylesBorins · Pull Request #14582 · nodejs/node

Conversation

@MylesBorins

@MylesBorins 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Aug 1, 2017
jasnell
jasnell approved these changes Aug 1, 2017
View reviewed changes
addaleax
addaleax approved these changes Aug 1, 2017
View reviewed changes

@addaleax addaleax left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@MylesBorins MylesBorins mentioned this pull request Aug 1, 2017
deps: update V8 to 6.0 8.x backport #14574
Closed
@MylesBorins

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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@targos

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@refack

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
fhinkel
fhinkel approved these changes Aug 2, 2017
View reviewed changes
@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
MylesBorins force-pushed the missing-V8-patches branch from 8730544 to 6afa220 Compare August 2, 2017 14:55
@MylesBorins

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MylesBorins MylesBorins changed the title Three missing V8 6.0 patches missing V8 6.0 patch Aug 2, 2017
@refack

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@refack

refack commented Aug 2, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@refack

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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@refack

refack commented Aug 2, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MylesBorins

MylesBorins commented Aug 2, 2017

Copy link
Copy Markdown
Contributor Author

Closing as it appears we do not need these commits
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

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

Uh oh!

There was an error while loading. Please reload this page.

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.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@MylesBorins @targos @refack @fhinkel @jasnell @addaleax @nodejs-github-bot @bzoz
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.