-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Add SDK analyzer assembly redirector #80969
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
Conversation
src/VisualStudio/Core/Def/ProjectSystem/SdkAnalyzerAssemblyRedirector.cs
Outdated
Show resolved
Hide resolved
src/VisualStudio/Core/Def/ProjectSystem/SdkAnalyzerAssemblyRedirector.cs
Outdated
Show resolved
Hide resolved
|
@jasonmalinowski for another look, thanks |
This reverts commit 67e5b88.
| var enable = Environment.GetEnvironmentVariable("DOTNET_ANALYZER_REDIRECTING"); | ||
| _enabled = !"0".Equals(enable, StringComparison.OrdinalIgnoreCase) && !"false".Equals(enable, StringComparison.OrdinalIgnoreCase); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be using VS feature flags or a user setting or something else? We don't usually use enviornment variables much if we can since we have other mechanisms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's also a bit weird that setting these environment variables would break tests. If we were going to keep it, I'd expect that was read at the non-unit derived type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I'm not familiar with VS feature flags, I guess that's a bit complicated to add one? Is there an example or docs how to add new one? I just wanted a way to opt out if necessary, but originally I didn't even have that and I think that might be fine too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feature flags would be for if you wanted to support a rollback more generally, or you were trying to A/B test this or something. If you just wanted a setting, then this could be a user setting. You'd add that setting to our unified settings registration (maybe there's a way to hide it though?) and follow what #81503 added.
src/VisualStudio/Core/Def/ProjectSystem/SdkAnalyzerAssemblyRedirector.cs
Outdated
Show resolved
Hide resolved
src/VisualStudio/Core/Def/ProjectSystem/SdkAnalyzerAssemblyRedirector.cs
Outdated
Show resolved
Hide resolved
src/VisualStudio/Core/Def/ProjectSystem/SdkAnalyzerAssemblyRedirector.cs
Show resolved
Hide resolved
src/VisualStudio/Core/Def/ProjectSystem/SdkAnalyzerAssemblyRedirector.cs
Outdated
Show resolved
Hide resolved
src/VisualStudio/Core/Def/ProjectSystem/SdkAnalyzerAssemblyRedirector.cs
Outdated
Show resolved
Hide resolved
| if (!analyzerPath.StartsWith(topLevelDirectory, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would this happen? Or just a paranoia check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the latter, I will add an assert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use Debug.Fail() here, so you can add a message or something. And add a comment. We normally don't use asserts much on the IDE side, but given a throw here might really break things (i.e. no analyzers added for you!), it's probably appropriate here.
src/VisualStudio/Core/Def/ProjectSystem/SdkAnalyzerAssemblyRedirector.cs
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| var subsetName = Path.GetFileName(topLevelDirectory); | ||
| if (!versions.TryGetValue(subsetName, out var version)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be nice if there was a comment what the versions JSON is or looks like. There's not a file I can look at right now so I'm guessing, but I would have assumed versioning was related to the individual DLL which is odd since this path isn't depending on that yet....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an example in the doc (which is linked in the doc comment of SdkAnalyzerAssemblyRedirector): https://github.com/dotnet/sdk/blob/main/documentation/general/analyzer-redirecting.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that's really helpful. How does versioning work in this design them? If we needed to have redirected versions for a 9.0 and 10.0, then there'd be two top level folders in the JSON with different names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we needed to have redirected versions for a 9.0 and 10.0
That's intentionally unsupported. There is always only one SDK version inserted into VS anyway.
then there'd be two top level folders in the JSON with different names?
I guess that would technically work. I will add a test. But that won't happen in the way we currently generate the payload in the sdk repo.
src/VisualStudio/Core/Def/ProjectSystem/SdkAnalyzerAssemblyRedirector.cs
Outdated
Show resolved
Hide resolved
jasonmalinowski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Wouldn't mind a few more tests around "very" unexpected path cases just to make sure we don't trip up on those. For example, you have lots of tests for when the paths look "about right", but would the path mapping code get messed up where the suffix does match, but that's the root of the drive? Or just C:\AnalyzerName.dll? We've had bugs like that in the past.
| if (!analyzerPath.StartsWith(topLevelDirectory, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use Debug.Fail() here, so you can add a message or something. And add a comment. We normally don't use asserts much on the IDE side, but given a throw here might really break things (i.e. no analyzers added for you!), it's probably appropriate here.
| } | ||
|
|
||
| var subsetName = Path.GetFileName(topLevelDirectory); | ||
| if (!versions.TryGetValue(subsetName, out var version)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that's really helpful. How does versioning work in this design them? If we needed to have redirected versions for a 9.0 and 10.0, then there'd be two top level folders in the JSON with different names?
| var enable = Environment.GetEnvironmentVariable("DOTNET_ANALYZER_REDIRECTING"); | ||
| if (!"0".Equals(enable, StringComparison.OrdinalIgnoreCase) && !"false".Equals(enable, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| return Path.GetFullPath(Path.Combine(AppDomain.CurrentDomain.BaseDirectory, @"CommonExtensions\Microsoft\DotNet")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this check if the folder is present first? Or we're trusting that nothing will try to use it before the File.Exists check doesn't find the metadata.json?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're trusting that nothing will try to use it before the File.Exists check doesn't find the metadata.json
Yes, in the current implementation, the directory path is not even stored anywhere else, so that File.Exists check should be enough I think.
| return false; | ||
| } | ||
|
|
||
| var directoryPathVersion = Path.GetFileName(Path.GetDirectoryName(directoryPath.Substring(0, index))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure there's a test if somebody were to add a reference to Z:\analyzers\dotnet\Foo.dll or wherever, where the suffix does match but there is no parent directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the suffix matches, that's enough and it will be redirected, there is no need for it to have a parent directory. But I can add a test for it, thanks.
| // ~~~~~~~~~~~~~~~~~~~~~~~ = topLevelDirectory | ||
| // ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ = analyzerPath | ||
|
|
||
| foreach (var topLevelDirectory in Directory.EnumerateDirectories(insertedAnalyzersDirectory)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way we can produce a file that produces all this up front so we don't need to search for binaries that ship in the product? This is having the user pay a cost for things we can absolutely determine because we insert these binaries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sounds doable, I will look into that, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 64ed67b with SDK counterpart in dotnet/sdk#52326.
|
@jasonmalinowski can you please take another look? I made some changes since your last sign off. Thanks. |
src/VisualStudio/Core/Def/ProjectSystem/SdkAnalyzerAssemblyRedirector.cs
Show resolved
Hide resolved
src/VisualStudio/Core/Def/ProjectSystem/SdkAnalyzerAssemblyRedirector.cs
Outdated
Show resolved
Hide resolved
|
/pr-val |
|
View PR Validation Run triggered by @jjonescz Parameters
|
Moved here from the SDK per David Kean's request (to avoid another assembly load). Originally lived here:
Validation:
https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/684831https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/696938https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/699576