Skip to content

Enforce analyzer consistency in our builds#43954

Merged
jaredpar merged 1 commit intodotnet:masterfrom
jaredpar:maintain-perf
May 5, 2020
Merged

Enforce analyzer consistency in our builds#43954
jaredpar merged 1 commit intodotnet:masterfrom
jaredpar:maintain-perf

Conversation

@jaredpar
Copy link
Member

@jaredpar jaredpar commented May 4, 2020

This changes the bootstrap compiler to enforce that our build doesn't
suffer from analyzer consistency issues. This will prevent us from
introducing build performance issues into the repository in the future.
Essentially adding enforcement to make sure that PRs like #43629 won't
ever be necessary again.

Related to #43629

This changes the bootstrap compiler to enforce that our build doesn't
suffer from analyzer consistency issues. This will prevent us from
introducing build performance issues into the repository in the future.
Essentially adding enforcement to make sure that PRs like dotnet#43629 won't
ever be necessary again.

Related to dotnet#43629
@sharwell
Copy link
Contributor

sharwell commented May 4, 2020

🎉

@jaredpar
Copy link
Member Author

jaredpar commented May 4, 2020

Validated locally that if I build without @sharwell fix that I get an error because of inconsistent analyzers:

P:\roslyn\eng\targets\Bootstrap.targets(4,5): error : Analyzer inconsistency building P:\roslyn\artifacts\obj\Microsoft.CodeAnalysis.CodeStyle\Debug\netstandard2.0\
Microsoft.CodeAnalysis.CodeStyle.dll [P:\roslyn\src\Interactive\csi\csi.csproj]

I actually get several errors.

@jaredpar jaredpar marked this pull request as ready for review May 4, 2020 18:40
@jaredpar jaredpar requested a review from a team as a code owner May 4, 2020 18:40
// a lot of false positives. The current value was chosen as a reasonable number for warranting an
// investigation.
if (s_failedServerConnectionCount > 20)
const int maxRejectCount = 5;
Copy link
Member

Choose a reason for hiding this comment

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

Are we confident in going from 20 => 5 for the non detectable errors?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Now this should only happen where there is basically a named pipe issue in our build. If there are more than 5 on a regular basis we can kick the number back up.

switch (tuple.ResponseType)
{
case ResponseType.AnalyzerInconsistency:
Log.LogError($"Analyzer inconsistency building {tuple.OutputAssembly}");
Copy link
Member

Choose a reason for hiding this comment

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

How do we know which analyzers had inconsistencies when this happens? Are other diagnostic messages printed in this scenario?

Copy link
Member Author

@jaredpar jaredpar May 4, 2020

Choose a reason for hiding this comment

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

No. There are no other diagnostics.

At the moment this isn't a user visible diagnostic. This is deliberately an implementation detail because this can happen in completely well functioning build. Surfacing a real diagnostic here would essentially be giving users false and unactionable information.

In this very specific scenario, a controlled bootstrap build, we know that this is wrong and hence we can emit an error.

@jaredpar jaredpar merged commit ca0a72f into dotnet:master May 5, 2020
@ghost ghost added this to the Next milestone May 5, 2020
@jaredpar jaredpar deleted the maintain-perf branch May 5, 2020 15:53
@JoeRobich JoeRobich modified the milestones: Next, 16.7.P2 May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants