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

[v10.x] zlib: add brotli support #27681

Closed
wants to merge 6 commits into from
Closed

[v10.x] zlib: add brotli support #27681

wants to merge 6 commits into from

Conversation

addaleax
Copy link
Member

Backport of #24938 (plus the test fix in #25697). Merge conflicts were pretty minimal:

  • doc/api/errors.md: Neighbouring lines, no actual conflict
  • doc/api/zlib.md: Mismatches in comment casing
  • src/node_metadata.{cc,h}: Only exist since src: move version metadata into node_metadata{.h, .cc} #24774, which is not backported to v10.x and would be much more work to backport
  • src/node_zlib.cc: MaybeLocal vs Local
  • test/parallel/test-process-versions.js: Conflicts because of llhttp being added before brotli
  • test/parallel/test-zlib-write-after-flush.js: Mismatches in comment casing
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Hackzzila and others added 6 commits May 13, 2019 21:33
@Hackzzila @addaleax
deps: add brotli
20c4185 
Co-authored-by: Anna Henningsen <anna@addaleax.net>

PR-URL: nodejs#24938
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@Hackzzila @addaleax
deps,tools: update license-builder.sh and LICENSE
3e56dca 
PR-URL: nodejs#24938
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@addaleax
zlib: add brotli support
b46cfea 
Refs: nodejs#20458

Co-authored-by: Hackzzila <admin@hackzzila.com>

PR-URL: nodejs#24938
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@addaleax
benchmark,test: add brotli
dfc9f0c 
Co-authored-by: Hackzzila <admin@hackzzila.com>

PR-URL: nodejs#24938
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@addaleax
doc: add documentation for brotli support
1328bf7 
PR-URL: nodejs#24938
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@AdamMajer @addaleax
test: fix zlib-brotli output assumptions
4b4d9cb 
On different architectures, it's possible for the compression algorithm
to produce slightly different outputs. So, don't assume we will
always get the same compressed output on all architectures. Instead,
verify that the decompressing pre-compressed string functions
correctly.

Fixes: nodejs#25568

PR-URL: nodejs#25697
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax addaleax added zlib Issues and PRs related to the zlib subsystem. semver-minor PRs that contain new features and should be released in the next minor version. notable-change PRs with changes that should be highlighted in changelogs. v10.x brotli Issues and PRs related to the brotli dependency. labels May 13, 2019
@nodejs-github-bot
Copy link
Collaborator

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3507

@nodejs-github-bot nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label May 13, 2019
@addaleax
Copy link
Member Author

/cc @MylesBorins @nodejs/lts

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 13, 2019 •
edited by addaleax
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/23095/

(Edit: both failures exist on v10.x-staging without these changes as well)

MylesBorins
MylesBorins approved these changes May 14, 2019
View reviewed changes
Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

LGTM.

Seems good to land as failures in CI already exist on staging

MylesBorins pushed a commit that referenced this pull request May 16, 2019
@Hackzzila @MylesBorins
deps: add brotli
c5c4a21 
Co-authored-by: Anna Henningsen <anna@addaleax.net>

Backport-PR-URL: #27681
PR-URL: #24938
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
@Hackzzila @MylesBorins
deps,tools: update license-builder.sh and LICENSE
499ed9f 
Backport-PR-URL: #27681
PR-URL: #24938
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
@addaleax @MylesBorins
zlib: add brotli support
aab0da6 
Refs: #20458

Co-authored-by: Hackzzila <admin@hackzzila.com>

Backport-PR-URL: #27681
PR-URL: #24938
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
@addaleax @MylesBorins
benchmark,test: add brotli
15b43e3 
Co-authored-by: Hackzzila <admin@hackzzila.com>

Backport-PR-URL: #27681
PR-URL: #24938
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
@addaleax @MylesBorins
doc: add documentation for brotli support
4048e0d 
Backport-PR-URL: #27681
PR-URL: #24938
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
@AdamMajer @MylesBorins
test: fix zlib-brotli output assumptions
16478de 
On different architectures, it's possible for the compression algorithm
to produce slightly different outputs. So, don't assume we will
always get the same compressed output on all architectures. Instead,
verify that the decompressing pre-compressed string functions
correctly.

Fixes: #25568

Backport-PR-URL: #27681
PR-URL: #25697
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

landed in 16478de...ebe51a4

MylesBorins pushed a commit that referenced this pull request May 16, 2019
@Hackzzila @MylesBorins
deps: add brotli
a9c68a0 
Co-authored-by: Anna Henningsen <anna@addaleax.net>

Backport-PR-URL: #27681
PR-URL: #24938
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
@Hackzzila @MylesBorins
deps,tools: update license-builder.sh and LICENSE
ce6fec5 
Backport-PR-URL: #27681
PR-URL: #24938
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
@addaleax @MylesBorins
zlib: add brotli support
53e9c85 
Refs: #20458

Co-authored-by: Hackzzila <admin@hackzzila.com>

Backport-PR-URL: #27681
PR-URL: #24938
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
@addaleax @MylesBorins
benchmark,test: add brotli
c670358 
Co-authored-by: Hackzzila <admin@hackzzila.com>

Backport-PR-URL: #27681
PR-URL: #24938
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
@addaleax @MylesBorins
doc: add documentation for brotli support
93edf90 
Backport-PR-URL: #27681
PR-URL: #24938
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
@AdamMajer @MylesBorins
test: fix zlib-brotli output assumptions
3ca5f23 
On different architectures, it's possible for the compression algorithm
to produce slightly different outputs. So, don't assume we will
always get the same compressed output on all architectures. Instead,
verify that the decompressing pre-compressed string functions
correctly.

Fixes: #25568

Backport-PR-URL: #27681
PR-URL: #25697
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@matthewdouglas matthewdouglas mentioned this pull request Feb 25, 2020
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MylesBorins MylesBorins MylesBorins approved these changes

Assignees
No one assigned
Labels
brotli Issues and PRs related to the brotli dependency. meta Issues and PRs related to the general management of the project. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@addaleax @nodejs-github-bot @MylesBorins @Hackzzila @AdamMajer