Skip to content

nodejs*: remove http-parser dependency#439353

Merged
emilazy merged 1 commit intoNixOS:stagingfrom
pyrox0:fix/nodejs-no-http-parser
Sep 13, 2025
Merged

nodejs*: remove http-parser dependency#439353
emilazy merged 1 commit intoNixOS:stagingfrom
pyrox0:fix/nodejs-no-http-parser

Conversation

@pyrox0
Copy link
Member

@pyrox0 pyrox0 commented Sep 1, 2025

http-parser has been unmaintained for a long time, and is marked to only be enabled with nodejs versions older than 11.4, which was released back in 2018, and the 11.x series was made EOL around that same time. Therefore, there is absolutely no need to keep this around.

part of #317263

Things done

  • Built on platform:
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested, as applicable:
  • Ran nixpkgs-review on this PR. See nixpkgs-review usage.
  • Tested basic functionality of all binary files, usually in ./result/bin/.
  • Nixpkgs Release Notes
    • Package update: when the change is major or breaking.
  • NixOS Release Notes
    • Module addition: when adding a new NixOS module.
    • Module update: when the change is significant.
  • Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

Add a 👍 reaction to pull requests you find important.

@pyrox0 pyrox0 mentioned this pull request Sep 1, 2025
Closed
14 tasks
@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 6.topic: nodejs Node.js is a free, open-source, cross-platform JavaScript runtime environment labels Sep 1, 2025
@nix-owners nix-owners bot requested a review from aduh95 September 1, 2025 22:16
aduh95
aduh95 reviewed Sep 2, 2025
View reviewed changes
@pyrox0
nodejs*: remove http-parser dependency
d566fed 
http-parser has been unmaintained for a long time, and is marked to only
be enabled with nodejs versions older than 11.4, which was released back
in 2018, and the 11.x series was made EOL around that same time.
Therefore, there is absolutely no need to keep this around.
@pyrox0 pyrox0 force-pushed the fix/nodejs-no-http-parser branch from f9ca557 to d566fed Compare September 3, 2025 16:21
@nixpkgs-ci nixpkgs-ci bot added 12.approvals: 1 This PR was reviewed and approved by one person. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages. labels Sep 3, 2025
@phanirithvij phanirithvij mentioned this pull request Sep 4, 2025
Closed
12 tasks
@phanirithvij
Copy link
Member

phanirithvij commented Sep 4, 2025

do we not need llhttp?

see the pr 252068 I linked above and llhttp references in nodejs

@aduh95
Copy link
Contributor

aduh95 commented Sep 4, 2025 •
edited
Loading

do we not need llhttp?

see the pr 252068 I linked above and llhttp references in nodejs

I have #401454 which should address this, but it would need #439939 and #439731 to land first

@pyrox0
Copy link
Member Author

pyrox0 commented Sep 4, 2025 •
edited
Loading

do we not need llhttp?

see the pr 252068 I linked above and llhttp references in nodejs

They have a vendored copy that is used if no shared library is provided. See https://github.com/nodejs/node/tree/main/deps/llhttp

@fabianhjr fabianhjr added the 1.severity: security Issues which raise a security issue, or PRs that fix one label Sep 8, 2025
@fabianhjr
Copy link
Member

fabianhjr commented Sep 8, 2025

Small nitpick, due to the commit name ofborg wasn't able to build any version of nodejs

@fabianhjr
Copy link
Member

fabianhjr commented Sep 8, 2025

Would be better to have/use the default version, eg nodejs: remove http-parser dependency without the asterisk

emilazy
emilazy approved these changes Sep 13, 2025
View reviewed changes
Copy link
Member

@emilazy emilazy left a comment

Choose a reason for hiding this comment

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

Don’t think the commit message nit is blocking here; it’s fairly common to use * in this way.

@emilazy emilazy merged commit 0daddff into NixOS:staging Sep 13, 2025
33 of 34 checks passed
@emilazy
Copy link
Member

emilazy commented Sep 13, 2025

(Not that it’s not good to get ofborg builds where possible, but I feel confident in this change given the maintainer approval.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aduh95 aduh95 aduh95 approved these changes

@emilazy emilazy emilazy approved these changes

Assignees

No one assigned

Labels

1.severity: security Issues which raise a security issue, or PRs that fix one 6.topic: nodejs Node.js is a free, open-source, cross-platform JavaScript runtime environment 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 12.approvals: 1 This PR was reviewed and approved by one person. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@pyrox0 @phanirithvij @aduh95 @fabianhjr @emilazy