Enable TaskHostFactory for .NET (Core) MSBuild by rainersigwald · Pull Request #6994 · dotnet/msbuild

rainersigwald · 2021-10-25T20:03:22Z

Context

A task author can force any task to run out of process by specifying that it must run with TaskFactory="TaskHostFactory". But we never made this work on .NET Core; it fails with an error like

S:\msbuild\foo.proj(8,5): error MSB4216: Could not run the "Message" task because MSBuild could not create or connect to a task host with runtime "CLR4" and architecture "x64".  Please ensure that (1) the requested runtime and/or architecture are available on the machine, and (2) that the required executable "S:\msbuild\.dotnet\sdk\6.0.100-rc.1.21458.32\MSBuild.exe" exists and can be run.

That's because the "default" task host runtime was hardcoded to be CLR4 (even when on .NET 6.0+).

Changes Made

Created GetCurrentMSBuildRuntime() and used it; plumbed the new value around and checked for it in a few places. Eliminated FEATURE_TASKHOST since it's on everywhere now. Unified the named-pipe-name computation code (before it was using a different pipe name for taskhost pipes on UNIX).

Testing

Re-enabled tests for this behavior that have been disabled on .NET Core. Extended some to account for the NET runtime.

This enables taskhost tests on .NET Core. Includes a couple of low- complexity test fixes.

…entity

rainersigwald · 2021-11-03T19:47:01Z

src/Shared/XMakeAttributes.cs

            internal const string clr2 = "CLR2";
            internal const string clr4 = "CLR4";
            internal const string currentRuntime = "CurrentRuntime";
+            internal const string net = "NET";


@baronfel I'd like your input on this new name please. We need to add an entry to the list that indicates ".NET Core or .NET 5.0+" and is distinguishable from the longstanding CLR2 and CLR4. We should probably get buyoff from somebody in .NET Runtime PM about it.

Reaching out to some folks for discussion 👍

Where are these values exposed? Presumably you want input on the literal itself, not the name of the constant field, right?

Correct. They are exposed to MSBuild task authors in the UsingTask element or "phantom parameters" on task invocations within targets (docs on existing CLR2/CLR4 options).

The existing options are "CLR2" which means "run on .NET Framework 3.5" and "CLR4" which means "run on .NET Framework 4.7.2+". We need one for "run on .NET 6.0+". If we'd done this before .NET 5.0, I would have selected CORE which I still kinda like, but since it's not .NET Core any more that's probably not the best choice.

I lean towards "NET" now, personally. There's a long internal email thread where I think we've settled on this nomenclature.

benvillalobos

Looks mostly fine. My main concern is variable/property naming in the long term. I feel like NET as a value is not explicitly clear that we're in a "net core" case and it could be confused for net48 or net framework

benvillalobos · 2021-11-03T19:57:31Z

src/Build/Definition/Toolset.cs


+                    reservedProperties.Add(ProjectPropertyInstance.Create(ReservedPropertyNames.msbuildRuntimeType,
+#if RUNTIME_TYPE_NETCORE
+                        "Core",


Nit: The new handshake option is NET which refers to a net core build. Here the runtime is being set to Core if it's on net core. For consistency should they have the same name? When I first saw the handshake option I felt like it should be named NETCORE or something.

benvillalobos · 2021-11-03T20:03:41Z

src/Shared/XMakeAttributes.cs

-            if ((runtimeA.Equals(MSBuildRuntimeValues.currentRuntime, StringComparison.OrdinalIgnoreCase) && runtimeB.Equals(MSBuildRuntimeValues.clr4, StringComparison.OrdinalIgnoreCase)) ||
-                (runtimeA.Equals(MSBuildRuntimeValues.clr4, StringComparison.OrdinalIgnoreCase) && runtimeB.Equals(MSBuildRuntimeValues.currentRuntime, StringComparison.OrdinalIgnoreCase)))
+            if ((runtimeA.Equals(MSBuildRuntimeValues.currentRuntime, StringComparison.OrdinalIgnoreCase) && runtimeB.Equals(GetCurrentMSBuildRuntime(), StringComparison.OrdinalIgnoreCase)) ||
+                (runtimeA.Equals(GetCurrentMSBuildRuntime(), StringComparison.OrdinalIgnoreCase) && runtimeB.Equals(MSBuildRuntimeValues.currentRuntime, StringComparison.OrdinalIgnoreCase)))


grumbling out loud: funny how we ended up in a scenario where MSBuildRuntimeValues.currentRuntime and GetCurrentMSBuildRuntime() are two different things. Which lead to the variable name on line 221 string actualCurrentRuntime 😅

benvillalobos · 2021-11-03T20:14:46Z

src/Shared/CommunicationsUtilities.cs

+                    {
+                        clrVersion = 4;
+                    }
+                    else if (runtimeVersion.Equals(XMakeAttributes.MSBuildRuntimeValues.clr2, StringComparison.OrdinalIgnoreCase))


nit: checking clr2 first is more intuitive. Or is this ordering intentional?

Just a straight port of the existing logic but I agree that putting it in order makes sense.

We should do the ordering based on which is expected to occur most often, so I'd even bump MSBuildRuntimeValue.net to the top.

That's an optimization that PGO should be great at and with small effect, so I don't think it's worth a readability hit without proof (and this isn't a super hot loop or anything).

Why do you think net would happen most often?

benvillalobos · 2021-11-03T20:15:57Z

src/Shared/UnitTests/XmakeAttributes_Tests.cs

 // Licensed under the MIT license. See LICENSE file in the project root for full license information.

 using Microsoft.Build.Shared;
+


nit whitespace

This is actually my preferred style:

Block for System. / BCL usings

Block for local-repo usings

Block for third-party usings

benvillalobos · 2021-11-03T20:17:01Z

src/Shared/XMakeAttributes.cs

+#if NET40_OR_GREATER
+            return MSBuildRuntimeValues.clr4;
+#else
+            return MSBuildRuntimeValues.net;


Thinking this should be netcore or core to be extra clear.

Forgind

Looks pretty good. Mostly little things

Forgind · 2021-11-05T17:15:26Z

src/Build.UnitTests/BackEnd/BuildManager_Tests.cs

        }

-#if FEATURE_TASKHOST && !NO_MSBUILDTASKHOST
+#if !NO_MSBUILDTASKHOST


Is the comment below still accurate? I'm wondering when we wouldn't have task hosts available.

We don't have the 3.5 task host on .NET Core or Mono builds.

Forgind · 2021-11-05T17:18:56Z

src/Build/Definition/Toolset.cs

+#if RUNTIME_TYPE_NETCORE
+                        "Core",
+#elif MONO
+                        NativeMethodsShared.IsMono ? "Mono" : "Full");


Why would it be MONO but not IsMono?

MSBuild compiled for mono but running on .NET Framework was a possible thing for a while. I don't think it's super relevant any more but this is copypasta from existing code and I'd rather have consistent behavior.

Forgind · 2021-11-05T17:21:43Z

src/Shared/CommunicationsUtilities.cs

+                    {
+                        clrVersion = 4;
+                    }
+                    else if (runtimeVersion.Equals(XMakeAttributes.MSBuildRuntimeValues.clr2, StringComparison.OrdinalIgnoreCase))


We should do the ordering based on which is expected to occur most often, so I'd even bump MSBuildRuntimeValue.net to the top.

Forgind · 2021-11-05T17:24:47Z

src/Shared/NamedPipeUtil.cs

    internal static class NamedPipeUtil
    {
-        internal static string GetPipeNameOrPath(string pipeName)
+        internal static string GetPipeNameOrPath(int? processId = null)


It looks like we're using this more. I'm curious if it will show up as extra MSBuild# string allocations.

Forgind · 2021-11-05T17:26:57Z

src/Shared/UnitTests/XmakeAttributes_Tests.cs


-        [Fact]
-        public void TestMergeRuntimeValues()
+        [Theory]


Nit:
This test and the previous one are similar in content but different in style. Switch to same style?

I don't think the juice is worth the squeeze on this.

src/Shared/XMakeAttributes.cs

Forgind · 2021-11-05T17:39:18Z

src/Tasks/Microsoft.Common.tasks

    <UsingTask TaskName="Microsoft.Build.Tasks.RegisterAssembly"                      AssemblyName="Microsoft.Build.Tasks.Core, Version=15.1.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" Condition="('$(MSBuildAssemblyVersion)' != '') and '$(DisableOutOfProcTaskHost)' != ''" />
-    <UsingTask TaskName="Microsoft.Build.Tasks.RegisterAssembly"                      AssemblyName="Microsoft.Build.Tasks.Core, Version=15.1.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" Runtime="CLR4" Condition="('$(MSBuildAssemblyVersion)' != '') and '$(DisableOutOfProcTaskHost)' == ''" />
-    <UsingTask TaskName="Microsoft.Build.Tasks.RegisterAssembly"                      AssemblyName="Microsoft.Build.Tasks.v3.5, Version=3.5.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" Runtime="CLR2" Condition="('$(MSBuildAssemblyVersion)' != '') and '$(DisableOutOfProcTaskHost)' == ''" />
+    <UsingTask TaskName="Microsoft.Build.Tasks.RegisterAssembly"                      AssemblyName="Microsoft.Build.Tasks.Core, Version=15.1.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" Runtime="CLR4" Condition="('$(MSBuildAssemblyVersion)' != '') and '$(DisableOutOfProcTaskHost)' == '' and '$(MSBuildRuntimeType)' != 'Core'" />


Do we need a core version of RegisterAssembly? Not sure if it's been ported. Same for UnregisterAssembly.

COM build operations in general aren't functional on Core today. This doesn't change that but does potentially make it slightly clearer.

Forgind · 2021-11-05T17:43:51Z

Oh, and I'm curious if there are any other issues (disabled tests) we can close with this PR. Have you checked/just know there aren't?

rainersigwald

Accidentally didn't send the comment I made promptly 🤦🏻

rainersigwald · 2021-11-03T20:23:11Z

src/Shared/CommunicationsUtilities.cs

+                    {
+                        clrVersion = 4;
+                    }
+                    else if (runtimeVersion.Equals(XMakeAttributes.MSBuildRuntimeValues.clr2, StringComparison.OrdinalIgnoreCase))


Just a straight port of the existing logic but I agree that putting it in order makes sense.

rainersigwald · 2021-11-04T21:32:43Z

src/Shared/XMakeAttributes.cs

            internal const string clr2 = "CLR2";
            internal const string clr4 = "CLR4";
            internal const string currentRuntime = "CurrentRuntime";
+            internal const string net = "NET";


Correct. They are exposed to MSBuild task authors in the UsingTask element or "phantom parameters" on task invocations within targets (docs on existing CLR2/CLR4 options).

The existing options are "CLR2" which means "run on .NET Framework 3.5" and "CLR4" which means "run on .NET Framework 4.7.2+". We need one for "run on .NET 6.0+". If we'd done this before .NET 5.0, I would have selected CORE which I still kinda like, but since it's not .NET Core any more that's probably not the best choice.

ladipro · 2021-11-15T15:03:26Z

src/Build/Instance/TaskFactories/AssemblyTaskFactory.cs

-                string architecture = null;
-
-                if (!mergedParameters.TryGetValue(XMakeAttributes.runtime, out runtime))
+                if (!mergedParameters.TryGetValue(XMakeAttributes.runtime, out _))


nit:

Suggested change

if (!mergedParameters.TryGetValue(XMakeAttributes.runtime, out _))

if (!mergedParameters.ContainsKey(XMakeAttributes.runtime))

🤦🏻 thanks

ladipro · 2021-11-15T15:03:54Z

src/Build/Instance/TaskFactories/AssemblyTaskFactory.cs

                }

-                if (!mergedParameters.TryGetValue(XMakeAttributes.architecture, out architecture))
+                if (!mergedParameters.TryGetValue(XMakeAttributes.architecture, out _))


nit:

Suggested change

if (!mergedParameters.TryGetValue(XMakeAttributes.architecture, out _))

if (!mergedParameters.ContainsKey(XMakeAttributes.architecture))

ladipro · 2021-11-15T15:08:42Z

src/Shared/CommunicationsUtilities.cs

+        /// <summary>
+        /// Using the .NET Core/.NET 5.0+ runtime
+        /// </summary>
+        NET = 64,


It looks like this comment should be updated:

msbuild/src/Shared/CommunicationsUtilities.cs

Line 78 in 37c6111

// We currently use 6 bits of this 32-bit integer. Very old builds will instantly reject any handshake that does not start with F5 or 06; slightly old builds always lead with 00.

rainersigwald · 2021-11-15T16:50:38Z

I'm curious if there are any other issues (disabled tests) we can close with this PR. Have you checked/just know there aren't?

I checked lightly and didn't see anything.

mooredynasty · 2022-05-02T21:43:49Z

I see this hit the main branch. I'm wondering what SDK version(s) this was/will be released in? And/or how to find that information myself. Thanks!

Forgind · 2022-05-02T22:04:41Z

I don't know if this is a good way to do it at all, but you can look at the release/version branches in the dotnet/sdk repo then go to eng/Versions.Details.xml and find Microsoft.Build. That gives you a commit hash for the last MSBuild in that version, and you can go to our commits list to figure out if that's before or after this.

Like for release/6.0.2xx, https://github.com/dotnet/sdk/blob/release/6.0.2xx/eng/Version.Details.xml#L71 says a02f736, which is a02f736, which came out in February, so it should also include this commit.

From there, you can do binary search.

rainersigwald added .NET Core Area: Engine Issues impacting the core execution of targets and tasks. labels Oct 25, 2021

rainersigwald added this to the MSBuild 17.1 milestone Oct 25, 2021

rainersigwald added 9 commits November 2, 2021 14:39

Enable out-of-proc TaskHosts on .NET Core

c3f9576

Promote FEATURE_TASKHOST to no-flag-needed

315e0de

This enables taskhost tests on .NET Core. Includes a couple of low- complexity test fixes.

Enable checking runtimetype in .tasks file

d62c4bc

Check runtime type in tasks file

c6c5cef

Extend TestRuntimeValuesMatch for net

67338c8

Extend RuntimeValuesMatch for net

6aa7795

Split and shouldly-ify TestMergeRuntimeValues

459ce25

Use explicit current runtime/arch in CreatableByTaskFactoryMatchingId…

825c796

…entity

Unify to NamedPipeUtil.GetPipeNameOrPath

dd98059

rainersigwald force-pushed the core-taskhost branch from e654a32 to dd98059 Compare November 3, 2021 19:42

rainersigwald requested a review from benvillalobos November 3, 2021 19:42

rainersigwald marked this pull request as ready for review November 3, 2021 19:42

rainersigwald commented Nov 3, 2021

View reviewed changes

benvillalobos reviewed Nov 3, 2021

View reviewed changes

Forgind reviewed Nov 5, 2021

View reviewed changes

rainersigwald commented Nov 8, 2021

View reviewed changes

ladipro approved these changes Nov 15, 2021

View reviewed changes

rainersigwald added 2 commits November 15, 2021 09:50

fixup! Enable out-of-proc TaskHosts on .NET Core

8bd8bb8

fixup! Enable out-of-proc TaskHosts on .NET Core

d2ac982

Forgind approved these changes Nov 15, 2021

View reviewed changes

Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Nov 15, 2021

Forgind merged commit 88882b9 into dotnet:main Nov 15, 2021

rainersigwald deleted the core-taskhost branch November 15, 2021 21:27

rainersigwald mentioned this pull request May 2, 2022

MSBuild loads and locks task assemblies even when using TaskHostFactory #6461

Closed

rainersigwald mentioned this pull request Jun 28, 2022

Document node reuse #2916

Open

KalleOlaviNiemitalo mentioned this pull request Sep 6, 2022

MSBuild task for creating Tar files #7944

Open

		// Licensed under the MIT license. See LICENSE file in the project root for full license information.

		using Microsoft.Build.Shared;

	if (!mergedParameters.TryGetValue(XMakeAttributes.runtime, out _))
	if (!mergedParameters.ContainsKey(XMakeAttributes.runtime))

Conversation

rainersigwald commented Oct 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Changes Made

Testing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benvillalobos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Forgind left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Forgind commented Nov 5, 2021

Uh oh!

rainersigwald left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

rainersigwald commented Oct 25, 2021 •

edited

Loading

mooredynasty commented May 2, 2022 •

edited

Loading