Skip to content

Conversation

@mcollina
Copy link
Member

In two place we still used _stream_*, causing #36615 and potentially other issues.

Note that #36615 does not impact master for some independent reason.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@mcollinamcollina requested a review from ronagDecember 24, 2020 15:29
@nodejs-github-botnodejs-github-bot added the zlib Issues and PRs related to the zlib subsystem. label Dec 24, 2020
@mcollinamcollina added dont-land-on-v10.x request-ci Add this label to start a Jenkins CI on a PR. labels Dec 24, 2020
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 24, 2020
@nodejs-github-bot
Copy link
Collaborator

@mcollina
Copy link
MemberAuthor

@nodejs/releasers @nodejs/tsc this should go to into one of the next v14 releases.

Copy link
Member

@richardlaurichardlau left a comment

Choose a reason for hiding this comment

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

LGTM.

Do we need to add a regression test to catch the breakage (I'm assuming no existing test caught #36615)?

@mcollina
Copy link
MemberAuthor

@richardlau I've added a test

@targos
Copy link
Member

@mcollina can you fix the lint error?

@mcollinamcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 27, 2020
@mcollina
Copy link
MemberAuthor

@targos done

@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 27, 2020
@nodejs-github-bot
Copy link
Collaborator

@mcollinamcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 28, 2020
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 28, 2020
@nodejs-github-bot
Copy link
Collaborator

@targostargos linked an issue Dec 28, 2020 that may be closed by this pull request
@mcollinamcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 28, 2020
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 28, 2020
@github-actions
Copy link
Contributor

Commit Queue failed
- Loading data for nodejs/node/pull/36618 ✔ Done loading data for nodejs/node/pull/36618 ----------------------------------- PR info ------------------------------------ Title stream,zlib: do not use _stream_* anymore. (#36618) Author Matteo Collina (@mcollina) Branch mcollina:always-use-stream -> nodejs:master Labels dont-land-on-v10.x, dont-land-on-v12.x, lts-watch-v14.x, zlib Commits 1 - stream,zlib: do not use _stream_* anymore. Committers 1 - Matteo Collina PR-URL: https://github.com/nodejs/node/pull/36618 Reviewed-By: Robert Nagy Reviewed-By: Rich Trott Reviewed-By: Richard Lau Reviewed-By: Luigi Pinca Reviewed-By: Benjamin Gruenbaum Reviewed-By: Michaël Zasso ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/36618 Reviewed-By: Robert Nagy Reviewed-By: Rich Trott Reviewed-By: Richard Lau Reviewed-By: Luigi Pinca Reviewed-By: Benjamin Gruenbaum Reviewed-By: Michaël Zasso -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last review: ⚠ - stream,zlib: do not use _stream_* anymore. ✔ Last GitHub Actions successful ℹ Last Full PR CI on 2020-12-28T17:53:03Z: https://ci.nodejs.org/job/node-test-pull-request/35126/ - Querying data for job/node-test-pull-request/35126/ ✔ Build data downloaded ✔ Last Jenkins CI successful ℹ This PR was created on Thu, 24 Dec 2020 15:29:15 GMT ✔ Approvals: 6 ✔ - Robert Nagy (@ronag): https://github.com/nodejs/node/pull/36618#pullrequestreview-558672980 ✔ - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/36618#pullrequestreview-558684130 ✔ - Richard Lau (@richardlau): https://github.com/nodejs/node/pull/36618#pullrequestreview-558965185 ✔ - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/36618#pullrequestreview-558695858 ✔ - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/36618#pullrequestreview-558712700 ✔ - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/36618#pullrequestreview-559085773 -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/449376978

@mcollina
Copy link
MemberAuthor

May I get a last approval @targos?

@mcollinamcollina added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Dec 28, 2020
@github-actionsgithub-actionsbot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 28, 2020
@github-actions
Copy link
Contributor

Commit Queue failed
- Loading data for nodejs/node/pull/36618 ✔ Done loading data for nodejs/node/pull/36618 ----------------------------------- PR info ------------------------------------ Title stream,zlib: do not use _stream_* anymore. (#36618) Author Matteo Collina (@mcollina) Branch mcollina:always-use-stream -> nodejs:master Labels dont-land-on-v10.x, dont-land-on-v12.x, lts-watch-v14.x, zlib Commits 1 - stream,zlib: do not use _stream_* anymore. Committers 1 - Matteo Collina PR-URL: https://github.com/nodejs/node/pull/36618 Reviewed-By: Robert Nagy Reviewed-By: Rich Trott Reviewed-By: Richard Lau Reviewed-By: Luigi Pinca Reviewed-By: Benjamin Gruenbaum Reviewed-By: Michaël Zasso ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/36618 Reviewed-By: Robert Nagy Reviewed-By: Rich Trott Reviewed-By: Richard Lau Reviewed-By: Luigi Pinca Reviewed-By: Benjamin Gruenbaum Reviewed-By: Michaël Zasso -------------------------------------------------------------------------------- ✔ Last GitHub Actions successful ℹ Last Full PR CI on 2020-12-28T19:16:44Z: https://ci.nodejs.org/job/node-test-pull-request/35126/ - Querying data for job/node-test-pull-request/35126/ ✔ Build data downloaded ✔ Last Jenkins CI successful ℹ This PR was created on Thu, 24 Dec 2020 15:29:15 GMT ✔ Approvals: 6 ✔ - Robert Nagy (@ronag): https://github.com/nodejs/node/pull/36618#pullrequestreview-558672980 ✔ - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/36618#pullrequestreview-558684130 ✔ - Richard Lau (@richardlau): https://github.com/nodejs/node/pull/36618#pullrequestreview-558965185 ✔ - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/36618#pullrequestreview-558695858 ✔ - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/36618#pullrequestreview-558712700 ✔ - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/36618#pullrequestreview-559319170 -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/master up to date... From https://github.com/nodejs/node * branch master -> FETCH_HEAD ✔ origin/master is now up-to-date - Downloading patch for 36618 From https://github.com/nodejs/node * branch refs/pull/36618/merge -> FETCH_HEAD ✔ Fetched commits as 053874939a60..be22d247cfc8 -------------------------------------------------------------------------------- [master ec57bf8288] stream,zlib: do not use _stream_* anymore. Author: Matteo Collina Date: Thu Dec 24 16:25:53 2020 +0100 3 files changed, 16 insertions(+), 3 deletions(-) create mode 100644 test/parallel/test-zlib-no-stream.js ✔ Patches applied -------------------------------------------------------------------------------- --------------------------------- New Message ---------------------------------- stream,zlib: do not use _stream_* anymore. 

PR-URL: #36618
Reviewed-By: Robert Nagy [email protected]
Reviewed-By: Rich Trott [email protected]
Reviewed-By: Richard Lau [email protected]
Reviewed-By: Luigi Pinca [email protected]
Reviewed-By: Benjamin Gruenbaum [email protected]
Reviewed-By: Michaël Zasso [email protected]

[master 83c0916250] stream,zlib: do not use stream* anymore.
Author: Matteo Collina [email protected]
Date: Thu Dec 24 16:25:53 2020 +0100
3 files changed, 16 insertions(+), 3 deletions(-)
create mode 100644 test/parallel/test-zlib-no-stream.js
✖ 83c0916250fb078d45eff18e06b7159eff4f69e4
✔ 0:0 skipping fixes-url fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 1:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✔ 0:0 valid subsystems subsystem
✖ 0:42 Do not use punctuation at end of title. title-format
✔ 0:0 Title is <= 50 columns. title-length

ℹ Please fix the commit message and try again.

https://github.com/nodejs/node/actions/runs/449646494

PR-URL: nodejs#36618 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
@aduh95
Copy link
Contributor

Landed in e57d8af

@aduh95aduh95 merged commit e57d8af into nodejs:masterDec 29, 2020
danielleadams pushed a commit that referenced this pull request Jan 12, 2021
PR-URL: #36618 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
@danielleadamsdanielleadams mentioned this pull request Jan 12, 2021
BethGriggs pushed a commit that referenced this pull request Jan 26, 2021
PR-URL: #36618 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
BethGriggs added a commit that referenced this pull request Jan 26, 2021
Notable changes: - **deps**: upgrade npm to 6.14.11 (Darcy Clarke) [#36838](#36838) - **stream,zlib**: do not use \_stream\_\* anymore (Matteo Collina) [#36618](#36618) PR-URL: TODO
@BethGriggsBethGriggs mentioned this pull request Jan 26, 2021
BethGriggs added a commit that referenced this pull request Jan 26, 2021
Notable changes: - **deps**: upgrade npm to 6.14.11 (Darcy Clarke) [#36838](#36838) - **stream,zlib**: do not use \_stream\_\* anymore (Matteo Collina) [#36618](#36618) PR-URL: #37074
BethGriggs added a commit that referenced this pull request Jan 27, 2021
Notable changes: - **deps**: upgrade npm to 6.14.11 (Darcy Clarke) [#36838](#36838) - **stream,zlib**: do not use \_stream\_\* anymore (Matteo Collina) [#36618](#36618) PR-URL: #37074
BethGriggs pushed a commit that referenced this pull request Jan 28, 2021
PR-URL: #36618 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
BethGriggs added a commit that referenced this pull request Feb 5, 2021
Notable changes: - **deps**: upgrade npm to 6.14.11 (Ruy Adorno) (#37173) - **stream,zlib**: do not use \_stream\_\* anymore (Matteo Collina) (#36618) PR-URL: #37074
BethGriggs added a commit that referenced this pull request Feb 8, 2021
Notable changes: - **deps**: - upgrade npm to 6.14.11 (Ruy Adorno) (#37173) - V8: backport dfcf1e86fac0 (Michaël Zasso) (#37245) - Note: Node.js is not believed to be vulnerable to CVE-2021-21148. - **stream,zlib**: do not use \_stream\_\* anymore (Matteo Collina) (#36618) PR-URL: #37074
BethGriggs added a commit that referenced this pull request Feb 8, 2021
Notable changes: - **deps**: - upgrade npm to 6.14.11 (Ruy Adorno) (#37173) - V8: backport dfcf1e86fac0 (Michaël Zasso) (#37245) - Note: Node.js is not believed to be vulnerable to CVE-2021-21148. - **stream,zlib**: do not use \_stream\_\* anymore (Matteo Collina) (#36618) PR-URL: #37074
BethGriggs added a commit that referenced this pull request Feb 9, 2021
Notable changes: - **deps**: - upgrade npm to 6.14.11 (Ruy Adorno) (#37173) - V8: backport dfcf1e86fac0 (Michaël Zasso) (#37245) - Note: Node.js is not believed to be vulnerable to CVE-2021-21148. - **stream,zlib**: do not use \_stream\_\* anymore (Matteo Collina) (#36618) PR-URL: #37074
@targostargos removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 5, 2021
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

zlibIssues and PRs related to the zlib subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

gunzipSync DOA in Node 14.15.2

10 participants

@mcollina@nodejs-github-bot@targos@aduh95@Trott@benjamingr@lpinca@ronag@richardlau@BethGriggs