-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Multi-Targeting Roslyn analyzers with support for pre-SDK format projects #41352
Comments
@jaredpar is this something that you'd need to look at? I think the SDK has some influence on what analyzer DLLs are loaded for a given build, so it could be a bug on our side as well, but IIRC you usually like to take a poke at analyzer-loading-related issues first. |
The good news is that I'm glad I invested in making the consistency check error message clearer cause it really makes this situation easier to understand. The bad news is that it won't be in the compiler until 17.11. I'll run this on a 17.11 build tomorrow and see what the message is. The very likely cause though is that there are two xunit.analyzers.dll instances in the build graph now with the same identity (assembly name + version) but different MVID. The compiler server is properly shutting down because it can't load both of them into the same process. Haven't looked at the build yet but I'm guessing one of these package is bringing along a copy of xunit.analyzers.dll which is conflicting with the official one. |
Can see from the binlog here that the xunit.analyzers.dll and xunit.analyzers.fixes.dll is being passed twice to the compiler. That leads to an immediate conflict and will shut down the server. This is being added by the I think this is a bug in how xunit is packaging here. I believe that rather than putting the fallback DLLs in At the compiler layer we can't do operations like de-dupe that needs to be done at a higher level. |
Yeah, then the analyzer does not work in the old csproj (XML) format, that was the initial reason why Brad added the 'dotnet/cs' folder. I tried a few variations of the same theme, incl. a 'roslyn1.0' folder (read that in the original issue). No variation, I tried, except Brad's original method, worked with the old project format. |
Is there a binlog available of 'the older format' and its results? I'd like to diff the behaviors here. It sounds like the SDK itself might need to add some compat/fallback behavior compensation for packages like these. |
I don't have one readily available, I can produce one as soon as I'm back on a PC. |
The SDK could do this. Essentially recognize that when there is the following pattern:
Then don't auto add the That approach though feels like we're just digging the hole deeper. Essentially we have a behavior gap between |
Because I always forget this, that target + Task implementation live over at https://github.com/dotnet/NuGet.BuildTasks. |
Correct. From #20355
|
The 'roslyn1.0' trick does not appear to work for the old csproj format. |
The way we made "old csproj format" projects work for dotnet/runtime packages is to add an MSBuild targets file in the
This is what it looks like for the <Project>
<Target Name="_System_Text_JsonGatherAnalyzers">
<ItemGroup>
<_System_Text_JsonAnalyzer Include="@(Analyzer)" Condition="'%(Analyzer.NuGetPackageId)' == 'System.Text.Json'" />
</ItemGroup>
</Target>
<Target Name="_System_Text_JsonAnalyzerMultiTargeting"
Condition="'$(SupportsRoslynComponentVersioning)' != 'true'"
AfterTargets="ResolvePackageDependenciesForBuild;ResolveNuGetPackageAssets"
DependsOnTargets="_System_Text_JsonGatherAnalyzers">
<ItemGroup>
<!-- Remove our analyzers targeting roslyn4.x -->
<Analyzer Remove="@(_System_Text_JsonAnalyzer)"
Condition="$([System.String]::Copy('%(_System_Text_JsonAnalyzer.Identity)').IndexOf('roslyn4')) >= 0"/>
</ItemGroup>
</Target>
<Target Name="_System_Text_JsonRemoveAnalyzers"
Condition="'$(DisableSystemTextJsonSourceGenerator)' == 'true'"
AfterTargets="ResolvePackageDependenciesForBuild;ResolveNuGetPackageAssets"
DependsOnTargets="_System_Text_JsonGatherAnalyzers">
<!-- Remove all our analyzers -->
<ItemGroup>
<Analyzer Remove="@(_System_Text_JsonAnalyzer)" />
</ItemGroup>
</Target>
</Project> dotnet/NuGet.BuildTasks#152 is tracking supporting it in the "old project" NuGet sources, but that hasn't received much traction. |
Yeah, i tried to adapt that one, but failed, as written in the original Post. All the examples I could find were removing 'too new', not the inverse, but as stated, I'm not really proficient with msbuild syntax |
I appreciate the work put into getting that working but it feels like patching over the problem. Is there a reason we didn't push the issue with |
I believe it was a combination of:
Do we know who owns https://github.com/dotnet/NuGet.BuildTasks/? |
If you can send me a binlog of what you tried, I can try to spot the problem. I don't really understand the "All the examples I could find were removing 'too new', not the inverse" comment. If you are in an "old format" project, how do you know what version of Roslyn the project is using? That's why you need to load the oldest one possible - because you don't know which one will work. |
I apologize if I was too obtuse. The 'dotnet/cs' path nuspec entry works with the old format, no other attempt of modifying the nuspec kept it working, so my attempt was to do the inverse of the .target files you posted, i.e. to remove that analyzer from that path from projects where 'SupportsRoslynComponentVersioning' is true, i.e., which support the versioned roslyn approach and that's where I failed. Admittingly I did not spend too much time on that approach, because I don't even know how to debug msbuild targets and the only evidence if it worked was if the node in the solution tree was populated.
The goal here is not to load the oldest one in VS with newer roslyn support, but to load the newest one the VS version can support, in line with your original PR which added versioned roslyn support. From my POV the current loading behavior should consider that non-versioned folder simply as 'roslyn0.9', if other versioned analyzer folders exist. |
I'm saying that unless you know what version Roslyn is being used, you don't know which one the VS version can support.
See the reasoning above. |
@baronfel The old file format project is a plain vanilla .NET 4.7.2 project, I put the content in the details below. <?xml version="1.0" encoding="utf-8"?>
<Project ToolsVersion="15.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<Import Project="packages\xunit.core.2.8.1\build\xunit.core.props" Condition="Exists('packages\xunit.core.2.8.1\build\xunit.core.props')" />
<Import Project="$(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\Microsoft.Common.props" Condition="Exists('$(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\Microsoft.Common.props')" />
<PropertyGroup>
<Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
<Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
<ProjectGuid>{EEFC6F63-DB8C-43DE-919B-E2A58D7F23C8}</ProjectGuid>
<OutputType>Library</OutputType>
<RootNamespace>Project1</RootNamespace>
<AssemblyName>Project1</AssemblyName>
<TargetFrameworkVersion>v4.7.2</TargetFrameworkVersion>
<FileAlignment>512</FileAlignment>
<AutoGenerateBindingRedirects>true</AutoGenerateBindingRedirects>
<Deterministic>true</Deterministic>
<NuGetPackageImportStamp>
</NuGetPackageImportStamp>
</PropertyGroup>
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' ">
<PlatformTarget>AnyCPU</PlatformTarget>
<DebugSymbols>true</DebugSymbols>
<DebugType>full</DebugType>
<Optimize>false</Optimize>
<OutputPath>bin\Debug\</OutputPath>
<DefineConstants>DEBUG;TRACE</DefineConstants>
<ErrorReport>prompt</ErrorReport>
<WarningLevel>4</WarningLevel>
</PropertyGroup>
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' ">
<PlatformTarget>AnyCPU</PlatformTarget>
<DebugType>pdbonly</DebugType>
<Optimize>true</Optimize>
<OutputPath>bin\Release\</OutputPath>
<DefineConstants>TRACE</DefineConstants>
<ErrorReport>prompt</ErrorReport>
<WarningLevel>4</WarningLevel>
</PropertyGroup>
<PropertyGroup>
<StartupObject />
</PropertyGroup>
<ItemGroup>
<None Include="App.config" />
<None Include="packages.config" />
</ItemGroup>
<ItemGroup>
<Compile Include="Test.cs" />
</ItemGroup>
<ItemGroup>
<Reference Include="Microsoft.Bcl.AsyncInterfaces, Version=8.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51, processorArchitecture=MSIL">
<HintPath>packages\Microsoft.Bcl.AsyncInterfaces.8.0.0\lib\net462\Microsoft.Bcl.AsyncInterfaces.dll</HintPath>
</Reference>
<Reference Include="System" />
<Reference Include="System.Buffers, Version=4.0.3.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51, processorArchitecture=MSIL">
<HintPath>packages\System.Buffers.4.5.1\lib\net461\System.Buffers.dll</HintPath>
</Reference>
<Reference Include="System.Memory, Version=4.0.1.2, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51, processorArchitecture=MSIL">
<HintPath>packages\System.Memory.4.5.5\lib\net461\System.Memory.dll</HintPath>
</Reference>
<Reference Include="System.Numerics" />
<Reference Include="System.Numerics.Vectors, Version=4.1.4.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a, processorArchitecture=MSIL">
<HintPath>packages\System.Numerics.Vectors.4.5.0\lib\net46\System.Numerics.Vectors.dll</HintPath>
</Reference>
<Reference Include="System.Runtime.CompilerServices.Unsafe, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a, processorArchitecture=MSIL">
<HintPath>packages\System.Runtime.CompilerServices.Unsafe.6.0.0\lib\net461\System.Runtime.CompilerServices.Unsafe.dll</HintPath>
</Reference>
<Reference Include="System.Text.Encodings.Web, Version=8.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51, processorArchitecture=MSIL">
<HintPath>packages\System.Text.Encodings.Web.8.0.0\lib\net462\System.Text.Encodings.Web.dll</HintPath>
</Reference>
<Reference Include="System.Text.Json, Version=8.0.0.3, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51, processorArchitecture=MSIL">
<HintPath>packages\System.Text.Json.8.0.3\lib\net462\System.Text.Json.dll</HintPath>
</Reference>
<Reference Include="System.Threading.Tasks.Extensions, Version=4.2.0.1, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51, processorArchitecture=MSIL">
<HintPath>packages\System.Threading.Tasks.Extensions.4.5.4\lib\net461\System.Threading.Tasks.Extensions.dll</HintPath>
</Reference>
<Reference Include="System.ValueTuple, Version=4.0.3.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51, processorArchitecture=MSIL">
<HintPath>packages\System.ValueTuple.4.5.0\lib\net47\System.ValueTuple.dll</HintPath>
</Reference>
<Reference Include="xunit.abstractions, Version=2.0.0.0, Culture=neutral, PublicKeyToken=8d05b1bb7a6fdb6c, processorArchitecture=MSIL">
<HintPath>packages\xunit.abstractions.2.0.3\lib\net35\xunit.abstractions.dll</HintPath>
</Reference>
<Reference Include="xunit.assert, Version=2.8.1.0, Culture=neutral, PublicKeyToken=8d05b1bb7a6fdb6c, processorArchitecture=MSIL">
<HintPath>packages\xunit.assert.2.8.1\lib\netstandard1.1\xunit.assert.dll</HintPath>
</Reference>
<Reference Include="xunit.core, Version=2.8.1.0, Culture=neutral, PublicKeyToken=8d05b1bb7a6fdb6c, processorArchitecture=MSIL">
<HintPath>packages\xunit.extensibility.core.2.8.1\lib\net452\xunit.core.dll</HintPath>
</Reference>
<Reference Include="xunit.execution.desktop, Version=2.8.1.0, Culture=neutral, PublicKeyToken=8d05b1bb7a6fdb6c, processorArchitecture=MSIL">
<HintPath>packages\xunit.extensibility.execution.2.8.1\lib\net452\xunit.execution.desktop.dll</HintPath>
</Reference>
</ItemGroup>
<ItemGroup>
<Analyzer Include="packages\xunit.analyzers.1.14.0\analyzers\dotnet\cs\xunit.analyzers.dll" />
<Analyzer Include="packages\xunit.analyzers.1.14.0\analyzers\dotnet\cs\xunit.analyzers.fixes.dll" />
</ItemGroup>
<Import Project="$(MSBuildToolsPath)\Microsoft.CSharp.targets" />
<Target Name="EnsureNuGetPackageBuildImports" BeforeTargets="PrepareForBuild">
<PropertyGroup>
<ErrorText>This project references NuGet package(s) that are missing on this computer. Use NuGet Package Restore to download them. For more information, see http://go.microsoft.com/fwlink/?LinkID=322105. The missing file is {0}.</ErrorText>
</PropertyGroup>
<Error Condition="!Exists('packages\xunit.core.2.8.1\build\xunit.core.props')" Text="$([System.String]::Format('$(ErrorText)', 'packages\xunit.core.2.8.1\build\xunit.core.props'))" />
<Error Condition="!Exists('packages\xunit.core.2.8.1\build\xunit.core.targets')" Text="$([System.String]::Format('$(ErrorText)', 'packages\xunit.core.2.8.1\build\xunit.core.targets'))" />
</Target>
<Import Project="packages\xunit.core.2.8.1\build\xunit.core.targets" Condition="Exists('packages\xunit.core.2.8.1\build\xunit.core.targets')" />
</Project> app.config <?xml version="1.0" encoding="utf-8" ?>
<configuration>
<startup>
<supportedRuntime version="v4.0" sku=".NETFramework,Version=v4.7.2" />
</startup>
</configuration> packages.config <?xml version="1.0" encoding="utf-8"?>
<packages>
<package id="Microsoft.Bcl.AsyncInterfaces" version="8.0.0" targetFramework="net472" />
<package id="System.Buffers" version="4.5.1" targetFramework="net472" />
<package id="System.Memory" version="4.5.5" targetFramework="net472" />
<package id="System.Numerics.Vectors" version="4.5.0" targetFramework="net472" />
<package id="System.Runtime.CompilerServices.Unsafe" version="6.0.0" targetFramework="net472" />
<package id="System.Text.Encodings.Web" version="8.0.0" targetFramework="net472" />
<package id="System.Text.Json" version="8.0.3" targetFramework="net472" />
<package id="System.Threading.Tasks.Extensions" version="4.5.4" targetFramework="net472" />
<package id="System.ValueTuple" version="4.5.0" targetFramework="net472" />
<package id="xunit" version="2.8.1" targetFramework="net472" />
<package id="xunit.abstractions" version="2.0.3" targetFramework="net472" />
<package id="xunit.analyzers" version="1.14.0" targetFramework="net472" developmentDependency="true" />
<package id="xunit.assert" version="2.8.1" targetFramework="net472" />
<package id="xunit.core" version="2.8.1" targetFramework="net472" />
<package id="xunit.extensibility.core" version="2.8.1" targetFramework="net472" />
<package id="xunit.extensibility.execution" version="2.8.1" targetFramework="net472" />
</packages> |
I understand that perfectly fine, that's why the idea was to identify the non-versioned analyzer, remove that one and hopefully let the changes from your PR do the task of selecting the correct one. The goal of my attempts was basically to modify the nuget import logic to ignore/remove the 'dotnet/cs' folder and let everything else intact and that's exactly where I failed, how to identify the identity of the analyzer from the 'dotnet/cs' folder, I kinda hoped to find the path in there somewhere or something similar.
Yes, that's why I tried roslyn1.0, as that was written exactly there in the case you need it In any case we deviate from the problem here, If you say this an packaging issue in xUnit, Brad already pushed a pre-release build to the pre-release feed which removes everything except roslyn3.11, based on a suggestion by @sharwell. Unfortunately the build time issue due to restarting the compilation server was only found after the release nuget package was done, this means a lot of people will encounter the slowdown, which is unfortunately quite noticeable and xunit.analyzers is a rather common nuget as it is a default part of xunit. I have no skin in the game here, I just found the build time issue, tracked it down to the xunit.analyzer package, Brad said he does not believe it's an xUnit issue, I agreed with that and opened the issue here, which from my pov is the least I can do as a community member. Now you can decide if you want to pass the ball back to Brad, fix it here or pass the ball to someone else. But reality has caught up with us and 352.674 times the affected nuget package has been downloaded by now, so if you happen to have telemetry data for that, please look up how often the compilation server restarted due to the issue from the beginning, my estimation is a number of somewhere in the tens of millions. (My projects alone are probably already in the range of 100 thousand times). And that adds up to a lot of wasted build time. |
This is very problematic. For example: roslyn won't be able to take this version of the NuPkg because it would be blocked by our PR system for this reason. |
As much as i dislike bifurcating the targets further, I think we should consider the structure of the xunit package as the standard for how you have an unconditional fallback. Basically use |
Question - we've run into this issue with our analyzers too and have users asking about it. Should we wait for a tooling solution from the SDK team or adopt the same work-around xUnit has? |
do the workaround @Aaronontheweb - we don't have a timeline to make this change yet. |
xUnit recently added for their xunit.analyzers nuget package the folder 'analyzers/dotnet/cs' with a roslyn 3.11 analyzer in parallel to all the other versioned roslyn analyzers (analyzers/dotnet/roslyn4.4/cs for example). Otherwise the roslyn analyzer would not work in pre-SDK format projects anymore.
The outcome of that is, that while the analyzer now works in pre-SDK format projects, the build process is several times slower per test project as apparently the compilation server needs to be restarted, because it loads the wrong dlls on a VS 17.10 installation.
I looked through the comments and solutions in #20355, but the examples linked there, e.g. the one for refit by @sharwell or by @eerhardt for S.T.J apply for the other way round, i.e. preventing an older SDK Version from loading a 'too new' src gen, I tried that approach anyway, but couldn't get it working, the analyzers node in the solution tree in VS is empty, admittingly I do not understand enough about MsBuild targets ;)
This is sufficently annoying, as per test project the build now takes roughly 4s longer, in my solution the build went up from 50s to roughly 150s (which fits, there are around ~30 test projects).
The original issue in xUnit is xunit/xunit#2943
Repro:
As for repro, a pretty much empty Test Project in VS does the trick
Then run from command line:
The output will now contain the compiler server error message, at the end of step 'CoreCompile'
You also see a noticeable pause after the error message. If you downgrade to 2.8.0, both the error message and pause are gone.
The text was updated successfully, but these errors were encountered: