Add analyzer redirecting VSIX #42861
Conversation
jjonescz
commented
Aug 20, 2024
•
jjonescz
commented
- Design doc: Add analyzer redirecting proposal #42437
- Part of Address analyzer and generator torn state with Visual Studio #42087
- Roslyn counterpart: Add analyzer redirecting API roslyn#74820
- Transport package inspired by Create a transport package for VS and VS authoring for analyzers #42451
- Needs a follow up in VS like https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/575718
dotnet-issue-labeler
bot
added
Area-Infrastructure
untriaged
src/Microsoft.Net.Sdk.AnalyzerRedirecting/RedirectingAnalyzerAssemblyResolver.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Net.Sdk.AnalyzerRedirecting/RedirectingAnalyzerAssemblyResolver.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Net.Sdk.AnalyzerRedirecting/RedirectingAnalyzerAssemblyResolver.cs
Outdated
Show resolved
Hide resolved
|
|
||
| string analyzerName = Path.GetFileNameWithoutExtension(analyzerPath); | ||
| string pathSuffix = analyzerPath.Substring(versionDirectory.Length + (EndsWithSlash(versionDirectory) ? 0 : 1)); | ||
| pathSuffix = Path.GetDirectoryName(pathSuffix); |
There was a problem hiding this comment.
I see - so pathSuffix here is meant to match exactly the path under the version directory in the product.
So for "C:\Program Files\dotnet\packs\Microsoft.WindowsDesktop.App.Ref\8.0.8\analyzers\dotnet\System.Windows.Forms.Analyzers.dll"
it would be analyzers\dotnet
For "C:\Program Files\dotnet\packs\Microsoft.WindowsDesktop.App.Ref\8.0.8\analyzers\dotnet\cs\System.Windows.Forms.Analyzers.CSharp.dll" it would be analyzers\dotnet\cs
It'd be good to have a small writeup of how this works in either code comments or a markdown file. There's a lot of policy / convention here and it'd be good to spell that out and review it.
There was a problem hiding this comment.
There is a design doc here: #42437. But it sounds like a good idea to replicate some of it in comments directly in the code, thanks.
There was a problem hiding this comment.
Yeah, or link to that design doc. I think this should work - if we tie the product version to the replacement.
It's a little different for SDK than targeting packs. For the SDK analyzers there's only ever one version that gets loaded, so it should always be redirected.
For the targeting packs, there might be many loaded, and we only want to hook the one that matches the latest inserted framework version. For older framework versions, they can continue to load the one from the targeting pack (or we could decide to insert those if we want to NGEN them). For newer framework versions they don't get replaced.
There was a problem hiding this comment.
I'm not sure I understand. Are you saying the redirection algorithm should be different somehow?
Currently it works the same for both sdk and targeting pack analyzers, something like this:
- in VS we will have for example
- "C:\Program Files\Microsoft Visual Studio\2022\Preview\SDK\RuntimeAnalyzers\AspNetCoreAnalyzers\8.0.8\analyzers\dotnet\cs\Microsoft.AspNetCore.App.Analyzers.dll"
- "C:\Program Files\Microsoft Visual Studio\2022\Preview\SDK\RuntimeAnalyzers\WebSDKAnalyzers\8.0.400\Sdks\Microsoft.NET.Sdk.Web\analyzers\cs\Microsoft.AspNetCore.Analyzers.dll"
- a project targets net8.0 and the user has installed SDK 8.0.100 so it tries to load "C:\Program Files\dotnet\packs\Microsoft.AspNetCore.App.Ref\8.0.1\analyzers\dotnet\cs\Microsoft.AspNetCore.App.Analyzers.dll"
- we will redirect that to the DLL deployed with VS
- here we are comparing 8.0.400 (deployed) vs 8.0.1 (being loaded), i.e., SDK vs runtime version, I think that's fine since we consider only major and minor components?
- a project uses SDK 8.0.100 and tries to load "C:\Program Files\dotnet\sdk\8.0.100\Sdks\Microsoft.NET.Sdk.Web\analyzers\cs\Microsoft.AspNetCore.Analyzers.dll"
- we will redirect that to the DLL deployed with VS (8.0.100 vs 8.0.400 are compatible)
- in other cases, if the major/minor versions don't match, we won't redirect anything
There was a problem hiding this comment.
we are comparing 8.0.400 (deployed) vs 8.0.1 (being loaded), i.e., SDK vs runtime version
I don't think comparing SDK versions to framework pack versions is a safe thing to do. One SDK might bundle multiple targeting pack versions. Sure it would have the latest targeting pack version, and that's the one we'd want to distribute, but that's coincidental and not a hard rule. I think it's better to just match the versions from the packs. So if the analyzer came from a targeting pack versioned 8.0.1, then it gets installed to an 8.0.1 folder. If the user then tries to load 8.0.n version, it gets replaced with that installed 8.0.1 version.
Base on your other comment here I think you aren't actually comparing SDK versions with framework pack versions, so this might be moot, but that further demonstrates the need for a bit of detail in the design doc or comments about how this replacement works for targeting packs.
src/Microsoft.Net.Sdk.AnalyzerRedirecting/RedirectingAnalyzerAssemblyResolver.cs
Outdated
Show resolved
Hide resolved
| } | ||
| string directoryPathVersion = Path.GetFileName(Path.GetDirectoryName(directoryPath.Substring(0, index))); | ||
|
|
||
| return getMajorMinorPart(directoryPathVersion) == getMajorMinorPart(version); |
There was a problem hiding this comment.
This is reasonable. I was imagining you'd use System.Version but I can see how that might complicate things. If you leave it this way you can avoid creating new strings by having a single method that compares the strings. Here's a sample
There was a problem hiding this comment.
Also System.Version cannot parse versions with a hyphen it seems.
src/Microsoft.Net.Sdk.AnalyzerRedirecting/SdkAnalyzerAssemblyRedirector.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Net.Sdk.AnalyzerRedirecting/SdkAnalyzerAssemblyRedirector.cs
Outdated
Show resolved
Hide resolved
| @@ -81,6 +80,10 @@ | |||
| <FileSignInfo Include="Valleysoft.DockerCredsProvider.dll" CertificateName="$(ExternalCertificateId)" /> | |||
| </ItemGroup> | |||
|
|
|||
| <ItemGroup> | |||
| <FileSignInfo Include="StreamJsonRpc.dll" CertificateName="MicrosoftSHA2" /> | |||
There was a problem hiding this comment.
I was getting this signing error in official builds when building the VSIX:
D:\a_work\1\s.packages\microsoft.dotnet.arcade.sdk\10.0.0-beta.24430.1\tools\Sign.proj(72,5): error SIGN001: Signing Microsoft library 'D:\a_work\1\s\artifacts\tmp\Release\ContainerSigning\8296\AnalyzerRedirecting/StreamJsonRpc.dll' with 3rd party certificate '3PartySHA2'. The library is considered Microsoft library due to its copyright: '© Microsoft Corporation. All rights reserved.'.
FWIW, the same is specified in roslyn: https://github.com/dotnet/roslyn/blob/c737a04f3e9a895b5b5fff6d4c5467c17e9c49e8/eng/Signing.props#L62
There was a problem hiding this comment.
Is this a new file you added? Why did your change cause it? @marcpopMSFT
There was a problem hiding this comment.
I think StreamJsonRpc is some dependency brought in because we are building AnalyzerRedirecting as VSIX.
|
|
||
| static bool majorAndMinorVersionsMatch(string directoryPath, string pathSuffix, string version) | ||
| { | ||
| // Find the version number in the directory path - it is in the directory name before the path suffix. |
There was a problem hiding this comment.
It might be worth calling out that this can also come from a NuGet package which would have the same version folder in the path.
| foreach (AnalyzerInfo analyzer in analyzers) | ||
| { | ||
| var directoryPath = Path.GetDirectoryName(fullPath); | ||
| if (directoryPath.EndsWith(analyzer.PathSuffix, StringComparison.OrdinalIgnoreCase) && |
There was a problem hiding this comment.
Can we guarantee that the path passed in here will be normalized in the same way that pathSuffix is? If fullPath is coming from a user-controlled item in MSBuild I could imagine it has all sorts of nonsense in it (relative paths, extra path separators, alternate path separators, etc).
There was a problem hiding this comment.
Good point. But since this is targeting .net framework, Path.GetDirectoryName normalizes the path. I will add a comment.
There was a problem hiding this comment.
You could also explicitly normalize it with Path.GetFullPath
|
|
||
| <PropertyGroup> | ||
| <RuntimeAnalyzersSourceRoot>$(ArtifactsBinDir)redist\$(Configuration)\dotnet\</RuntimeAnalyzersSourceRoot> | ||
| <NetCoreRuntimeAnalyzersSubPath>packs\Microsoft.NetCore.App.Ref\*\analyzers</NetCoreRuntimeAnalyzersSubPath> |
There was a problem hiding this comment.
Will the SDK ever carry more than one pack? I guess if it did, we'd still be OK to include multiple versions since you match based on the major and minor.
How would this interact with VS on installation -- is it possible we'd ever have two of these analyzer packs installed, or would the newer one replace the old one?
There was a problem hiding this comment.
That should work fine - the analyzers are deployed with the version number in their path like
C:\Program Files\Microsoft Visual Studio\2022\Preview\DotNetRuntimeAnalyzers\AspNetCoreAnalyzers\9.0.0-preview.7.24406.2\analyzers\dotnet\cs\Microsoft.AspNetCore.App.Analyzers.dll
so multiple of them could live side by side.
There was a problem hiding this comment.
Ok, so if that happens would ever have more than one of these DotNetRuntimeAnalyzers installer packages inserted into VS that are trying to install the same files? I'm guessing no - if possible we should ensure that's the case.
| <add key="vs-impl" value="https://pkgs.dev.azure.com/azure-public/vside/_packaging/vs-impl/nuget/v3/index.json" /> | ||
| <!-- Used for Rich Navigation indexing task --> | ||
| <add key="richnav" value="https://pkgs.dev.azure.com/azure-public/vside/_packaging/vs-buildservices/nuget/v3/index.json" /> | ||
| <add key="general-testing" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/general-testing/nuget/v3/index.json" /> |
There was a problem hiding this comment.
This will go away before merging. This PR depends on a roslyn API which is not merged yet (it's in PR dotnet/roslyn#74820). So I manually published the roslyn packages to the "general testing" feed to use them here.
eng/Version.Details.xml
Outdated
| @@ -93,43 +93,43 @@ | |||
| <Sha>20cca61a546fe378948f0550a0026ec6077c1600</Sha> | |||
| <SourceBuild RepoName="fsharp" ManagedOnly="true" /> | |||
| </Dependency> | |||
| <Dependency Name="Microsoft.Net.Compilers.Toolset" Version="4.12.0-3.24458.2"> | |||
| <Dependency Name="Microsoft.Net.Compilers.Toolset" Version="4.12.0-3.24459.1"> | |||
There was a problem hiding this comment.
Shouldn't these version changes come through Arcade code flow?
There was a problem hiding this comment.
It shouldn't hurt to include dependency updates in your PR if your code depends on an external repo's changes that have not yet flowed to your current repo. It's ok as long as this file was modified using:
darc update-dependencies --channel <CHANNEL>
Ideally, a separate deps flow show come into the sdk repo with these same changes, and this PR can be rebased on top of it.
There was a problem hiding this comment.
Same as #42861 (comment), this will go away before merging - we will need to merge the roslyn part first, then let it flow into SDK, then we will be able to remove these version/nuget changes.
eng/Version.Details.xml
Outdated
| @@ -93,43 +93,43 @@ | |||
| <Sha>20cca61a546fe378948f0550a0026ec6077c1600</Sha> | |||
| <SourceBuild RepoName="fsharp" ManagedOnly="true" /> | |||
| </Dependency> | |||
| <Dependency Name="Microsoft.Net.Compilers.Toolset" Version="4.12.0-3.24458.2"> | |||
| <Dependency Name="Microsoft.Net.Compilers.Toolset" Version="4.12.0-3.24459.1"> | |||
There was a problem hiding this comment.
It shouldn't hurt to include dependency updates in your PR if your code depends on an external repo's changes that have not yet flowed to your current repo. It's ok as long as this file was modified using:
darc update-dependencies --channel <CHANNEL>
Ideally, a separate deps flow show come into the sdk repo with these same changes, and this PR can be rebased on top of it.
| var path = Path.Combine(sourceFolder, file); | ||
| if (!File.Exists(path)) | ||
| { | ||
| throw new InvalidOperationException($"File not found: {path}"); |
There was a problem hiding this comment.
Do we usually add exception strings as resource strings in this repo? Is it ok to have it hardcoded in English?
There was a problem hiding this comment.
This is a build-only code, it's not shipped as part of the product, so I would hope it's fine.
| public sealed class SdkAnalyzerAssemblyRedirector : IAnalyzerAssemblyRedirector | ||
| { | ||
| private readonly string? _insertedAnalyzersDirectory; | ||
| private readonly Lazy<ImmutableDictionary<string, List<AnalyzerInfo>>> _analyzerMap; |
There was a problem hiding this comment.
What benefit does a Lazy get us here?
There was a problem hiding this comment.
Probably none, I will remove it, thanks.
| [Theory] | ||
| [InlineData("8.0.100")] | ||
| [InlineData("9.1.100")] | ||
| public void DifferentMajorMinorVersion(string version) |
There was a problem hiding this comment.
Think we should add some tests for same major, different minor, and same major/minor, different band. IE:
- 9.0.100 vs 9.1.100
- 9.0.100 vs 9.0.200
- 9.0.100 vs 9.0.101
Think it would also be good to add tests for 9 vs 10, in both major and minor spots, to make sure that we're interpreting numbers correctly as versions. IE:
- 9.0.100 vs 10.0.100
- 9.9.100 vs 9.10.100
test/Microsoft.Net.Sdk.AnalyzerRedirecting.Tests/SdkAnalyzerAssemblyRedirectorTests.cs
Show resolved
Hide resolved
