doc: add guide for maintaining V8 #9777
Closed
Conversation
This file contains 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
mhdawson
approved these changes
Nov 24, 2016
|
|
||
| # Proposal: Dealing with the need to float patches to a stable/beta | ||
|
|
||
| Sometimes upstream V8 may not want to merge a fix to their stable branches, but we might. An example of this would be a fix for a performance regression that only affects Node.js and now the broweser. At the moment we don't have a mechanism to deal with this situation. If we float a patch and bump the V8 version, we might run into a problem if upstream releases a fix with the same version number. |
There was a problem hiding this comment.
"now the broweser" should be "not the browser"
sam-github
reviewed
Nov 24, 2016
| @@ -0,0 +1,281 @@ | |||
| # Maintaining V8 in Node | |||
|
|
|||
| [ofrobots@google.com](mailto:ofrobots@google.com), [hablich@chromium.org](mailto:hablich@chromium.org), 2016-08-05 | |||
There was a problem hiding this comment.
authors? people to ask for help? something else?
There was a problem hiding this comment.
It was authors, but I have removed that line.
sam-github
reviewed
Nov 24, 2016
|
|
||
| Once a bug in Node.js has been identified to be caused by V8, the first step is to identify the versions of Node and V8 affected. The bug may be present in multiple different locations, each of which follows a slightly different process. | ||
|
|
||
| * Unfixed bugs. If the bug exists in the V8 master branch. |
sam-github
reviewed
Nov 24, 2016
| * Fixed, but needs backport. The bug may need porting to one or more branches. | ||
| * Backporting to active branches. | ||
| * Backporting to abandoned branches. | ||
| * Backports identified by the V8 team. |
There was a problem hiding this comment.
is this different from backporting to active branches? doesn't v8 backport to active?
There was a problem hiding this comment.
I have tried to clarify this a bit further. The idea was that there are three classes of bugs, Node.js bugs that have (1) or have not (2) been fixed upstream, and bugs that have not been encountered by Node.js yet but upstream has identified as potentially interesting to Node (3).
sam-github
reviewed
Nov 24, 2016
|
|
||
| ## Unfixed Upstream Bugs | ||
|
|
||
| If the bug can be reproduced on the `vee-eight-lkgr` branch, Chromium canary, or V8 tip-of-tree, and the test case is valid, then the bug needs to be fixed upstream first. |
There was a problem hiding this comment.
where is this branch? I don't see it in nodejs/node.
There was a problem hiding this comment.
Thanks. Added link. FYI, it is at https://github.com/v8/node/tree/vee-eight-lkgr but we can bring this over to nodejs/node too if it is helpful.
sam-github
reviewed
Nov 24, 2016
| * In some cases the patch may require extra effort to merge in case V8 has changed substantially. For important issues we may be able to lean on the V8 team to get help with reimplementing the patch. | ||
| * Run Node's [V8-CI](https://ci.nodejs.org/job/node-test-commit-v8-linux/) in addition to the [Node CI](https://ci.nodejs.org/job/node-test-pull-request/). | ||
|
|
||
| An example for workflow how to cherry-pick consider the following bug: https://crbug.com/v8/5199. From the bug we can see that it was merged by V8 into 5.2 and 5.3, and not in V8 5.1 (since it was already abandoned). Since Node.js `v6.x` uses V8 5.1, the fix needed to cherry-picked. To cherry-pick, here's an example workflow: |
sam-github
reviewed
Nov 24, 2016
|
|
||
| PR-URL: <pr link> | ||
| ``` | ||
| * Open a PR against the `v6.x-staging` branch in the Node.js repo. Launch the normal and [V8-CI](https://ci.nodejs.org/job/node-test-commit-v8-linux/) using the Node.js CI system. |
There was a problem hiding this comment.
v6.x-staging is just an example, right? because backports might be necessary to any LTS staging branch
sam-github
reviewed
Nov 24, 2016
| Upgrading major versions would be much harder to do with the patch mechanism above. A better strategy is to | ||
|
|
||
| 1. Audit the current master branch and look at the patches that have been floated since the last major V8 update. | ||
| 1. Replace the copy of V8 in Node.js with a fresh checkout of the latest stable V8 branch. Special care must be taken to recursively update the DEPS that V8 has a compile time dependency on (at the moment of this writing, these are only trace_event and gtest_prod.h) |
There was a problem hiding this comment.
trace_event, or trace_event.h?
There was a problem hiding this comment.
trace event is a component. gtest_prod.h is just a file.
targos
approved these changes
Nov 24, 2016
| For example, at the time of this writing: | ||
|
|
||
| * **Stable**: V8 5.4 is currently shipping as part of Chromium stable. This branch was created approx. 6 weeks before from when V8 5.3 shipped as stable. | ||
| * **Beta**: V8 5.5 is currently in beta. It will be promoted to stable next; approximately 6 weeks after V8 5.3 shipped as stable. |
ofrobots
force-pushed
the
maintaining-v8
branch
from
0983c42
to
f07f5c2
Compare
3 tasks
|
I will land this later today. |
|
Thanks. Landed as 11b0872. |
ofrobots
added a commit
that referenced
this pull request
Nov 29, 2016
Ref: nodejs/Release#137 PR-URL: #9777 Reviewed-By: mhdawson - Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
addaleax
pushed a commit
that referenced
this pull request
Dec 5, 2016
Ref: nodejs/Release#137 PR-URL: #9777 Reviewed-By: mhdawson - Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
MylesBorins
pushed a commit
that referenced
this pull request
Dec 13, 2016
Ref: nodejs/Release#137 PR-URL: #9777 Reviewed-By: mhdawson - Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
MylesBorins
pushed a commit
that referenced
this pull request
Dec 13, 2016
Ref: nodejs/Release#137 PR-URL: #9777 Reviewed-By: mhdawson - Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
MylesBorins
pushed a commit
that referenced
this pull request
Dec 21, 2016
Ref: nodejs/Release#137 PR-URL: #9777 Reviewed-By: mhdawson - Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
MylesBorins
pushed a commit
that referenced
this pull request
Dec 21, 2016
Ref: nodejs/Release#137 PR-URL: #9777 Reviewed-By: mhdawson - Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Checklist
Affected core subsystem(s)
doc
Description of change
This document attempts to describe the processes for maintaining the V8 brances in Node.js LTS and Current releases.
Previously: nodejs/Release#137