doc: add missing step for llhttp update#44136
Conversation
Refs: nodejs#43946 - Add instructions on updating llhttp version in file maintained only in Node.js repository as it needs to be kept in sync with what is copied from the llhttp repository when an update is made. Signed-off-by: Michael Dawson <mdawson@devrus.com>
|
Review requested:
|
| * update the llhttp version number in the project line in CMakeLists.txt | ||
| to match that set in include/llhttp.h |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That does makes sense. If that will be auto updated then we won't need this update. @mcollina is that correct?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Exactly. One should not have to download llhttp source and compile locally since we release from GitHub now.
There was a problem hiding this comment.
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.
tniessen
left a comment
tniessen
left a comment
There was a problem hiding this comment.
If this isn't resolved otherwise.
|
@ShogunPanda, I'll try to pull the suggestions from another thread into this PR sometime this week so we can get it landed. |
|
@ShogunPanda thanks, closing this one. |
7 participants
Refs: #43946
file maintained only in Node.js repository as it
needs to be kept in sync with what is copied from
the llhttp repository when an update is made.
Signed-off-by: Michael Dawson mdawson@devrus.com