From 27276f33d09a47cc1f8dc924d4c3663368034f07 Mon Sep 17 00:00:00 2001 From: Rainer Sigwald Date: Fri, 6 Dec 2019 14:19:28 -0600 Subject: [PATCH 1/7] Introduce FEATURE_ASSEMBLYLOADCONTEXT ```sh-session $ rep -r "#if !FEATURE_ASSEMBLY_LOADFROM" "#if FEATURE_ASSEMBLYLOADCONTEXT" *.cs 1680 files processed. 4 files changed. 8 replacements. $ rep -r "#if FEATURE_ASSEMBLY_LOADFROM" "#if !FEATURE_ASSEMBLYLOADCONTEXT" *.cs 1680 files processed. 7 files changed. 29 replacements. ``` And manual move/rename of the definition. --- .../BackEnd/TaskExecutionHost_Tests.cs | 2 +- .../BackEnd/TaskHostConfiguration_Tests.cs | 12 ++++---- .../SdkResolution/SdkResolverLoader.cs | 4 +-- src/Directory.BeforeCommon.targets | 2 +- src/Shared/AssemblyNameExtension.cs | 4 +-- src/Shared/TaskEngineAssemblyResolver.cs | 2 +- src/Shared/TypeLoader.cs | 8 +++--- .../AssemblyDependency/AssemblyInformation.cs | 28 +++++++++---------- src/Tasks/GenerateResource.cs | 16 +++++------ 9 files changed, 39 insertions(+), 39 deletions(-) diff --git a/src/Build.UnitTests/BackEnd/TaskExecutionHost_Tests.cs b/src/Build.UnitTests/BackEnd/TaskExecutionHost_Tests.cs index 6b9360ee0ea..f2c4d85b94f 100644 --- a/src/Build.UnitTests/BackEnd/TaskExecutionHost_Tests.cs +++ b/src/Build.UnitTests/BackEnd/TaskExecutionHost_Tests.cs @@ -1157,7 +1157,7 @@ private void InitializeHost(bool throwOnExecute) ProjectInstance project = CreateTestProject(); TypeLoader typeLoader = new TypeLoader(IsTaskFactoryClass); -#if FEATURE_ASSEMBLY_LOADFROM +#if !FEATURE_ASSEMBLYLOADCONTEXT AssemblyLoadInfo loadInfo = AssemblyLoadInfo.Create(Assembly.GetAssembly(typeof(TaskBuilderTestTask.TaskBuilderTestTaskFactory)).FullName, null); #else AssemblyLoadInfo loadInfo = AssemblyLoadInfo.Create(typeof(TaskBuilderTestTask.TaskBuilderTestTaskFactory).GetTypeInfo().FullName, null); diff --git a/src/Build.UnitTests/BackEnd/TaskHostConfiguration_Tests.cs b/src/Build.UnitTests/BackEnd/TaskHostConfiguration_Tests.cs index 716b94050f0..e13787dfe95 100644 --- a/src/Build.UnitTests/BackEnd/TaskHostConfiguration_Tests.cs +++ b/src/Build.UnitTests/BackEnd/TaskHostConfiguration_Tests.cs @@ -114,7 +114,7 @@ public void ConstructorWithNullLocation() ); } -#if FEATURE_ASSEMBLY_LOADFROM +#if !FEATURE_ASSEMBLYLOADCONTEXT /// /// Test that an exception is thrown when the path to the task assembly is empty /// @@ -267,7 +267,7 @@ public void TestTranslationWithNullDictionary() TaskHostConfiguration deserializedConfig = packet as TaskHostConfiguration; Assert.Equal(config.TaskName, deserializedConfig.TaskName); -#if FEATURE_ASSEMBLY_LOADFROM +#if !FEATURE_ASSEMBLYLOADCONTEXT Assert.Equal(config.TaskLocation, deserializedConfig.TaskLocation); #endif Assert.Null(deserializedConfig.TaskParameters); @@ -305,7 +305,7 @@ public void TestTranslationWithEmptyDictionary() TaskHostConfiguration deserializedConfig = packet as TaskHostConfiguration; Assert.Equal(config.TaskName, deserializedConfig.TaskName); -#if FEATURE_ASSEMBLY_LOADFROM +#if !FEATURE_ASSEMBLYLOADCONTEXT Assert.Equal(config.TaskLocation, deserializedConfig.TaskLocation); #endif Assert.NotNull(deserializedConfig.TaskParameters); @@ -348,7 +348,7 @@ public void TestTranslationWithValueTypesInDictionary() TaskHostConfiguration deserializedConfig = packet as TaskHostConfiguration; Assert.Equal(config.TaskName, deserializedConfig.TaskName); -#if FEATURE_ASSEMBLY_LOADFROM +#if !FEATURE_ASSEMBLYLOADCONTEXT Assert.Equal(config.TaskLocation, deserializedConfig.TaskLocation); #endif Assert.NotNull(deserializedConfig.TaskParameters); @@ -389,7 +389,7 @@ public void TestTranslationWithITaskItemInDictionary() TaskHostConfiguration deserializedConfig = packet as TaskHostConfiguration; Assert.Equal(config.TaskName, deserializedConfig.TaskName); -#if FEATURE_ASSEMBLY_LOADFROM +#if !FEATURE_ASSEMBLYLOADCONTEXT Assert.Equal(config.TaskLocation, deserializedConfig.TaskLocation); #endif Assert.NotNull(deserializedConfig.TaskParameters); @@ -429,7 +429,7 @@ public void TestTranslationWithITaskItemArrayInDictionary() TaskHostConfiguration deserializedConfig = packet as TaskHostConfiguration; Assert.Equal(config.TaskName, deserializedConfig.TaskName); -#if FEATURE_ASSEMBLY_LOADFROM +#if !FEATURE_ASSEMBLYLOADCONTEXT Assert.Equal(config.TaskLocation, deserializedConfig.TaskLocation); #endif Assert.NotNull(deserializedConfig.TaskParameters); diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs index 612fd2ea1a1..255b0354725 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs @@ -16,7 +16,7 @@ namespace Microsoft.Build.BackEnd.SdkResolution { internal class SdkResolverLoader { -#if !FEATURE_ASSEMBLY_LOADFROM +#if FEATURE_ASSEMBLYLOADCONTEXT private readonly CoreClrAssemblyLoader _loader = new CoreClrAssemblyLoader(); #endif @@ -135,7 +135,7 @@ protected virtual IEnumerable GetResolverTypes(Assembly assembly) protected virtual Assembly LoadResolverAssembly(string resolverPath, LoggingContext loggingContext, ElementLocation location) { -#if FEATURE_ASSEMBLY_LOADFROM +#if !FEATURE_ASSEMBLYLOADCONTEXT return Assembly.LoadFrom(resolverPath); #else _loader.AddDependencyLocation(Path.GetDirectoryName(resolverPath)); diff --git a/src/Directory.BeforeCommon.targets b/src/Directory.BeforeCommon.targets index e0317fa9eda..4d85540d8d9 100644 --- a/src/Directory.BeforeCommon.targets +++ b/src/Directory.BeforeCommon.targets @@ -24,7 +24,6 @@ true $(DefineConstants);FEATURE_APPDOMAIN_UNHANDLED_EXCEPTION $(DefineConstants);FEATURE_ASPNET_COMPILER - $(DefineConstants);FEATURE_ASSEMBLY_LOADFROM $(DefineConstants);FEATURE_ASSEMBLY_LOCATION $(DefineConstants);FEATURE_ASSEMBLY_GETENTRYASSEMBLY $(DefineConstants);FEATURE_ASSEMBLYNAME_CLONE @@ -120,6 +119,7 @@ $([System.IO.Path]::Combine($(ToolPackagesDir)Microsoft.Net.Compilers, $(CompilerToolsVersion), "tools"))$([System.IO.Path]::DirectorySeparatorChar) + $(DefineConstants);FEATURE_ASSEMBLYLOADCONTEXT $(DefineConstants);FEATURE_PROCESSSTARTINFO_ENVIRONMENT $(DefineConstants);FEATURE_RUNTIMEINFORMATION $(DefineConstants);USE_MSBUILD_DLL_EXTN diff --git a/src/Shared/AssemblyNameExtension.cs b/src/Shared/AssemblyNameExtension.cs index a4b0e0e2e00..78b09cdefbf 100644 --- a/src/Shared/AssemblyNameExtension.cs +++ b/src/Shared/AssemblyNameExtension.cs @@ -11,7 +11,7 @@ using System.Configuration.Assemblies; using System.Runtime.Serialization; using System.IO; -#if !FEATURE_ASSEMBLY_LOADFROM +#if FEATURE_ASSEMBLYLOADCONTEXT using System.Reflection.PortableExecutable; using System.Reflection.Metadata; #endif @@ -183,7 +183,7 @@ private AssemblyNameExtension(SerializationInfo info, StreamingContext context) internal static AssemblyNameExtension GetAssemblyNameEx(string path) { AssemblyName assemblyName = null; -#if FEATURE_ASSEMBLY_LOADFROM +#if !FEATURE_ASSEMBLYLOADCONTEXT try { assemblyName = AssemblyName.GetAssemblyName(path); diff --git a/src/Shared/TaskEngineAssemblyResolver.cs b/src/Shared/TaskEngineAssemblyResolver.cs index 91aef3e99f7..e58b0ea73f0 100644 --- a/src/Shared/TaskEngineAssemblyResolver.cs +++ b/src/Shared/TaskEngineAssemblyResolver.cs @@ -7,7 +7,7 @@ using System.Diagnostics; using System.Globalization; -#if !FEATURE_ASSEMBLY_LOADFROM +#if FEATURE_ASSEMBLYLOADCONTEXT using System.Runtime.Loader; #endif using Microsoft.Build.Shared; diff --git a/src/Shared/TypeLoader.cs b/src/Shared/TypeLoader.cs index d0cb1bf1aa6..95befd0430e 100644 --- a/src/Shared/TypeLoader.cs +++ b/src/Shared/TypeLoader.cs @@ -19,7 +19,7 @@ namespace Microsoft.Build.Shared /// internal class TypeLoader { -#if !FEATURE_ASSEMBLY_LOADFROM +#if FEATURE_ASSEMBLYLOADCONTEXT /// /// AssemblyContextLoader used to load DLLs outside of msbuild.exe directory /// @@ -41,7 +41,7 @@ internal class TypeLoader /// private Func _isDesiredType; -#if !FEATURE_ASSEMBLY_LOADFROM +#if FEATURE_ASSEMBLYLOADCONTEXT static TypeLoader() { s_coreClrAssemblyLoader = new CoreClrAssemblyLoader(); @@ -152,7 +152,7 @@ private static Assembly LoadAssembly(AssemblyLoadInfo assemblyLoadInfo) { if (assemblyLoadInfo.AssemblyName != null) { -#if FEATURE_ASSEMBLY_LOADFROM +#if !FEATURE_ASSEMBLYLOADCONTEXT loadedAssembly = Assembly.Load(assemblyLoadInfo.AssemblyName); #else loadedAssembly = Assembly.Load(new AssemblyName(assemblyLoadInfo.AssemblyName)); @@ -160,7 +160,7 @@ private static Assembly LoadAssembly(AssemblyLoadInfo assemblyLoadInfo) } else { -#if FEATURE_ASSEMBLY_LOADFROM +#if !FEATURE_ASSEMBLYLOADCONTEXT loadedAssembly = Assembly.UnsafeLoadFrom(assemblyLoadInfo.AssemblyFile); #else // If the Assembly is provided via a file path, the following rules are used to load the assembly: diff --git a/src/Tasks/AssemblyDependency/AssemblyInformation.cs b/src/Tasks/AssemblyDependency/AssemblyInformation.cs index c1d6344fec6..5acebabb7eb 100644 --- a/src/Tasks/AssemblyDependency/AssemblyInformation.cs +++ b/src/Tasks/AssemblyDependency/AssemblyInformation.cs @@ -14,7 +14,7 @@ using Microsoft.Build.Shared; using Microsoft.Build.Shared.FileSystem; -#if !FEATURE_ASSEMBLY_LOADFROM || MONO +#if FEATURE_ASSEMBLYLOADCONTEXT || MONO using System.Reflection.PortableExecutable; using System.Reflection.Metadata; #endif @@ -30,7 +30,7 @@ internal class AssemblyInformation : DisposableBase { private AssemblyNameExtension[] _assemblyDependencies; private string[] _assemblyFiles; -#if FEATURE_ASSEMBLY_LOADFROM +#if !FEATURE_ASSEMBLYLOADCONTEXT private readonly IMetaDataDispenser _metadataDispenser; private readonly IMetaDataAssemblyImport _assemblyImport; private static Guid s_importerGuid = new Guid(((GuidAttribute)Attribute.GetCustomAttribute(typeof(IMetaDataImport), typeof(GuidAttribute), false)).Value); @@ -39,14 +39,14 @@ internal class AssemblyInformation : DisposableBase private readonly string _sourceFile; private FrameworkName _frameworkName; -#if !FEATURE_ASSEMBLY_LOADFROM || MONO +#if FEATURE_ASSEMBLYLOADCONTEXT || MONO private bool _metadataRead; #endif -#if FEATURE_ASSEMBLY_LOADFROM && !MONO +#if !FEATURE_ASSEMBLYLOADCONTEXT && !MONO private static string s_targetFrameworkAttribute = "System.Runtime.Versioning.TargetFrameworkAttribute"; #endif -#if FEATURE_ASSEMBLY_LOADFROM +#if !FEATURE_ASSEMBLYLOADCONTEXT // Borrowed from genman. private const int GENMAN_STRING_BUF_SIZE = 1024; private const int GENMAN_LOCALE_BUF_SIZE = 64; @@ -68,7 +68,7 @@ internal AssemblyInformation(string sourceFile) ErrorUtilities.VerifyThrowArgumentNull(sourceFile, nameof(sourceFile)); _sourceFile = sourceFile; -#if FEATURE_ASSEMBLY_LOADFROM +#if !FEATURE_ASSEMBLYLOADCONTEXT if (NativeMethodsShared.IsWindows) { // Create the metadata dispenser and open scope on the source file. @@ -82,7 +82,7 @@ internal AssemblyInformation(string sourceFile) #endif } -#if FEATURE_ASSEMBLY_LOADFROM +#if !FEATURE_ASSEMBLYLOADCONTEXT private static Assembly ReflectionOnlyAssemblyResolve(object sender, ResolveEventArgs args) { string[] nameParts = args.Name.Split(MSBuildConstants.CommaChar); @@ -274,7 +274,7 @@ private FrameworkName GetFrameworkName() // Assembly.GetCustomAttributes* for an attribute which belongs // to an assembly that mono cannot find, causes a crash! // Instead, opt for using PEReader and friends to get that info -#if FEATURE_ASSEMBLY_LOADFROM && !MONO +#if !FEATURE_ASSEMBLYLOADCONTEXT && !MONO if (!NativeMethodsShared.IsWindows) { if (String.Equals(Environment.GetEnvironmentVariable("MONO29679"), "1", StringComparison.OrdinalIgnoreCase)) @@ -345,7 +345,7 @@ private FrameworkName GetFrameworkName() #endif } -#if !FEATURE_ASSEMBLY_LOADFROM || MONO +#if FEATURE_ASSEMBLYLOADCONTEXT || MONO /// /// Read everything from the assembly in a single stream. /// @@ -454,7 +454,7 @@ private static AssemblyName GetAssemblyName(MetadataReader metadataReader, Assem // Enabling this for MONO, because it's required by GetFrameworkName. // More details are in the comment for that method -#if !FEATURE_ASSEMBLY_LOADFROM || MONO +#if FEATURE_ASSEMBLYLOADCONTEXT || MONO // This method copied from DNX source: https://github.com/aspnet/dnx/blob/e0726f769aead073af2d8cd9db47b89e1745d574/src/Microsoft.Dnx.Tooling/Utils/LockFileUtils.cs#L385 // System.Reflection.Metadata 1.1 is expected to have an API that helps with this. /// @@ -514,7 +514,7 @@ private static List GetFixedStringArguments(MetadataReader reader, Custo } #endif -#if FEATURE_ASSEMBLY_LOADFROM +#if !FEATURE_ASSEMBLYLOADCONTEXT /// /// Release interface pointers on Dispose(). /// @@ -586,7 +586,7 @@ internal static string GetRuntimeVersion(string path) /// The array of assembly dependencies. private AssemblyNameExtension[] ImportAssemblyDependencies() { -#if FEATURE_ASSEMBLY_LOADFROM +#if !FEATURE_ASSEMBLYLOADCONTEXT var asmRefs = new List(); if (!NativeMethodsShared.IsWindows) @@ -683,7 +683,7 @@ private AssemblyNameExtension[] ImportAssemblyDependencies() /// The extra files of assembly dependencies. private string[] ImportFiles() { -#if FEATURE_ASSEMBLY_LOADFROM +#if !FEATURE_ASSEMBLYLOADCONTEXT var files = new List(); IntPtr fileEnum = IntPtr.Zero; var fileTokens = new UInt32[GENMAN_ENUM_TOKEN_BUF_SIZE]; @@ -726,7 +726,7 @@ private string[] ImportFiles() #endif } -#if FEATURE_ASSEMBLY_LOADFROM +#if !FEATURE_ASSEMBLYLOADCONTEXT /// /// Allocate assembly metadata structure buffer. /// diff --git a/src/Tasks/GenerateResource.cs b/src/Tasks/GenerateResource.cs index 1ce665dbed7..16967fb784f 100644 --- a/src/Tasks/GenerateResource.cs +++ b/src/Tasks/GenerateResource.cs @@ -2284,7 +2284,7 @@ internal string StronglyTypedClassName /// private bool _stronglyTypedClassIsPublic; -#if FEATURE_ASSEMBLY_LOADFROM +#if !FEATURE_ASSEMBLYLOADCONTEXT /// /// Class that gets called by the ResxResourceReader to resolve references /// to assemblies within the .RESX. @@ -2432,7 +2432,7 @@ internal void Run( _portableLibraryCacheInfo = new List(); _usePreserializedResources = usePreserializedResources; -#if FEATURE_ASSEMBLY_LOADFROM +#if !FEATURE_ASSEMBLYLOADCONTEXT // If references were passed in, we will have to give the ResxResourceReader an object // by which it can resolve types that are referenced from within the .RESX. if ((_assemblyFiles != null) && (_assemblyFiles.Length > 0)) @@ -2443,7 +2443,7 @@ internal void Run( try { -#if FEATURE_ASSEMBLY_LOADFROM +#if !FEATURE_ASSEMBLYLOADCONTEXT // Install assembly resolution event handler. _eventHandler = new ResolveEventHandler(ResolveAssembly); AppDomain.CurrentDomain.AssemblyResolve += _eventHandler; @@ -2465,7 +2465,7 @@ internal void Run( } finally { -#if FEATURE_ASSEMBLY_LOADFROM +#if !FEATURE_ASSEMBLYLOADCONTEXT // Remove the event handler. AppDomain.CurrentDomain.AssemblyResolve -= _eventHandler; _eventHandler = null; @@ -2473,7 +2473,7 @@ internal void Run( } } -#if FEATURE_ASSEMBLY_LOADFROM +#if !FEATURE_ASSEMBLYLOADCONTEXT /// /// Callback to resolve assembly names to assemblies. /// @@ -2991,7 +2991,7 @@ private void ReadResources(String filename, bool shouldUseSourcePath, String out if (format == Format.Assembly) // Multiple input .resources files within one assembly { -#if FEATURE_ASSEMBLY_LOADFROM +#if !FEATURE_ASSEMBLYLOADCONTEXT ReadAssemblyResources(filename, outFileOrDir); #else throw new InputFormatNotSupportedException("Reading resources from Assembly not supported on .NET Core MSBuild"); @@ -3084,7 +3084,7 @@ private void AddResourcesUsingMinimalCoreResxParsing(string filename, ReaderInfo } } -#if FEATURE_ASSEMBLY_LOADFROM +#if !FEATURE_ASSEMBLYLOADCONTEXT /// /// Reads resources from an assembly. /// @@ -3982,7 +3982,7 @@ internal int LinePosition #endregion // Code from ResGen.EXE } -#if FEATURE_ASSEMBLY_LOADFROM +#if !FEATURE_ASSEMBLYLOADCONTEXT /// /// This implemention of ITypeResolutionService is passed into the ResxResourceReader /// class, which calls back into the methods on this class in order to resolve types From ec69e9a70896f194602a63ea6a5529477586f006 Mon Sep 17 00:00:00 2001 From: Rainer Sigwald Date: Wed, 6 Nov 2019 17:32:58 -0600 Subject: [PATCH 2/7] Per-plugin AssemblyLoadContext Augment MSBuild's handling of assembly loading for plugins (currently tasks, SDK resolvers, and loggers) to use an AssemblyLoadContext, allowing plugins to use different versions of dependencies. Creates a new ALC per assembly path (of initially-loaded assembly, not indirect dependencies). Closes #1754. Closes #4635. Promote task isolation spec "possible solution"s to be the implemented solution. --- .../specs/task-isolation-and-dependencies.md | 11 +- .../SdkResolution/SdkResolverLoader.cs | 1 - src/Build/Microsoft.Build.csproj | 3 + src/MSBuild/MSBuild.csproj | 1 + src/Shared/CoreCLRAssemblyLoader.cs | 129 +----------------- src/Shared/MSBuildLoadContext.cs | 84 ++++++++++++ src/Shared/TypeLoader.cs | 1 - 7 files changed, 97 insertions(+), 133 deletions(-) create mode 100644 src/Shared/MSBuildLoadContext.cs diff --git a/documentation/specs/task-isolation-and-dependencies.md b/documentation/specs/task-isolation-and-dependencies.md index db4ca2af03d..f130c37e8a4 100644 --- a/documentation/specs/task-isolation-and-dependencies.md +++ b/documentation/specs/task-isolation-and-dependencies.md @@ -3,12 +3,13 @@ Tasks in MSBuild are dynamically loaded assemblies with potentially separate and colliding dependency trees. Currently MSBuild on .NET Core has no isolation between tasks and as such only one version of any given assembly can be loaded. Prime example of this is Newtonsoft.Json which has multiple versions, but all the tasks must agree on it to work. This problem is also described in #1754. -## Possible solution +## Solution Use [`AssemblyLoadContext`](https://docs.microsoft.com/en-us/dotnet/api/system.runtime.loader.assemblyloadcontext?view=netcore-2.2) (ALC) to provide binding isolation for task assemblies. Each task assembly would be loaded into its own ALC instance. * The ALC would resolve all dependencies of the task assemblies (see dependency resolution below) * ALC would fallback to the Default for dependencies which the assembly doesn't carry with itself (frameworks and so on) * ALC would probably have to forcefully fallback for MSBuild assemblies since it's possible that tasks will carry these, but the system requires for the MSBuild assemblies to be shared. -We would probably also want to load groups of tasks which belong together into the same ALC (for example based on their location on disk) to improve performance. This will need some care as there's no guarantee that two random tasks have compatible dependency trees. + +We also want to load groups of tasks which belong together into the same ALC (for example based on their location on disk) to improve performance. This will need some care as there's no guarantee that two random tasks have compatible dependency trees. As implemented, each task assembly is loaded into its own ALC. ## Potential risks * Has some small probability of causing breaks. Currently all assemblies from all tasks are loaded into the default context and thus are "visible" to everybody. Tasks with following properties might not work: @@ -20,13 +21,11 @@ We would probably also want to load groups of tasks which belong together into t * None of these changes would have any effect on MSBuild on .NET Framework * Task isolation alone could be achieved on existing MSBuild - - # Task dependency resolution ## Problem definition Tasks with complex and specifically platform specific dependencies don't work out of the box. For example if a task uses [`LibGit2Sharp`](https://www.nuget.org/packages/LibGit2Sharp) package it will not work as is. `LibGit2Sharp` has native dependencies which are platform specific. While the package carries all of them, there's no built in support for the task to load the right ones. For example [source link](https://github.com/dotnet/sourcelink/blob/master/src/Microsoft.Build.Tasks.Git/GitLoaderContext.cs) runs into this problem. -## Possible solution +## Solution .NET Core uses `.deps.json` files to describe dependencies of components. It would be natural to treat task assemblies as components and use associated .deps.json file to determine their dependencies. This would make the system work nicely end to end with the .NET Core CLI/SDK and VS integration. In .NET Core 3 there's a new type [`AssemblyDependencyResolver`](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyDependencyResolver.cs) which implements parsing and processing of a `.deps.json` for a component (or assembly). The usage is to create an instance of the resolver pointing to the assembly (in MSBuild case the task assembly). The resolver parses the `.deps.json` and stores the information. It exposes two methods to resolve managed and native dependencies. It was designed to be used as the underlying piece to implement custom ALC. So it would work nicely with task isolation above. @@ -36,3 +35,5 @@ It was designed to be used as the underlying piece to implement custom ALC. So i ## Additional consideration * Task dependency resolution requires APIs which are only available in .NET Core 3.0 (no plan to backport), as such MSBuild will have to target netcoreapp3.0 to use these APIs. + +We decided not to implement `AssemblyDependencyResolver` in the .NET Core 3.x timeframe because of the uncertain impact of the change. We should reconsider in the .NET 5 timeframe. diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs index 255b0354725..c3a86600cde 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs @@ -138,7 +138,6 @@ protected virtual Assembly LoadResolverAssembly(string resolverPath, LoggingCont #if !FEATURE_ASSEMBLYLOADCONTEXT return Assembly.LoadFrom(resolverPath); #else - _loader.AddDependencyLocation(Path.GetDirectoryName(resolverPath)); return _loader.LoadFromPath(resolverPath); #endif } diff --git a/src/Build/Microsoft.Build.csproj b/src/Build/Microsoft.Build.csproj index 5b96af2673d..224ed1742ba 100644 --- a/src/Build/Microsoft.Build.csproj +++ b/src/Build/Microsoft.Build.csproj @@ -761,6 +761,9 @@ SharedUtilities\TypeLoader.cs + + SharedUtilities\MSBuildLoadContext.cs + VisualStudioConstants.cs diff --git a/src/MSBuild/MSBuild.csproj b/src/MSBuild/MSBuild.csproj index fdf66662f84..7dd25dec391 100644 --- a/src/MSBuild/MSBuild.csproj +++ b/src/MSBuild/MSBuild.csproj @@ -138,6 +138,7 @@ + true diff --git a/src/Shared/CoreCLRAssemblyLoader.cs b/src/Shared/CoreCLRAssemblyLoader.cs index 8a3cdb023a8..cc87a1a5bfa 100644 --- a/src/Shared/CoreCLRAssemblyLoader.cs +++ b/src/Shared/CoreCLRAssemblyLoader.cs @@ -5,8 +5,6 @@ using System.Diagnostics; using System.IO; using System.Reflection; -using System.Runtime.Loader; -using Microsoft.Build.Shared.FileSystem; namespace Microsoft.Build.Shared { @@ -16,35 +14,8 @@ namespace Microsoft.Build.Shared internal sealed class CoreClrAssemblyLoader { private readonly Dictionary _pathsToAssemblies = new Dictionary(StringComparer.OrdinalIgnoreCase); - private readonly Dictionary _namesToAssemblies = new Dictionary(); - private readonly HashSet _dependencyPaths = new HashSet(StringComparer.OrdinalIgnoreCase); private readonly object _guard = new object(); - private bool _resolvingHandlerHookedUp = false; - - private static readonly string[] _extensions = new[] { "ni.dll", "ni.exe", "dll", "exe" }; - private static readonly Version _currentAssemblyVersion = new Version(Microsoft.Build.Shared.MSBuildConstants.CurrentAssemblyVersion); - private static readonly HashSet _wellKnownAssemblyNames = new HashSet( - new[] - { - "Microsoft.Build", - "Microsoft.Build.Framework", - "Microsoft.Build.Tasks.Core", - "Microsoft.Build.Utilities.Core" - }); - - public void AddDependencyLocation(string fullPath) - { - if (fullPath == null) - { - throw new ArgumentNullException(nameof(fullPath)); - } - - lock (_guard) - { - _dependencyPaths.Add(fullPath); - } - } public Assembly LoadFromPath(string fullPath) { @@ -57,113 +28,19 @@ public Assembly LoadFromPath(string fullPath) lock (_guard) { - if (!_resolvingHandlerHookedUp) - { - AssemblyLoadContext.Default.Resolving += TryResolveAssembly; - _resolvingHandlerHookedUp = true; - } - Assembly assembly; if (_pathsToAssemblies.TryGetValue(fullPath, out assembly)) { return assembly; } - return LoadAndCache(AssemblyLoadContext.Default, fullPath); - } - } - - private Assembly TryGetWellKnownAssembly(AssemblyLoadContext context, AssemblyName assemblyName) - { - if (!_wellKnownAssemblyNames.Contains(assemblyName.Name)) - { - return null; - } - - // Ensure we are attempting to load a matching version - // of the Microsoft.Build.* assembly. - assemblyName.Version = _currentAssemblyVersion; - - var searchPaths = new[] { Assembly.GetExecutingAssembly().Location }; - return TryResolveAssemblyFromPaths(context, assemblyName, searchPaths); - } - - private Assembly TryResolveAssembly(AssemblyLoadContext context, AssemblyName assemblyName) - { - lock (_guard) - { - Assembly assembly = TryGetWellKnownAssembly(context, assemblyName); - - if (assembly != null) - { - return assembly; - } - - if (_namesToAssemblies.TryGetValue(assemblyName.FullName, out assembly)) - { - return assembly; - } - return TryResolveAssemblyFromPaths(context, assemblyName, _dependencyPaths); - } - } + var contextForAssemblyPath = new MSBuildLoadContext(fullPath); - private Assembly TryResolveAssemblyFromPaths(AssemblyLoadContext context, AssemblyName assemblyName, IEnumerable searchPaths) - { - foreach (var cultureSubfolder in string.IsNullOrEmpty(assemblyName.CultureName) - // If no culture is specified, attempt to load directly from - // the known dependency paths. - ? new[] { string.Empty } - // Search for satellite assemblies in culture subdirectories - // of the assembly search directories, but fall back to the - // bare search directory if that fails. - : new[] { assemblyName.CultureName, string.Empty }) - { - foreach (var searchPath in searchPaths) - { - foreach (var extension in _extensions) - { - var candidatePath = Path.Combine(searchPath, - cultureSubfolder, - $"{assemblyName.Name}.{extension}"); - - if (IsAssemblyAlreadyLoaded(candidatePath) || - !FileSystems.Default.FileExists(candidatePath)) - { - continue; - } + assembly = contextForAssemblyPath.LoadFromAssemblyPath(fullPath); - AssemblyName candidateAssemblyName = AssemblyLoadContext.GetAssemblyName(candidatePath); - if (candidateAssemblyName.Version != assemblyName.Version) - { - continue; - } - - return LoadAndCache(context, candidatePath); - } - } + return assembly; } - - return null; - } - - /// - /// Assumes we have a lock on _guard - /// - private Assembly LoadAndCache(AssemblyLoadContext context, string fullPath) - { - var assembly = context.LoadFromAssemblyPath(fullPath); - var name = assembly.FullName; - - _pathsToAssemblies[fullPath] = assembly; - _namesToAssemblies[name] = assembly; - - return assembly; - } - - private bool IsAssemblyAlreadyLoaded(string path) - { - return _pathsToAssemblies.ContainsKey(path); } } } diff --git a/src/Shared/MSBuildLoadContext.cs b/src/Shared/MSBuildLoadContext.cs new file mode 100644 index 00000000000..4c873c706da --- /dev/null +++ b/src/Shared/MSBuildLoadContext.cs @@ -0,0 +1,84 @@ +// Copyright (c) Microsoft. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + + +using Microsoft.Build.Shared.FileSystem; +using System; +using System.Collections.Immutable; +using System.IO; +using System.Reflection; +using System.Runtime.Loader; + +#nullable enable +namespace Microsoft.Build.Shared +{ + /// + /// This class is used to isolate the types used by an MSBuild plugin + /// (SDK resolver, logger, or task). + /// + internal class MSBuildLoadContext : AssemblyLoadContext + { + private readonly string _directory; + private readonly object _guard = new object(); + + private static readonly ImmutableHashSet _wellKnownAssemblyNames = + new[] + { + "MSBuild", + "Microsoft.Build", + "Microsoft.Build.Framework", + "Microsoft.Build.Tasks.Core", + "Microsoft.Build.Utilities.Core", + }.ToImmutableHashSet(); + + private static readonly string[] _extensions = new[] { "ni.dll", "ni.exe", "dll", "exe" }; + + + public MSBuildLoadContext(string assemblyPath) + { + _directory = Directory.GetParent(assemblyPath).FullName; + } + + protected override Assembly? Load(AssemblyName assemblyName) + { + if (_wellKnownAssemblyNames.Contains(assemblyName.Name!)) + { + // Force MSBuild assemblies to be loaded in the default ALC + // and unify to the current version. + return null; + } + + foreach (var cultureSubfolder in string.IsNullOrEmpty(assemblyName.CultureName) + // If no culture is specified, attempt to load directly from + // the known dependency paths. + ? new[] { string.Empty } + // Search for satellite assemblies in culture subdirectories + // of the assembly search directories, but fall back to the + // bare search directory if that fails. + : new[] { assemblyName.CultureName, string.Empty }) + { + foreach (var extension in _extensions) + { + var candidatePath = Path.Combine(_directory, + cultureSubfolder, + $"{assemblyName.Name}.{extension}"); + + if (!FileSystems.Default.FileExists(candidatePath)) + { + continue; + } + + AssemblyName candidateAssemblyName = AssemblyLoadContext.GetAssemblyName(candidatePath); + if (candidateAssemblyName.Version != assemblyName.Version) + { + continue; + } + + return LoadFromAssemblyPath(candidatePath); + } + } + + return null; + } + } +} diff --git a/src/Shared/TypeLoader.cs b/src/Shared/TypeLoader.cs index 95befd0430e..f85bb819c8c 100644 --- a/src/Shared/TypeLoader.cs +++ b/src/Shared/TypeLoader.cs @@ -178,7 +178,6 @@ private static Assembly LoadAssembly(AssemblyLoadInfo assemblyLoadInfo) else { var baseDir = Path.GetDirectoryName(assemblyLoadInfo.AssemblyFile); - s_coreClrAssemblyLoader.AddDependencyLocation(baseDir); loadedAssembly = s_coreClrAssemblyLoader.LoadFromPath(assemblyLoadInfo.AssemblyFile); } #endif From b1b09ce76f59a85e9c163300ba1109c0b8db6dfa Mon Sep 17 00:00:00 2001 From: Rainer Sigwald Date: Thu, 14 Nov 2019 12:47:01 -0600 Subject: [PATCH 3/7] Test for task ALC behavior --- .../ValidateAssemblyLoadContext.cs | 43 +++++++++++++++++++ src/MSBuild.UnitTests/XMake_Tests.cs | 29 +++++++++++++ 2 files changed, 72 insertions(+) create mode 100644 src/MSBuild.UnitTests/ValidateAssemblyLoadContext.cs diff --git a/src/MSBuild.UnitTests/ValidateAssemblyLoadContext.cs b/src/MSBuild.UnitTests/ValidateAssemblyLoadContext.cs new file mode 100644 index 00000000000..095d1f1a45c --- /dev/null +++ b/src/MSBuild.UnitTests/ValidateAssemblyLoadContext.cs @@ -0,0 +1,43 @@ +// Copyright (c) Microsoft. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +#if FEATURE_ASSEMBLYLOADCONTEXT + +using Microsoft.Build.Framework; +using Microsoft.Build.Shared; +using Microsoft.Build.Utilities; +using System.Runtime.Loader; + +namespace Microsoft.Build.UnitTests +{ + public class ValidateAssemblyLoadContext : Task + { + public override bool Execute() + { + var thisLoadContext = AssemblyLoadContext.GetLoadContext(typeof(ValidateAssemblyLoadContext).Assembly); + + // Check by name because this always reports false, presumably due to ALC shenanigans: + // if (thisLoadContext is MSBuildLoadContext context) + if (thisLoadContext.GetType().FullName == typeof(MSBuildLoadContext).FullName) + { +#if NETCOREAPP && !NETCOREAPP2_1 // TODO: enable this functionality when targeting .NET Core 3.0+ + if (!thisLoadContext.Name.EndsWith(typeof(ValidateAssemblyLoadContext).Assembly.GetName().Name + ".dll")) + { + Log.LogError($"Unexpected AssemblyLoadContext name: \"{thisLoadContext.Name}\", but the current executing assembly was {typeof(ValidateAssemblyLoadContext).Assembly.GetName().Name}"); + } + else + { + Log.LogMessage(MessageImportance.High, $"Task {nameof(ValidateAssemblyLoadContext)} loaded in AssemblyLoadContext named {thisLoadContext.Name}"); + } +#endif + } + else + { + Log.LogError($"Load context was a {thisLoadContext.GetType().AssemblyQualifiedName} instead of an {typeof(MSBuildLoadContext).AssemblyQualifiedName}"); + } + + return !Log.HasLoggedErrors; + } + } +} +#endif diff --git a/src/MSBuild.UnitTests/XMake_Tests.cs b/src/MSBuild.UnitTests/XMake_Tests.cs index 652fe6593ce..9e448504a9c 100644 --- a/src/MSBuild.UnitTests/XMake_Tests.cs +++ b/src/MSBuild.UnitTests/XMake_Tests.cs @@ -18,6 +18,7 @@ using Xunit.Abstractions; using Shouldly; using System.IO.Compression; +using System.Reflection; namespace Microsoft.Build.UnitTests { @@ -2119,6 +2120,34 @@ public void BinaryLogContainsImportedFiles() } } +#if FEATURE_ASSEMBLYLOADCONTEXT + /// + /// Ensure that tasks get loaded into their own . + /// + /// + /// When loading a task from a test assembly in a test within that assembly, the assembly is already loaded + /// into the default context. So put the test here and isolate the task into an MSBuild that runs in its + /// own process, causing it to newly load the task (test) assembly in a new ALC. + /// + [Fact] + public void TasksGetAssemblyLoadContexts() + { + string customTaskPath = Assembly.GetExecutingAssembly().Location; + + string projectContents = $@" + + + + + +"; + + ExecuteMSBuildExeExpectSuccess(projectContents); + } + +#endif + + private string CopyMSBuild() { string dest = null; From 53e530c0b5faabd050979bde5c806314697ea098 Mon Sep 17 00:00:00 2001 From: Rainer Sigwald Date: Fri, 6 Dec 2019 17:22:43 -0600 Subject: [PATCH 4/7] Remove 'msbuild directory trumps all' load rule This broke the new test, and would remove much of the benefit of the ALC because of the flattened directory structure of the .NET Core SDK. But it might also be the reason some tasks keep going. So keep the MSBuild directory as a fallback location. --- src/Shared/MSBuildLoadContext.cs | 12 ++++++++++++ src/Shared/TypeLoader.cs | 18 +----------------- 2 files changed, 13 insertions(+), 17 deletions(-) diff --git a/src/Shared/MSBuildLoadContext.cs b/src/Shared/MSBuildLoadContext.cs index 4c873c706da..e1ee9f914e6 100644 --- a/src/Shared/MSBuildLoadContext.cs +++ b/src/Shared/MSBuildLoadContext.cs @@ -78,6 +78,18 @@ public MSBuildLoadContext(string assemblyPath) } } + // If the Assembly is provided via a file path, the following rules are used to load the assembly: + // - if the simple name of the assembly exists in the same folder as msbuild.exe, then that assembly gets loaded, indifferent of the user specified path + // - otherwise, the assembly from the user specified path is loaded, if it exists. + + var assemblyNameInExecutableDirectory = Path.Combine(BuildEnvironmentHelper.Instance.CurrentMSBuildToolsDirectory, + assemblyName.Name); + + if (FileSystems.Default.FileExists(assemblyNameInExecutableDirectory)) + { + return LoadFromAssemblyPath(assemblyNameInExecutableDirectory); + } + return null; } } diff --git a/src/Shared/TypeLoader.cs b/src/Shared/TypeLoader.cs index f85bb819c8c..828b8320823 100644 --- a/src/Shared/TypeLoader.cs +++ b/src/Shared/TypeLoader.cs @@ -163,23 +163,7 @@ private static Assembly LoadAssembly(AssemblyLoadInfo assemblyLoadInfo) #if !FEATURE_ASSEMBLYLOADCONTEXT loadedAssembly = Assembly.UnsafeLoadFrom(assemblyLoadInfo.AssemblyFile); #else - // If the Assembly is provided via a file path, the following rules are used to load the assembly: - // - if the simple name of the assembly exists in the same folder as msbuild.exe, then that assembly gets loaded, indifferent of the user specified path - // - otherwise, the assembly from the user specified path is loaded, if it exists. - - var assemblyNameInExecutableDirectory = Path.Combine(BuildEnvironmentHelper.Instance.CurrentMSBuildToolsDirectory, - Path.GetFileName(assemblyLoadInfo.AssemblyFile)); - - if (FileSystems.Default.FileExists(assemblyNameInExecutableDirectory)) - { - var simpleName = Path.GetFileNameWithoutExtension(assemblyLoadInfo.AssemblyFile); - loadedAssembly = Assembly.Load(new AssemblyName(simpleName)); - } - else - { - var baseDir = Path.GetDirectoryName(assemblyLoadInfo.AssemblyFile); - loadedAssembly = s_coreClrAssemblyLoader.LoadFromPath(assemblyLoadInfo.AssemblyFile); - } + loadedAssembly = s_coreClrAssemblyLoader.LoadFromPath(assemblyLoadInfo.AssemblyFile); #endif } } From 369be8153d87d3f7dbf475f38e45475f895b399a Mon Sep 17 00:00:00 2001 From: Rainer Sigwald Date: Mon, 6 Jan 2020 16:01:53 -0600 Subject: [PATCH 5/7] Use default ALC for fallback-to-msbuild-path assemblies This ensures that types from these assemblies are loaded only one time and version-unified (if the fallback is used). --- src/Shared/MSBuildLoadContext.cs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/Shared/MSBuildLoadContext.cs b/src/Shared/MSBuildLoadContext.cs index e1ee9f914e6..1e5d15d03de 100644 --- a/src/Shared/MSBuildLoadContext.cs +++ b/src/Shared/MSBuildLoadContext.cs @@ -79,15 +79,16 @@ public MSBuildLoadContext(string assemblyPath) } // If the Assembly is provided via a file path, the following rules are used to load the assembly: - // - if the simple name of the assembly exists in the same folder as msbuild.exe, then that assembly gets loaded, indifferent of the user specified path - // - otherwise, the assembly from the user specified path is loaded, if it exists. + // - the assembly from the user specified path is loaded, if it exists, into the custom ALC, or + // - if the simple name of the assembly exists in the same folder as msbuild.exe, then that assembly gets loaded + // into the default ALC (so it's shared with other uses). var assemblyNameInExecutableDirectory = Path.Combine(BuildEnvironmentHelper.Instance.CurrentMSBuildToolsDirectory, assemblyName.Name); if (FileSystems.Default.FileExists(assemblyNameInExecutableDirectory)) { - return LoadFromAssemblyPath(assemblyNameInExecutableDirectory); + return AssemblyLoadContext.Default.LoadFromAssemblyPath(assemblyNameInExecutableDirectory); } return null; From 6d4c3b1e2adcd6ce97233b85489c514b2b990949 Mon Sep 17 00:00:00 2001 From: Rainer Sigwald Date: Mon, 6 Jan 2020 16:02:36 -0600 Subject: [PATCH 6/7] Cache assembly loads I checked the cache to see if something had been added, but never actually added anything . . . --- src/Shared/CoreCLRAssemblyLoader.cs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/Shared/CoreCLRAssemblyLoader.cs b/src/Shared/CoreCLRAssemblyLoader.cs index cc87a1a5bfa..25b5f21ee6e 100644 --- a/src/Shared/CoreCLRAssemblyLoader.cs +++ b/src/Shared/CoreCLRAssemblyLoader.cs @@ -34,11 +34,15 @@ public Assembly LoadFromPath(string fullPath) return assembly; } - var contextForAssemblyPath = new MSBuildLoadContext(fullPath); assembly = contextForAssemblyPath.LoadFromAssemblyPath(fullPath); + if (assembly != null) + { + _pathsToAssemblies[fullPath] = assembly; + } + return assembly; } } From f561312ea936d5dd8e85567ab803a16c36e3ff73 Mon Sep 17 00:00:00 2001 From: Rainer Sigwald Date: Thu, 9 Jan 2020 17:10:58 -0600 Subject: [PATCH 7/7] Clarify comment about MSBuildLoadContext test The confusing situation can be resolved by an IVT change, filed as #5041. --- src/MSBuild.UnitTests/ValidateAssemblyLoadContext.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/MSBuild.UnitTests/ValidateAssemblyLoadContext.cs b/src/MSBuild.UnitTests/ValidateAssemblyLoadContext.cs index 095d1f1a45c..4cdcab25b0d 100644 --- a/src/MSBuild.UnitTests/ValidateAssemblyLoadContext.cs +++ b/src/MSBuild.UnitTests/ValidateAssemblyLoadContext.cs @@ -16,8 +16,11 @@ public override bool Execute() { var thisLoadContext = AssemblyLoadContext.GetLoadContext(typeof(ValidateAssemblyLoadContext).Assembly); - // Check by name because this always reports false, presumably due to ALC shenanigans: - // if (thisLoadContext is MSBuildLoadContext context) + // The straightforward implementation of this check: + // if (thisLoadContext is MSBuildLoadContext context) + // fails here because MSBuildLoadContext (in this test assembly) is from MSBuild.exe via + // IVT, but the one that actually gets used for task isolation is in Microsoft.Build.dll. + // This probably doesn't need to be how it is forever: https://github.com/microsoft/msbuild/issues/5041 if (thisLoadContext.GetType().FullName == typeof(MSBuildLoadContext).FullName) { #if NETCOREAPP && !NETCOREAPP2_1 // TODO: enable this functionality when targeting .NET Core 3.0+