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

DotNet SDK 2.1.300 no longer loads satellite assemblies for StyleCop.Analyzers #29014

Closed
nguerrera opened this issue Aug 1, 2018 · 13 comments
Closed
Assignees
Labels
Area-Compilers Bug Resolution-External The behavior lies outside the functionality covered by this repository Tenet-Localization Some piece of UI isn’t localized, often due to hard-coding of strings or other visible elements.

Comments

@nguerrera
Copy link
Contributor

From @vweijsters on June 13, 2018 20:37

A localization issue was reported for StyleCop.Analyzers when using SDK 2.1.300 (see DotNetAnalyzers/StyleCopAnalyzers#2715).
I've done builds with diagnostic logging for 2.1.201 and 2.1.300 and I've compared the output.
The logging for 2.1.201 shows that satellite assemblies for StyleCop.Analyzers are detected and used. The satellite assemblies are absent from the log file for 2.1.300.

TestDotNet.zip contains the test project used and the diagnostic log files when building with 2.1.201 and 2.1.300.

Copied from original issue: dotnet/sdk#2332

@nguerrera
Copy link
Contributor Author

From @livarcocc on June 13, 2018 20:51

@dsplaisted @nguerrera can one of you take a look when you have a chance?

@nguerrera
Copy link
Contributor Author

Hmm, I did change things to stop passing every analyzer satellite as an analyzer to the compiler. This appeared to be a bug with over-aggressive pattern matching for analyzer assemblies in assets file.

My expectation was that like any other assembly, analyzers would find their satellites by convention next to them in culture-specific sub-folders. Therefore, there would be no need to pass them to the compiler.

I suspect the issue is that on CoreCLR, the compiler has the same problem that MSBuild once had: dotnet/msbuild#2203. I further suspect that the over-aggressive pattern matching was masking that by forcing the compiler to pre-load all analyzer satellites. If I'm right, I doubt it was deliberately designed to work that way as it would be bad for performance to require satellites for unused cultures to be loaded.

(Aside: we shouldn't even have any pattern matching of our own to identify analyzer assemblies, but NuGet still hasn't been updated to identify them for us as they do for compilation and runtime assets: NuGet/Home#6279)

cc @agocke @jaredpar

@nguerrera
Copy link
Contributor Author

I've moved this to roslyn because the reason this once worked was a pure accident due to a bug where we were causing the compiler to preload all satellite assemblies. The right fix is enable the compiler to load the culture-specific satellite assemblies on demand as msbuild does on .NET Core.

In the long term, this is something that I would hope the CoreCLR would handle on its own just as it works with Assembly.LoadFrom on desktop.

cc @jeffschwMSFT

@Pilchie
Copy link
Member

Pilchie commented Aug 1, 2018

Also @tmeschter

@jeffschwMSFT
Copy link
Member

Satellite assemblies are probed based on paths provided by the app.deps.json. In this case are these plug-in satellite assemblies? If that is the case, then the best we can do for now is to have a custom resolve event resolve these.

cc @swaroop-sridhar

@tmeschter
Copy link
Contributor

tmeschter commented Aug 1, 2018

Yes, analyzer assemblies are plug-ins to the compiler. The compiler should either be using an AssemblyLoadContext that handles this automatically or implement the code to find them itself.

@nguerrera
Copy link
Contributor Author

The resolve event is already fired from the resource manager's attempt to load them, but Roslyn's handler doesn't resolve them. See https://github.com/Microsoft/msbuild/pull/2229/files where msbuild added logic to their handler. Roslyn is not blocked from doing this to fix the bug.

Note that on full framework, both roslyn and msbuild already work as expected via Assembly.LoadFrom. This is just another issue where plugins on .NET Core are much harder than on .NET Framework right now. We've had a number of the same bugs get discovered in independent tools. We need something in the framework that is closer to a pit of success.

@jeffschwMSFT
Copy link
Member

+1 on this is an existing issue and one that we are looking into making a better experience in .NET Core 3.

@sharwell sharwell added Bug Tenet-Localization Some piece of UI isn’t localized, often due to hard-coding of strings or other visible elements. Area-Compilers labels Aug 2, 2018
@dfyx
Copy link

dfyx commented Oct 12, 2018

+1

One of the recent Visual Studio updates includes dotnet SDK 2.1.402 which broke some of my builds

Edit: I tried downgrading by removing C:\Program Files\dotnet\sdk\2.1.402 which appears to work but then I can't build .NET Core 2.1 projects.

@jeffschwMSFT
Copy link
Member

cc @sdmaclea

@fubar-coder
Copy link

@jeffschwMSFT Any news about this issue?

@sdmaclea
Copy link

sdmaclea commented Dec 17, 2018

This is a known difference in .NET Core versus .NET Framework, This is planned to be resolved in the .NET Core 3.0 timeframe.

There is a workaround. Posted in dotnet/core#2041 (comment). dotnet/coreclr#20979 tracks this for .NET Core 3.0.

@nguerrera also mentions above that Roslyn is able to fix the issue with a work around that was applied to msbuild.

@gafter
Copy link
Member

gafter commented Jul 16, 2019

This is reported now fixed in https://github.com/dotnet/coreclr/issues/20979

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Bug Resolution-External The behavior lies outside the functionality covered by this repository Tenet-Localization Some piece of UI isn’t localized, often due to hard-coding of strings or other visible elements.
Projects
None yet
Development

No branches or pull requests

9 participants