diff --git a/src/Build.UnitTests/BackEnd/TaskHostCallback_Tests.cs b/src/Build.UnitTests/BackEnd/TaskHostCallback_Tests.cs index 8c3a0e205cf..5848692f084 100644 --- a/src/Build.UnitTests/BackEnd/TaskHostCallback_Tests.cs +++ b/src/Build.UnitTests/BackEnd/TaskHostCallback_Tests.cs @@ -1,4 +1,4 @@ -// Licensed to the .NET Foundation under one or more agreements. +// Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. using System.Collections.Generic; @@ -38,7 +38,6 @@ public TaskHostCallback_Tests(ITestOutputHelper output) public void IsRunningMultipleNodes_WorksWithExplicitTaskHostFactory(int maxNodeCount, bool expectedResult) { using TestEnvironment env = TestEnvironment.Create(_output); - env.SetEnvironmentVariable("MSBUILDENABLETASKHOSTCALLBACKS", "1"); string projectContents = $@" @@ -70,7 +69,6 @@ public void IsRunningMultipleNodes_WorksWithExplicitTaskHostFactory(int maxNodeC public void IsRunningMultipleNodes_WorksWhenAutoEjectedInMultiThreadedMode(int maxNodeCount, bool expectedResult) { using TestEnvironment env = TestEnvironment.Create(_output); - env.SetEnvironmentVariable("MSBUILDENABLETASKHOSTCALLBACKS", "1"); string testDir = env.CreateFolder().Path; // IsRunningMultipleNodesTask lacks MSBuildMultiThreadableTask attribute, so it's auto-ejected to TaskHost in MT mode @@ -107,41 +105,6 @@ public void IsRunningMultipleNodes_WorksWhenAutoEjectedInMultiThreadedMode(int m logger.FullLog.ShouldContain($"IsRunningMultipleNodes = {expectedResult}"); } - /// - /// Verifies that accessing IsRunningMultipleNodes when callbacks are disabled - /// logs error MSB5022 (BuildEngineCallbacksInTaskHostUnsupported). - /// This preserves the pre-callback behavior where unsupported IBuildEngine - /// methods in TaskHost log an error. - /// - [Fact] - public void IsRunningMultipleNodes_LogsErrorWhenCallbacksNotSupported() - { - using TestEnvironment env = TestEnvironment.Create(_output); - - // Explicitly do NOT set MSBUILDENABLETASKHOSTCALLBACKS -- callbacks should be disabled - string projectContents = $@" - - - - <{nameof(IsRunningMultipleNodesTask)}> - - - -"; - - TransientTestProjectWithFiles project = env.CreateTestProjectWithFiles(projectContents); - ProjectInstance projectInstance = new(project.ProjectFile); - - var logger = new MockLogger(_output); - BuildResult buildResult = BuildManager.DefaultBuildManager.Build( - new BuildParameters { MaxNodeCount = 4, EnableNodeReuse = false, Loggers = [logger] }, - new BuildRequestData(projectInstance, targetsToBuild: ["Test"])); - - // MSB5022 error should be logged -- the callback was not forwarded - logger.ErrorCount.ShouldBeGreaterThan(0); - logger.FullLog.ShouldContain("MSB5022"); - } - /// /// Verifies RequestCores callback works when task is explicitly run in TaskHost via TaskHostFactory. /// The first RequestCores call should always return at least 1 (the implicit core). @@ -150,7 +113,6 @@ public void IsRunningMultipleNodes_LogsErrorWhenCallbacksNotSupported() public void RequestCores_WorksWithExplicitTaskHostFactory() { using TestEnvironment env = TestEnvironment.Create(_output); - env.SetEnvironmentVariable("MSBUILDENABLETASKHOSTCALLBACKS", "1"); string projectContents = $@" @@ -182,7 +144,6 @@ public void RequestCores_WorksWithExplicitTaskHostFactory() public void RequestAndReleaseCores_WorksWithExplicitTaskHostFactory() { using TestEnvironment env = TestEnvironment.Create(_output); - env.SetEnvironmentVariable("MSBUILDENABLETASKHOSTCALLBACKS", "1"); string projectContents = $@" @@ -215,7 +176,6 @@ public void RequestAndReleaseCores_WorksWithExplicitTaskHostFactory() public void RequestCores_WorksWhenAutoEjectedInMultiThreadedMode() { using TestEnvironment env = TestEnvironment.Create(_output); - env.SetEnvironmentVariable("MSBUILDENABLETASKHOSTCALLBACKS", "1"); string testDir = env.CreateFolder().Path; // RequestCoresTask lacks MSBuildMultiThreadableTask attribute, so it's auto-ejected to TaskHost in MT mode @@ -358,96 +318,6 @@ public void GlobalProperties_UseBuildLevelWhenChangeWaveDisabled() logger.FullLog.ShouldContain("GlobalPropertyCount = 0"); } - /// - /// Regression test for https://github.com/dotnet/msbuild/issues/13333 - /// When callbacks are not supported (cross-version OOP TaskHost), RequestCores must - /// throw NotImplementedException (not log a build error and return 0). - /// Real callers (MonoAOTCompiler, EmccCompile, ILStrip, EmitBundleBase) catch this - /// exception and fall back to their own parallelism estimate. The previous behavior - /// of logging BuildErrorEventArgs caused the build to fail silently. - /// - [Fact] - public void RequestCores_ThrowsNotImplementedWhenCallbacksNotSupported() - { - using TestEnvironment env = TestEnvironment.Create(_output); - - // Explicitly do NOT set MSBUILDENABLETASKHOSTCALLBACKS -- callbacks should be disabled. - // Use RequestCoresWithFallbackTask which catches NotImplementedException like real callers do. - string projectContents = $@" - - - - <{nameof(RequestCoresWithFallbackTask)} CoreCount=""4""> - - - - -"; - - TransientTestProjectWithFiles project = env.CreateTestProjectWithFiles(projectContents); - ProjectInstance projectInstance = new(project.ProjectFile); - - var logger = new MockLogger(_output); - BuildResult buildResult = BuildManager.DefaultBuildManager.Build( - new BuildParameters { MaxNodeCount = 4, EnableNodeReuse = false, Loggers = [logger] }, - new BuildRequestData(projectInstance, targetsToBuild: ["Test"])); - - // Build must succeed -- the task catches NotImplementedException and falls back. - buildResult.OverallResult.ShouldBe(BuildResultCode.Success); - - // No errors should be logged -- NotImplementedException is caught by the task, not by MSBuild. - logger.ErrorCount.ShouldBe(0); - - // The task should have used its fallback path (NotImplementedException was thrown). - logger.FullLog.ShouldContain("RequestCores threw NotImplementedException, using fallback"); - - // GrantedCores should be the task's own fallback (CoreCount), not 0. - logger.FullLog.ShouldContain("GrantedCores = 4"); - } - - /// - /// Regression test for https://github.com/dotnet/msbuild/issues/13333 - /// When callbacks are not supported, the full caller pattern (RequestCores with catch, - /// then skip ReleaseCores) must work. This matches MonoAOTCompiler/EmccCompile/ILStrip: - /// try { cores = be9.RequestCores(N); } - /// catch (NotImplementedException) { be9 = null; } - /// finally { be9?.ReleaseCores(cores); } - /// ReleaseCores must NOT be called when the fallback fires (be9 is nulled). - /// - [Fact] - public void RequestAndReleaseCores_FallbackSkipsReleaseWhenCallbacksNotSupported() - { - using TestEnvironment env = TestEnvironment.Create(_output); - - // Explicitly do NOT set MSBUILDENABLETASKHOSTCALLBACKS -- callbacks should be disabled - string projectContents = $@" - - - - <{nameof(RequestCoresWithFallbackTask)} CoreCount=""4"" ReleaseAfter=""true""> - - - - -"; - - TransientTestProjectWithFiles project = env.CreateTestProjectWithFiles(projectContents); - ProjectInstance projectInstance = new(project.ProjectFile); - - var logger = new MockLogger(_output); - BuildResult buildResult = BuildManager.DefaultBuildManager.Build( - new BuildParameters { MaxNodeCount = 4, EnableNodeReuse = false, Loggers = [logger] }, - new BuildRequestData(projectInstance, targetsToBuild: ["Test"])); - - // Build must succeed. - buildResult.OverallResult.ShouldBe(BuildResultCode.Success); - logger.ErrorCount.ShouldBe(0); - - // Fallback fired -- ReleaseCores should have been skipped (nulled in catch). - logger.FullLog.ShouldContain("RequestCores threw NotImplementedException, using fallback"); - logger.FullLog.ShouldNotContain("ReleaseCores("); - } - /// /// Verifies BuildProjectFile callback works when task is explicitly run in TaskHost via TaskHostFactory. /// The child project should build and the task should return success. @@ -456,7 +326,6 @@ public void RequestAndReleaseCores_FallbackSkipsReleaseWhenCallbacksNotSupported public void BuildProjectFile_WorksWithExplicitTaskHostFactory() { using TestEnvironment env = TestEnvironment.Create(_output); - env.SetEnvironmentVariable("MSBUILDENABLETASKHOSTCALLBACKS", "1"); string childProject = env.CreateFile("Child.proj", """ @@ -496,7 +365,6 @@ public void BuildProjectFile_WorksWithExplicitTaskHostFactory() public void BuildProjectFile_ForwardsGlobalProperties() { using TestEnvironment env = TestEnvironment.Create(_output); - env.SetEnvironmentVariable("MSBUILDENABLETASKHOSTCALLBACKS", "1"); string childProject = env.CreateFile("Child.proj", """ @@ -535,7 +403,6 @@ public void BuildProjectFile_ForwardsGlobalProperties() public void BuildProjectFile_ReturnsTargetOutputs() { using TestEnvironment env = TestEnvironment.Create(_output); - env.SetEnvironmentVariable("MSBUILDENABLETASKHOSTCALLBACKS", "1"); string childProject = env.CreateFile("Child.proj", """ @@ -583,7 +450,6 @@ public void BuildProjectFile_ReturnsTargetOutputs() public void BuildProjectFile_ChildFailure_ReturnsFalse() { using TestEnvironment env = TestEnvironment.Create(_output); - env.SetEnvironmentVariable("MSBUILDENABLETASKHOSTCALLBACKS", "1"); string childProject = env.CreateFile("Child.proj", """ @@ -623,7 +489,6 @@ public void BuildProjectFile_ChildFailure_ReturnsFalse() public void BuildProjectFile_WorksWhenAutoEjectedInMultiThreadedMode() { using TestEnvironment env = TestEnvironment.Create(_output); - env.SetEnvironmentVariable("MSBUILDENABLETASKHOSTCALLBACKS", "1"); string testDir = env.CreateFolder().Path; string childProject = Path.Combine(testDir, "Child.proj"); @@ -663,47 +528,5 @@ public void BuildProjectFile_WorksWhenAutoEjectedInMultiThreadedMode() logger.FullLog.ShouldContain("external task host"); logger.FullLog.ShouldContain("ChildBuiltInMT"); } - - /// - /// Verifies that BuildProjectFile when callbacks are disabled logs error MSB5022. - /// - [Fact] - public void BuildProjectFile_LogsErrorWhenCallbacksNotSupported() - { - using TestEnvironment env = TestEnvironment.Create(_output); - - string childProject = env.CreateFile("Child.proj", """ - - - - - - """).Path; - - // Explicitly do NOT set MSBUILDENABLETASKHOSTCALLBACKS - string projectContents = $@" - - - - <{nameof(BuildProjectFileTask)} ProjectFile=""{childProject}"" Targets=""Build""> - - - -"; - - TransientTestProjectWithFiles project = env.CreateTestProjectWithFiles(projectContents); - ProjectInstance projectInstance = new(project.ProjectFile); - - var logger = new MockLogger(_output); - BuildResult buildResult = BuildManager.DefaultBuildManager.Build( - new BuildParameters { MaxNodeCount = 4, EnableNodeReuse = false, Loggers = [logger] }, - new BuildRequestData(projectInstance, targetsToBuild: ["Test"])); - - // MSB5022 error should be logged - logger.ErrorCount.ShouldBeGreaterThan(0); - logger.FullLog.ShouldContain("MSB5022"); - // Child should not have been built - logger.FullLog.ShouldNotContain("ShouldNotRun"); - } } } diff --git a/src/Framework/Traits.cs b/src/Framework/Traits.cs index 4dc17f9722d..ab29ceb21b8 100644 --- a/src/Framework/Traits.cs +++ b/src/Framework/Traits.cs @@ -130,12 +130,6 @@ public Traits() /// TODO: Replace with command line flag when feature is completed. The environment variable is intented to avoid exposing the flag early. public readonly bool EnableRarNode = !string.IsNullOrEmpty(Environment.GetEnvironmentVariable("MSBuildRarNode")); - /// - /// Enable IBuildEngine callbacks in the TaskHost process. - /// Temporary escape hatch until all callback stages are complete and PacketVersion is bumped to 3. - /// - public readonly bool EnableTaskHostCallbacks = Environment.GetEnvironmentVariable("MSBUILDENABLETASKHOSTCALLBACKS") == "1"; - /// /// Name of environment variables used to enable MSBuild server. /// diff --git a/src/MSBuild/OutOfProcTaskHostNode.cs b/src/MSBuild/OutOfProcTaskHostNode.cs index e2c8dc87507..2e57d7142ae 100644 --- a/src/MSBuild/OutOfProcTaskHostNode.cs +++ b/src/MSBuild/OutOfProcTaskHostNode.cs @@ -210,15 +210,14 @@ private readonly AsyncLocal _currentTaskContext /// /// Minimum packet version required for IBuildEngine callback support. - /// When all callback stages are complete, PacketVersion will be bumped to this value. /// private const byte CallbacksMinPacketVersion = 4; /// /// Whether the owning worker node supports IBuildEngine callbacks. - /// True if the worker node's packet version is high enough, or if the feature is force-enabled via env var. + /// True if the worker node's packet version is high enough. /// - private bool CallbacksSupported => _parentPacketVersion >= CallbacksMinPacketVersion || Traits.Instance.EnableTaskHostCallbacks; + private bool CallbacksSupported => _parentPacketVersion >= CallbacksMinPacketVersion; /// /// Gets the effective configuration for the current task thread. diff --git a/src/Shared/INodePacket.cs b/src/Shared/INodePacket.cs index 17acfc429f7..e4ff9167c97 100644 --- a/src/Shared/INodePacket.cs +++ b/src/Shared/INodePacket.cs @@ -339,11 +339,12 @@ internal static class NodePacketTypeExtensions /// 1: .NET task host support. /// 2: Added support for translating/reading HostServices, ProjectFile, TargetName in TaskHostConfiguration. /// 3: Added App Host support. + /// 4: Added IsRunningMultipleNodes, Request/ReleaseCores, BuildProjectFile callbacks support for OOP TaskHost. /// /// When incrementing this version, ensure compatibility with existing /// task hosts and update the corresponding deserialization logic. /// - public const byte PacketVersion = 3; + public const byte PacketVersion = 4; // Flag bits in upper 2 bits private const byte ExtendedHeaderFlag = 0x40; // Bit 6: 01000000