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

Zlib: Update zlib to v1.2.13, intel-zlib to v1.2.13_jtk #84602

Merged
merged 5 commits into from
Apr 13, 2023

Conversation

GrabYourPitchforks
Copy link
Member

@GrabYourPitchforks GrabYourPitchforks commented Apr 11, 2023

We're getting reports of compliance tooling flagging our distribution of zlib. While there are no current CVEs affecting how we use zlib, upgrading to the latest version will silence these alerts and restore the compliance status.

Though the three PRs #84602, #84603, and #84604 are all related to zlib; they're each fully standalone and can be reviewed as isolated units.

@ghost
Copy link

ghost commented Apr 11, 2023

Tagging subscribers to this area: @dotnet/area-system-io-compression
See info in area-owners.md if you want to be subscribed.

Issue Details

We're getting reports of compliance tooling flagging our distribution of zlib. While there are no current CVEs affecting how we use zlib, upgrading to the latest version will silence these alerts and restore the compliance status.

Author: GrabYourPitchforks
Assignees: carlossanlop, ViktorHofer
Labels:

area-System.IO.Compression

Milestone: -

Copy link
Member

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, @GrabYourPitchforks.

Would it make sense to try to add System.IO.Compression tests that verify the changes in the latest zlib versions? If it's even possible.

Just left a question about a pdf file. Otherwise, LGTM assuming the CI passes.

"RepositoryUrl": "https://github.com/jtkukunas/zlib",
"CommitHash": "bf55d56b068467329f5a6f29bee31bc80d694023"
"RepositoryUrl": "https://github.com/intel/zlib",
"CommitHash": "6160a8f20c3626aec2f8b0fda5bf2e65bfe31781"
Copy link
Member

Choose a reason for hiding this comment

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

Glad to see the intel zlib is now under the intel org.

I can see that 6160a is the commit for the 1.2.13 release: https://github.com/intel/zlib/tags

v1.2.12
(21767c654d31d2dccdde4330529775c6c5fd5389)
v1.2.13
(04f42ceca40f73e2978b50e93806c2a18c1281fc)
Copy link
Member

Choose a reason for hiding this comment

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

I can see 04f4 is the latest release in madler/zlib: https://github.com/madler/zlib/releases

Copy link
Member

Choose a reason for hiding this comment

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

Do we need this in our repo?

Copy link
Member

Choose a reason for hiding this comment

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

The CI scripts, docs, tests etc. under src/native/external directory are unusued but keeping them intact is by design. We try not to modify the upstream tree, and when it is necessary, we update the corresponding src/native/external/{dependency name}-version.txt file (typically with a reference to upstream commit or reason of modification).

This approach makes it easier to keep track of changes and let us reproduce the subtree structure by reading the vertion.txt file without going through the git history of both (upstream and runtime) repos.

@GrabYourPitchforks
Copy link
Member Author

@carlossanlop @am11 Was the preference to keep the .pdf? It's a 19KB binary file that gets updated every release, where the only change is the footer which contains the date and version. I'd rather keep it out of our repo because it's not adding value.

I can update the -version.txt file to reflect its removal.

@carlossanlop
Copy link
Member

I'm ok either way, @GrabYourPitchforks .

@am11 raised a good point about bringing the contents of the zlib folder with as few modifications as possible.

You also raised the alternative of indicating the removal in version.txt, even though the file only weighs 19kb.

I'm more inclined to removing it, but I promise won't get mad if you decide to keep it.

@GrabYourPitchforks
Copy link
Member Author

Ok, I'll remove it for now, and we can always bring it back later if the removal ends up causing pain. Thanks all for the discussion. :)

@GrabYourPitchforks GrabYourPitchforks merged commit 6875ba0 into main Apr 13, 2023
@GrabYourPitchforks GrabYourPitchforks deleted the levib/zlib_upgrade branch April 13, 2023 00:58
@ghost ghost locked as resolved and limited conversation to collaborators May 13, 2023
@GrabYourPitchforks
Copy link
Member Author

/backport to release/7.0-staging

@github-actions github-actions bot unlocked this conversation Jul 26, 2023
@github-actions
Copy link
Contributor

Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/5672343630

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants