Ensure that netcore MSBuild can handle a task DLL that has dependencies to other DLLs

[rainersigwald]

Consider

<UsingTask TaskName="NuGetPack"
           AssemblyFile="$(PackagingTaskDir)Microsoft.DotNet.Build.Tasks.Packaging.dll"/>

Where the task assembly has dependencies on other DLLs in the PackagingTaskDir folder. Full-framework MSBuild handles this situation fine, but on .NET Core I see an error like:

error MSB4018: The "NugetPack" task failed unexpectedly.\r [o:\msbuild\src\dirs.proj]
error MSB4018: This is an unhandled exception from a task -- PLEASE OPEN A BUG AGAINST THE TASK OWNER.\r [o:\msbuild\src\dirs.proj]
error MSB4018: System.IO.FileNotFoundException: Could not load file or assembly 'NuGet.Packaging, Version=3.4.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35' or one of its dependencies. The system cannot find the file specified.\r [o:\msbuild\src\dirs.proj]

We should handle this situation--it's not that uncommon to have ThingHandling.dll and Thing.BuildTasks.dll that depends on it.

[eerhardt]

/cc @livarcocc - as he is running into this issue in the CLI build.

[livarcocc]

This is actually blocking the CLI from moving into MSBuild. We have tasks that depend on Microsoft.WindowsAzure.Storage and MSBuild fails to load that Dll, even though it is present in the same folder as the task dll.

[analogrelay]

This is also pretty much blocking ASP.NET moving to MSBuild. We have a hacky workaround right now by publishing all our tasks to the same directory as the MSBuild executable but that's not really appropriate (and will start getting harder to do once we're pulling MSBuild in from dotnet-cli).

Ideally, it should load tasks up in a load context that will cause dependencies to be resolved next to the task assembly itself.

[analogrelay]

This is starting to become a fairly significant blocker for us. I sketched out a possible solution but haven't been able to wrap my head around the build system enough to get the tests fully updated and running :) xplat...anurse:anurse/sketch-658

I can keep digging and get a PR together but I don't want to do a bunch of work just to find out there's a grand plan in place ;)

The gist of the change is that when an assembly path is used to load an assembly, it is added to the search paths for other assemblies so that dependencies are loaded from the same place. This code appears to have already been present in the Analyzer load context used by Roslyn that was initially present.

Is someone actively looking at this? It's a pretty high priority to us in order to start dogfooding MSBuild in the ASP.NET build system.

/cc @cdmihai @davidfowl @Sarabeth-Jaffe-Microsoft

[cdmihai]

Thank you @anurse for looking into it! That looks like the way to implement it. Feel free to send a PR.

You should add / update the tests for this scenario here: https://github.com/Microsoft/msbuild/blob/xplat/src/Shared/UnitTests/TypeLoader_Tests.cs#L74-L92
That test is copying PortableTask.dll in some temp directory and checks whether MSBuild was able to load it. Since you already changed that task to load stuff from Newtsonoft I guess the current test will test this new scenario as well. Maybe make sure there's a read of jObj so there's no funky compiler optimization that removes it.

[analogrelay]

I'm still struggling with the tests, but I'll see what I can get going. The compiler shouldn't optimize out the constructor call because it doesn't know the side-effects of it (it could affect static state) so it should be fine, but I can throw in a quick read to make sure.

[rainersigwald]

@anurse Please let us know about your struggles so we can smooth the path for the next person!

[analogrelay]

The biggest thing right now is figuring out how to properly add a dependency to the PortableTask and get it to deploy properly. Right now Newtonsoft.Json already ends up in the output (for some other reason?) so I need to find something else and it turns out the build isn't currently copying package references to the output.

It's also hard to track down exactly what build outputs are used in which tests, but I'm starting to figure it out :).

[TheRealPiotrP]

@rainersigwald I've been discussing this issue with @gkhanna79 and @eerhardt quite a bit. Have you considered using a new AssemblyLoadContext for each extension and providing a well-known scheme by which that context finds an extensions dependencies? It seems this would both give you control over dependency management and also would prevent extension load-order from affecting runtime behavior of builds.

[analogrelay]

I'm pretty lost in these tests and MSBuild targets :). It looks like because the task project is a "class library" (i.e. with no runtimes), dependent packages are not copied to the output directory so it's hard to assemble a proper test. I haven't even been able to manually verify yet.

Unfortunately I don't really have the bandwidth to take this much farther. I'll try to get some manual verification done but I'm going to have to go back to some other work shortly. I'm also not sure we want to quickly monkey-patch this in, given that it's so fundamental a feature. Custom Task resolution needs to be designed and implemented properly (for example, how are differing versions of the same assembly handled? From our experience, Load Contexts don't handle this well), but unfortunately it's blocking a lot of adoption (certainly within ASP.NET) right now so the timelines are tight. /cc @davidfowl

[rainersigwald]

@piotrpMSFT That sounds intriguing but I don't know how to go about it, and I'm not turning up any obviously-related documentation in Bing or Google. Is there a how-to or even a pointer to the APIs you'd expect we use somewhere?

[analogrelay]

Ok, I've managed to verify that my sketch does what we want (loads dependencies from the same directory as the task), but it doesn't do anything to isolate individual Tasks from each other.

Load Contexts are a very underdocumented feature unfortunately. Perhaps it would be good to get a meeting together with the stakeholders and the folks who know enough about Load Contexts to put them in the right... context (pun entirely intended). @gkhanna79 would that be someone from your area?

[gkhanna79]

I am working on docs for AssemblyLoadContext and intend to share a draft later this week. Meanwhile, https://github.com/dotnet/corefx/tree/master/src/System.Runtime.Loader/tests are good example to look into and am happy to chat about them offline.

[tmeschter]

Assume a task is in TaskAssembly.dll, and that TaskAssembly.dll depends on B.dll which is located next to it in the file system. When we need to load TaskAssembly.dll we could simply get the default AssemblyLoadContext and then call AssemblyLoadContext.LoadFromAssemblyPath(string). Assuming that this method works like Assembly.LoadFrom(string) on the desktop, I would expect B.dll to be found and loaded automatically when needed.

Does that sound right, or am I missing something about the issue or the workings of AssemblyLoadContext?

[analogrelay]

@tmeschter As far as I can tell, AssemblyLoadContext does not have the same behavior as Assembly.LoadFrom in that case. It only loads that direct assembly from the specified path and further resolutions (of dependencies, for example) are done in the default search paths.

However, MSBuild could create a custom Load Context to have whatever resolution policy it wants :). Which is the solution proposed here.

[gkhanna79]

@anurse is correct.

[tmeschter]

@anurse @gkhanna79 Ahh, I see. The similarity in the names led me to assume they worked in similar manners.

So then there is no built-in equivalent for the LoadFrom loading context found in Desktop?

[gkhanna79]

So then there is no built-in equivalent for the LoadFrom loading context found in Desktop?

Correct.

[tmeschter]

I've been asked to help out with MSBuild issues, and I can pick this one up. I'll start with the code already written by @anurse.

[analogrelay]

@tmeschter Awesome, thanks so much for taking this on! It'll be great to get this resolved.

We may want to review exactly how we set up the load contexts. For example, we could set up a separate context for each <UsingTask> so that the tasks are somewhat isolated (ALC does not provide isolation but does allow for different resolution policies per load, in effect). Might be worth a quick sync up with @gkhanna79, who knows WAY more about ALC than I do ;P.

For now, if you want to just take my code though it would be better than what we've currently got :).

[tmeschter]

@anurse @AndyGerlicher I think we'll want to address this in two steps.

1. Validate and commit the code you've already written. I think all that remains on this step is for me to write a simple automated test to verify that we find assemblies located next to already-loaded assemblies. This approach has shortcomings (notably, the order in which assemblies are loaded could affect whether or not they are found) but covers the 90% case and should be sufficient to unblock ASP.NET and CLI.
2. Implement proper assembly loading and dependency management, addressing load order, assembly identity, and other thorny problems (e.g., what do we do if we find an assembly with the right name but the wrong version or public key token, where should we look for satellite assemblies, etc.).

How does that sound?

[rainersigwald]

@tmeschter Sounds good to me. Also worth pointing out that we've lived with the problems inherent in approach 1 for all of time up to now on the full framework, so that may be good enough forever . . .

[tmeschter]

@rainersigwald In that case I'll focus on the first part, and log a new issue pertaining to the second describing the various problems. Then we can decide which, if any, we feel like addressing and when.

I'm coming at this from an opinionated view point: I wrote the code to handle finding and loading analyzer assemblies for the C# and VB compilers, and there we needed very tight control over assembly loading. The same requirements may not apply here.

[natemcmaster]

Custom Task resolution needs to be designed and implemented properly (for example, how are differing versions of the same assembly handled? From our experience, Load Contexts don't handle this well)

re @anurse: this issue occured in RC2, but RTM mostly addresses. Load contexts should be able to loaddifferent versions (with some exceptions. See https://github.com/dotnet/coreclr/issues/6187)

@tmeschter can you keep me in the loop on the work you're doing with load contexts? Some ASP.NET Core tooling I'm working on has similar requirements

[tmeschter]

@natemcmaster Will do.

[tmeschter]

This was fixed by 2d0acb1. Further discussion should occur on #858.