Isolate MSBuildTaskHost from the rest of MSBuild Codebase#13232
Isolate MSBuildTaskHost from the rest of MSBuild Codebase#13232rainersigwald merged 143 commits intomainfrom
Conversation
Copies all shared files and files linked from other projects directly into MSBuildTaskHost. In addition, all <Compile> items in MSBuildTaskHost.csproj have been updated to point to the new files.
Split the PropertyGroup for $(DefineConstants) into two: one for net3* and one for net4*
This polyfill is unused in MSBuildTaskHost.
Since MSBuildTaskHost only targets .NET 3.5, it does not require conditional compilation for FEATURE_LEGACY_GETCURRENTDIRECTORY. It *always* includes the code path that provides an optimized GetCurrentDirectory on .NET Framework targets earlier than 4.6.2.
Since MSBuildTaskHost only targets .NET 3.5, it does not require conditional compilation for FEATURE_LEGACY_GETFULLPATH. It *always* includes the code path that provides an optimized GetFullPath on .NET Framework targets earlier than 4.6.2.
Since MSBuildTaskHost only targets .NET 3.5, System.Reflection.Assembly.Location is always available.
The FEATURE_CULTUREINFO_GETCULTURES code path is unused by MSBuildTaskHost.
The FEATURE_APM code path is always used by MSBuildTaskHost. .NET Framework 3.5 does support System.Threading.Tasks, so MSBuildTaskHost uses the older "asynchronous programming model (APM)".
…Host Many of the FEATURE_* conditional compilation constants defined for .NET 3.5 builds never appear in code compiled within MSBuildTaskHost. This change removes all of those for .NET 3.5 builds.
The FEATURE_PIPE_SECURITY and FEATURE_NAMED_PIPE_SECURITY_CONSTRUCTOR code paths is always used by MSBuildTaskHost.
The FEATURE_SECURITY_PERMISSIONS code paths are always available in MSBuildTaskHost.
The FEATURE_SECURITY_PRINCIPAL_WINDOWS code paths are always available in MSBuildTaskHost.
The FEATURE_THREAD_ABORT code paths are always available in MSBuildTaskHost.
The FEATURE_VISUALSTUDIOSETUP code paths are never available in MSBuildTaskHost. This constant is always removed from .NET 3.5 builds.
The FeatureAppDomain, FeatureSystemConfiguration, and FeatureXamlTypes properties are not used in .NET 3.5 builds. NOTE: It seems that FeatureAppDomain is the only one of these properties that are used anywhere. However, it is used in a target in Microsoft.Build, which is not built for .NET 3.5.
The FEATURE_REPORTFILEACCESSES code paths are not available in MSBuildTaskHost. This constant was never included in .NET 3.5 builds.
BuildEnvironmentHelper includes a check for AppContext.BaseDirectory that is always returns null on .NET 3.5 and the MSBuildTaskHost. This change removes that check and related code.
VisualStudioLocationHelper.GetInstances() always returns an empty list on .NET 3.5. So, BuildEnvironmentHelper.TryFromSetupApi (the only caller) can be removed from MSBuildTaskHost along with all of VisualStuiodLocationHelper.
RUNTIME_TYPE_NETCORE code paths aren't ever compiled in .NET 3.5 builds.
CopyOnWriteDictionary, ReadOnlyEmptyCollection, and ReadOnlyEmptyDictionary are never used in MSBuildTaskHost and can be safely removed.
Since MSBuildTaskHost only builds for .NET 3.5, there are many code blocks specific to other .NET versions that can be removed. NOTE: Disabled code blocks were intentionally NOT removed from polyfill types.
BUILDINGAPPXTASKS is not relevant when building MSBuildTaskHost, so code compiled with BUILDINGAPPXTASKS can be removed.
MSBuildTaskHost is always compiled with CLR2COMPATIBILITY, so code blocks that aren't compiled with that conditional compilation constant can be removed.
MSBuildTaskHost is always compiled with TASKHOST. So, code blocks disabled when compiled with TASKHOST can be removed.
MSBuildTaskHost is never compiled with FEATURE_ASSEMBLYLOADCONTEXT.
MSBuildTaskHost is never compiled with FEATURE_PIPEOPTIONS_CURRENTUSERONLY. So, code blocks disabled with FEATURE_PIPEOPTIONS_CURRENTUSERONLY can be removed.
MSBuildTaskHost is always compiled with FEATURE_NET35_TASKHOST.
MSBuildTaskHost is always compiled with NO_FRAMEWORK_IVT. So, code blocks disabled under NO_FRAMEWORK_IVT can be removed.
SimaTian
left a comment
There was a problem hiding this comment.
MSBuildTaskHost doesn't get much benefit from MSBuildNameIgnoreCaseComparer over using StringComparer.OrdinalIgnoreCase. So, this change removes MSBuildNameIgnoreCaseComparer.cs and IConstrainedEqualityComparer.cs from MSBuildTaskHost rather than include the extra complexity.
I'm not disputing this claim but I would love to know a bit more about the reasoning.
(or if we have some sort of benchmark data for this, even better)
is there some difference in the assumptions for the TaskHost as opposed to general MSBuild environment?
is it that we don't care about TaskHost performance since it's legacy support only and so it's "almost deprecated" and so maintainability has high enough priority?
or something else entirely?
SimaTian
left a comment
There was a problem hiding this comment.
The only bits left in XMakeAttributes are for checking MSBuild's architecture and runtime. This change formalizes that and removes XMakeAttributes.cs.
I love this. This genuinely made me laugh.
First we gut XMakeAttributes few commits ago and then we revisit, notice it is mostly hollow and kill it altogether. Yessssss.
SimaTian
left a comment
There was a problem hiding this comment.
Note that "node reuse" is always false in MSBuildTaskHost
Similar confusion about our TaskHost naming as before. Maybe we should make the naming more explicit and rename this one as a "OldTaskHost" or something?
Because sidecar taskshost (and by extension the other taskhost that is different from this one) I was working on before was definitely handling node reuse.
This naming is a bit unfortunate but it's what has been used since .NET Framework 4.0 so IMO it's too late to change. |
As @rainersigwald mentioned, "MSBuildTaskHost.exe" only runs on Windows on .NET Framework 3.5. It's a special task host for running tasks targeting .NET 3.5 and doesn't use current versions of the other MSBuild binaries. In fact, the only MSBuild binary that it references is "Microsoft.Build.Framework, 3.5.0.0" which gets loaded from the GAC. "MSBuildTaskHost" is always launched with "nodereuse:false", so it's effectively launched for every .NET 3.5 task that's encountered. In practice, that's typically just the .NET 3.5 version of the GenerateResource task, which is legacy and doesn't build out of this repo. As for whether |
|
In general I want |
SimaTian
left a comment
There was a problem hiding this comment.
Awesome PR. I'm all for it. This solves so many pain points.
Thank you.
|
I've read it as closely as I could, with the caveat that started skimming somewhere around the last 30 or so commits. |
I had introduced TaskHostLaunchArgs to consolidate some of the logic used to gather the arguments need to launch nodes. However, this will conflict with another change that is currently out for review: #13175. This change replaces TaskHostLaunchArgs with a NodeLaunchData type that looks more like the one in #13175. It is not identical, but it should make it a bit easier to merge the two changes.
Summary
MSBuildTaskHost.exeprovides legacy support for running .NET Framework 3.5 tasks. It is itself a .NET Framework 3.5 application and loads theMicrosoft.Build.*3.5.0.0 assemblies from the GAC when .NET 3.5 is installed. Today, common targets force three .NET 3.5 tasks to run in MSBuildTaskHost:RegisterAssembly,UnregisterAssembly, and especially,GenerateResource. For .NET 3.5 builds, it is essentially that the 3.5 version ofGenerateResourceruns, since theBinaryFormatteroutput format changed between CLR 2.0 and CLR 4.0.For more than 15 years, MSBuildTaskHost has been compiled from the same shared source files as the rest of MSBuild. This made sense in the .NET Framework 4.0 era, when the delta between 3.5 and 4.0 was small. But as MSBuild has evolved, this arrangement has become increasingly restrictive. Any shared code that uses modern .NET features (
Span<T>,Task<T>, immutable collections, etc.) becomes problematic because MSBuildTaskHost cannot consume them. Shared code changes also risk breaking MSBuildTaskHost and, by extension, the ability to build legacy .NET 3.5 projects.This PR isolates MSBuildTaskHost into its own self‑contained codebase, decoupled from the rest of MSBuild.
Approach
The isolation work follows these steps:
NET,RUNTIME_TYPE_NETCORE, orFEATURE_*flags irrelevant to .NET 3.5 (e.g.,FEATURE_VISUALSTUDIOSETUP).CLR2COMPATIBILITYorFEATURE_ASSEMBLY_LOCATION.Microsoft.Build.Framework3.5.0.0 (e.g.,IBuildEngine3,ITaskItem2,RunInSTAAttribute).Additional Changes Outside MSBuildTaskHost
Refactor handshake components to make the tools directory path deterministic by using the
MSBuildTaskHost.exedirectory when computing handshake salt. (22fb64e)Add a test that verifies building a .NET 3.5 WinForms application on Windows/.NET Framework when .NET 3.5 is installed. (9941c15)
Next Steps
Remove dead code paths from all shared MSBuild files now that .NET 3.5 compatibility is no longer required. (In progress)
Audit conditional compilation constants across MSBuild. (In progress)
Move remaining shared code into a dedicated shared binary (
Microsoft.Build.Framework). (Requires reworking how string resources are consumed across MSBuild binaries.)Future Work
Refactor task host communication to introduce a “protocol adapter” layer. Now that MSBuildTaskHost is effectively frozen in time, this adapter would shield it from future protocol changes while allowing MSBuild to evolve independently.