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

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

Open
UlisesGascon opened this issue Jan 9, 2024 · 7 comments
Open

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

UlisesGascon opened this issue Jan 9, 2024 · 7 comments

Comments

@UlisesGascon
Copy link
Member

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 nodejs/node#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 [email protected]. 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 [email protected]. 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

20.0.0 dependencies

Screenshot 2024-01-08 at 19 26 12

@targos
Copy link
Member

targos commented Jan 9, 2024

I wonder how nodejs/node#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
Copy link
Member

RafaelGSS commented Jan 9, 2024

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
Copy link
Member Author

I wonder how nodejs/node#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
Copy link
Member

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
Copy link

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
Copy link
Member Author

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 [email protected] was included in [email protected] 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.

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

6 participants
@ShogunPanda @ruyadorno @targos @UlisesGascon @RafaelGSS and others