Add doc on maintaining V8 in Node #137

ofrobots · 2016-09-22T22:55:52Z

This document attempts to describe the processes for maintaining the V8 brances in Node.js LTS and Current releases. This might possibly belong in the main repo instead of LTS.

Ref: #111

cjihrig · 2016-09-22T23:00:22Z

Another possible location for this would be the wiki, alongside https://github.com/nodejs/node/wiki/OpenSSL-upgrade-process.

bnoordhuis · 2016-09-23T09:15:22Z

FWIW, there is also a deps/openssl/doc/UPGRADING.md in the main repo. Admittedly, I don't know what the canonical source is...

bnoordhuis

LGTM with nits. The main repo might be a better place for it, though.

bnoordhuis · 2016-09-23T09:18:49Z

doc/maintaining-V8.md

+
+## Backporting to Active Branches
+
+If the bug exists in any of the active V8 branches, we may need to get the fix backported. At any given time there are[ two active branches](https://build.chromium.org/p/client.v8.branches/console) (beta and stable) in addition to master. The following steps are needed to backport the fix:


Small typo: there should be a space before the [, not after.

bnoordhuis · 2016-09-23T09:21:54Z

doc/maintaining-V8.md

+
+## Minor updates (patch level)
+
+Because there may floating patches on the version of V8 in Node.js, it is safest to apply the patch level updates as a patch. For example, imagine that upstream V8 is at 5.0.71.47 and Node.js is at 5.0.71.32. It would be best to compute the diff between these tags on the V8 repository, and then apply that patch on the copy of V8 in Node.js. This should preserve the patches/backports that Node.js may be floating (or else cause a merge conflict).


s/may/may be/

bnoordhuis · 2016-09-23T09:24:31Z

doc/maintaining-V8.md

+curl -L https://github.com/v8/v8/compare/${V8_OLD_VERSION}...${V8_NEW_VERSION}.patch > /tmp/patch-1
+cd deps/v8
+patch -p1 < /tmp/patch-1
+cd ../..


Wouldn't curl -L ... | git apply --directory=deps/v8 work just as well?

I usually use this approach as well

bnoordhuis · 2016-09-23T09:26:15Z

doc/maintaining-V8.md

+git log --oneline deps/v8
+```
+
+To replace the copy of V8 in Node, use the '[update-v8](https://gist.github.com/targos/8da405e96e98fdff01a395bed365b816)' script<sup>2</sup>. For example, if you want to replace the copy of V8 in Node.js with the branch-head for V8 5.1 branch:


We could (and probably should) pull that script into tools/.

Yep. @targos is also working on a port with more features. At some point it would be good to make this tooling directly available.

MylesBorins

LGTM with some small nits

MylesBorins · 2016-09-23T13:48:12Z

doc/maintaining-V8.md

+curl -L https://github.com/v8/v8/compare/${V8_OLD_VERSION}...${V8_NEW_VERSION}.patch > /tmp/patch-1
+cd deps/v8
+patch -p1 < /tmp/patch-1
+cd ../..


I usually use this approach as well

MylesBorins · 2016-09-23T13:48:37Z

doc/maintaining-V8.md

+git log --oneline deps/v8
+```
+
+To replace the copy of V8 in Node, use the '[update-v8](https://gist.github.com/targos/8da405e96e98fdff01a395bed365b816)' script<sup>2</sup>. For example, if you want to replace the copy of V8 in Node.js with the branch-head for V8 5.1 branch:


mhdawson · 2016-09-23T20:35:38Z

LGTM with the suggestion that we make V8-CI an actual link to https://ci.nodejs.org/job/node-test-commit-v8-linux/

ofrobots · 2016-09-23T23:24:39Z

Addressed comments, Thanks!

After the pointer from @cjihrig I was indeed wondering if the wiki would be a good place for this. Then I discovered that this page exists: https://github.com/nodejs/node/wiki/V8-upgrade-process 😄.

Given that the wiki does not have high discoverability, perhaps the main repo would be a good place to host this doc (and maybe we should point the wiki to the doc in the main repo). If there are no objections, I will close the PR here and proceed with a PR in the main repo.

targos · 2016-09-23T23:25:56Z

doc/maintaining-V8.md

+    * If there isn't already a V8 bug tracking the fix, open a new merge request bug using this [Node.js specific template](https://bugs.chromium.org/p/v8/issues/entry?template=Node.js%20merge%20request).
+    * If a bug already exists
+        * Add a reference to the GitHub issue.
+        * Attach *merge-request-x.x* labels to the bug for any active branches that still contain the bug. (e.g. merge-request-5.3, merge-request-5.4)


I suppose only V8 collaborators can do that ?

/cc @natorion

That template can be used by everybody. Changing the labels is restricted though.

I'm asking about adding merge-request-x.x labels or add someone to the cc list.

Unfortunately that is currently not possible on V8's issue tracker.

I will adjust the doc to suggest that folks on @nodejs/v8 can make these edits on the V8 issue tracker.

That should work from the V8 side.

targos · 2016-09-23T23:26:05Z

doc/maintaining-V8.md

+    * If a bug already exists
+        * Add a reference to the GitHub issue.
+        * Attach *merge-request-x.x* labels to the bug for any active branches that still contain the bug. (e.g. merge-request-5.3, merge-request-5.4)
+        * Add ofrobots-at-google.com to the cc list.


Same question

Changing the labels requires edit permissions on the V8 bug tracker.

targos · 2016-09-23T23:27:13Z

doc/maintaining-V8.md

+        * Add ofrobots-at-google.com to the cc list.
+* Once the merge has been approved, it should be merged using the [merge script documented in the V8 wiki](https://github.com/v8/v8/wiki/Merging%20&%20Patching). Merging requires commit access to the V8 repository. If you don't have commit access you can indicate someone on the V8 team can do the merge for you.
+* It is possible that the merge request may not get approved, for example if it is considered to be a feature or otherwise too risky for V8 stable. In such cases we float the patch on the Node side. See the process on 'Backporting to Abandoned branches'.
+* Once the fix has merged upstream, it can be picked up during an update of the V8 branch, (see below).


s/has/has been/

targos · 2016-09-23T23:29:29Z

doc/maintaining-V8.md

+
+* For each abandoned V8 branch corresponding to an LTS branch that is affected by the bug:
+    * Open a cherry-pick PR on nodejs/node targeting the appropriate *vY.x-staging* branch (e.g. *v6.x-staging* to fix an issue in V8-5.1).
+    * Increase the patch level version in v8-version.h. This will not cause any problems with versioning because V8 will not publish other patches for this branch, so Node.js can effectively bump the patch version.


It will cause problems if we are backporting a patch that V8 did not want to merge in a supported branch. Should we keep the patch level unchanged in this case ?

Version bumps should only be done for abandoned branches.

For stable branches, upstream controls what gets merged (and does the version bumps). You're right that for stable branches we could have a disagreement where upstream is unwilling to accept a merge that we may end up needing to float. One idea we have been kicking around is that we could move to a 5-place version number in V8, e.g.: 5.4.500.30.${embedder}. The ${embedder} represents the number of patches an embedder is floating on top of an official V8 version. This would also help with auditing the floating patches in the Node commit history. WDYT?

Seems reasonable to me.

@ofrobots So the floating patch version would only reset to 0 on a major v8 bump? Seems a little unintuitive, but definitely better than what we have now.

I've edited the doc to add this proposal.

@gibfahn: yep, a major bump is the likely place where the patch level would likely go down to 0. There may be other cases though e.g. when we float an emergency patch but upstream fixes it different and we no longer need the patch etc.

targos · 2016-09-23T23:31:43Z

doc/maintaining-V8.md

+
+Such fixes are tagged with the following labels in the V8 issue tracker:
+
+*   ~~NodeJS-~~Backport-Review ([V8](https://bugs.chromium.org/p/v8/issues/list?can=1&q=label%3ABackport-Review), [Chromium](https://bugs.chromium.org/p/chromium/issues/list?can=1&q=label%3ABackport-Review)): to be reviewed if this is applicable to abandoned branches in use by Node.js. This list if regularly reviewed by the node team at Google to determine applicability to Node.js.


is the NodeJS- prefix supposed to be removed ?

These tags are going to renamed very soon to include the 'NodeJS-' prefix.

I just renamed the labels and migrated the issues.

targos · 2016-09-23T23:34:53Z

doc/maintaining-V8.md

+cd deps/v8
+patch -p1 < /tmp/patch-1
+cd ../..
+git commit -am 'deps: update V8 from V8_OLD_VERSION to V8_NEW_VERSION'


The patch can add new files (often tests).
I use git add deps/v8 && git commit -m '...' to not miss anything

targos · 2016-09-23T23:35:44Z

doc/maintaining-V8.md

+git commit -am 'deps: update V8 from V8_OLD_VERSION to V8_NEW_VERSION'
+```
+
+V8 also keeps tags of the form *5.4-lkgr* which point to the *Last Known Good Release* from the 5.4 branch that can be useful in the update process above.


I think lkgr means Last Known Good Revision

targos · 2016-09-23T23:36:26Z

doc/maintaining-V8.md

+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, that is only trace_event)


trace_event and gtest_prod.h

targos · 2016-09-23T23:39:56Z

+1 for putting this in the main repo

mhdawson · 2016-09-26T19:39:53Z

Would still like to see V8-CI linked to the actual job in the CI

targos · 2016-09-27T06:12:44Z

doc/maintaining-V8.md

+git merge --ff-only origin/master
+git checkout -b V8_NEW_VERSION
+curl -L https://github.com/v8/v8/compare/${V8_OLD_VERSION}...${V8_NEW_VERSION}.patch | git apply --directory=deps/v8
+# You may want to amend the commit message to indicate 


Seems that something was lost here

ofrobots · 2016-11-24T01:45:19Z

I've incorporated all the comments here into the doc and made it up to date. I'm going close this issue and open one on the main repo.

ofrobots · 2016-11-24T01:53:34Z

nodejs/node#9777

Ref: nodejs/Release#137

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>

bnoordhuis approved these changes Sep 23, 2016

View reviewed changes

MylesBorins approved these changes Sep 23, 2016

View reviewed changes

targos reviewed Sep 23, 2016

View reviewed changes

targos approved these changes Sep 23, 2016

View reviewed changes

ofrobots force-pushed the v8-doc branch from 36f0299 to 20880bf Compare September 27, 2016 00:58

Add doc on maintaining V8 in Node

c347998

ofrobots force-pushed the v8-doc branch from 20880bf to c347998 Compare September 27, 2016 01:12

targos reviewed Sep 27, 2016

View reviewed changes

richardlau mentioned this pull request Sep 28, 2016

deps: cherry-pick 34880eb3dc from V8 upstream nodejs/node#8673

Closed

2 tasks

ofrobots mentioned this pull request Oct 20, 2016

V8 Backports Triage nodejs/node#9190

Closed

3 tasks

MylesBorins mentioned this pull request Nov 14, 2016

Backport --shared to V4.x codebase to allow building as a shared library / DLL nodejs/node#9385

Closed

2 tasks

sam-github mentioned this pull request Nov 15, 2016

Backport support for building as DLL on Windows to V6 nodejs/node#8084

Closed

2 tasks

ofrobots closed this Nov 24, 2016

ofrobots mentioned this pull request Nov 24, 2016

doc: add guide for maintaining V8 nodejs/node#9777

Closed

2 tasks

ofrobots added a commit to ofrobots/node that referenced this pull request Nov 26, 2016

doc: add guide for maintaining V8

f07f5c2

Ref: nodejs/Release#137

ofrobots added a commit to nodejs/node that referenced this pull request Nov 29, 2016

doc: add guide for maintaining V8

11b0872

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 to nodejs/node that referenced this pull request Dec 5, 2016

doc: add guide for maintaining V8

6ab920a

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>


		## Backporting to Active Branches

		If the bug exists in any of the active V8 branches, we may need to get the fix backported. At any given time there are[ two active branches](https://build.chromium.org/p/client.v8.branches/console) (beta and stable) in addition to master. The following steps are needed to backport the fix:


		## Minor updates (patch level)

		Because there may floating patches on the version of V8 in Node.js, it is safest to apply the patch level updates as a patch. For example, imagine that upstream V8 is at 5.0.71.47 and Node.js is at 5.0.71.32. It would be best to compute the diff between these tags on the V8 repository, and then apply that patch on the copy of V8 in Node.js. This should preserve the patches/backports that Node.js may be floating (or else cause a merge conflict).


		Such fixes are tagged with the following labels in the V8 issue tracker:

		* ~~NodeJS-~~Backport-Review ([V8](https://bugs.chromium.org/p/v8/issues/list?can=1&q=label%3ABackport-Review), [Chromium](https://bugs.chromium.org/p/chromium/issues/list?can=1&q=label%3ABackport-Review)): to be reviewed if this is applicable to abandoned branches in use by Node.js. This list if regularly reviewed by the node team at Google to determine applicability to Node.js.

Add doc on maintaining V8 in Node #137

Add doc on maintaining V8 in Node #137

Conversation

ofrobots commented Sep 22, 2016

cjihrig commented Sep 22, 2016

bnoordhuis commented Sep 23, 2016

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MylesBorins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson commented Sep 23, 2016 • edited Loading

ofrobots commented Sep 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natorion Sep 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

targos Sep 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

targos Sep 23, 2016 • edited Loading

Choose a reason for hiding this comment

targos commented Sep 23, 2016

mhdawson commented Sep 26, 2016

targos Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

ofrobots commented Nov 24, 2016

ofrobots commented Nov 24, 2016

mhdawson commented Sep 23, 2016 •

edited

Loading

natorion Sep 28, 2016 •

edited

Loading

targos Sep 23, 2016 •

edited

Loading

targos Sep 23, 2016 •

edited

Loading

targos Sep 27, 2016 •

edited

Loading