Repository navigation
Help wanted: Backporting to v11.x #25403
Description
Activity
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.
Reacted by Ruben BridgewaterThe numbers of open backport requests is now down by ~40%. Most still open ones do not seem to be trivial to backport though.
Reacted by antsmartianLooks 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.
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)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.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.ywould be. The meaning of the current backport labels is documented, somewhere, but I'm having trouble finding it./cc @BethGriggs
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.
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.
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.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 landbe unreasonable? I assume its deliberate that the actual final stage of posting theLanded in XXXXcomment and pushing the commit is done by hand, but if it was automated, theLanded 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?@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.
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".
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.
4 remaining items
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.
Reopening because this is starting to be a problem again.
@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 thathasHAS 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!@gireeshpunathil I’m not sure, @joyeecheung might be able to tell your more about the PR and what it conflicts with…
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 deadprocess.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
Reacted by antsmartian and Anna HenningsenReacted by Gireesh Punathil and Joyee Cheung- [
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-majorcommit (#24965). Teasing out Joyee's changes from those changes in the priorsemver-majorpull 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?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.
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)
I updated the list
Reacted by antsmartianWe're back to a good state. Thank you everyone who helped :)
Hi @nodejs/collaborators ! Backport requests to
v11.x-stagingare 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?