Skip to content

Update AnalyzerRunner to support IIncrementalAnalyzer testing#36113

Merged
sharwell merged 7 commits intodotnet:masterfrom
sharwell:incremental-analyzers
Jun 4, 2019
Merged

Update AnalyzerRunner to support IIncrementalAnalyzer testing#36113
sharwell merged 7 commits intodotnet:masterfrom
sharwell:incremental-analyzers

Conversation

@sharwell
Copy link
Copy Markdown
Contributor

@sharwell sharwell commented Jun 1, 2019

This is a supporting change for upcoming profile-based optimization of SQLite and the incremental analyzer infrastructure for VS for Mac scenarios.

/cc @Therzok

@sharwell sharwell requested review from a team as code owners June 1, 2019 22:07
@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

CyrusNajmabadi commented Jun 1, 2019

Would love to know what data this reveals once you collect it. If you can make that information public, it would be very appreciated. Thanks! :)

I imagine there will be some results that are nto at all surprising, but also a few we did not expect.

@sharwell sharwell force-pushed the incremental-analyzers branch from 1d3513d to 2136892 Compare June 1, 2019 22:43
@sharwell
Copy link
Copy Markdown
Contributor Author

sharwell commented Jun 1, 2019

If you can make that information public ...

Literally the entire reason why this PR exists and the PRs from @Therzok are not yet merged is we are ensuring the public can reproduce the benchmarks that led to any changes. 😄

Copy link
Copy Markdown
Contributor

@Therzok Therzok left a comment

Choose a reason for hiding this comment

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

LGTM. I'll try and set it up locally to see performance characteristics.

@sharwell
Copy link
Copy Markdown
Contributor Author

sharwell commented Jun 2, 2019

@Therzok I have another issue to resolve with loading Roslyn.sln before this is ready to use. I'll let you know when I've updated the PR.

@sharwell sharwell force-pushed the incremental-analyzers branch 2 times, most recently from 9cd07e5 to ac469ef Compare June 3, 2019 13:15
@sharwell sharwell force-pushed the incremental-analyzers branch from ac469ef to ba24589 Compare June 3, 2019 13:20

<!-- Set to non-existent file to prevent common targets from importing Microsoft.CodeAnalysis.targets -->
<CodeAnalysisTargets>*none*</CodeAnalysisTargets>
<CodeAnalysisTargets>NON_EXISTENT_FILE</CodeAnalysisTargets>
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@dotnet/roslyn-infrastructure I meant to submit this as a separate pull request but I forgot to extract it. Please review this change.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What does this change actually do? Was *none* a magic value before?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There is no change in the behavior. MSBuild uses Exists($(CodeAnalysisTargets)) for a condition. *none* does not exist, but unlike the new value, also contains * characters that are not valid. This change avoids a bunch of first chance exceptions that were making it hard for me to figure out why AnalyzerRunner was failing to load Roslyn.sln.

<ItemGroup>
<PackageReference Include="SQLitePCLRaw.bundle_green" Version="$(SQLitePCLRawbundle_greenVersion)" PrivateAssets="all" />
<PackageReference Include="Microsoft.Build.Locator" Version="$(MicrosoftBuildLocatorVersion)" />
<PackageReference Include="SourceBrowser" Version="$(SourceBrowserVersion)" ExcludeAssets="build;compile" PrivateAssets="all" />
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

📝 I will file a follow-up issue to remove this once MSBuild Locator provides its own AppDomainManager.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

➡️ Filed #36165

Copy link
Copy Markdown
Member

@JoeRobich JoeRobich left a comment

Choose a reason for hiding this comment

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

LGTM

@jinujoseph jinujoseph added this to the 16.2.P3 milestone Jun 4, 2019
@sharwell sharwell merged commit 2e4d6d6 into dotnet:master Jun 4, 2019
@sharwell sharwell deleted the incremental-analyzers branch June 4, 2019 22:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants