Help wanted: Backporting to v11.x

[addaleax]

Hi @nodejs/collaborators ! Backport requests to v11.x-staging are queuing up and it’s getting pretty problematic.

If you feel like you can pick up items from https://github.com/nodejs/node/issues?page=2&q=label%3Abackport-requested-v11.x+is%3Aclosed, in particular some of the older ones, that would help a lot. The backporting guide is here, for those who want to revisit :)

In the meantime, I would consider measures like marking all PRs that mostly move code around as blocked, so that at least the problem doesn’t get worse. What do you think?

[addaleax]

Oh, and for anybody who wants to help: Really, please take a look at the oldest PRs first. These PRs tend to touch a lot of common code, so first backporting one of the more recent ones is going to make backporting the older ones even harder.

Partial backports would also help tremendously.

[BridgeAR]

The numbers of open backport requests is now down by ~40%. Most still open ones do not seem to be trivial to backport though.

[sam-github]

Looks like the ones left are mostly @joyeecheung , a handful of @addaleax , and one @cjihrig --- the master refactors are great, but if they aren't backported they might form a wall beyond which even small changes won't cherry-pick without conflict.

[joyeecheung]

One observation: the relationships between the PRs pending backports seem to be similar. If the commits touching a particular file are landed on master as A-B-C-D-E...-Z, oftentimes is no point trying to cherry-pick B/C...Z without backporting A first where A:

• May happen to require a manual backport in nature, e.g. it's semver-major but with some small refactoring
• Touches a significant amount of code in that file (say >30%)
• Involves a file move or split

It usually makes the situation worse if one of the later commits is backported manually out-of-order, say, H - then every commit in the chain may not be landed cleanly even if someone manage to backport A on top of H, to make the subsequent cherry-picking easier the prematurely-backported H has to be reverted first.

(It should be possible to make some smarter tools to at least be aware of the dependencies of commits for a particular file, e.g. as demonstrated by git log <sha> /path/to/file)

[joyeecheung]

Is the current cherry-picking process done manually (as per https://github.com/nodejs/node/blob/master/doc/releases.md#1-update-the-staging-branch ) or is that streamlined somehow? If so the streamlining should take #25403 (comment) into account, adding some additional labels (like backport-blocked-vx.y) may help.

[sam-github]

I've noticed the issue with out of order backports as well, things can get progressively worse.

I haven't worked actively on LTS branches in the last year, but the process is pretty flexible. The -staging branches can be rewritten, its not necessary to "git revert" commits on them, for example, they can be dropped. I'm not sure what the meaning of backport-blocked-vx.y would be. The meaning of the current backport labels is documented, somewhere, but I'm having trouble finding it.

/cc @BethGriggs

[joyeecheung]

The -staging branches can be rewritten, its not necessary to "git revert" commits on them, for example, they can be dropped.

It is possible to reorder the commits after the backports are merged, but when someone who is not a releaser trying to backport locally (and open backport PRs afterwards), reverting those commits would help getting the tree into a shape that is easier to cherry-pick on cleanly.

[BridgeAR]

We ran into issues like these before and we also spoke about these things a couple of times (e.g., on the collaborator summit).

For me there's really just one way to solve this issue (that I am aware of): as soon as a releaser hits a PR that they are not able to backport we block the next release until it's backported (or what ever it relies upon). This normally only applies to PRs which refactor a lot or which imply a deep understanding of a specific code part. It could get more difficult after a while if we have big breaking changes but there's also often also a remedy for that: we could start backporting semver-major PRs in a non-breaking fashion. It often makes sense to have bigger code changes than the actual breaking change (as in: the other code changes might not be necessary without the actual breaking part). So backporting those non-breaking parts would be great as well.

But enforcing this is difficult as we can not expect everyone who opens a PR to always be available for backporting and we someone else might have to do it instead at times (which is not always that easy).

We could give that a try if others agree to it as well. I believe in that case, not only the @nodejs/releasers but all @nodejs/collaborators should agree to it.

[joyeecheung]

BTW I am writing a tool to help analyzing the dependencies of commits

Though currently it is quite slow..(also it does not handle ill-formatted commit messages, like in the screenshot the console commit is backported, just that the backported commit has PR-URL: #24709, I could grep for those as well but that would double the search time.

[sam-github]

we can not expect everyone who opens a PR to always be available for backporting

Definitely true for non-collaborator PRs, and for people who make occaisonal contributions, but I think its reasonable to encourage (strongly encourage?) regular contributors to node to do the work to get their PRs into shape to be released. If they can't, they can also respond with a request for help.

It would be nice if some tooling auto-labelled and posted into PRs telling people after they merge whether they land clean in the release staging branches.

Would adding this to git node land be unreasonable? I assume its deliberate that the actual final stage of posting the Landed in XXXX comment and pushing the commit is done by hand, but if it was automated, the Landed in ... could also include a "this will need backporting, please do ..." note. Or the note could be output as part of what needs to be pasted manually?

[joyeecheung]

@sam-github I think this should be preferably be done by the bot, as switching between branches locally may break people's workflows. Also as I explained in #25403 (comment) it's not a simple matter of backporting PRs as soon as possible, they should also be backported in the correct order to reduce conflicts, otherwise the backports may be even less productive.

[sam-github]

Agreed. Still, the correct order is likely the order they land in master... so reminding people to backport once it lands is close to the correct order. It may be useful to at least give them (or whoever is running git node land?) a heads up that at the time it landed, this PR was not automatically a candidate for release in Current.

In the case that their PR doesn't land clean in staging because it depends on a previous PR to master that hasn't been backported yet, then yes - their backport is blocked, they shouldn't start backporting yet. That's useful information.

What kind of conditions can bots respond to?

As commits land in staging branches, the status of PRs in terms of whether they do or do not need backporting changes... they might initially appear to need backporting, but are better described as blocked by a previous PR that has not (yet?) backported -- once the previous PR is backported the PR in question may change from "backport needed" to "cherry picks clean".

[targos]

I think we need a sort of simple status website that reports which is the next PR that either needs to be backported or marked don't land.

[joyeecheung]

@sam-github I am afraid backporting every PR ASAP is too stressful for both the contributors and the releasers unless we automate the work of finding out the conflicts and dependencies and pinging for backport somehow. The knowledge of the relationship among PRs needs to be tracked properly so having multiple human beings share the work could result in extra overhead (1 / n * n > 1 when the knowledge is not centralized or communicated properly).

We could ultilize the fact that if we have PR landed in the order of A-B-C, where:

• A touches file 1, 2, 3
• B touches file 4
• C touches file 1

then most of the times, it does not really matter if they are backported in the order of ACB or ABC - we usually only care that C should be backported after A. Sometimes B may still be relevant even if there's no obvious dependency in the changed files but according to my observation that's pretty rare.

[codebytere]

hey @sam-github, I have some tooling like that already in place in another org, which could take place here in the following steps:

1. A collaborator tags an author-ready PR that has not yet landed with a label such as backport-to/<TARGET_STAGING_BRANCH>. This also serves to help backporters unsure of whether or not a PR should be backported to a certain staging branch.
2. A Check begins to run
3. It passes if the backport is clean and thus can just be applied by a backporter cleanly without a backport PR.
4. If it fails, a label is automatically applied to the PR requesting a manual backport to said target staging branch. The failed diff would also be output into the Checks UI.

Would this type of process help? I'm happy to clean up that tooling for this specific purpose and work to get it going if so!

[sam-github]

@codebytere I'm not active on the LTS WG any more, and its the people doing the work who should comment, but I suspect that wouldn't help node so much.

FYI, releasing relies heavily on https://github.com/nodejs/branch-diff, which is used to determine commits that need to be merged down. It skips commits that have already been cherrypicked (so differ by SHA), and it also skips (for most use-cases) semver-major commits and some book keeping commits (changelog updates, IIRC, since we don't merge 12.x changelog updates to 10.x branches).

Its output is then fed into git cherry-pick, and it trundles along until a commit fails to pick clean, and the person running the tool checks out if its trivially fixable, and if not, adds the backport-requested label manually to the PR.

I think there must be some kind of automation that could help here, but you can see that in our process the adding of the label already means that the PR is known to not backport, so having a bot confirm that isn't necessary. Also, analyzing whether a PR backports requires first cherry-picking all the commits before it except for the semver-major, otherwise it will likely fail to cherry-pick standalone, but that's a false negative, it might just needs its deps to land first.

[addaleax]

I think we’re caught up to a “normal” level of pending backports now – thanks @joyeecheung and @BridgeAR for the backporting work!

I’ll close this as this particular issue has been resolved now, but we may want to spend some time thinking about how we deal with this kind of situation in the future.

[addaleax]

Reopening because this is starting to be a problem again.

[gireeshpunathil]

@addaleax - I reviewed the list of current items, and thought will spend some effort clearing it a bit. Looking at the oldest one, #25263 has a file (for example lib/internal/bootstrap/node.js) that has undergone more than 50 revisions in the master that has HAS NOT gone into v11.x branch, probably through 50+ PRs (I did not count). So does that mean that all those PRs need to be backported first for this one to be applied? Or am I looking things differently? Any guidance will be appreciated. thanks!

[addaleax]

@gireeshpunathil I’m not sure, @joyeecheung might be able to tell your more about the PR and what it conflicts with…

[targos]

Here is the list of commits that are waiting to be backported, in the order that they landed on master:

• [ddbb7d7777] - deps: cherry-pick 56f6a76 from upstream V8 (Ruben Bridgewater) #25269
• [270ffb0fa7] - src,lib: remove dead process.binding() code (Anna Henningsen) #25829
• [09a5f0252e] - process: move deprecation warning initialization into pre_execution.js (Joyee Cheung) #25825
• [c2d374fccc] - src: remove unused method in env.h (gengjiawen) #25934
• [84000835e2] - process: group main thread execution preparation code (Joyee Cheung) #26000
• [69714ab1c4] - process: normalize process.argv before user code execution (Joyee Cheung) #26000
• [ba4df925eb] - src: remove redundant void (gengjiawen) #26003

[lance]

Hi - I was poking around on some of these commits to see about helping out. However, in a lot of cases, it's not very simple. For example, Joyee's pull request #25263 landed after a semver-major commit (#24965). Teasing out Joyee's changes from those changes in the prior semver-major pull request is non-trivial. I don't mind a challenge, but am looking for a little guidance. If you were to attempt this backport, how would you do it?

[joyeecheung]

Forgot to mention: I landed the tool described #25403 (comment) in node-core-utils as git node backport: https://github.com/nodejs/node-core-utils/blob/master/docs/git-node.md#git-node-backport (it's on master, not released to npm yet). That should help figuring out the correct order to backport PRs.

I plan to work on a flag that detects semver-ness (which requires network access) when requested in the prompt for that tool before releasing it.

[joyeecheung]

If you were to attempt this backport, how would you do it?

I am not entirely sure why #24965 has to be semver-major but I think in this case #25263 has to be recreated on v11 rather than being backported using git magic. (It's easier to tell what needs to be done from https://github.com/nodejs/node/pull/25263/files?w=1 which strips the indentation changes)

[antsmartian]

@targos: #25352, #25481 backports are done. So it would be great if you could strike through (those are done), so that others can easily know what else to pick.

[targos]

I updated the list

[targos]

We're back to a good state. Thank you everyone who helped :)