Improve loading of Roslyn assemblies on validate package task #22277
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Anipik
reviewed
Oct 22, 2021
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Compatibility.Common.targets
Show resolved
Hide resolved
Anipik
approved these changes
Oct 22, 2021
ericstj
reviewed
Oct 22, 2021
src/Compatibility/Microsoft.DotNet.Compatibility/ValidatePackage.cs
Outdated
Show resolved
Hide resolved
src/Compatibility/Microsoft.DotNet.Compatibility/ValidatePackage.cs
Outdated
Show resolved
Hide resolved
src/Compatibility/Microsoft.DotNet.Compatibility/ValidatePackage.cs
Outdated
Show resolved
Hide resolved
joperezr
reviewed
Oct 22, 2021
joperezr
approved these changes
Oct 22, 2021
ericstj
reviewed
Oct 22, 2021
| private Assembly LoadRoslyn(AssemblyName name, Func<string, Assembly> loadFromPath) | ||
| { | ||
| const string codeAnalysisName = "Microsoft.CodeAnalysis"; | ||
| if (name.Name.StartsWith(codeAnalysisName, StringComparison.Ordinal)) |
There was a problem hiding this comment.
Do we care about some other random assembly that matches the Microsoft.CodeAnalysis prefix but isn't one we'd care about? It seems unlikely since we're only hooking this for the duration of the task execution so it should be only our code running in the AppDomain at the time. I think that's the case at least.
There was a problem hiding this comment.
That's why I changed it to the prefix cause it seemed simple and I don't see any reason why the AppDomain nor the Task ALC (which is exclusively used by this task) would try to load another Roslyn assembly. That said though, I don't mind being more defensive and doing a strict comparison for the 2 we know we care 😊
There was a problem hiding this comment.
I went the defensive side and explicitly checked for both assemblies as we had it.
ericstj
approved these changes
Oct 22, 2021
safern
force-pushed
the
ImproveRoslynLoadingPV
branch
from
a9ddf5e to
4ba8e3e
Compare
This was referenced Oct 26, 2021
Merged
ericstj
added a commit
to dotnet/runtime
that referenced
this pull request
Mar 11, 2022
This fixes an issue where a non-CSProj built package would load MS.CA.dll , then a CSProj would try to load MS.CA.CS.dll of a different version and it's MS.CA types would not agree with those already bound to the task from previous load. Further details: dotnet/sdk#22277
mmitche
pushed a commit
to dotnet/runtime
that referenced
this pull request
Mar 14, 2022
…6144) * Update dependencies from https://github.com/dotnet/arcade build 2022022.7 Microsoft.DotNet.XUnitConsoleRunner , Microsoft.DotNet.CodeAnalysis , Microsoft.DotNet.Build.Tasks.Workloads , Microsoft.DotNet.Build.Tasks.Templating , Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk , Microsoft.DotNet.Build.Tasks.Packaging , Microsoft.DotNet.Build.Tasks.Installers , Microsoft.DotNet.Build.Tasks.Feed , Microsoft.DotNet.Build.Tasks.Archives , Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.ApiCompat , Microsoft.DotNet.XUnitExtensions , Microsoft.DotNet.GenAPI , Microsoft.DotNet.VersionTools.Tasks , Microsoft.DotNet.GenFacades , Microsoft.DotNet.SharedFramework.Sdk , Microsoft.DotNet.RemoteExecutor , Microsoft.DotNet.PackageTesting , Microsoft.DotNet.Helix.Sdk From Version 2.5.1-beta.22107.2 -> To Version 2.5.1-beta.22122.7 * Update dependencies from https://github.com/dotnet/emsdk build 20220308.1 Microsoft.NET.Workload.Emscripten.Manifest-6.0.100 From Version 6.0.2 -> To Version 6.0.3 * Fix weird character problem * Update dependencies from https://github.com/dotnet/emsdk build 20220308.2 Microsoft.NET.Workload.Emscripten.Manifest-6.0.100 From Version 6.0.2 -> To Version 6.0.4 * Update dependencies from https://github.com/dotnet/arcade build 20220309.8 Microsoft.DotNet.XUnitConsoleRunner , Microsoft.DotNet.CodeAnalysis , Microsoft.DotNet.Build.Tasks.Workloads , Microsoft.DotNet.Build.Tasks.Templating , Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk , Microsoft.DotNet.Build.Tasks.Packaging , Microsoft.DotNet.Build.Tasks.Installers , Microsoft.DotNet.Build.Tasks.Feed , Microsoft.DotNet.Build.Tasks.Archives , Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.ApiCompat , Microsoft.DotNet.XUnitExtensions , Microsoft.DotNet.GenAPI , Microsoft.DotNet.VersionTools.Tasks , Microsoft.DotNet.GenFacades , Microsoft.DotNet.SharedFramework.Sdk , Microsoft.DotNet.RemoteExecutor , Microsoft.DotNet.PackageTesting , Microsoft.DotNet.Helix.Sdk From Version 2.5.1-beta.22107.2 -> To Version 2.5.1-beta.22159.8 * On run package validation for CSProj-built packages This fixes an issue where a non-CSProj built package would load MS.CA.dll , then a CSProj would try to load MS.CA.CS.dll of a different version and it's MS.CA types would not agree with those already bound to the task from previous load. Further details: dotnet/sdk#22277 * Update dependencies from https://github.com/dotnet/emsdk build 20220311.2 Microsoft.NET.Workload.Emscripten.Manifest-6.0.100 , Microsoft.NET.Workload.Emscripten.Manifest-6.0.200 , Microsoft.NET.Workload.Emscripten.Manifest-6.0.300 From Version 6.0.4 -> To Version 6.0.4 * Update dependencies from https://github.com/dotnet/arcade build 20220311.1 Microsoft.DotNet.XUnitConsoleRunner , Microsoft.DotNet.CodeAnalysis , Microsoft.DotNet.Build.Tasks.Workloads , Microsoft.DotNet.Build.Tasks.Templating , Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk , Microsoft.DotNet.Build.Tasks.Packaging , Microsoft.DotNet.Build.Tasks.Installers , Microsoft.DotNet.Build.Tasks.Feed , Microsoft.DotNet.Build.Tasks.Archives , Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.ApiCompat , Microsoft.DotNet.XUnitExtensions , Microsoft.DotNet.GenAPI , Microsoft.DotNet.VersionTools.Tasks , Microsoft.DotNet.GenFacades , Microsoft.DotNet.SharedFramework.Sdk , Microsoft.DotNet.RemoteExecutor , Microsoft.DotNet.PackageTesting , Microsoft.DotNet.Helix.Sdk From Version 2.5.1-beta.22107.2 -> To Version 2.5.1-beta.22161.1 Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Steve Pfister <steve.pfister@microsoft.com> Co-authored-by: Alexander Köplinger <alex.koeplinger@outlook.com> Co-authored-by: Eric StJohn <ericstj@microsoft.com>
joperezr
pushed a commit
that referenced
this pull request
Mar 14, 2022
* Improve loading of Roslyn assemblies on validate package task * PR Feedback
This was referenced Dec 13, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With: dotnet/runtime#59908 we realized various things:
We were loading the roslyn assemblies into the default Assembly Load Context because we were using
Assembly.LoadFromon .NET Core and that is a legacy API on Core which just loads the assembly to the default load context. That could lead to different problems one of them for example: if there is an MSBuild Task that runs before package validation and that task depends on Microsoft.CodeAnalysis and carries it's own copy of it with the package and it is the same assembly version, since it could be loaded into the same default Assembly Load Context, we could endup in a state where we loadMicrosoft.CodeAnalysis.CSharpfrom a different path and have miss match problems.If the toolset package is referenced, we were only setting the RoslynAssembliesPath for projects with the
.csprojorvbprojextensions, so for example indotnet/runtimewe have an.ilprojand another project with.pkgprojextension which because of this reason, ended using the roslyn from the SDK and causing this possible miss match described on point 1.If package validation runs on a package that has no
liborrefassets, say a tools or a native package,API Compatwould never be invoked so onlyMicrosoft.CodeAnalysiswould be loaded, leading to potential miss matches ofMicrosoft.CodeAnalysis.dllvsMicrosoft.CodeAnalysis.CSharp.dllwhenever we run package validation on a package on the same build where we do need to runAPI Compat(which is actually what was causing the issue in dotnet/runtime) even if loading the assemblies into the Task ALC rather than the default ALC. So to fix this we will defensively load both assemblies when either one of them is requested to make sure on the first instance of the task on the current MSBuild node, we load both assemblies from the same location regardless of if we need them or not into the task ALC.What we encountered in dotnet/runtime was a combination of 2 and 3.