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
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 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
src/Microsoft.Net.Sdk.AnalyzerRedirecting/SdkAnalyzerAssemblyRedirector.cs
Show resolved
Hide resolved
src/Microsoft.Net.Sdk.AnalyzerRedirecting/SdkAnalyzerAssemblyRedirector.cs
Show resolved
Hide resolved
test/Microsoft.Net.Sdk.AnalyzerRedirecting.Tests/SdkAnalyzerAssemblyRedirectorTests.cs
Show resolved
Hide resolved
Yeah, there are source build errors like the following which I hope will go away once the Roslyn PR is merged and we can update it properly here. Failed to synchronize repo sdk Failed to find all requested refs (6b364f666a4014bc8269131ef30df18c16a83511, 889d6fefa9e7ffb51f97ee1628856d27697c8fc9) in https://github.com/dotnet/roslyn. |
jjonescz
force-pushed
the
analyzer-redirecting
branch
from
1a77bc9 to
cf5c1f3
Compare
| @@ -67,6 +67,8 @@ | |||
| <PackageVersion Include="Microsoft.TestPlatform.Build" Version="$(MicrosoftTestPlatformBuildPackageVersion)" /> | |||
| <PackageVersion Include="Microsoft.TestPlatform.CLI" Version="$(MicrosoftTestPlatformCLIPackageVersion)" /> | |||
| <PackageVersion Include="Microsoft.VisualStudio.Composition" Version="17.4.16" /> | |||
| <PackageVersion Include="Microsoft.VisualStudio.Sdk" Version="17.2.32505.173" /> | |||
There was a problem hiding this comment.
Do you know how often these get updated and how best to keep them updated?
There was a problem hiding this comment.
This gets a non-preview release with every VS minor version. But I don't think this needs to be updated unless a new functionality from the VS SDK is needed (or if there are some transitive dependency conflicts). That's why it's fine that this is using an old version 17.2 instead of the latest 17.13 (but the real reason I chose 17.2 was because there were some conflicts with transitive dependencies IIRC).
| <IncludeDebugSymbolsInVSIXContainer>true</IncludeDebugSymbolsInVSIXContainer> | ||
| <IncludeDebugSymbolsInLocalVSIXDeployment>true</IncludeDebugSymbolsInLocalVSIXDeployment> | ||
| <IncludeCopyLocalReferencesInVSIXContainer>false</IncludeCopyLocalReferencesInVSIXContainer> | ||
| <VsixVersion Condition="'$(VsixVersion)' == ''">42.42.42.4242424</VsixVersion> |
There was a problem hiding this comment.
when is this ever set to a different value?
There was a problem hiding this comment.
Hm, good point, I thought arcade would set this, but maybe something is missing.
There was a problem hiding this comment.
It does look like the arcade targets set this. src/Microsoft.DotNet.Arcade.Sdk/tools/VisualStudio.targets. I think it does set this to a default version if not specified. So maybe this line is not necessary?
There was a problem hiding this comment.
Ah, okay, I've remembered why this is needed. (I will add a comment.)
Arcade imports VisualStudio.targets only on .NET Framework MSBuild. But SDK's CI builds are using Core MSBuild. Filed also dotnet/arcade#15617.
when is this ever set to a different value?
And yes, it's set to a different value in production builds, see the target GetVsixVersion below in this file.
src/Microsoft.Net.Sdk.AnalyzerRedirecting/Microsoft.Net.Sdk.AnalyzerRedirecting.csproj
Show resolved
Hide resolved
| @@ -405,6 +409,27 @@ | |||
| </ItemGroup> | |||
| </Target> | |||
|
|
|||
| <Target Name="GenerateRuntimeAnalyzersNupkg" | |||
| DependsOnTargets="GenerateLayout;MsiTargetsSetupInputOutputs" | |||
| Condition=" '$(OS)' == 'Windows_NT' And '$(Architecture)' == 'x64' And '$(DotNetBuild)' != 'true' " | |||
There was a problem hiding this comment.
@jjonescz I just saw this PR. Why did you add the DotNetBuild condition here? Shouldn't the package get produced in the product build? Note that starting with P4 we will ship the product from the VMR and this asset will be missing from the product then.
There was a problem hiding this comment.
Thanks, I will look into removing the condition.
