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.

Sign up for GitHub

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

Jump to bottom

doc: add missing step for llhttp update #44136

Closed
wants to merge 1 commit into from
Closed

doc: add missing step for llhttp update #44136

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions doc/contributing/maintaining-http.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ In order to update Node.js with a new version of llhttp:
* copy the contents of the `release` directory from the directory you
checked llhttp out to
[deps/llhttp](https://github.com/nodejs/node/tree/HEAD/deps/llhttp)
* update the llhttp version number in the project line in CMakeLists.txt
to match that set in include/llhttp.h
Comment on lines +90 to +91
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised we need to do this. AFAICT this should have been updated in the make release step from llhttp: https://github.com/nodejs/llhttp/blob/da60b0bb00b46f0ac58ea48506c6fb6c1b5c112c/Makefile#L54
which should write an updated version of https://github.com/nodejs/llhttp/blob/main/CMakeLists.txt into the release directory.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah the TAG stuff looks new nodejs/llhttp#170.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That does makes sense. If that will be auto updated then we won't need this update. @mcollina is that correct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcollina, ping?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcollina ping ?

Copy link
Contributor

@climba03003 climba03003 Sep 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the step that need to change is line 86.

From
* run `make release` in the directory that you checked out llhttp.
to (manually)
* run `TAG=<version> make release` in the directory that you checked out llhttp.
or (automatic)
* run ``TAG=`node -e \"process.stdout.write(require('./package').version)\"` make release`` in the directory that you checked out llhttp.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is that if llhttp is properly released and downloaded this should not be done at all. Whoever is importing llhttp should just download it from GitHub and copy over.

Let me figure it out.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is that if llhttp is properly released and downloaded this should not be done at all.

If the step is about downloading, which will be the tag release/vx.y.z.
For example, https://github.com/nodejs/llhttp/releases/tag/release%2Fv6.0.9

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly. One should not have to download llhttp source and compile locally since we release from GitHub now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side node: I've just updated llhttp to require the RELEASE variable when running releases command manually, so now CMakeList.txt, llhttp.h and package.json should all report the same version.


It should look like the following:

Expand Down