From dd7831c03cc4d668fdd432d65a75087cf88a388e Mon Sep 17 00:00:00 2001 From: Jeff Kluge Date: Wed, 31 Mar 2021 13:29:18 -0700 Subject: [PATCH 1/2] Correctly fail a restore if an SDK can't be resolved or a top level target like Restore doesn't exist Fixes #6281 --- ref/Microsoft.Build/net/Microsoft.Build.cs | 3 + .../netstandard/Microsoft.Build.cs | 3 + .../BackEnd/BuildManager/BuildManager.cs | 5 + .../BackEnd/BuildManager/BuildRequestData.cs | 15 +++ .../RequestBuilder/TargetBuilder.cs | 10 +- .../Shared/BuildRequestConfiguration.cs | 5 + src/Build/Definition/ProjectLoadSettings.cs | 5 + src/Build/Evaluation/Evaluator.cs | 3 +- src/MSBuild.UnitTests/XMake_Tests.cs | 97 ++++++++++++++++++- src/MSBuild/XMake.cs | 7 +- 10 files changed, 144 insertions(+), 9 deletions(-) diff --git a/ref/Microsoft.Build/net/Microsoft.Build.cs b/ref/Microsoft.Build/net/Microsoft.Build.cs index df63aa205be..7645217f959 100644 --- a/ref/Microsoft.Build/net/Microsoft.Build.cs +++ b/ref/Microsoft.Build/net/Microsoft.Build.cs @@ -772,6 +772,7 @@ public enum ProjectLoadSettings DoNotEvaluateElementsWithFalseCondition = 32, IgnoreInvalidImports = 64, ProfileEvaluation = 128, + FailOnUnresolvedSdk = 256, } public partial class ProjectMetadata : System.IEquatable { @@ -1047,6 +1048,8 @@ public enum BuildRequestDataFlags SkipNonexistentTargets = 16, ProvideSubsetOfStateAfterBuild = 32, IgnoreMissingEmptyAndInvalidImports = 64, + SkipNonexistentNonTopLevelTargets = 128, + FailOnUnresolvedSdk = 256, } public partial class BuildResult { diff --git a/ref/Microsoft.Build/netstandard/Microsoft.Build.cs b/ref/Microsoft.Build/netstandard/Microsoft.Build.cs index 2e945e6c024..a34525978b8 100644 --- a/ref/Microsoft.Build/netstandard/Microsoft.Build.cs +++ b/ref/Microsoft.Build/netstandard/Microsoft.Build.cs @@ -772,6 +772,7 @@ public enum ProjectLoadSettings DoNotEvaluateElementsWithFalseCondition = 32, IgnoreInvalidImports = 64, ProfileEvaluation = 128, + FailOnUnresolvedSdk = 256, } public partial class ProjectMetadata : System.IEquatable { @@ -1042,6 +1043,8 @@ public enum BuildRequestDataFlags SkipNonexistentTargets = 16, ProvideSubsetOfStateAfterBuild = 32, IgnoreMissingEmptyAndInvalidImports = 64, + SkipNonexistentNonTopLevelTargets = 128, + FailOnUnresolvedSdk = 256, } public partial class BuildResult { diff --git a/src/Build/BackEnd/BuildManager/BuildManager.cs b/src/Build/BackEnd/BuildManager/BuildManager.cs index 559d426e357..f5caca9ae84 100644 --- a/src/Build/BackEnd/BuildManager/BuildManager.cs +++ b/src/Build/BackEnd/BuildManager/BuildManager.cs @@ -1702,6 +1702,11 @@ private void ExecuteGraphBuildScheduler(GraphBuildSubmission submission) projectLoadSettings |= ProjectLoadSettings.IgnoreMissingImports | ProjectLoadSettings.IgnoreInvalidImports | ProjectLoadSettings.IgnoreEmptyImports; } + if (submission.BuildRequestData.Flags.HasFlag(BuildRequestDataFlags.FailOnUnresolvedSdk)) + { + projectLoadSettings |= ProjectLoadSettings.FailOnUnresolvedSdk; + } + return new ProjectInstance( path, properties, diff --git a/src/Build/BackEnd/BuildManager/BuildRequestData.cs b/src/Build/BackEnd/BuildManager/BuildRequestData.cs index 4123d7e0922..18993b6cc1b 100644 --- a/src/Build/BackEnd/BuildManager/BuildRequestData.cs +++ b/src/Build/BackEnd/BuildManager/BuildRequestData.cs @@ -75,6 +75,21 @@ public enum BuildRequestDataFlags /// This is especially useful during a restore since some imports might come from packages that haven't been restored yet. /// IgnoreMissingEmptyAndInvalidImports = 1 << 6, + + /// + /// When this flag is present, non top level target(s) in the build request will be skipped if those targets + /// are not defined in the Project to build. The build will still fail if a top lvel target does not exist. + /// This only applies to this build request (if another target calls the "missing target" at any other point + /// this will still result in an error). + /// + SkipNonexistentNonTopLevelTargets = 1 << 7, + + /// + /// When this flag is present, an unresolved MSBuild project SDK will fail the build. This flag is used to + /// change the behavior to still fail when an SDK is missing + /// because those are more fatal. + /// + FailOnUnresolvedSdk = 1 << 8, } /// diff --git a/src/Build/BackEnd/Components/RequestBuilder/TargetBuilder.cs b/src/Build/BackEnd/Components/RequestBuilder/TargetBuilder.cs index 79142963436..3bd6f83b6e5 100644 --- a/src/Build/BackEnd/Components/RequestBuilder/TargetBuilder.cs +++ b/src/Build/BackEnd/Components/RequestBuilder/TargetBuilder.cs @@ -143,10 +143,16 @@ public async Task BuildTargets(ProjectLoggingContext loggingContext foreach (string targetName in targetNames) { var targetExists = _projectInstance.Targets.TryGetValue(targetName, out ProjectTargetInstance targetInstance); - if (!targetExists && entry.Request.BuildRequestDataFlags.HasFlag(BuildRequestDataFlags.SkipNonexistentTargets)) + // Ignore the missing target if: + // SkipNonexistentTargets is set + // -or- + // SkipNonexistentNonTopLevelTargets and the target is is not a top level target + if (!targetExists + && entry.Request.BuildRequestDataFlags.HasFlag(BuildRequestDataFlags.SkipNonexistentTargets) + || entry.Request.BuildRequestDataFlags.HasFlag(BuildRequestDataFlags.SkipNonexistentNonTopLevelTargets) && !entry.Request.Targets.Contains(targetName)) { _projectLoggingContext.LogComment(Framework.MessageImportance.Low, - "TargetSkippedWhenSkipNonexistentTargets", targetName); + "TargetSkippedWhenSkipNonexistentTargets", targetName); } else { diff --git a/src/Build/BackEnd/Shared/BuildRequestConfiguration.cs b/src/Build/BackEnd/Shared/BuildRequestConfiguration.cs index 97f9531e074..8398cb68479 100644 --- a/src/Build/BackEnd/Shared/BuildRequestConfiguration.cs +++ b/src/Build/BackEnd/Shared/BuildRequestConfiguration.cs @@ -466,6 +466,11 @@ private void SetProjectBasedState(ProjectInstance project) projectLoadSettings |= ProjectLoadSettings.IgnoreMissingImports | ProjectLoadSettings.IgnoreInvalidImports | ProjectLoadSettings.IgnoreEmptyImports; } + if (buildRequestDataFlags.HasFlag(buildRequestDataFlags & BuildRequestDataFlags.FailOnUnresolvedSdk)) + { + projectLoadSettings |= ProjectLoadSettings.FailOnUnresolvedSdk; + } + return new ProjectInstance( ProjectFullPath, globalProperties, diff --git a/src/Build/Definition/ProjectLoadSettings.cs b/src/Build/Definition/ProjectLoadSettings.cs index 7f1c9ad2d80..ef51122eca5 100644 --- a/src/Build/Definition/ProjectLoadSettings.cs +++ b/src/Build/Definition/ProjectLoadSettings.cs @@ -59,5 +59,10 @@ public enum ProjectLoadSettings /// Whether to profile the evaluation /// ProfileEvaluation = 128, + + /// + /// Used in combination with to still treat an unresolved MSBuild project SDK as an error. + /// + FailOnUnresolvedSdk = 256, } } diff --git a/src/Build/Evaluation/Evaluator.cs b/src/Build/Evaluation/Evaluator.cs index f9126e6c61f..b3b18242db1 100644 --- a/src/Build/Evaluation/Evaluator.cs +++ b/src/Build/Evaluation/Evaluator.cs @@ -1761,7 +1761,8 @@ private List ExpandAndLoadImports(string directoryOfImportin if (!sdkResult.Success) { - if (_loadSettings.HasFlag(ProjectLoadSettings.IgnoreMissingImports)) + // Ignore the missing import if IgnoreMissingImports is set unless FailOnUnresolvedSdk is also set + if (_loadSettings.HasFlag(ProjectLoadSettings.IgnoreMissingImports) && !_loadSettings.HasFlag(ProjectLoadSettings.FailOnUnresolvedSdk)) { ProjectImportedEventArgs eventArgs = new ProjectImportedEventArgs( importElement.Location.Line, diff --git a/src/MSBuild.UnitTests/XMake_Tests.cs b/src/MSBuild.UnitTests/XMake_Tests.cs index b3c65a114a6..8723b61b5bb 100644 --- a/src/MSBuild.UnitTests/XMake_Tests.cs +++ b/src/MSBuild.UnitTests/XMake_Tests.cs @@ -2108,6 +2108,79 @@ public void RestoreIgnoresMissingImports() logContents.ShouldContain(guid2); } + /// + /// When specifying /t:restore, fail when an SDK can't be resolved. Previous behavior was to try and continue anyway but then "restore" would succeed and build workflows continue on. + /// + [Fact] + public void RestoreFailsOnUnresolvedSdk() + { + string projectContents = ObjectModelHelpers.CleanupFileContents( +$@" + + + + +"); + + string logContents = ExecuteMSBuildExeExpectFailure(projectContents, arguments: "/t:restore"); + + logContents.ShouldContain("error MSB4236: The SDK 'UnresolvedSdk' specified could not be found."); + } + + /// + /// Verifies a non-existent target doesn't fail restore as long as its not considered "top-level" or a target that we're directly executing, in this case Restore. + /// + [Fact] + public void RestoreSkipsNonExistentNonTopLevelTargets() + { + string restoreFirstProps = $"{Guid.NewGuid():N}.props"; + + string projectContents = ObjectModelHelpers.CleanupFileContents( +$@" + + {restoreFirstProps} + + + + + + + + + + + + + + + +"); + + string logContents = ExecuteMSBuildExeExpectSuccess(projectContents, arguments: "/restore"); + + logContents.ShouldContain("Restore target ran"); + logContents.ShouldContain("Build target ran"); + logContents.ShouldContain("Initial target ran"); + } + + /// + /// Verifies restore will fail if the "top-level" target doesn't exist, in this case Restore. + /// + [Fact] + public void RestoreFailsWhenTopLevelTargetIsNonExistent() + { + string projectContents = ObjectModelHelpers.CleanupFileContents( +@" + + + +"); + + string logContents = ExecuteMSBuildExeExpectFailure(projectContents, arguments: "/t:restore"); + + logContents.ShouldContain("error MSB4057: The target \"Restore\" does not exist in the project."); + } + /// /// We check if there is only one target name specified and this logic caused a regression: https://github.com/Microsoft/msbuild/issues/3317 /// @@ -2312,6 +2385,24 @@ private string CopyMSBuild() } private string ExecuteMSBuildExeExpectSuccess(string projectContents, IDictionary filesToCreate = null, IDictionary envsToCreate = null, params string[] arguments) + { + (bool result, string output) = ExecuteMSBuildExe(projectContents, filesToCreate, envsToCreate, arguments); + + result.ShouldBeTrue(() => output); + + return output; + } + + private string ExecuteMSBuildExeExpectFailure(string projectContents, IDictionary filesToCreate = null, IDictionary envsToCreate = null, params string[] arguments) + { + (bool result, string output) = ExecuteMSBuildExe(projectContents, filesToCreate, envsToCreate, arguments); + + result.ShouldBeFalse(() => output); + + return output; + } + + private (bool result, string output) ExecuteMSBuildExe(string projectContents, IDictionary filesToCreate = null, IDictionary envsToCreate = null, params string[] arguments) { using (TestEnvironment testEnvironment = UnitTests.TestEnvironment.Create()) { @@ -2336,10 +2427,8 @@ private string ExecuteMSBuildExeExpectSuccess(string projectContents, IDictionar bool success; string output = RunnerUtilities.ExecMSBuild($"\"{testProject.ProjectFile}\" {String.Join(" ", arguments)}", out success, _output); - - success.ShouldBeTrue(() => output); - - return output; + + return (success, output); } } } diff --git a/src/MSBuild/XMake.cs b/src/MSBuild/XMake.cs index f6e3bc27f4b..7301ac1c84e 100644 --- a/src/MSBuild/XMake.cs +++ b/src/MSBuild/XMake.cs @@ -1422,16 +1422,19 @@ private static (BuildResultCode result, Exception exception) ExecuteRestore(stri // Create a new request with a Restore target only and specify: // - BuildRequestDataFlags.ClearCachesAfterBuild to ensure the projects will be reloaded from disk for subsequent builds - // - BuildRequestDataFlags.SkipNonexistentTargets to ignore missing targets since Restore does not require that all targets exist + // - BuildRequestDataFlags.SkipNonexistentNonTopLevelTargets to ignore missing non-top-level targets since Restore does not require that all targets + // exist, only top-level ones like Restore itself // - BuildRequestDataFlags.IgnoreMissingEmptyAndInvalidImports to ignore imports that don't exist, are empty, or are invalid because restore might // make available an import that doesn't exist yet and the might be missing a condition. + // - BuildRequestDataFlags.FailOnUnresolvedSdk to still fail in the case when an MSBuild project SDK can't be resolved since this is fatal and should + // fail the build. BuildRequestData restoreRequest = new BuildRequestData( projectFile, restoreGlobalProperties, toolsVersion, targetsToBuild: new[] { MSBuildConstants.RestoreTargetName }, hostServices: null, - flags: BuildRequestDataFlags.ClearCachesAfterBuild | BuildRequestDataFlags.SkipNonexistentTargets | BuildRequestDataFlags.IgnoreMissingEmptyAndInvalidImports); + flags: BuildRequestDataFlags.ClearCachesAfterBuild | BuildRequestDataFlags.SkipNonexistentNonTopLevelTargets | BuildRequestDataFlags.IgnoreMissingEmptyAndInvalidImports | BuildRequestDataFlags.FailOnUnresolvedSdk); return ExecuteBuild(buildManager, restoreRequest); } From 9a23e416012950a8354a490cef64b218e74ceaa6 Mon Sep 17 00:00:00 2001 From: Jeff Kluge Date: Thu, 1 Apr 2021 14:13:29 -0700 Subject: [PATCH 2/2] Fix condition where initial targets were skipped because they were non entry targets --- ref/Microsoft.Build/net/Microsoft.Build.cs | 2 +- .../netstandard/Microsoft.Build.cs | 2 +- .../BackEnd/BuildManager/BuildRequestData.cs | 6 ++-- .../RequestBuilder/TargetBuilder.cs | 29 ++++++++------- src/MSBuild.UnitTests/XMake_Tests.cs | 35 ++++++++++++++++--- src/MSBuild/XMake.cs | 4 +-- 6 files changed, 54 insertions(+), 24 deletions(-) diff --git a/ref/Microsoft.Build/net/Microsoft.Build.cs b/ref/Microsoft.Build/net/Microsoft.Build.cs index 7645217f959..53c843c112a 100644 --- a/ref/Microsoft.Build/net/Microsoft.Build.cs +++ b/ref/Microsoft.Build/net/Microsoft.Build.cs @@ -1048,7 +1048,7 @@ public enum BuildRequestDataFlags SkipNonexistentTargets = 16, ProvideSubsetOfStateAfterBuild = 32, IgnoreMissingEmptyAndInvalidImports = 64, - SkipNonexistentNonTopLevelTargets = 128, + SkipNonexistentNonEntryTargets = 128, FailOnUnresolvedSdk = 256, } public partial class BuildResult diff --git a/ref/Microsoft.Build/netstandard/Microsoft.Build.cs b/ref/Microsoft.Build/netstandard/Microsoft.Build.cs index a34525978b8..b1f8429b5cb 100644 --- a/ref/Microsoft.Build/netstandard/Microsoft.Build.cs +++ b/ref/Microsoft.Build/netstandard/Microsoft.Build.cs @@ -1043,7 +1043,7 @@ public enum BuildRequestDataFlags SkipNonexistentTargets = 16, ProvideSubsetOfStateAfterBuild = 32, IgnoreMissingEmptyAndInvalidImports = 64, - SkipNonexistentNonTopLevelTargets = 128, + SkipNonexistentNonEntryTargets = 128, FailOnUnresolvedSdk = 256, } public partial class BuildResult diff --git a/src/Build/BackEnd/BuildManager/BuildRequestData.cs b/src/Build/BackEnd/BuildManager/BuildRequestData.cs index 18993b6cc1b..673ee5f0fdf 100644 --- a/src/Build/BackEnd/BuildManager/BuildRequestData.cs +++ b/src/Build/BackEnd/BuildManager/BuildRequestData.cs @@ -77,12 +77,12 @@ public enum BuildRequestDataFlags IgnoreMissingEmptyAndInvalidImports = 1 << 6, /// - /// When this flag is present, non top level target(s) in the build request will be skipped if those targets - /// are not defined in the Project to build. The build will still fail if a top lvel target does not exist. + /// When this flag is present, non entry target(s) in the build request will be skipped if those targets + /// are not defined in the Project to build. The build will still fail if an entry target does not exist. /// This only applies to this build request (if another target calls the "missing target" at any other point /// this will still result in an error). /// - SkipNonexistentNonTopLevelTargets = 1 << 7, + SkipNonexistentNonEntryTargets = 1 << 7, /// /// When this flag is present, an unresolved MSBuild project SDK will fail the build. This flag is used to diff --git a/src/Build/BackEnd/Components/RequestBuilder/TargetBuilder.cs b/src/Build/BackEnd/Components/RequestBuilder/TargetBuilder.cs index 3bd6f83b6e5..33486a859be 100644 --- a/src/Build/BackEnd/Components/RequestBuilder/TargetBuilder.cs +++ b/src/Build/BackEnd/Components/RequestBuilder/TargetBuilder.cs @@ -143,21 +143,24 @@ public async Task BuildTargets(ProjectLoggingContext loggingContext foreach (string targetName in targetNames) { var targetExists = _projectInstance.Targets.TryGetValue(targetName, out ProjectTargetInstance targetInstance); - // Ignore the missing target if: - // SkipNonexistentTargets is set - // -or- - // SkipNonexistentNonTopLevelTargets and the target is is not a top level target - if (!targetExists - && entry.Request.BuildRequestDataFlags.HasFlag(BuildRequestDataFlags.SkipNonexistentTargets) - || entry.Request.BuildRequestDataFlags.HasFlag(BuildRequestDataFlags.SkipNonexistentNonTopLevelTargets) && !entry.Request.Targets.Contains(targetName)) + + if (!targetExists) { - _projectLoggingContext.LogComment(Framework.MessageImportance.Low, - "TargetSkippedWhenSkipNonexistentTargets", targetName); - } - else - { - targets.Add(new TargetSpecification(targetName, targetExists ? targetInstance.Location : _projectInstance.ProjectFileLocation)); + // Ignore the missing target if: + // SkipNonexistentTargets is set + // -or- + // SkipNonexistentNonEntryTargets and the target is is not a top level target + if (entry.Request.BuildRequestDataFlags.HasFlag(BuildRequestDataFlags.SkipNonexistentTargets) + || entry.Request.BuildRequestDataFlags.HasFlag(BuildRequestDataFlags.SkipNonexistentNonEntryTargets) && !entry.Request.Targets.Contains(targetName)) + { + _projectLoggingContext.LogComment(Framework.MessageImportance.Low, + "TargetSkippedWhenSkipNonexistentTargets", targetName); + + continue; + } } + + targets.Add(new TargetSpecification(targetName, targetExists ? targetInstance.Location : _projectInstance.ProjectFileLocation)); } // Push targets onto the stack. This method will reverse their push order so that they diff --git a/src/MSBuild.UnitTests/XMake_Tests.cs b/src/MSBuild.UnitTests/XMake_Tests.cs index 8723b61b5bb..e014f5652e3 100644 --- a/src/MSBuild.UnitTests/XMake_Tests.cs +++ b/src/MSBuild.UnitTests/XMake_Tests.cs @@ -2128,10 +2128,10 @@ public void RestoreFailsOnUnresolvedSdk() } /// - /// Verifies a non-existent target doesn't fail restore as long as its not considered "top-level" or a target that we're directly executing, in this case Restore. + /// Verifies a non-existent target doesn't fail restore as long as its not considered an entry target, in this case Restore. /// [Fact] - public void RestoreSkipsNonExistentNonTopLevelTargets() + public void RestoreSkipsNonExistentNonEntryTargets() { string restoreFirstProps = $"{Guid.NewGuid():N}.props"; @@ -2164,10 +2164,10 @@ public void RestoreSkipsNonExistentNonTopLevelTargets() } /// - /// Verifies restore will fail if the "top-level" target doesn't exist, in this case Restore. + /// Verifies restore will fail if the entry target doesn't exist, in this case Restore. /// [Fact] - public void RestoreFailsWhenTopLevelTargetIsNonExistent() + public void RestoreFailsWhenEntryTargetIsNonExistent() { string projectContents = ObjectModelHelpers.CleanupFileContents( @" @@ -2181,6 +2181,33 @@ public void RestoreFailsWhenTopLevelTargetIsNonExistent() logContents.ShouldContain("error MSB4057: The target \"Restore\" does not exist in the project."); } + /// + /// Verifies restore will run InitialTargets. + /// + [Fact] + public void RestoreRunsInitialTargets() + { + string projectContents = ObjectModelHelpers.CleanupFileContents( + @" + + + + + + + + + + + +"); + + string logContents = ExecuteMSBuildExeExpectSuccess(projectContents, arguments: "/t:restore"); + + logContents.ShouldContain("InitialTarget target ran"); + logContents.ShouldContain("Restore target ran"); + } + /// /// We check if there is only one target name specified and this logic caused a regression: https://github.com/Microsoft/msbuild/issues/3317 /// diff --git a/src/MSBuild/XMake.cs b/src/MSBuild/XMake.cs index 7301ac1c84e..792410a9964 100644 --- a/src/MSBuild/XMake.cs +++ b/src/MSBuild/XMake.cs @@ -1422,7 +1422,7 @@ private static (BuildResultCode result, Exception exception) ExecuteRestore(stri // Create a new request with a Restore target only and specify: // - BuildRequestDataFlags.ClearCachesAfterBuild to ensure the projects will be reloaded from disk for subsequent builds - // - BuildRequestDataFlags.SkipNonexistentNonTopLevelTargets to ignore missing non-top-level targets since Restore does not require that all targets + // - BuildRequestDataFlags.SkipNonexistentNonEntryTargets to ignore missing non-entry targets since Restore does not require that all targets // exist, only top-level ones like Restore itself // - BuildRequestDataFlags.IgnoreMissingEmptyAndInvalidImports to ignore imports that don't exist, are empty, or are invalid because restore might // make available an import that doesn't exist yet and the might be missing a condition. @@ -1434,7 +1434,7 @@ private static (BuildResultCode result, Exception exception) ExecuteRestore(stri toolsVersion, targetsToBuild: new[] { MSBuildConstants.RestoreTargetName }, hostServices: null, - flags: BuildRequestDataFlags.ClearCachesAfterBuild | BuildRequestDataFlags.SkipNonexistentNonTopLevelTargets | BuildRequestDataFlags.IgnoreMissingEmptyAndInvalidImports | BuildRequestDataFlags.FailOnUnresolvedSdk); + flags: BuildRequestDataFlags.ClearCachesAfterBuild | BuildRequestDataFlags.SkipNonexistentNonEntryTargets | BuildRequestDataFlags.IgnoreMissingEmptyAndInvalidImports | BuildRequestDataFlags.FailOnUnresolvedSdk); return ExecuteBuild(buildManager, restoreRequest); }