Determinism test needs to use bootstrap compiler#49483
Determinism test needs to use bootstrap compiler#49483jaredpar merged 2 commits intodotnet:masterfrom
Conversation
|
@dotnet/roslyn-infrastructure PTAL |
eng/test-determinism.ps1
Outdated
There was a problem hiding this comment.
That change only fixed the field emit, it missed the parameters which have the same problem.
For the determinism test to be effective it should be using the bootstrap compiler. That was the intent of this job but at some point the bootstrap got disabled. This fixes that and excludes some binaries that are now failing determinism. Tracking issue for the failed binaries dotnet#49482
|
@AArnott this fixes the performance problem where the compiler server does not work for projects with your analyzers installed. |
|
@sharwell I don't recall the problem you're referring to. Can you job my memory? |
|
@dotnet/roslyn-compiler PTAL. The logic for why we should not restart the server for missing dependencies is covered in #46684. |
|
@AArnott the context is #46671. We had to include custom build logic because the vs-threading analyzers package was tanking build performance at no fault of its own (on my machine, builds went from ~2 minutes without the code fixes assembly to ~6 minutes with it, even though the assembly provided no build features). After this change, the code fixes assembly in that package will no longer negatively impact build throughput. |
|
Not sure if this applies to your analyzers but any analyzer DLL that also packaged a code fixer inside it effectively never used the compiler server. The compiler server is very sensitive about analyzer load errors because it has to deal with real problems around analyzers polluting the assembly load space of the process. That is because the server sees all of the analyzers for all compilations that go through it where as the command line csc just sees the analyzers for the current build. Quick example of where this can cause a problem. Consider two projects (UtilA and UtilB) which depend on two versions of analyzer FindTheBugz which is not strong name signed. The compiler server can handle either of them just fine but not both. The first one to build loads their version of FindTheBugz one the second just gets the version the first loaded. The compiler server though detects this problem, redirects the build request to the command line compiler and restarts the server. One of the load errors the server would shut down on was missing dependencies. That was written originally under the guise of "better safe than sorry". Over time though it's caused subsantial perf and usability problems. After some discussion decided this is actually an unnecessary restriction and this is lifting it. |
|
|
||
| $script:skipList = @( | ||
| # Added to work around https://github.com/dotnet/roslyn/issues/48417 | ||
| "Microsoft.CodeAnalysis.EditorFeatures2.UnitTests.dll" |
There was a problem hiding this comment.
"Microsoft.CodeAnalysis.EditorFeatures2.UnitTests.dll" [](start = 2, length = 54)
The fix for parameters got merged.
There was a problem hiding this comment.
Great. I'll either get this changed here or submit a follow up PR to remove this line.
| { | ||
| private static readonly ImmutableArray<string> s_defaultIgnorableReferenceNames = ImmutableArray.Create("mscorlib", "System", "Microsoft.CodeAnalysis", "netstandard"); | ||
|
|
||
| public static bool Check(string baseDirectory, IEnumerable<CommandLineAnalyzerReference> analyzerReferences, IAnalyzerAssemblyLoader loader, IEnumerable<string>? ignorableReferenceNames = null) |
There was a problem hiding this comment.
ignorableReferenceNames [](start = 170, length = 23)
Is this parameter still used?
There was a problem hiding this comment.
No. Thought I had deleted that. Will fix.
AlekseyTs
left a comment
There was a problem hiding this comment.
LGTM (iteration 2), modulo a couple of comments.
* upstream/master: (47 commits) Make compiler server logging explicit (dotnet#48557) [master] Update dependencies from dotnet/arcade (dotnet#48274) Handle removed project in ExternalErrorDiagnosticUpdateSource Report an error for ref-returning auto-properties declared in an interface. (dotnet#49510) Add usings on paste (dotnet#48501) Determinism test needs to use bootstrap compiler (dotnet#49483) Simplify and reduce amount of work performed by SourcePropertySymbolBase constructor. (dotnet#49360) Updates test Simplify Split Document/Workspace handler into only handling open/closed documents respectively. only report watson once. Additional usage of a PooledHashset. (dotnet#49459) Loc checkin Update src/Features/CSharp/Portable/CodeRefactorings/ConvertLocalFunctionToMethod/CSharpConvertLocalFunctionToMethodCodeRefactoringProvider.cs Preserve annotation on comment trivia when performing formatting. Validate arguments to IAsyncCompletionSource methods Determinism fixes for AnonymousTypes in VB (dotnet#49467) Collect nfw information for a crash we're seeing. Make sure to not discard text changes when no reference changes are present Create an unsafe method from a local function when necessary (dotnet#49389) ...
For the determinism test to be effective it should be using the
bootstrap compiler. That was the intent of this job but at some point
the bootstrap got disabled. This fixes that and excludes some binaries
that are now failing determinism.
Tracking issue for the failed binaries #49482This also fixes the issue where we should not shut down the server for missing dependencies. The logic for that is covered in the issue #46684
closes #46684