Revert failure on missing SDK

Reverts dotnet#6312 and dotnet#6372. SHAs reverted: 29dc5e1 and da900e2 respectively.
Forgind · May 10, 2021 · 91f89ed · 91f89ed
1 parent c040391
commit 91f89ed
Show file tree

Hide file tree

Showing 9 changed files with 17 additions and 234 deletions.
diff --git 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
@@ -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
@@ -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.
         /// </summary>
         IgnoreMissingEmptyAndInvalidImports = 1 << 6,
-
-        /// <summary>
-        /// 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).
-        /// </summary>
-        SkipNonexistentNonEntryTargets = 1 << 7,
-
-        /// <summary>
-        /// When this flag is present, an unresolved MSBuild project SDK will fail the build.  This flag is used to
-        /// change the <see cref="IgnoreMissingEmptyAndInvalidImports" /> behavior to still fail when an SDK is missing
-        /// because those are more fatal.
-        /// </summary>
-        FailOnUnresolvedSdk = 1 << 8,
     }
 
     /// <summary>

diff --git a/src/Build/BackEnd/Components/RequestBuilder/TargetBuilder.cs b/src/Build/BackEnd/Components/RequestBuilder/TargetBuilder.cs
@@ -143,24 +143,15 @@ public async Task<BuildResult> 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
@@ -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
@@ -59,10 +59,5 @@ public enum ProjectLoadSettings
         /// Whether to profile the evaluation
         /// </summary>
         ProfileEvaluation = 128,
-
-        /// <summary>
-        /// Used in combination with <see cref="IgnoreMissingImports" /> to still treat an unresolved MSBuild project SDK as an error.
-        /// </summary>
-        FailOnUnresolvedSdk = 256,
     }
 }
diff --git 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
@@ -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);
         }
 
-        /// <summary>
-        /// 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.
-        /// </summary>
-        [Fact]
-        public void RestoreFailsOnUnresolvedSdk()
-        {
-            string projectContents = ObjectModelHelpers.CleanupFileContents(
-$@"<Project>
-  <Sdk Name=""UnresolvedSdk"" />
-  <Target Name=""Restore"">
-    <Message Text=""Restore target ran"" />
-  </Target>
-</Project>");
-
-            string logContents = ExecuteMSBuildExeExpectFailure(projectContents, arguments: "/t:restore");
-
-            logContents.ShouldContain("error MSB4236: The SDK 'UnresolvedSdk' specified could not be found.");
-        }
-
-        /// <summary>
-        /// 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.
-        /// </summary>
-        [Fact]
-        public void RestorePassesOnUnresolvedSdkUnderChangewave()
-        {
-            string projectContents = ObjectModelHelpers.CleanupFileContents(
-$@"<Project>
-  <Sdk Name=""UnresolvedSdk"" />
-  <Target Name=""Restore"">
-    <Message Text=""Restore target ran"" />
-  </Target>
-</Project>");
-
-            using TestEnvironment env = Microsoft.Build.UnitTests.TestEnvironment.Create();
-
-            string logContents = ExecuteMSBuildExeExpectSuccess(projectContents,
-                envsToCreate: new Dictionary<string, string>() { ["MSBUILDDISABLEFEATURESFROMVERSION"]=ChangeWaves.Wave16_10.ToString() },
-                arguments: " /t:restore");
-
-            logContents.ShouldNotContain("MSB4236");
-        }
-
-
-        /// <summary>
-        /// Verifies a non-existent target doesn't fail restore as long as its not considered an entry target, in this case Restore.
-        /// </summary>
-        [Fact]
-        public void RestoreSkipsNonExistentNonEntryTargets()
-        {
-            string restoreFirstProps = $"{Guid.NewGuid():N}.props";
-
-            string projectContents = ObjectModelHelpers.CleanupFileContents(
-$@"<Project DefaultTargets=""Build"" InitialTargets=""TargetThatComesFromRestore"">
-  <PropertyGroup>
-    <RestoreFirstProps>{restoreFirstProps}</RestoreFirstProps>
-  </PropertyGroup>
-  
-  <Import Project=""$(RestoreFirstProps)"" />
-  <Target Name=""Restore"">
-    <Message Text=""Restore target ran"" />
-    <ItemGroup>
-      <Lines Include=""&lt;Project&gt;&lt;Target Name=&quot;TargetThatComesFromRestore&quot;&gt;&lt;Message Text=&quot;Initial target ran&quot; /&gt;&lt;/Target&gt;&lt;/Project&gt;"" />
-    </ItemGroup>
-    
-    <WriteLinesToFile File=""$(RestoreFirstProps)"" Lines=""@(Lines)"" Overwrite=""true"" />
-  </Target>
-
-  <Target Name=""Build"">
-    <Message Text=""Build target ran&quot;"" />
-  </Target>
-</Project>");
-
-            string logContents = ExecuteMSBuildExeExpectSuccess(projectContents, arguments: "/restore");
-
-            logContents.ShouldContain("Restore target ran");
-            logContents.ShouldContain("Build target ran");
-            logContents.ShouldContain("Initial target ran");
-        }
-
-        /// <summary>
-        /// Verifies restore will fail if the entry target doesn't exist, in this case Restore.
-        /// </summary>
-        [Fact]
-        public void RestoreFailsWhenEntryTargetIsNonExistent()
-        {
-            string projectContents = ObjectModelHelpers.CleanupFileContents(
-@"<Project DefaultTargets=""Build"">
-  <Target Name=""Build"">
-    <Message Text=""Build target ran&quot;"" />
-  </Target>
-</Project>");
-
-            string logContents = ExecuteMSBuildExeExpectFailure(projectContents, arguments: "/t:restore");
-
-            logContents.ShouldContain("error MSB4057: The target \"Restore\" does not exist in the project.");
-        }
-
-        /// <summary>
-        /// Verifies restore will not fail if the entry target doesn't exist, when changewave applied.
-        /// </summary>
-        [Fact]
-        public void RestorePassesWhenEntryTargetIsNonExistentUnderChangewave()
-        {
-            string projectContents = ObjectModelHelpers.CleanupFileContents(
-@"<Project DefaultTargets=""Build"">
-  <Target Name=""Build"">
-    <Message Text=""Build target ran&quot;"" />
-  </Target>
-</Project>");
-
-            string logContents = ExecuteMSBuildExeExpectSuccess(projectContents,
-                envsToCreate: new Dictionary<string, string>() { ["MSBUILDDISABLEFEATURESFROMVERSION"] = ChangeWaves.Wave16_10.ToString() },
-                arguments: "/t:restore");
-
-            logContents.ShouldNotContain("MSB4057");
-        }
-
-        /// <summary>
-        /// Verifies restore will run InitialTargets.
-        /// </summary>
-        [Fact]
-        public void RestoreRunsInitialTargets()
-        {
-            string projectContents = ObjectModelHelpers.CleanupFileContents(
-                @"<Project DefaultTargets=""Build"" InitialTargets=""InitialTarget"">
-  <Target Name=""InitialTarget"">
-    <Message Text=""InitialTarget target ran&quot;"" />
-  </Target>
-
-  <Target Name=""Restore"">
-    <Message Text=""Restore target ran&quot;"" />
-  </Target>
-
-  <Target Name=""Build"">
-    <Message Text=""Build target ran&quot;"" />
-  </Target>
-</Project>");
-
-            string logContents = ExecuteMSBuildExeExpectSuccess(projectContents, arguments: "/t:restore");
-
-            logContents.ShouldContain("InitialTarget target ran");
-            logContents.ShouldContain("Restore target ran");
-        }
-
         /// <summary>
         /// We check if there is only one target name specified and this logic caused a regression: https://github.com/Microsoft/msbuild/issues/3317
         /// </summary>
@@ -2458,24 +2312,6 @@ private string CopyMSBuild()
         }
 
         private string ExecuteMSBuildExeExpectSuccess(string projectContents, IDictionary<string, string> filesToCreate = null,  IDictionary<string, string> 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<string, string> filesToCreate = null, IDictionary<string, string> 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<string, string> filesToCreate = null, IDictionary<string, string> 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
@@ -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 <Import /> 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 <Import /> 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);
         }