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.

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

Managing floating V8 patches #111

Closed
natorion opened this issue Jun 9, 2016 · 30 comments
Closed

Managing floating V8 patches #111

natorion opened this issue Jun 9, 2016 · 30 comments

Comments

@natorion
Copy link

natorion commented Jun 9, 2016

As discussed in a previous LTS WG meeting, the V8 team would like help the LTS effort. One concrete area where we could help is how V8 patches are being managed on the LTS (and other) branches.

Node.js currently is maintaining patches in their copy of V8. This copy of V8 is located in a subdirectory without any reference to the imported V8 commit. On top of this import Node.js maintains floating patches in order to make Node.js work with V8 or add more bug fixes and features. The important point is that those floating patches are not in an official V8 branch in the V8 repo.

It would be beneficial for Node.js to have a solution to clearly identify those floating patches from the V8 base import. Additionally it would be helpful to be able to keep track of which patches got merged into which branch.

Some of them are pulled into V8 as long as the branch is still active on the V8 side. It would be beneficial if Node.js didn't have to maintain their in-tree V8 fork anymore but push all the patches upstream into a V8 repo.

The Node.js team at Google outlined two potential solutions. Feel free to add your own ideas!

Maintain all the floating patches in the V8 repository on Chrome's infrastructure

A prerequisite is that the Node.js build is able to pull in a specific V8 commit/ref. This is similar to V8's DEPS format.

This means that V8 creates separate release branches for Node.js in V8's repository: e.g. node-master, node-current, node-lts, node-maintenance. Node is then going to directly import the HEAD of each branch into their respective Node.js branch.

Node.js benefits:

  • Node.js gets V8 test coverage for the patches
    • V8 only runs a very limited subset of the ToT waterfall on those branches
    • Node.js recently added ability to run V8 tests in their CI, which could be used.
  • It makes it easier for Node.js developers to contact V8 developers for code-reviews
    • On the other side Node.js developers need to use V8's infrastructure
  • Tracking of merges works if V8's merge scripts are used.

Node.js drawbacks:

  • Node.js maintainers need to heed V8's workflow for patches (including the tools)
  • Node.js is completely dependant on V8's infrastructure
    • As a contingency node.js probably still wants to be able to float patches which raises the question why we are doing this?
  • Complicates branching/versioning/handling merges because two branches with the same origin might start to differ and e.g. 5.6.333.67 might point to two different commits.

Maintain all the floating patches in a V8 fork on GitHub

A prerequisite is that the Node.js build is able to pull in a specific V8 commit/ref. This is similar to V8's DEPS format.

Another solution would be create a V8 fork in the github nodejs or v8 organizations which automatically gets synced with the official V8 repo (Currently V8 is doing the reverse and maintaining a Node.js fork for integrating a new V8 version.). On the V8 fork the branches node-master, node-current, node-lts, node-maintenance are created. Node.js can then directly import the HEAD of each branch into their respective Node.js branch.

Node.js benefits:

  • Node.js developers can use their own merge processes and tools
  • Node.js' build does not need to be integrated with another system (Chrome infrastructure)
  • Tracking of merges works if V8's merge scripts are used.
    • We should be able to adapt tools/release/merge_to_branch.py to be able service a V8 fork repo

Node.js drawbacks:

  • No test coverage from the V8 infrastructure
    • Which probably is not needed because Node.js infrastructure can execute V8 tests
  • Complicates branching/versioning/handling merges because two branches with the same origin might start to differ and e.g. 5.6.333.67 might point to two different commits.

It would be great to get some more input on this analysis.

@bnoordhuis
Copy link
Member

/cc @nodejs/ctc and @nodejs/v8 - this is relevant to your interests.

If we want to be a good OSS citizen, option 1 is arguably best. If a bug fix or feature is beneficial to node.js, chances are good it's beneficial to other projects that depend on V8. A V8 LTS branch would make a lot of people quite happy, I'm sure.

V8 only runs a very limited subset of the ToT waterfall on those branches

That might be an issue. How limited is very limited?

Node.js maintainers need to heed V8's workflow for patches (including the tools)

IME, the tooling is not terrible once you get the hang of it but the infrastructure is very opaque to people outside the googleplex. I'm still not sure what the difference between the trybots and the main waterfall is, for example.

On a semi-related topic, what about integration testing? Ideally, we apply a change to the V8 tree and the node.js tree and check that both test suites pass. That's something we can make work on our own CI infrastructure but probably not on Google's.

@MylesBorins
Copy link
Contributor

I'm very much in support of an official LTS release stream. I've gotten started with using Depot tools before and would be willing to dive in more if that will help. Do we have individuals from google that are willing to volunteer as mentors to help individuals from the node project get up and running on your stack?

@natorion
Copy link
Author

natorion commented Jun 10, 2016

V8 only runs a very limited subset of the ToT waterfall on those branches

That might be an issue. How limited is very limited?

Compare our bots servicing beta and stable branch to essentially all the bots combined on the other of our waterfalls (e.g. main waterfall). This works for us because our branches are close to ToT and we get a lot of coverage from the general Chromium bots downstream.

On a semi-related topic, what about integration testing? Ideally, we apply a change to the V8 tree and the node.js tree and check that both test suites pass. That's something we can make work on our own CI infrastructure but probably not on Google's.

We are already running something similar on our waterfall. This bots tries to continually integrate V8's LKGR into a fork of node master. Doing the same for an LTS is a different beast because one would need infrastructure which is compatible > 30 months backwards.

I'm very much in support of an official LTS release stream.

Just to clarify, you mean the second idea, a V8 fork on GitHub in Node.js' domain, right? If you have specific questions regarding depot_tools, I am sure we can get them answered.

CC @ofrobots

@MylesBorins
Copy link
Contributor

@natorion I was referring to

V8 creates separate release branches

I'm open to whatever has the most traction between the two teams!

@MylesBorins
Copy link
Contributor

@natorion would you like to join us for the next LTS working group meeting?

@ofrobots
Copy link

I think @natorion is away this week. He was planning to attend an LTS meeting, but we haven't had one for a while. A Europe friendly meeting time for the next one would be nice, but not a requirement. /cc @jasnell.

@natorion
Copy link
Author

I would gladly try to join the next meeting. When is it scheduled?

@MylesBorins
Copy link
Contributor

/cc @nodejs/v8 and @natorion

We'd like to schedule a meeting time / day that works for everyone. @mhdawson is going to be posting a doodle in here in the near future, if you could fill it out with available dates / times that would be great!

@mhdawson
Copy link
Member

Link to doodle poll http://doodle.com/poll/dwcpugyzsvitta94

@ofrobots
Copy link

I'm out of office during the week of July 4. Could there be options for other weeks?

@MylesBorins
Copy link
Contributor

@natorion what is your schedule looking like.

As an aside would you be willing to change the title to mention V8?

@natorion natorion changed the title Managing floating patches Managing floating V8 patches Jun 29, 2016
@natorion
Copy link
Author

I just added my schedule.

@MylesBorins
Copy link
Contributor

@natorion just wanted to let you know that we just landed nodejs/node#7451 which allows us to run the V8 test suite in our CI on v4.x

@MylesBorins
Copy link
Contributor

Bringing this over from nodejs/node#7584

We didn't mention how many LTS V8 branches we would be maintaining

is it safe to assume that we are discussing a V8 LTS branch for every Node LTS release stream?

It looks like the time from the last doodle has come and past. Let's try this again

Fill out availability for week of July 11 - 15 in this doodle

@jasnell
Copy link
Member

jasnell commented Jul 8, 2016

Yes, I believe that is a safe assumption.
On Jul 7, 2016 3:49 PM, "Myles Borins" notifications@github.com wrote:

Bringing this over from nodejs/node#7584
nodejs/node#7584

We didn't mention how many LTS V8 branches we would be maintaining

is it safe to assume that we are discussing a V8 LTS branch for every Node
LTS release stream?

It looks like the time from the last doodle has come and past. Let's try
this again

Fill out availability for week of July 11 - 15 in this doodle
http://doodle.com/poll/bxpmtk9v6wy76dde


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#111 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAa2eTZtQgQBgd32cvHpUR79wfi-ukXxks5qTYKJgaJpZM4IychZ
.

@natorion
Copy link
Author

natorion commented Jul 8, 2016

I agree. Can you please CC me on the agenda issue or ping this thread when you decided on a date for the next LTS? It seems I missed the announcement from the 27th June meeting.

@MylesBorins
Copy link
Contributor

MylesBorins commented Jul 8, 2016

I think we should tentatively call this for July 11th at 1pm UTC (6 am PDT, 9 am EST, and 11 pm in AUS).

This time works for @rvagg @natorion and myself

/cc @mhdawson @Fishrock123 @ofrobots

https://www.timeanddate.com/worldclock/fixedtime.html?msg=Node.js+Foundation+LTS+WG+Meeting&iso=20160711T13&p1=%3A

@mhdawson
Copy link
Member

mhdawson commented Jul 8, 2016

I have another meeting at that time, but I think I can skip it for next week and make the LTS discussion so cound me in

@ofrobots
Copy link

I will try to make it, but I don't promise to be coherent at 6am on my first day back from vacation :).

@MylesBorins
Copy link
Contributor

@ofrobots does Tuesday or Wednesday work better? We can do the same time and @fhinkel will be able to join too

@mhdawson does that work for you?

@ofrobots
Copy link

I can try and make it to any of the days above; but don't change things on my behalf.

@natorion
Copy link
Author

How about we meet tomorrow so @ofrobots has at least a grace period after his vacation and @fhinkel can also attend?

@mhdawson
Copy link
Member

I can make 9:30 EST on Tuesday or 9 on Wednesday

@natorion
Copy link
Author

Where do I find the meeting ID?

@MylesBorins
Copy link
Contributor

Hey everyone. It seems like people can make it to Wednesday 1pm UTC (6 am PDT, 9 am EST, and 11 pm in AUS).

https://www.timeanddate.com/worldclock/fixedtime.html?msg=Node.js+Foundation+LTS+WG+Meeting&iso=20160713T13&p1=%3A

Will set up a meeting issue today.

@MylesBorins
Copy link
Contributor

MylesBorins commented Jul 12, 2016

notes from @bnoordhuis

I'll see if I can join. In case I can't make it:

  1. If we're going to use Google's infrastructure, what time frame are we looking at? I understand @ofrobots's concern about conflicting version numbers but don't think maintenance should come to a standstill while we wait.
  2. Apropos infrastructure, I understand builders are torn down when a release branch is EOL'd. Does that mean only a subset of the normal waterfall is used and is it still worthwhile for us in that case? We can ramp up our own infrastructure with only modest effort.
  3. Using Google's infrastructure, we'll need to test changes against our test suite separately. Inconvenient, can something be done about that?

@targos
Copy link
Member

targos commented Jul 26, 2016

So the decision was to work with an intermediate fork of v8/v8 ? Something like https://github.com/nodejs/v8 ?

@MylesBorins
Copy link
Contributor

@targos I don't think a final decision was made, but it would appear that relying on the chromium infra was not going to be the most ideal way to do things. /cc @nodejs/v8

@ofrobots
Copy link

It is a TODO on my end up to write up a proposal on how a forked repo could be used. The idea would be to either use https://github.com/nodejs/v8 or https://github.com/v8/v8-node.

@MylesBorins
Copy link
Contributor

@ofrobots do you think we could close this for now or do you still want to put time into using the forked version of v8?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants