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.

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

deps: update zlib to upstream 926ac23 #44412

Closed
wants to merge 9 commits into from

Conversation

RafaelGSS
Copy link
Member

Updated as described in doc/contributing/maintaining-zlib.md.

Attempt to solve: nodejs/security-wg#824

@nodejs-github-bot nodejs-github-bot added dependencies Pull requests that update a dependency file. needs-ci PRs that need a full CI run. zlib Issues and PRs related to the zlib subsystem. labels Aug 26, 2022
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

RSLGTM

@targos
Copy link
Member

targos commented Aug 29, 2022

As I said in #44254 (comment), we have to adapt zlib.gyp somehow.

@RafaelGSS
Copy link
Member Author

As I said in #44254 (comment), we have to adapt zlib.gyp somehow.

Yeah, I'll try to work on it this week.

@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 2, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 2, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@RaisinTen RaisinTen added the needs-benchmark-ci PR that need a benchmark CI run. label Sep 4, 2022
@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 6, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 6, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@targos
Copy link
Member

targos commented Sep 7, 2022

Don't we need ab587ca anymore?

@RafaelGSS
Copy link
Member Author

Don't we need ab587ca anymore?

fill_window_sse.c was removed. The chunkcopy still exists but the failing test doesn't seem related to this file.

@targos
Copy link
Member

targos commented Sep 7, 2022

It's not related to the failing test. This commit fixed #35629

@RafaelGSS
Copy link
Member Author

I'll trigger the build again just to see if I get a different build output. The last ones don't show anything relevant to me. I've set up a Windows x64 machine to see if I can reproduce this behaviour, but didn't work (it's building without issues).

@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 8, 2022
@@ -18,6 +18,7 @@
'adler32.c',
'compress.c',
'contrib/optimizations/insert_string.h',
'cpu_features.c',
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are adding cpu_features.c to sources here already, is there any need to re-add it to the sources below? I guess we could treat the cpu_features.h file too in the same way.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that makes sense. Look afb64b0

@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 31, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 31, 2022
@nodejs-github-bot
Copy link
Collaborator

Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

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

LGTM

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@baparham
Copy link

baparham commented Nov 7, 2022

FWIW upstream (chromium zlib fork) just landed the update to canonical zlib 1.2.13 in https://chromium.googlesource.com/chromium/src/+/e2e230364bcf8fe55dde5dca8310da6906ae64ad

Since this isn't merged in yet, it would make sense to push for upgrading Node.js to this latest version. Or does it make more sense to do it in two stages?

@nodejs-github-bot
Copy link
Collaborator

@RafaelGSS
Copy link
Member Author

RafaelGSS commented Nov 9, 2022

FWIW upstream (chromium zlib fork) just landed the update to canonical zlib 1.2.13 in https://chromium.googlesource.com/chromium/src/+/e2e230364bcf8fe55dde5dca8310da6906ae64ad

Since this isn't merged in yet, it would make sense to push for upgrading Node.js to this latest version. Or does it make more sense to do it in two stages?

I prefer to do it in two stages, considering the fact this PR changes the .gyp files.


FWIW All the CI failures are non-related to the PR (no space left, windows race condition bug , and so forth)

@lpinca
Copy link
Member

lpinca commented Nov 9, 2022

I've opened #45387 for a direct update to upstream 8bbd6c31.

@RafaelGSS
Copy link
Member Author

Closing in favor of #45387

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file. needs-benchmark-ci PR that need a benchmark CI run. needs-ci PRs that need a full CI run. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.