search

nodejs / Release

Node.js Release Working Group
4.04k stars 577 forks source link

problems with llhttp 9.x upgrades in v20.x #973

Open UlisesGascon opened 10 months ago

UlisesGascon commented 10 months ago

I created this issue for visibility, based on team slack discussion

When I was working on the release 20.11.0 proposal (step 10) I was checking the dependencies and I got an error with the llhttp version.

I got the version 8.1.3, but the expected is 9.1.3 based on https://github.com/nodejs/node/pull/50080 as was included in the proposal.

The thing is that version 8.1.3 does not exist at all, as the current version is 8.1.0 since Node@20.0.0. So, the issue is that LLHTTP_VERSION_MAJOR didn't got updated when cherry picking this commit, only the LLHTTP_VERSION_PATCH.

I added the label dont-land-on-v20 for now. But this error will occur again when landing new deps upgrades for llhttp in Node@20.x. So maybe we can work on a backport for the next release

20.11.0 Proposal dependencies ![Screenshot 2024-01-08 at 19 25 56](https://github.com/nodejs/Release/assets/5110813/65592918-9e30-49a7-8f82-dcf0c5f9104d)
20.0.0 dependencies ![Screenshot 2024-01-08 at 19 26 12](https://github.com/nodejs/Release/assets/5110813/5c80b60d-73ad-4921-9257-ed622a1c3950)
targos commented 10 months ago

I wonder how https://github.com/nodejs/node/pull/50080 could be cherry-picked cleanly to v20.x-staging.

This change at least should have generated a conflict: CleanShot 2024-01-09 at 11 24 42

RafaelGSS commented 10 months ago

cc: @shogunpanda

We should flag all v9.x updates as dont-land-on-v20.x and dont-land-on-v18.x. There's just one update for v8.x (v8.1.1) and we didn't receive an automatic update for that.

@UlisesGascon I think just dropping the commit from v20.x-staging and rebasing again should be enough.

UlisesGascon commented 10 months ago

I wonder how https://github.com/nodejs/node/pull/50080 could be cherry-picked cleanly to v20.x-staging.

I was wondering the same, but now that I tried to cherry pick it again in staging.. I realised that I introduced the regression when I was solving the conflict in the llhttp.h (before the holidays). Seems like I forgot to manually patch LLHTTP_VERSION_MAJOR value to match the current release version.

Screenshot 2024-01-09 at 14 46 19

@UlisesGascon I think just dropping the commit from v20.x-staging and rebasing again should be enough.

Yes, I dropped the commit and the versions in the binaries are as expected. So, as soon as the CI/CITGM is clear I will release the proposal.

Next steps

I will work in a backport for this, so it can be landed easier in the next release šŸ‘

RafaelGSS commented 10 months ago

I will work in a backport for this, so it can be landed easier in the next release šŸ‘

There's no need for a backport for that. It's a change that should exist only on v21.x and main since it's semver-major

ShogunPanda commented 10 months ago

I agree. llhttp 9.1.2 was a breaking change which should not belong to v20.x. Can you please clarify how 8.1.3 was created? Is any of the commit broken or what?

UlisesGascon commented 10 months ago

Can you please clarify how 8.1.3 was created? Is any of the commit broken or what?

Seems like the issue was due a manual regression that I introduced (see). As the llhttp@8.1.1 was included in Node@20.10.0 when I was working in the release I didn't update the major version reference value to 9 when solving the commit conflict, so the llhttp library version was wrong reflected as 8.1.3 instate of 9.1.3 as expected.