-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Deliver CoreFx façades next to MSBuild.exe #1542
Comments
In addition to distributing the facades MSBuild should have some docs / be explicit about exactly which version of Desktop / Net Core it supports for a given version. From an extensibility stand point I need to know what version of Net Standard I should target with my extension. For an extension targeting that version I should have no need to deploy facades as MSBuild should be providing them. |
Talked about this with @jaredpar and @AndyGerlicher yesterday. The plan is:
|
JoC would like you to talk to Eric St. John first. |
I'd suggest getting @ericstj and @weshaggard's feedback on this. I think we made the statement yesterday that the facades are part of the platform and thus should be provided by MSBuild. I've realized that's not always true- we sometimes deliver facades in NuGet packages to make things work on versions of .NET that have already shipped. And if the facades really were part of the platform, then they should already be there and MSBuild shouldn't need to provide them. I don't mean to imply that we shouldn't include the facades with MSBuild- it might be the right thing to do. But I think we should talk to the facade experts first. |
It's very common when targeting Net Standard, even when running on desktop, for an EXE to be required to deploy facades in order to successfully run. Today MSBuild is deploying only a minimal subset of the facades. That leaves every extension guessing which of the remaining faceds it should / shouldn't be deploying in order to run. The most consistent story here is for MSBuild, which is an extensibility platform, to ship the full set of facades. Consider other extensibility environments like Visual Studio which had the same problem and came to the same conclusion. |
I think that MSBuild should include the largest set of desktop facades that is available at the time it ships in order to enable folks to limit their redist dependencies from tasks. It's not tied to a "netstandard" version, the facades were talking about here are used for desktop, not netstandard. So I'd expect you to include the latest facades for whichever version of desktop you are targeting. The set should be at least MSBuild and its closure, probably all of NETStandard.Library and its closure. As Jared suggests you can go as far as pulling in every package we ship from CoreFx that supports desktop, I'm leaning this way myself. While we're at it, how about throwing in a version of JSON.Net and some other popular nuget packages? I'm only half-kidding here, dealing with dependencies in desktop MSBuild tasks is a pain. To restate, we're only talking about desktop here, for netcore you get what's in the shared framework. I suppose if you wanted to bundle some additional assemblies next to your netcoreapp version of MSBuild you could, but its less interesting because the binding model for netcore is less rigid (no binding redirects). If you're recommending folks publish a Finally in all these cases folks need to know what not to put in their packages because build/publish isn't going to do the right thing. Perhaps in v.Next you can make use of the trimming / conflict resolution we're putting in to model what's in your MSBuild "platform". |
MSBuild is both a desktop and CoreClr application. It now supports hosting task DLLs which are portable and target Net Standard. Why should they target desktop here?
Look at this from the other angle. I'm an MSBuild task author and I want to build a portable task. How can MSBuild communicate the set of facades that will be available to my task when run on either runtime? Anything not guaranteed to be available is a facade my task needs to bundle. |
That's a requirement and something they should already doing for their EXE. There is no such thing as a portable layout for an exe. Consider that the System.Runtime.dll, etc for desktop are very different from the .NET core: you cannot have one application layout that works in both places. They need not target desktop, but they should already be publishing for it.
Yeah I understand that. MSBuild is free to go off and define their own set of assemblies they consider their platform: not just facades, but actual contracts (even 3rd party libraries as I previously suggested). That's a different pivot than desktop facades and its a discussion that looks more like "what's the set of API that MSBuild provides as a platform". I was trying to scope this thread because, presumably, NETCoreApp has already done a decent analysis here with shared framework and thats a superset of what MSBuild would want. Sure, MSBuild is free to be different, but then they cannot piggy-back on the shared-framework trimming that already exists. Also, as I mentioned I don't think "portable" task is a thing, unless it has caveats. There is no runtime resolution of TFM specific assets. This is why apps must be TFM-specific as I mentioned above. So if you want to have a portable plugin model, you need to make sure the host app can provide any TFM-specific assets that your plugins might want to use. MSBuild sould consider this when determining what things to bundle.
It looks to me like @rainersigwald was suggesting that they merely document it. I think you might be able to use the features in https://github.com/dotnet/sdk/blob/master/src/Tasks/Microsoft.NET.Build.Tasks/ResolvePublishAssemblies.cs to either list an MSBuild (meta-)package a platform package, or mark all the packages you want to be platform as private assets, but I haven't tried it. In v2 we'll have conflict resolution that supports a platform manifest, like I mentioned previously. |
Yes. But they can define the minimum set of facades / contracts that their EXE will bring with it on all platforms.
The future of .Net is portability but your saying we can't have a portable way to build applications? Tasks are just DLLs. There is no reason I shouldn't be able to build a portable DLL and have MSBuild host it. Forcing me to build a different DLL for every platform that MSBuild supports doesn't make sense. |
Sure, and they'll need different files for each platform if the component has a different implementation on that platform. Those different versions of the files can't live in the same path...
Like I said before, you can build a portable application today, but you need to publish a layout for each platform(TFM). This is because the apps dependencies have platform(TFM) specific implementations. Now fast-forward to NETStandard2.0 where we've asserted that a larger set of the platform is available and inbox in the platform and you can see how it will likely eliminate the need for any dependencies with platform(TFM) specific implementations. You'd still hit the same issue if took a dependency that needed a platform specific implementation app-local, but NETStandard2.0 aims to reduce the likelihood of that.
Again, I didn't say you needed to build a different dll. I said there's a caveat here that dependencies may be TFM specific and both need to be published. I agree with you that MSBuild should try to realize a portable plugin model by providing those TFM specific files. I'm saying that MSBuild can't present it as completely portable, since any dependency that's TFM specific and not in their provided set would require the plugin to publish that dependency separately. Now we could ask for MSbuild to define a convention for "fat" layouts that have TFM-specific assets side-by-side, but that feels like feature creep. |
@ericstj I'll need some more info on how to gather the facade assemblies so we can package them up. If I made a build project that targets |
@jeffkl that'll give you the set that's in NETStandard.Library and its closure. MSBuild may be depending on packages above NETStandard.Library and you'd want to include those as well. If you have an entry point package, a package that represents MSBuild.Exe and its dependencies, just publish that for net46 and it'll give you all the facades needed for MSBuild to run. That's the first option I suggested. Now if you added a NETStandard.Library ref to that project (if you don't already have one) and publish it for net46 that'll give you all the facades needed to satisfy NETStandard.Library. If you want to provide additional packages as a convenience for developers, as we've suggested, just add more references and publish for net46. I'm putting together a few samples to demonstrate, will share in a moment. |
Here are some samples: https://gist.github.com/ericstj/af57217e1972f78408d66b4eed042093 |
Thanks @ericstj I'll try and make progress on this today |
Thanks @jeffkl. All the talk above about fat packages and what not is relevant if MSBuild recommends that folks build a task as Here's an example of what that looks like: |
Alright, just to verify. We have to use project.json because we can't use the new SDK stuff just yet. So this is my project.json: {
"dependencies": {
"NETStandard.Library": "1.6.0",
"Microsoft.CSharp": "4.0.1",
"Microsoft.NETCore.Platforms": "1.0.1",
"Microsoft.NETCore.Targets": "1.0.1",
"Microsoft.VisualBasic": "10.0.1",
"Microsoft.Win32.Registry": "4.0.0",
"Microsoft.Win32.Registry.AccessControl": "4.0.0",
"System.Buffers": "4.0.0",
"System.Collections": "4.0.11",
"System.Collections.Concurrent": "4.0.12",
"System.Collections.Immutable": "1.2.0",
"System.Collections.NonGeneric": "4.0.1",
"System.Collections.Specialized": "4.0.1",
"System.ComponentModel": "4.0.1",
"System.ComponentModel.Annotations": "4.1.0",
"System.ComponentModel.EventBasedAsync": "4.0.11",
"System.ComponentModel.Primitives": "4.1.0",
"System.ComponentModel.TypeConverter": "4.1.0",
"System.Data.Common": "4.1.0",
"System.Data.SqlClient": "4.1.0",
"System.Diagnostics.Contracts": "4.0.1",
"System.Diagnostics.DiagnosticSource": "4.0.0",
"System.Diagnostics.FileVersionInfo": "4.0.0",
"System.Diagnostics.Process": "4.1.0",
"System.Diagnostics.StackTrace": "4.0.1",
"System.Diagnostics.TextWriterTraceListener": "4.0.0",
"System.Diagnostics.TraceSource": "4.0.0",
"System.Drawing.Primitives": "4.0.0",
"System.Dynamic.Runtime": "4.0.11",
"System.Globalization.Extensions": "4.0.1",
"System.IO.FileSystem.AccessControl": "4.0.0",
"System.IO.FileSystem.DriveInfo": "4.0.0",
"System.IO.FileSystem.Watcher": "4.0.0",
"System.IO.MemoryMappedFiles": "4.0.0",
"System.IO.Packaging": "4.0.0",
"System.IO.Pipes": "4.0.0",
"System.IO.UnmanagedMemoryStream": "4.0.1",
"System.Linq.Parallel": "4.0.1",
"System.Linq.Queryable": "4.0.1",
"System.Net.Http.Rtc": "4.0.1",
"System.Net.Http.WinHttpHandler": "4.0.0",
"System.Net.NameResolution": "4.0.0",
"System.Net.NetworkInformation": "4.1.0",
"System.Net.Ping": "4.0.0",
"System.Net.Primitives": "4.0.11",
"System.Net.Requests": "4.0.11",
"System.Net.Security": "4.0.0",
"System.Net.WebHeaderCollection": "4.0.1",
"System.Net.WebSockets": "4.0.0",
"System.Net.WebSockets.Client": "4.0.0",
"System.Numerics.Vectors": "4.1.1",
"System.Reflection.Context": "4.0.1",
"System.Reflection.DispatchProxy": "4.0.1",
"System.Reflection.Emit": "4.0.1",
"System.Reflection.Emit.ILGeneration": "4.0.1",
"System.Reflection.Emit.Lightweight": "4.0.1",
"System.Reflection.Metadata": "1.3.0",
"System.Reflection.TypeExtensions": "4.1.0",
"System.Resources.Reader": "4.0.0",
"System.Resources.Writer": "4.0.0",
"System.Runtime.CompilerServices.Unsafe": "4.0.0",
"System.Runtime.CompilerServices.VisualC": "4.0.0",
"System.Runtime.InteropServices.WindowsRuntime": "4.0.1",
"System.Runtime.Serialization.Json": "4.0.2",
"System.Runtime.Serialization.Primitives": "4.1.1",
"System.Runtime.Serialization.Xml": "4.1.1",
"System.Runtime.WindowsRuntime": "4.0.11",
"System.Runtime.WindowsRuntime.UI.Xaml": "4.0.1",
"System.Security.AccessControl": "4.0.0",
"System.Security.Claims": "4.0.1",
"System.Security.Cryptography.Cng": "4.2.0",
"System.Security.Cryptography.Csp": "4.0.0",
"System.Security.Cryptography.Pkcs": "4.0.0",
"System.Security.Cryptography.ProtectedData": "4.0.0",
"System.Security.Principal": "4.0.1",
"System.Security.Principal.Windows": "4.0.0",
"System.Security.SecureString": "4.0.0"
},
"frameworks": {
"net46": { }
}
} I ran the following:
Which places the following assemblies in the publish folder:
So if we ship these with MSBuild in Visual Studio, this will make it so task authors who target netstandard1.3 will not have to ship their own dependencies? |
Don't import netcore50. I've created a project.json based sample:https://gist.github.com/ericstj/af57217e1972f78408d66b4eed042093#file-all-project-json I've shared the list I get when publishing here: https://gist.github.com/ericstj/af57217e1972f78408d66b4eed042093#file-net46-dlls-txt Your file list was missing the following, possibly truncated?
Your file list included System.Net.Http.Rtc.dll incorrectly due to your import of netcore50 and direct-reference to this UWP-specific package.
They will not have to bundle these dependencies, correct. If you do include all of these in your desktop layout you should make sure to include the same in the NETCoreApp layout for parity since some of these aren't in the shared framework. That should happen automatically if you reference the packages in the project used to publish your MSBuild.exe for NETCoreApp. |
My mistake, for some reason I thought I needed to include a runtime which caused restore errors. Removing the runtime let me remove the import.
I was under the impression that we needed to stick with whatever version of .NET Framework that VS supports which is .NET 4.6. So I didn't add the ones that you said were .NET 4.6.1. Is this incorrect?
Won't the Also, we think we'll need to add binding redirects for the facades to upgrade 0.0.0.0-[our version] to our version, do you agree? Assembly redirects aren't possible in .NET Core though I suppose. |
There is only one package that was net461 specific, it was commented out:
The .NET Core host is the one responsible for parsing a
Yes, you should do this in MSBuild.exe.config. You don't need redirects on .NET Core, it automatically allows a higher version than the ref. |
Does DependencyModel allow you to process Deps.json? VM (and related support) should be agnostic of appmodel activation support artifacts. But if someone wishes to build one, I agree, they can absolutely build it. |
Even if it did, the deps.json wouldn't have the right info in it unless the task restored and published for NETCoreApp. /cc @eerhardt |
Question here: does this set of facades map to Net Core App 1.0 or 1.1? |
Here's my prototype: https://github.com/jeffkl/msbuild/commits/facades I'm trying it in VS right now... |
We had to do this in the dotnet/sdk as well. See https://github.com/dotnet/sdk/blob/master/build/Nuget/Microsoft.NET.Sdk.Nuget.targets, where we are copying a bunch of assemblies from our nuget cache and deploying them next to our task.dll. For .netcoreapp1.0, the only "System" assembly we needed was
If you target |
Good point, right now it's 1.0 which was an assumption because everyone is focusing on LTS. Using 1.1 would bring in those higher versions without much risk since most of these are facades. @jeffkl see the updated project.json here: https://gist.github.com/ericstj/af57217e1972f78408d66b4eed042093#file-all-1-1-project-json @weshaggard this includes some standalone OOB assemblies (like System.Net.Http and System.Collections.Immutable) we might want to exclude those, yes?
@eerhardt that's not the point. Those are the easy ones: they'll get published next to the task. Suppose a package has NETCoreApp-specific assemblies. Those won't be next to the task or present in the deps. |
@ericstj terminology question. What do you call the Net Standard 1.3 facades for the version specific to Net Core App 1.1? |
@ericstj okay that means I have to remove
|
Moved this out from RTM. I don't think we can take this change at this point given there's a workaround for task authors. If anyone strongly objects let me know. @MattGertz for awareness as well. |
Reluctantly agree, given the new bar JoC set last night. @jaredpar, where does this leave us? |
@jeffkl yeah, you can remove those. |
@MattGertz just means that extensions will need to bring along facades with them for 15.0. At this point though we should probably just skip the 1.3 facades altogether and focus on 2.0. Post RTW obviously. |
2.0 will definitely be a better deployment model for tasks, but I do think MSBuild needs to better consider the implications of task dependencies, something similar to what the old CLI did with "tools". /cc @eerhardt |
I agree with what you're saying, but it seems like the runtime should have the ability to do this? I wouldn't want msbuild to be aware of a |
I wouldn't go so far as to prescribe I think you just need to draw out what e2e looks like for an msbuild task with dependencies, including if those dependencies differ by framework. NS2.0 makes a lot of this moot for the framework dependencies but doesn't do so for things higher in the stack like 3rd party dependencies. It might be reasonable to say that the answer is something in the tooling that warns a user if they try to publish something for netstandard* and a package has fx specific assets for TFMs supporting netstandard. You could then say that the solution for that warning is to publish the task for specific TFMs then switch at runtime with |
Hi everyone. I think this trail is already closed but my solution when I encounter this error is uninstall and re-install the typescript.exe and it works. |
I'm closing this since we've moved to .NETStandard2.0 and all facades are in later versions of the .NET Framework. |
We just encountered an intermittent failure fixed by dotnet/sdk#629 that occurred because façade assemblies are no longer available in the
MSBuild.exe
directory.Prior to RC.3, the façade assemblies were available because Roslyn placed them next to its assemblies, which were in the MSBuild
bin
directory. After #1406 moved Roslyn to its own directory, the façades were no longer available to tasks that did not redistribute them (unless some other mechanism had loaded them already).Since we expect it to be common to build a task assembly that depends on these façades, it would be nice if MSBuild itself distributed them so they're available to the loader for all tasks and the tasks don't have to each redistribute them.
The text was updated successfully, but these errors were encountered: