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-ng patch for WD suppresion fixes #105523

Closed
wants to merge 3 commits into from

Conversation

carlossanlop
Copy link
Member

Follow up of #105433

Add patch that include the zlib-ng changes.

I also made a couple of minor changes to the code to make sure it matches both the patch and the upstream PR.

Upstream PR: zlib-ng/zlib-ng#1762

Copy link
Contributor

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

@jkotas
Copy link
Member

jkotas commented Jul 26, 2024

What's the value of the checked in patch? Why is it not good enough to have a link to a commit(s) to apply?

@carlossanlop
Copy link
Member Author

What's the value of the checked in patch? Why is it not good enough to have a link to a commit(s) to apply?

I noticed that if I go to https://github.com/dotnet/runtime/pull/105433.patch , the patch that gets generated contains the whole commit history from the PR, not just the final commit. That's unnecessary noise.

This proposed patch on the other hand was generated out of main, from the commit after I merged the PR.

It also contains the nits that I missed in the PR (and that AI am additionally fixing here).

@jkotas
Copy link
Member

jkotas commented Jul 26, 2024

I noticed that if I go to https://github.com/dotnet/runtime/pull/105433.patch , the patch that gets generated contains the whole commit history from the PR, not just the final commit.

If you do not like that, you can replace the link to the patch with the link to the commit: https://github.com/dotnet/runtime/commit/ecdb625035e0e3fb7c51e908713d96d2cb2080c8

We do not maintain the patch files for any other vendored libraries. We link to commits or PRs that provide the same information:

It also contains the nits that I missed in the PR

I would not bother with nits unless they are actual bugs. I expect you will get a bunch of feedback on the upstream PR and the upstreamed change will be different from our private patch. That's fine. We will pick it up during the next zlibng version bump.

@carlossanlop
Copy link
Member Author

I guess we can close this then. It's going to be temporary anyway, since we will eventually update to the zlib-ng release containing the final fix.

Just wanted to note that we did have patches for zlib and zlib-intel before, but I deleted them yesterday alongside the source code of zlib and zlib-intel. Here: https://github.com/dotnet/runtime/tree/release/9.0-preview7/src/native/external/patches

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants