From 2d8014ec5b7a5aeb46804bac0e4615f72728ca34 Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Fri, 7 May 2021 14:42:31 -0700 Subject: [PATCH] Revert failure on missing SDK Reverts #6312 and #6372. SHAs reverted: 29dc5e1da5e0e7b70e49c1b53f3e1e4f5acbec6c and da900e2a6d7109e973f9eb712b4afdb456610c9b respectively. --- documentation/wiki/ChangeWaves.md | 1 - .../BackEnd/BuildManager/BuildManager.cs | 5 - .../BackEnd/BuildManager/BuildRequestData.cs | 15 -- .../RequestBuilder/TargetBuilder.cs | 23 +-- .../Shared/BuildRequestConfiguration.cs | 5 - src/Build/Definition/ProjectLoadSettings.cs | 5 - src/Build/Evaluation/Evaluator.cs | 3 +- src/MSBuild.UnitTests/XMake_Tests.cs | 170 +----------------- src/MSBuild/XMake.cs | 24 +-- 9 files changed, 17 insertions(+), 234 deletions(-) diff --git a/documentation/wiki/ChangeWaves.md b/documentation/wiki/ChangeWaves.md index b5ec4e3736b..1e25829f60b 100644 --- a/documentation/wiki/ChangeWaves.md +++ b/documentation/wiki/ChangeWaves.md @@ -27,7 +27,6 @@ The opt-out comes in the form of setting the environment variable `MSBuildDisabl - [Error when a property expansion in a condition has whitespace](https://github.com/dotnet/msbuild/pull/5672) - [Allow Custom CopyToOutputDirectory Location With TargetPath](https://github.com/dotnet/msbuild/pull/6237) - [Allow users that have certain special characters in their username to build successfully when using exec](https://github.com/dotnet/msbuild/pull/6223) -- [Fail restore operations when there is no `Restore` target or an SDK is unresolveable](https://github.com/dotnet/msbuild/pull/6312) ### 17.0 ## Change Waves No Longer In Rotation diff --git a/src/Build/BackEnd/BuildManager/BuildManager.cs b/src/Build/BackEnd/BuildManager/BuildManager.cs index ef032ea7791..2e950017adb 100644 --- a/src/Build/BackEnd/BuildManager/BuildManager.cs +++ b/src/Build/BackEnd/BuildManager/BuildManager.cs @@ -1724,11 +1724,6 @@ 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 673ee5f0fdf..4123d7e0922 100644 --- a/src/Build/BackEnd/BuildManager/BuildRequestData.cs +++ b/src/Build/BackEnd/BuildManager/BuildRequestData.cs @@ -75,21 +75,6 @@ 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 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). - /// - SkipNonexistentNonEntryTargets = 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 617fea73b47..21bdf35cb01 100644 --- a/src/Build/BackEnd/Components/RequestBuilder/TargetBuilder.cs +++ b/src/Build/BackEnd/Components/RequestBuilder/TargetBuilder.cs @@ -143,24 +143,15 @@ public async Task BuildTargets(ProjectLoggingContext loggingContext foreach (string targetName in targetNames) { var targetExists = _projectInstance.Targets.TryGetValue(targetName, out ProjectTargetInstance targetInstance); - - if (!targetExists) + if (!targetExists && entry.Request.BuildRequestDataFlags.HasFlag(BuildRequestDataFlags.SkipNonexistentTargets)) { - // 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; - } + _projectLoggingContext.LogComment(Framework.MessageImportance.Low, + "TargetSkippedWhenSkipNonexistentTargets", targetName); + } + else + { + targets.Add(new TargetSpecification(targetName, targetExists ? targetInstance.Location : _projectInstance.ProjectFileLocation)); } - - 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/Build/BackEnd/Shared/BuildRequestConfiguration.cs b/src/Build/BackEnd/Shared/BuildRequestConfiguration.cs index 659066306cc..97f9531e074 100644 --- a/src/Build/BackEnd/Shared/BuildRequestConfiguration.cs +++ b/src/Build/BackEnd/Shared/BuildRequestConfiguration.cs @@ -466,11 +466,6 @@ internal void LoadProjectIntoConfiguration( projectLoadSettings |= ProjectLoadSettings.IgnoreMissingImports | ProjectLoadSettings.IgnoreInvalidImports | ProjectLoadSettings.IgnoreEmptyImports; } - if (buildRequestDataFlags.HasFlag(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 ef51122eca5..7f1c9ad2d80 100644 --- a/src/Build/Definition/ProjectLoadSettings.cs +++ b/src/Build/Definition/ProjectLoadSettings.cs @@ -59,10 +59,5 @@ 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 c675c1108e8..f8453160b18 100644 --- a/src/Build/Evaluation/Evaluator.cs +++ b/src/Build/Evaluation/Evaluator.cs @@ -1785,8 +1785,7 @@ static string EvaluateProperty(string value, IElementLocation location, if (!sdkResult.Success) { - // Ignore the missing import if IgnoreMissingImports is set unless FailOnUnresolvedSdk is also set - if (_loadSettings.HasFlag(ProjectLoadSettings.IgnoreMissingImports) && !_loadSettings.HasFlag(ProjectLoadSettings.FailOnUnresolvedSdk)) + if (_loadSettings.HasFlag(ProjectLoadSettings.IgnoreMissingImports)) { ProjectImportedEventArgs eventArgs = new ProjectImportedEventArgs( importElement.Location.Line, diff --git a/src/MSBuild.UnitTests/XMake_Tests.cs b/src/MSBuild.UnitTests/XMake_Tests.cs index 1513d20e3f9..b3c65a114a6 100644 --- a/src/MSBuild.UnitTests/XMake_Tests.cs +++ b/src/MSBuild.UnitTests/XMake_Tests.cs @@ -19,7 +19,6 @@ using Shouldly; using System.IO.Compression; using System.Reflection; -using Microsoft.Build.Utilities; namespace Microsoft.Build.UnitTests { @@ -2109,151 +2108,6 @@ 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."); - } - - /// - /// When specifying /t:restore under an old changewave, do not 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 RestorePassesOnUnresolvedSdkUnderChangewave() - { - string projectContents = ObjectModelHelpers.CleanupFileContents( -$@" - - - - -"); - - using TestEnvironment env = Microsoft.Build.UnitTests.TestEnvironment.Create(); - - string logContents = ExecuteMSBuildExeExpectSuccess(projectContents, - envsToCreate: new Dictionary() { ["MSBUILDDISABLEFEATURESFROMVERSION"]=ChangeWaves.Wave16_10.ToString() }, - arguments: " /t:restore"); - - logContents.ShouldNotContain("MSB4236"); - } - - - /// - /// 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 RestoreSkipsNonExistentNonEntryTargets() - { - 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 entry target doesn't exist, in this case Restore. - /// - [Fact] - public void RestoreFailsWhenEntryTargetIsNonExistent() - { - string projectContents = ObjectModelHelpers.CleanupFileContents( -@" - - - -"); - - string logContents = ExecuteMSBuildExeExpectFailure(projectContents, arguments: "/t:restore"); - - logContents.ShouldContain("error MSB4057: The target \"Restore\" does not exist in the project."); - } - - /// - /// Verifies restore will not fail if the entry target doesn't exist, when changewave applied. - /// - [Fact] - public void RestorePassesWhenEntryTargetIsNonExistentUnderChangewave() - { - string projectContents = ObjectModelHelpers.CleanupFileContents( -@" - - - -"); - - string logContents = ExecuteMSBuildExeExpectSuccess(projectContents, - envsToCreate: new Dictionary() { ["MSBUILDDISABLEFEATURESFROMVERSION"] = ChangeWaves.Wave16_10.ToString() }, - arguments: "/t:restore"); - - logContents.ShouldNotContain("MSB4057"); - } - - /// - /// 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 /// @@ -2458,24 +2312,6 @@ 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()) { @@ -2500,8 +2336,10 @@ private string ExecuteMSBuildExeExpectFailure(string projectContents, IDictionar bool success; string output = RunnerUtilities.ExecMSBuild($"\"{testProject.ProjectFile}\" {String.Join(" ", arguments)}", out success, _output); - - return (success, output); + + success.ShouldBeTrue(() => output); + + return output; } } } diff --git a/src/MSBuild/XMake.cs b/src/MSBuild/XMake.cs index b86c58acce1..525f7df79d2 100644 --- a/src/MSBuild/XMake.cs +++ b/src/MSBuild/XMake.cs @@ -1440,31 +1440,17 @@ private static (BuildResultCode result, Exception exception) ExecuteRestore(stri restoreGlobalProperties["MSBuildRestoreSessionId"] = Guid.NewGuid().ToString("D"); // Create a new request with a Restore target only and specify: - BuildRequestDataFlags flags; - - if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave16_10)) - { - flags = BuildRequestDataFlags.ClearCachesAfterBuild // ensure the projects will be reloaded from disk for subsequent builds - | BuildRequestDataFlags.SkipNonexistentNonEntryTargets // ignore missing non-entry targets since Restore does not require that all targets - // exist, only top-level ones like Restore itself - | BuildRequestDataFlags.IgnoreMissingEmptyAndInvalidImports // 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; // still fail in the case when an MSBuild project SDK can't be resolved since this is fatal and should - // fail the build. - } - else - { - // pre-16.10 flags allowed `-restore` to pass when there was no `Restore` target - flags = BuildRequestDataFlags.ClearCachesAfterBuild | BuildRequestDataFlags.SkipNonexistentTargets | BuildRequestDataFlags.IgnoreMissingEmptyAndInvalidImports; - } - + // - 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.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. BuildRequestData restoreRequest = new BuildRequestData( projectFile, restoreGlobalProperties, toolsVersion, targetsToBuild: new[] { MSBuildConstants.RestoreTargetName }, hostServices: null, - flags); + flags: BuildRequestDataFlags.ClearCachesAfterBuild | BuildRequestDataFlags.SkipNonexistentTargets | BuildRequestDataFlags.IgnoreMissingEmptyAndInvalidImports); return ExecuteBuild(buildManager, restoreRequest); }