Put fail-restore behavior under 16.10 changewave

Add the behavior improvements from dotnet#6312 to the 16.10 changewave since they cause failures in cases that didn't fail before.
rainersigwald · Apr 26, 2021 · ebc4a8b · ebc4a8b
1 parent f29a6fd
commit ebc4a8b
Show file tree

Hide file tree

Showing 3 changed files with 64 additions and 8 deletions.
diff --git a/documentation/wiki/ChangeWaves.md b/documentation/wiki/ChangeWaves.md
@@ -27,6 +27,7 @@ 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/MSBuild.UnitTests/XMake_Tests.cs b/src/MSBuild.UnitTests/XMake_Tests.cs
@@ -19,6 +19,7 @@
 using Shouldly;
 using System.IO.Compression;
 using System.Reflection;
+using Microsoft.Build.Utilities;
 
 namespace Microsoft.Build.UnitTests
 {
@@ -2127,6 +2128,31 @@ public void RestoreFailsOnUnresolvedSdk()
             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>
@@ -2181,6 +2207,26 @@ public void RestoreFailsWhenEntryTargetIsNonExistent()
             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>

diff --git a/src/MSBuild/XMake.cs b/src/MSBuild/XMake.cs
@@ -1421,14 +1421,23 @@ 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 =
-                  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.
+            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;
+            }
 
             BuildRequestData restoreRequest = new BuildRequestData(
                 projectFile,