Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Shadow copying analyzer assembly loader. #1474

Merged
merged 9 commits into from
Apr 30, 2019

Conversation

savpek
Copy link
Contributor

@savpek savpek commented Apr 19, 2019

To resolve #1465

Stripped down version from roslyn code base.

@savpek savpek marked this pull request as ready for review April 22, 2019 05:43
@savpek
Copy link
Contributor Author

savpek commented Apr 22, 2019

Ready for review @rchande @filipw @david-driscoll @mholo65

@filipw
Copy link
Member

filipw commented Apr 23, 2019

do we have any known repro steps for a case that could be included as a test?
perhaps something like trying to use an analyzer from a given path and then deleting that path from the test?

@savpek
Copy link
Contributor Author

savpek commented Apr 23, 2019

Only thing comes to mind is to configure nuget restore folder to local folder next of project and test that it can remove/move nuget packages after startup. That should fail if analyzer dll:s are not shadow copies. Probably RestorePackagesPath will do the trick in csproj.

@filipw can you check pr #1440 because it contains test-assets/test-projects/ProjectWithRulesets/, which could be renamed -> test-assets/test-projects/ProjectWithAnalyzers/ -> can be used part of this test + other thing related analyzers when full e2e is needed.

@filipw
Copy link
Member

filipw commented Apr 23, 2019

yes the test infra there looks good there, and is very welcome.

I'd personally prefer the order of merging - first this PR, and only then the other.
Would it be a problem for you to pluck that test infra from that other PR and use it here with a custom RestorePackagesPath as discussed? thanks a lot

@savpek
Copy link
Contributor Author

savpek commented Apr 24, 2019

Started to implement tests, there's definetly something that i don't understand about analyzer loading from package references yet 😄 They are not loaded as analyzers (item property type doesn't match) but they magically appears anyway. This escalates in tests because they implement something differently and project scoped analyzers doesn't get loaded. It's likely that something has been implemented incorrectly in current version and it's pure luck that it works. I will debug deeper later this week ...

@savpek
Copy link
Contributor Author

savpek commented Apr 27, 2019

image

Test idea seems legit (poc made without shadow copying loader).

Problem behind tests was #1458 -> restore must currently be done before host is started on tests, it should reload analyzers when ever restore is done but currently it doesn't behave that way. I will fix that in another pr and write bit dummier version of test in this where restore is made beforehand.

@savpek
Copy link
Contributor Author

savpek commented Apr 27, 2019

Review fixes ready @filipw

Copy link
Member

@filipw filipw left a comment

Choose a reason for hiding this comment

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

great work - LGTM!

@filipw
Copy link
Member

filipw commented Apr 28, 2019

Any pending thoughts @mholo65 @rchande @david-driscoll?

Copy link
Member

@bjorkstromm bjorkstromm left a comment

Choose a reason for hiding this comment

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

Why was CachingCodeFixProviderForProjects removed from Project System and Manager? Is it used anywhere else? Reviewing on mobile so I’m probably not getting the whole picture.

@savpek
Copy link
Contributor Author

savpek commented Apr 28, 2019

@mholo65 i noticed that project has analyzer refecenes available at workspace -> made it lazy. This was refactoring, way it originally should be done if i had better knowledge at that time. By making it lazy with cache it moves loading from startup which makes startup faster and makes startup more robust. Original version failed fatally (and silently, i think few of current issues at load up may be even caused by this) if there was something wrong with project analyzers like locked or missing dll. Noticed that behavior when i tried to replicate locked / invalid dlls for assembly loader.

Copy link
Member

@bjorkstromm bjorkstromm left a comment

Choose a reason for hiding this comment

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

@savpek thanks for the response! Checked the earlier PR and saw it was added there. This is great. Thank you!

@filipw filipw merged commit 8962150 into OmniSharp:master Apr 30, 2019
@filipw filipw mentioned this pull request May 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Omnisharp locking analyzer DLLs
4 participants