Disable analyzers only when building in VS #34848

captainsafia · 2021-07-29T18:51:48Z

This check was causing analyzers not to run when building on the CLI. Updating to only apply for VS builds.

BrennanConroy · 2021-07-29T19:43:31Z

Caught one!

src/Servers/Kestrel/Core/src/MinDataRate.cs(47,32): error RS0016: (NETCORE_ENGINEERING_TELEMETRY=Build) Symbol 'ToString' is not part of the declared API.

halter73

I think we just add MinDataRate.ToString() to the PublicApi.Unshipped so we don't have to revert #34680. @davidfowl

I guess we can mark this PR api-ready-for-review and look at it after it's merged. I can't imagine there being any pushback.

JunTaoLuo

I think we should get this in asap so we don't continue to "accidentally modify APIs"

dougbu · 2021-07-29T22:01:03Z

I think we should get this in asap so we don't continue to "accidentally modify APIs"

Agreed. Fix the missing ToString() method in VS (16.x, not 17.x 😺) and be done w/ it. Use admin privs to shorten the window for busted stuff

pranavkm · 2021-07-29T23:40:31Z

Directory.Build.props

@@ -15,7 15,7 @@
 <AnalysisMode>Default</AnalysisMode>
 </PropertyGroup>

- <PropertyGroup Condition=" '$(VisualStudioVersion)' == '17.0' ">
+ <PropertyGroup Condition=" '$(BuildingInsideVisualStudio)' == 'true' AND '$(VisualStudioVersion)' == '17.0' ">


Could someone remind me why we did this? Is this because of all of the failures to load the analyzer dll?

Yes. #34664 And I didn't verify the analyzers still ran on builds outside of VS 😞

nit: Remembered users could run desktop msbuild in our projects even if we don't recommend it.

Suggested change

<PropertyGroup Condition=" '$(BuildingInsideVisualStudio)' == 'true' AND '$(VisualStudioVersion)' == '17.0' ">

<PropertyGroup Condition=" '$(MSBuildRuntimeType)' != 'Core' AND '$(VisualStudioVersion)' == '17.0' ">

dougbu · 2021-07-29T23:45:08Z

@pranavkm please see https://teams.microsoft.com/l/message/19:[email protected]/1626473819862?tenantId=72f988bf-86f1-41af-91ab-2d7cd011db47&groupId=fdff90ed-0b3b-4caa-a30a-efb4dd47665f&parentMessageId=1626473819862&teamName=DotNet&channelName=ASP.NET Build&createdTime=1626473819862

dougbu

@dotnet/aspnet-build please double-check this then merge w/o waiting for CI. This should stop the bleeding in subsequent PRs though we need to be careful w/ PRs already marked as successful.

dougbu · 2021-07-30T00:09:34Z

@dotnet/aspnet-build never mind on the admin squash. Build failures are real and look like additional API changes as well as a new non-MIT header. ☹️

And I didn't verify the analyzers still ran on builds outside of VS 😞

@halter73 counter-intuitively, $(VisualStudioVersion) is 17.0 when building in dotnet msbuild with the current SDK. No way you could have predicted that.

@dsplaisted @rainersigwald @wli3 is that property setting coming from the SDK itself❔ If not, where❔ I expected no property setting outside desktop msbuild.

@captainsafia @BrennanConroy @JunTaoLuo and others playing in this PR: Have a look at other Conditions using $(VisualStudioVersion), especially the one in eng/targets/CSharp.Common.targets. Ignore the *.vcxproj files and src/Servers/IIS/build/Build.Settings because they're used only in native builds whether inside VS or not.

dougbu · 2021-07-30T00:16:06Z

/fyi @davidfowl one non-MIT header (in WebApplicationServiceCollection.cs) comes from d434b1c.

/fyi @javiercn and @JunTaoLuo the other non-MIT header (in ManifestStaticWebAssetsFileProviderTests.cs) comes from 014df77. Downside of the CI not enforcing comments on the PR…

captainsafia · 2021-07-30T00:26:15Z

Have a look at other Conditions using $(VisualStudioVersion), especially the one in eng/targets/CSharp.Common.targets

Since this is part of the same workaround to disable loading analyzers in certain versions of VS, I think it's fine to make this conditional on the build happening in VS too. Not sure if it'll actually cause any tangible warnings in the CI but we'll see.

…gurationExtensions

captainsafia · 2021-07-30T01:14:15Z

@dotnet/aspnet-build Ooooook. Looks like the build legs are starting to go green. I think this is good to merge at your discretion.

dougbu

Let's hope this beats other API changes and file additions 😀

dougbu · 2021-07-30T01:59:35Z

Directory.Build.props

@@ -15,7 15,7 @@
 <AnalysisMode>Default</AnalysisMode>
 </PropertyGroup>

- <PropertyGroup Condition=" '$(VisualStudioVersion)' == '17.0' ">
+ <PropertyGroup Condition=" '$(BuildingInsideVisualStudio)' == 'true' AND '$(VisualStudioVersion)' == '17.0' ">


nit: Remembered users could run desktop msbuild in our projects even if we don't recommend it.

Suggested change

<PropertyGroup Condition=" '$(BuildingInsideVisualStudio)' == 'true' AND '$(VisualStudioVersion)' == '17.0' ">

<PropertyGroup Condition=" '$(MSBuildRuntimeType)' != 'Core' AND '$(VisualStudioVersion)' == '17.0' ">

dougbu · 2021-07-30T02:01:24Z

eng/targets/CSharp.Common.targets

@@ -36,7 36,7 @@

 <!-- Enable .NET code style analysis during build for src projects. -->
 <!-- Workaround bug where turning this on produces warnings in VS -->
- <EnforceCodeStyleInBuild Condition="'$(VisualStudioVersion)' &lt; '17.0'">false</EnforceCodeStyleInBuild>
+ <EnforceCodeStyleInBuild Condition="'$(BuildingInsideVisualStudio)' == 'true' AND '$(VisualStudioVersion)' &lt; '17.0'">false</EnforceCodeStyleInBuild>


same nit:

Suggested change

<EnforceCodeStyleInBuild Condition="'$(BuildingInsideVisualStudio)' == 'true' AND '$(VisualStudioVersion)' < '17.0'">false</EnforceCodeStyleInBuild>

<EnforceCodeStyleInBuild Condition=" '$(MSBuildRuntimeType)' != 'Core' AND '$(VisualStudioVersion)' < '17.0' ">false</EnforceCodeStyleInBuild>

Separately have we confirmed < works with strings that look like floating-point numbers❔ @rainersigwald❔

it does but consider using the newish version comparison property functions

dougbu · 2021-07-30T02:13:38Z

Ick Components.E2ETest has not been reliable recently ☹️ /fyi @JunTaoLuo if you're still wrapping up blops

dougbu · 2021-07-30T02:15:18Z

@captainsafia please get this in w/ your admin privs if the test failure is all that goes wrong in this build. No need to wait for Helix jobs for example and don't bother retrying the failed 'Test: Windows Server 2016 x64' job.

captainsafia · 2021-07-30T02:38:38Z

I think this is safe to merge. There's a few build legs that are still queued to run (macOS arm64, Linux arm64) but I think it is safe to merge.

I'll follow up to feedback and any potential fallout as part of my build-ops rotation.

captainsafia requested a review from Pilchie as a code owner July 29, 2021 18:51

captainsafia requested review from BrennanConroy and halter73 July 29, 2021 18:51

halter73 approved these changes Jul 29, 2021

View reviewed changes

JunTaoLuo approved these changes Jul 29, 2021

View reviewed changes

captainsafia requested a review from Tratcher as a code owner July 29, 2021 22:04

captainsafia enabled auto-merge (squash) July 29, 2021 22:06

captainsafia and others added 3 commits July 29, 2021 16:19

Disable analyzers only when building in VS

faf22f5

Add API annotation for ToString

1d0b23d

fixups

442fb30

BrennanConroy force-pushed the safia/fix-analyzer-patch branch from f35aa68 to 442fb30 Compare July 29, 2021 23:34

captainsafia requested a review from a team as a code owner July 29, 2021 23:34

JamesNK approved these changes Jul 29, 2021

View reviewed changes

pranavkm reviewed Jul 29, 2021

View reviewed changes

dougbu approved these changes Jul 29, 2021

View reviewed changes

Fix up some more warnings

9131dcb

Update condition for EnforceCodeStyleInBuild

bfb3079

captainsafia requested a review from a team as a code owner July 30, 2021 00:28

TanayParikh approved these changes Jul 30, 2021

View reviewed changes

captainsafia added 3 commits July 30, 2021 00:42

Remove duplicate entry in PublicAPI files

0cafb6f

Update namespace for JSComponentConfigurationExtensions

ccc3d4d

Remove duplicate Microsoft.AspNetCore.Components.Web.JSComponentConfi…

e8579aa

…gurationExtensions

Merge remote-tracking branch 'origin/main' into safia/fix-analyzer-patch

7a4e76e

Fix license header in test file

45fa914

dougbu approved these changes Jul 30, 2021

View reviewed changes

captainsafia disabled auto-merge July 30, 2021 02:37

captainsafia merged commit 08c8b41 into main Jul 30, 2021

captainsafia deleted the safia/fix-analyzer-patch branch July 30, 2021 02:38

ghost added this to the 6.0-rc1 milestone Jul 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable analyzers only when building in VS #34848

Disable analyzers only when building in VS #34848

captainsafia commented Jul 29, 2021

BrennanConroy commented Jul 29, 2021

halter73 left a comment

JunTaoLuo left a comment

dougbu commented Jul 29, 2021

pranavkm Jul 29, 2021

halter73 Jul 29, 2021

dougbu Jul 30, 2021

dougbu commented Jul 29, 2021

dougbu left a comment

dougbu commented Jul 30, 2021

dougbu commented Jul 30, 2021

captainsafia commented Jul 30, 2021

captainsafia commented Jul 30, 2021

dougbu left a comment

dougbu Jul 30, 2021

dougbu Jul 30, 2021

rainersigwald Jul 30, 2021

dougbu commented Jul 30, 2021

dougbu commented Jul 30, 2021

captainsafia commented Jul 30, 2021

	<PropertyGroup Condition=" '$(BuildingInsideVisualStudio)' == 'true' AND '$(VisualStudioVersion)' == '17.0' ">
	<PropertyGroup Condition=" '$(MSBuildRuntimeType)' != 'Core' AND '$(VisualStudioVersion)' == '17.0' ">

	<EnforceCodeStyleInBuild Condition="'$(BuildingInsideVisualStudio)' == 'true' AND '$(VisualStudioVersion)' < '17.0'">false</EnforceCodeStyleInBuild>
	<EnforceCodeStyleInBuild Condition=" '$(MSBuildRuntimeType)' != 'Core' AND '$(VisualStudioVersion)' < '17.0' ">false</EnforceCodeStyleInBuild>

Disable analyzers only when building in VS #34848

Disable analyzers only when building in VS #34848

Conversation

captainsafia commented Jul 29, 2021

BrennanConroy commented Jul 29, 2021

halter73 left a comment

Choose a reason for hiding this comment

JunTaoLuo left a comment

Choose a reason for hiding this comment

dougbu commented Jul 29, 2021

pranavkm Jul 29, 2021

Choose a reason for hiding this comment

halter73 Jul 29, 2021

Choose a reason for hiding this comment

dougbu Jul 30, 2021

Choose a reason for hiding this comment

dougbu commented Jul 29, 2021

dougbu left a comment

Choose a reason for hiding this comment

dougbu commented Jul 30, 2021

dougbu commented Jul 30, 2021

captainsafia commented Jul 30, 2021

captainsafia commented Jul 30, 2021

dougbu left a comment

Choose a reason for hiding this comment

dougbu Jul 30, 2021

Choose a reason for hiding this comment

dougbu Jul 30, 2021

Choose a reason for hiding this comment

rainersigwald Jul 30, 2021

Choose a reason for hiding this comment

dougbu commented Jul 30, 2021

dougbu commented Jul 30, 2021

captainsafia commented Jul 30, 2021