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

[release/8.0] Remove implicit narrowing conversions from zlib #91962

Merged
merged 3 commits into from
Sep 14, 2023

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Sep 12, 2023

Backport of #91245 to release/8.0

/cc @GrabYourPitchforks

Customer Impact

No customer impact, in theory. The actual codegen for the targeted methods should be identical.

This allows us to resolve a binskim warning (we didn't enable warning code C4244) against our compression binary.

Testing

From #91245:

On my Windows x64 box, I validated the zlib-intel build under MSVC. Then I changed my local .cmake files to force usage of zlib instead of zlib-intel and validated that it built correctly under MSVC. On my WSL box, I modified my local .cmake files to force it to use these zlib sources instead of the OS's inbox zlib. I also injected an #error line into deflate.c just to confirm that clang was in fact using our local files.

Risk

Very low. The actual codegen should be identical. Testing across Linux (a special build which uses our copy of zlib) and Windows didn't show any regressions.

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

@ghost
Copy link

ghost commented Sep 12, 2023

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

Issue Details

Backport of #91245 to release/8.0

/cc @GrabYourPitchforks

Customer Impact

Testing

Risk

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

area-Infrastructure-libraries

Milestone: -

@ghost
Copy link

ghost commented Sep 12, 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

Backport of #91245 to release/8.0

/cc @GrabYourPitchforks

Customer Impact

No customer impact, in theory. The actual codegen for the targeted methods should be identical.

This allows us to resolve a binskim warning (we didn't enable warning code C4244) against our compression binary.

Testing

From #91245:

On my Windows x64 box, I validated the zlib-intel build under MSVC. Then I changed my local .cmake files to force usage of zlib instead of zlib-intel and validated that it built correctly under MSVC. On my WSL box, I modified my local .cmake files to force it to use these zlib sources instead of the OS's inbox zlib. I also injected an #error line into deflate.c just to confirm that clang was in fact using our local files.

Risk

Very low. The actual codegen should be identical. Testing across Linux (a special build which uses our copy of zlib) and Windows didn't show any regressions.

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

Servicing-consider, 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.

Signing off on behalf of @dotnet/area-system-io-compression

@ericstj / @jeffhandley / @artl93 do you approve?

Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

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

I support merging this in for .NET 8 to resolve a binskim warning before release.

@ericstj
Copy link
Member

ericstj commented Sep 14, 2023

Have we proposed the same changes to upstream ZLIBs? Patches are fine to unblock us, but we should do our best to let the upstream repo know about this request.

I'm in agreement on approving for 8.0

@carlossanlop
Copy link
Member

Have we proposed the same changes to upstream ZLIBs?

@ericstj Yes:

madler/zlib#852
intel/zlib#41

@carlossanlop
Copy link
Member

@artl93 can we get your seal of approval?

Copy link
Member

@artl93 artl93 left a comment

Choose a reason for hiding this comment

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

M2 Approved.

@carlossanlop carlossanlop merged commit dbd7885 into release/8.0 Sep 14, 2023
174 checks passed
@carlossanlop carlossanlop deleted the backport/pr-91245-to-release/8.0 branch September 14, 2023 20:19
@radical radical mentioned this pull request Sep 26, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Oct 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants