-
Notifications
You must be signed in to change notification settings - Fork 694
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
Disable CA2022 errors #5738
Disable CA2022 errors #5738
Conversation
@nkolev92 can we get checks running for this PR? |
@@ -104,7 +104,9 @@ public static Stream OpenPackageSignatureFileStream(BinaryReader reader) | |||
var buffer = new byte[localFileHeader.UncompressedSize]; | |||
|
|||
reader.BaseStream.Seek(offsetToData, SeekOrigin.Begin); | |||
#pragma warning disable CA2022 // Avoid inexact read |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like a false positive in the CA2022 analyzer has been fixed in dotnet/roslyn-analyzers#7268. Do we still need these suppressions? Is this a case where installing the .NET SDK from daily builds, which contains analyzer fixes, would not raise an error in NuGet.Client?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. False positive fix is for SystemIOMemoryStream
, SystemIOUnmanagedMemoryStream
only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the line at https://github.com/NuGet/NuGet.Client/blob/dev/build/DotNetSdkVersions.txt#L2 to -Channel 9.0 -Quality daily
and then executed the configure.ps1 script locally. This change resulted in the installation of the 9.0.100-preview.4.24209.7
.NET SDK. After trying to build the repository locally, I noticed no CA2022 errors.
Would it be possible to try reproducing the CA2022 errors using the latest daily build version of the .NET SDK 9.0.1xx?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's good. The problem is that we need pick up a new SDK in VMR to avoid this errors. We will re-bootstrap after Preview 4 ships, in May.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will re-bootstrap after Preview 4 ships, in May.
Does this mean that we should consider this PR now and revert the changes once Preview 4 ships in May?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a plan and a tracking issue to revert these suppression in all affected repos: dotnet/source-build#4322
Fixes: NuGet/Home#13361
Backport of a patch from: dotnet/installer#19145