Fix more warnings reported by CodeAnalysis #3393

heng-liu · 2020-05-14T15:19:24Z

Bug

Last working version:
How are we preventing it in future:

Fix

Details: fix more warnings raised by CodeAnalysis.FxCopAnalyzer
As string.Replace(), string.IndexOf(), string.Equals() have new overload methods in new TFM(what we target for xplat verification)
string.GetHashCode() won't be fixed.

Testing/Validation

Tests Added: No
Reason for not adding tests:
Validation:

zivkan

most of the changes you made are for APIs that exist in net472 and netstandrd2.0. Don't #if in those cases, make all TFMs use the StringComparison overload.

The new compiler constant and MSBuild property might not be needed.

zivkan · 2020-05-14T18:01:22Z

build/common.targets

+    <TFMsBeforeNet5 Condition=" '$(TargetFramework)' == 'net472' OR '$(TargetFramework)' == 'netstandard2.0' OR '$(TargetFramework)' == 'netcoreapp2.1'">true</TFMsBeforeNet5>
+    <TFMsBeforeNet5 Condition=" '$(TFMsBeforeNet5)' != 'true'">false</TFMsBeforeNet5>


you don't need the false condition. MSBuild only has string data types, not booleans.

For the condition, instead of comparing exact short tfm names, can you do:

'$(TargetFrameworkIdentifier)' == '.NETCoreApp' and [MSBuild]::VersionGreaterThanOrEquals($(TargetFrameworkVersion), '5.0'))

I am under the impression that some of the constants should be defined by the SDK. https://github.com/dotnet/designs/blob/master/accepted/2020/net5/net5.md#what-about-net-10

cc @dsplaisted

Not sure where in the SDK they are usually implemented.

Thanks. Fixed.

nkolev92 · 2020-05-14T20:09:19Z

src/NuGet.Core/NuGet.Packaging/PackageCreation/Authoring/PackageBuilder.cs

@@ -780,7 +780,11 @@ private HashSet<string> WriteFiles(ZipArchive package, HashSet<string> filesWith
                        }
                        else
                        {
+#if IS_NET5_OR_LATER


I thought the framework defines some constants. https://github.com/dotnet/designs/blob/master/accepted/2020/net5/net5.md#what-about-net-10

Thanks Nikolche!
I tried not to use NET5_0, as considering the forwards compatibility.
But it looks like the const only have backwards compatibility.
In order to make it easier to update code, especially when doing multi-targeting, we should make them additive, so that when targeting net6.0 both NET6_0 and NET5_0 are defined. The same applies to OS bindings.

So in that case, NET5_0 only should do it for us, correct?

Yes, you're right! NET5_0 should work in this case, even if we target net6.0 in future.

That is true, although I am not sure whether NET5_0 works yet. @sfoslund is working on that here: dotnet/sdk#11635

Tested just now, NET5_0 doesn't work yet.
Will define it by myself as a temporary workaround.

heng-liu · 2020-05-17T04:17:56Z

most of the changes you made are for APIs that exist in net472 and netstandrd2.0. Don't #if in those cases, make all TFMs use the StringComparison overload.

For String.Replace(String, String) and String.IndexOf(Char, StringComparison), the StringComparison overload exists in netcore(netcoreapp2.1 and above) , but not in net472.
Shall I change preprocessor from NET5_0 to IS_CORECLR? Thanks.

dsplaisted · 2020-05-18T17:02:49Z

most of the changes you made are for APIs that exist in net472 and netstandrd2.0. Don't #if in those cases, make all TFMs use the StringComparison overload.

For String.Replace(String, String) and String.IndexOf(Char, StringComparison), the StringComparison overload exists in netcore(netcoreapp2.1 and above) , but not in net472.
Shall I change preprocessor from NET5_0 to IS_CORECLR? Thanks.

You should be able to use NETCOREAPP. Its defined by the .NET SDK, and unlike NET5_0`, should already work.

heng-liu · 2020-05-18T17:39:42Z

Thanks @dsplaisted !
Chnaged to NETCOREAPP.

* fix string warnings raised by CodeAnalysis

* Signing: Support signature verification on .NET Core Remove 2nd N.B.T.P package (#2984) remove WorkaroundNetStandard target, change SDK for build from 3.0 preview to 3.1 stable (#3161) * remove WorkaroundNetStandard target, change SDK for build from 3.0 preview to 3.1 stable version, fix error; change LockSDKVersion default value from empty string to false; apply Linux workaround in my change Retarget to netcore5.0 (#3162) * retarget to netcore5.0; Linux script workaround need to apply for downloading SDK for build add pkcs and cng packages Temporary fix on patching SDK, add System.Security.Cryptography.Pkcs.dll and update deps.json, use dll from .nuget/packages Address PR comments implement add cert to store on Mac; display openssl version (#3166) Implement timestamp integrity verification; Implement an equivalent NativeCms; enable signing APIs and tests (#3168) * implement timestamp integrity verification; implement ManagedCms; enable signing APIs and tests fix xplat verification branch after rebase (#3191) Fixes: NuGet/Home#9012 check different cross-platform error messages for self-signed cert test (#3208) Fixes: NuGet/Home#8933 Xplat Signing/Verification : Fix broken tests for setting privatekey of a X509Certificate2 (#3195) use workaround to set private key for X509Certificate2 (netcore), change ToRSA to workaround for netcore(especially for non-windows) enable PathTooLongException test (#3216) Fixes: NuGet/Home#8920 enable TrustedSignerActionsProvider on netcore, along with tests (#3213) Fixes: NuGet/Home#8921 Enable SignedPackageIntegrityVerificationTests for netcore5.0 (#3220) Enable SignedPackageIntegrityVerificationTests for netcore, remove extra preprocessors Enable timestamp provider test and fix 3 broken tests (#3210) enable all unit tests; enable 8 more tests; enable timestampProviderTest; fix 3 broken tests in TimestampProviderTests Fix 4 tests in SignatureTrustAndValidityVerificationProviderTests (#3242) fix untrusted msg for different platforms fixed broken tests using utility methods (#3263) disable outdated tests (#3274) enable 4 tests on Mac (#3275) fix 5 broken tests in SignatureTrustAndValidityVerificationProviderTests, add workaround for Linux (#3252) fix broken test case ExecuteCommandAsync_WithAmbiguousMatch_ThrowsAsync (#3291) * Enable test ExecuteCommandAsync_WithMultiplePackagesAndInvalidCertificate_RaisesErrorsOnceAsync (#3342) * enable a test, change the comments; extend test limitation on Linux * Updated SDK version and removed the hardcoded version when reading from JSON file (#3372) * update Pkcs package to the preview3 version (#3382) * fix 2 warnings in CodeAnalysis (#3388) * fix 2 warnings in CodeAnalysis * disable dynamic revoke tests on Mac (#3373) * fix 1 test (#3371) * Fix more warnings reported by CodeAnalysis (#3393) * fix string warnings raised by CodeAnalysis * Retarget xplat verification to netcoreapp5.0 (#3386) * retarget netcoreapp5.0 (a workaround for net5.0) * fix all the rest warnings raised by code analysis (#3401) * Suppress CA1307 warning for string.GetHashCode() method (#3404) * Linux test failure due to generic exception (#3405) * add TFM netcoreapp5.0 to N.B.T.Console and N.P.Core projects (#3419) * add TFM netcoreapp5.0 to N.B.T.Console and N.P.Core projects * add PIN prompt for netcore (#3389) * Run dotnet nuget tests on multiple TFMs (#3387) * enabled mac assertions * Revert "enabled mac assertions" This reverts commit d8a52e2. * Enable test GetCertificateChain_WithUntrustedRoot_Throws on Mac (#3428) * run signing integration tests cross plat (#3415) * added GetNonce() to IRfc3161TimestampRequest interface (#3427) * disable test for netcore (#3445) * log Status and StatusInformation, fix 2 tests (#3268) * Disable verification during extraction for Mono, disable a test on Mono (#3451) * disable verification during extraction on mono,disable a mono test on Mac * adapt 2 newly added dotnet integration tests for multiple TFMs (#3460) * remove useless Sdk2x.targets * get TimestampTests working again (#3218) * get TimestampTests working again Fixes: NuGet/Home#8935 * change GetTimestampAsync into internal * fix runFuncTests.sh and InstallCLIforBuild.sh * address feedback * upgrade dotnet 5 to preview4 as preview5 is broken * Fix exception thrown in SigningTestFixture during disposing (#3464) Co-authored-by: Kartheek Penagamuri <52756182+kartheekp-ms@users.noreply.github.com> Co-authored-by: Andy Zivkovic <zivkan@users.noreply.github.com> Co-authored-by: Kat Marchán <kzm@zkat.tech>

heng-liu requested review from aortiz-msft, cristinamanum, dominoFire, donnie-msft, dtivel, kartheekp-ms, nkolev92, rrelyea, zivkan and zkat as code owners May 14, 2020 15:19

zivkan requested changes May 14, 2020

View reviewed changes

nkolev92 reviewed May 14, 2020

View reviewed changes

heng-liu force-pushed the dev-hengliu-xplat-fixMoreWarnings branch from 1966e02 to c5f62c2 Compare May 17, 2020 03:51

fix string warnings for net5 and later

f093ccc

heng-liu force-pushed the dev-hengliu-xplat-fixMoreWarnings branch from b54db0f to f093ccc Compare May 17, 2020 04:04

change preprocessor

4f98e81

zivkan approved these changes May 18, 2020

View reviewed changes

heng-liu merged commit 3eae65f into dev-feature-xplatVerification May 18, 2020

heng-liu deleted the dev-hengliu-xplat-fixMoreWarnings branch May 18, 2020 18:06

heng-liu added a commit that referenced this pull request May 20, 2020

Fix more warnings reported by CodeAnalysis (#3393)

3a2fedb

* fix string warnings raised by CodeAnalysis

heng-liu added a commit that referenced this pull request May 26, 2020

Fix more warnings reported by CodeAnalysis (#3393)

2221417

* fix string warnings raised by CodeAnalysis

heng-liu added a commit that referenced this pull request Jun 12, 2020

Fix more warnings reported by CodeAnalysis (#3393)

2bb699e

* fix string warnings raised by CodeAnalysis

heng-liu added a commit that referenced this pull request Jun 16, 2020

Fix more warnings reported by CodeAnalysis (#3393)

ff98f12

* fix string warnings raised by CodeAnalysis

heng-liu added a commit that referenced this pull request Jun 17, 2020

Fix more warnings reported by CodeAnalysis (#3393)

73e2ef1

* fix string warnings raised by CodeAnalysis

heng-liu added a commit that referenced this pull request Jun 19, 2020

Fix more warnings reported by CodeAnalysis (#3393)

679f549

* fix string warnings raised by CodeAnalysis

heng-liu added a commit that referenced this pull request Jun 19, 2020

Fix more warnings reported by CodeAnalysis (#3393)

27cb541

* fix string warnings raised by CodeAnalysis

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix more warnings reported by CodeAnalysis #3393

Fix more warnings reported by CodeAnalysis #3393

heng-liu commented May 14, 2020

zivkan left a comment •

edited

Loading

zivkan May 14, 2020

nkolev92 May 14, 2020

heng-liu May 17, 2020

nkolev92 May 14, 2020

heng-liu May 14, 2020

nkolev92 May 14, 2020

heng-liu May 14, 2020

dsplaisted May 14, 2020

heng-liu May 14, 2020

heng-liu commented May 17, 2020

dsplaisted commented May 18, 2020

heng-liu commented May 18, 2020

		<TFMsBeforeNet5 Condition=" '$(TargetFramework)' == 'net472' OR '$(TargetFramework)' == 'netstandard2.0' OR '$(TargetFramework)' == 'netcoreapp2.1'">true</TFMsBeforeNet5>
		<TFMsBeforeNet5 Condition=" '$(TFMsBeforeNet5)' != 'true'">false</TFMsBeforeNet5>

Fix more warnings reported by CodeAnalysis #3393

Fix more warnings reported by CodeAnalysis #3393

Conversation

heng-liu commented May 14, 2020

Bug

Fix

Testing/Validation

zivkan left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heng-liu commented May 17, 2020

dsplaisted commented May 18, 2020

heng-liu commented May 18, 2020

zivkan left a comment •

edited

Loading