Skip executing analyzers by default on local builds for solutions in the repo #44047

mavasani · 2020-05-07T13:56:09Z

Overview

This change skips executing analyzers by default on explicit builds of solutions in this repo (both inside VS and on command line prompt). We will continue running analyzers for the following:

Inside IDE while typing: We want to identify analyzer warnings and suggestions during development and apply code fixes or fix all operations. This is the core scenario where we need to execute analyzers for inner dev loop.
CI legs: We build our solutions with analyzers enabled on the Correctness leg to catch build breaking analyzer warnings. This is the core scenario where we need to execute analyzers for build time analyzer enforcement.

This change should speed up the local build time of Roslyn.sln by ~2X.

Running analyzers locally after this change

The only time a developer would want to run analyzers locally, outside the IDE typing scenario, is to compute the exact set of warnings for a specific project/solution before pushing local changes to CI. They can now perform the following steps to achieve this:

On command line:
1. Run build -runAnalyzers at the root of the repo.
2. Build specific project/solution with msbuild argument /p:UseRoslynAnalyzers=true or /p:RunCodeAnalysis=true.
Inside VS:
1. Start VS with %RoslynEnforceCodeStyle% = true, all builds in VS should execute analyzers
2. Use Run Code Analysis command from Analyze menu on selected project/solution

Perf testing - build time measurements

Steps

restore
taskkill /F /IM MSBuild.exe
taskkill /F /IM VBCSCompiler.exe
[cold] msbuild /m /v:m /t:rebuild Roslyn.sln
[hot] msbuild /m /v:m /t:rebuild Roslyn.sln

Measurements

Master branch (commit 27654b0)
1. [cold - step 4]: 6 minutes 20 seconds
2. [hot - step 5]: 5 minutes 45 seconds
This PR branch
1. [cold - step 4]: 3 minutes
2. [hot - step 5]: 2 minutes 35 seconds

Functional testing

Local validation:
1. Command line:
  1. build.cmd does not run analyzers.
  2. build.cmd -runAnalyzers runs analyzers.
  3. msbuild Roslyn.sln does not run analyzers.
  4. msbuild Roslyn.sln /p:UseRoslynAnalyzers=true and msbuild Roslyn.sln /p:RunCodeAnalysis=true runs analyzers
2. VS:
  1. Build and Rebuild menu commands do not run analyzers.
  2. Analyzers run on open files by default.
  3. Switching to "Entire solution" background analysis scope also run analyzers on closed files.
  4. Run Code Analysis command on individual projects force completes all applicable analyzers and diagnostics are populated in error list (NOTE: Solution level command seems to be causing a ServiceHub exception right now, I'll file a bug).
  5. FixAll operations find and fix violations in closed files, regardless of the background analysis scope.
CI validation:
1. Dummy commit 71716fd with a build enforced code style violation leads to:
  1. build-correctness test leg to fail with this warning treated as error.
  2. All other test legs pass, no analyzers are executed as /p:UseRoslynAnalyzers= false was used.
2. Revert the above commit to have no code style violations: All test legs pass (this PR must be green)

This change skips executing analyzers by default on explicit builds (both inside VS and on command line prompt). This should speed up the overall build time of Roslyn.sln when building locally. The only time a developer would want analyzers to run locally outside the typing scenario is to compute the exact set of warnings for a specific project/solution before pushing your local changes to CI. They can now perform the following steps to achieve this: 1. On command line: Build with `/p:UseRoslynAnalyzers=true` or `/p:RunCodeAnalysis=true` 2. Inside VS: 1. Start VS with `%RoslynEnforceCodeStyle% = true`, all builds in VS should execute analyzers 2. Use `Run Code Analysis` command from `Analyze` menu on selected project/solution

mavasani · 2020-05-07T17:29:30Z

azure-pipelines-official.yml

@@ -103,7 +103,6 @@ stages:
                -officialSourceBranchName $(SourceBranchName)
                -officialIbcSourceBranchName $(IbcSourceBranchName)
                -officialIbcDropId $(IbcDropId)
-                -skipAnalyzers


Analyzer execution has been switched from opt-out mode to opt-in mode in build scripts.

skipAnalyzers flag has been replaced with runAnalyzers flag, which defaults to false. It is only explicitly set to true for build-correctness test leg.

… CI failure" This reverts commit 71716fd.

RikkiGibson · 2020-05-07T20:45:07Z

.vscode/tasks.json will need to change. I can do it in a followup PR. Do we have usage of -skipAnalyzers/--skipAnalyzers in any other scripts?

eng/build.ps1

mavasani · 2020-05-07T21:07:15Z

.vscode/tasks.json will need to change. I can do it in a followup PR. Do we have usage of -skipAnalyzers/--skipAnalyzers in any other scripts?

Thanks. I believe those are the only remaining references in scripts. I am not sure how to test the changes in .vscode/tasks.json, so have left it untouched. Doing the necessary changes + testing in a follow-up PR sounds good to me.

eng/build.ps1

tmat · 2020-05-07T23:03:43Z

--skipanalyzers)

This needs a change

Refers to: eng/build.sh:132 in 4f70532. [](commit_id = 4f70532, deletion_comment = False)

tmat · 2020-05-07T23:04:04Z

skip_analyzers=false

Change

Refers to: eng/build.sh:67 in 4f70532. [](commit_id = 4f70532, deletion_comment = False)

mavasani · 2020-05-07T23:17:18Z

--skipanalyzers)

This needs a change

Ah, case sensitive search tricked me! Fixing..

mavasani · 2020-05-08T14:16:33Z

@sharwell @tmat Any further feedback?

AlekseyTs · 2020-05-08T16:22:41Z

@mavasani Could you please prioritize getting required sign-offs and merging this PR ASAP? Thanks!

jaredpar · 2020-05-08T16:37:55Z

eng/targets/Imports.targets

+          Otherwise, default to 'true'.
+  -->
+  <PropertyGroup Condition="'$(UseRoslynAnalyzers)' == ''">
+    <UseRoslynAnalyzers Condition="'$(DesignTimeBuild)' != 'true' AND '$(ContinuousIntegrationBuild)' != 'true' AND !('$(RoslynEnforceCodeStyle)' == 'true' OR '$(RunCodeAnalysis)' == 'true')">false</UseRoslynAnalyzers>


When does $(RunCodeAnalysis) get set to true? Not familiar with the workflows that enable that property.

This property is set to true when you execute "Run Code Analysis" context command in Visual Studio, either from top level "Analyze" menu OR from solution explorer context menu for project/solution: https://docs.microsoft.com/en-us/visualstudio/code-quality/how-to-run-code-analysis-manually-for-managed-code?view=vs-2019. Starting VS2019 16.5, this command force completes all the applicable analyzers on the selected project/solution.

mavasani · 2020-05-08T16:47:32Z

@mavasani Could you please prioritize getting required sign-offs and merging this PR ASAP? Thanks!

I have merged the PR and will address any further feedback with a follow-up PR. Thanks!

tmat · 2020-05-08T18:16:30Z

LGTM

mavasani changed the title ~~Skip executing analyzers by default on build for solutions in the repo~~ Skip executing analyzers by default on local builds for solutions in the repo May 7, 2020

mavasani force-pushed the SkipAnalyzersByDefaultInBuild branch 3 times, most recently from 2a9722d to 142a84f Compare May 7, 2020 16:34

mavasani added 2 commits May 7, 2020 10:05

Dummy commit violating build enforced code style rule to test CI failure

71716fd

mavasani force-pushed the SkipAnalyzersByDefaultInBuild branch from 142a84f to 71716fd Compare May 7, 2020 17:06

mavasani commented May 7, 2020

View reviewed changes

Revert "Dummy commit violating build enforced code style rule to test…

4f70532

… CI failure" This reverts commit 71716fd.

mavasani marked this pull request as ready for review May 7, 2020 20:01

mavasani requested a review from a team as a code owner May 7, 2020 20:01

mavasani requested review from sharwell, jaredpar and vatsalyaagrawal May 7, 2020 20:01

sharwell reviewed May 7, 2020

View reviewed changes

eng/build.ps1 Show resolved Hide resolved

tmat reviewed May 7, 2020

View reviewed changes

eng/build.ps1 Outdated Show resolved Hide resolved

Address feedback

59f745b

jaredpar reviewed May 8, 2020

View reviewed changes

jaredpar approved these changes May 8, 2020

View reviewed changes

mavasani merged commit 96af121 into dotnet:master May 8, 2020

ghost added this to the Next milestone May 8, 2020

mavasani deleted the SkipAnalyzersByDefaultInBuild branch May 8, 2020 16:46

RikkiGibson mentioned this pull request May 8, 2020

Remove skipAnalyzers from vscode tasks #44081

Merged

JoeRobich modified the milestones: Next, 16.7.P2 May 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip executing analyzers by default on local builds for solutions in the repo #44047

Skip executing analyzers by default on local builds for solutions in the repo #44047

mavasani commented May 7, 2020 •

edited

Loading

mavasani May 7, 2020

RikkiGibson commented May 7, 2020

mavasani commented May 7, 2020

tmat commented May 7, 2020

tmat commented May 7, 2020

mavasani commented May 7, 2020

mavasani commented May 8, 2020

AlekseyTs commented May 8, 2020

jaredpar May 8, 2020

mavasani May 8, 2020

mavasani commented May 8, 2020

tmat commented May 8, 2020

Skip executing analyzers by default on local builds for solutions in the repo #44047

Skip executing analyzers by default on local builds for solutions in the repo #44047

Conversation

mavasani commented May 7, 2020 • edited Loading

Overview

Running analyzers locally after this change

Perf testing - build time measurements

Steps

Measurements

Functional testing

mavasani May 7, 2020

Choose a reason for hiding this comment

RikkiGibson commented May 7, 2020

mavasani commented May 7, 2020

tmat commented May 7, 2020

tmat commented May 7, 2020

mavasani commented May 7, 2020

mavasani commented May 8, 2020

AlekseyTs commented May 8, 2020

jaredpar May 8, 2020

Choose a reason for hiding this comment

mavasani May 8, 2020

Choose a reason for hiding this comment

mavasani commented May 8, 2020

tmat commented May 8, 2020

mavasani commented May 7, 2020 •

edited

Loading