FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: add missing step for llhttp update by mhdawson · Pull Request #44136 · nodejs/node · GitHub

/ node Public

doc: add missing step for llhttp update - #44136

Closed
mhdawson wants to merge 1 commit into
nodejs:mainfrom
mhdawson:llhttp-isntructions
ConversationCommits (1)ChecksFiles changed
Closed

doc: add missing step for llhttp update#44136
mhdawson wants to merge 1 commit into
nodejs:mainfrom
mhdawson:llhttp-isntructions

Conversation

mhdawson commented Aug 4, 2022

Copy link
Copy Markdown
Member

Refs: #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

doc: add missing step for llhttp update
e41e990 
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>

nodejs-github-bot commented Aug 4, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/tsc

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Aug 4, 2022
mhdawson mentioned this pull request Aug 4, 2022
Closed
mcollina approved these changes Aug 4, 2022
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

richardlau reviewed Aug 4, 2022
View reviewed changes
Comment thread doc/contributing/maintaining-http.md
Comment on lines +90 to +91
* update the llhttp version number in the project line in CMakeLists.txt
to match that set in include/llhttp.h

richardlau Aug 4, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

richardlau Aug 4, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

mhdawson Aug 4, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

mhdawson Aug 19, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@mcollina, ping?

mhdawson Sep 7, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@mcollina ping ?

climba03003 Sep 9, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

ShogunPanda Sep 9, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

climba03003 Sep 9, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

ShogunPanda Sep 12, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

ShogunPanda Sep 13, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 approved these changes Aug 6, 2022
View reviewed changes

tniessen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

If this isn't resolved otherwise.

mcollina approved these changes Sep 7, 2022
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

ShogunPanda mentioned this pull request Sep 9, 2022
Merged

mhdawson commented Sep 13, 2022

Copy link
Copy Markdown
Member Author

@ShogunPanda, I'll try to pull the suggestions from another thread into this PR sometime this week so we can get it landed.

ShogunPanda mentioned this pull request Sep 15, 2022
Closed

ShogunPanda commented Sep 15, 2022

Copy link
Copy Markdown
Contributor

@mhdawson Please see #44652, it should replace this PR.

mhdawson commented Sep 15, 2022

Copy link
Copy Markdown
Member Author

@ShogunPanda thanks, closing this one.

mhdawson closed this Sep 15, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ShogunPanda ShogunPanda left review comments
richardlau richardlau left review comments
mcollina mcollina approved these changes
tniessen tniessen approved these changes
+1 more reviewer
climba03003 climba03003 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

doc Issues and PRs related to the documentations.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL