Skip to content

Improve implementation of analyzer loader#11956

Merged
tmat merged 3 commits intodotnet:masterfrom
tmat:AnalyzerLoader
Jun 15, 2016
Merged

Improve implementation of analyzer loader#11956
tmat merged 3 commits intodotnet:masterfrom
tmat:AnalyzerLoader

Conversation

@tmat
Copy link
Copy Markdown
Member

@tmat tmat commented Jun 13, 2016 •
edited
Loading

  • Avoid holding a lock while calling the CLR loader
  • unify Desktop and Core CLR implementations
  • avoid dependency on the order of analyzers on the command line when compiling via csc/vbc

Fixes deadlock when bootstraping Roslyn on Linux with CoreFX rc4-24211-01

Note: VSI tests are expected to fail atm due to some cross-repo dependencies.
Need to merge https://github.com/dotnet/roslyn-internal/pull/1062 first.

I have filed issue #11977 to add more test coverage and #11978 to suppress warnings on Linux reported by analyzers once this is change is merged.

@tmat tmat force-pushed the AnalyzerLoader branch 11 times, most recently from 887b228 to fca086e Compare June 13, 2016 22:20
@tmat
Copy link
Copy Markdown
Member Author

tmat commented Jun 13, 2016

@mavasani Please review
@srivatsn @ManishJayaswal @tmeschter FYI

@tmat tmat mentioned this pull request Jun 13, 2016
Closed
mavasani
mavasani reviewed Jun 13, 2016
View reviewed changes
Comment thread src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.cs Outdated
Copy link
Copy Markdown
Contributor

@mavasani mavasani Jun 13, 2016

Choose a reason for hiding this comment

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

Dictionary<string, HashSet<string>>?

Copy link
Copy Markdown
Member Author

@tmat tmat Jun 13, 2016

Choose a reason for hiding this comment

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

That would be unnecessary imo. Usually there won't be many analyzers with the same simple assembly name. Rahter there will usually be one.

@mavasani
Copy link
Copy Markdown
Contributor

mavasani commented Jun 14, 2016

👍 Thanks!

@tmat
Copy link
Copy Markdown
Member Author

tmat commented Jun 15, 2016

test vsi please

@tmat
Copy link
Copy Markdown
Member Author

tmat commented Jun 15, 2016 •
edited
Loading

@dotnet-bot retest windows_vsi_p2_open_prtest please
// Previous failure: http://dotnet-ci.cloudapp.net/job/Private/job/dotnet_roslyn-internal/job/master/job/windows_vsi_p2_open_prtest/140/
// Retest reason: ibcmerge failure: #11868

@tmat
Copy link
Copy Markdown
Member Author

tmat commented Jun 15, 2016

@dotnet-bot retest windows_vsi_p2_open_prtest please
// Previous failure: http://dotnet-ci.cloudapp.net/job/Private/job/dotnet_roslyn-internal/job/master/job/windows_vsi_p2_open_prtest/141/
// Retest reason:

@tmat tmat force-pushed the AnalyzerLoader branch from fca086e to 02369b0 Compare June 15, 2016 16:10
@tmat
Copy link
Copy Markdown
Member Author

tmat commented Jun 15, 2016

test vsi please

@tmat
Copy link
Copy Markdown
Member Author

tmat commented Jun 15, 2016

@mavasani The failing integration test passes on my machine.

@tmat
Copy link
Copy Markdown
Member Author

tmat commented Jun 15, 2016

@dotnet-bot retest windows_vsi_p2_open_prtest please
// Previous failure: http://dotnet-ci.cloudapp.net/job/Private/job/dotnet_roslyn-internal/job/master/job/windows_vsi_p2_open_prtest/146/
// Retest reason:

@tmat tmat merged commit c966b31 into dotnet:master Jun 15, 2016
@tmat tmat deleted the AnalyzerLoader branch June 15, 2016 22:04
@mavasani mavasani mentioned this pull request Jul 7, 2016
Closed
@Youssef1313 Youssef1313 mentioned this pull request Mar 31, 2021
Open
@jasonmalinowski jasonmalinowski mentioned this pull request Nov 23, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

cla-already-signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tmat @mavasani @dnfclas