Merge pull request #5711 from haiyuzhu/users/haiz/AddVerbosityForTarg…

…etCircularDependence

When target dependencies form a cycle, list the names of the involved targets in the error message

src/Build.UnitTests/BackEnd/TargetBuilder_Tests.cs

@@ -1250,6 +1250,34 @@ public void TestCircularDependencyInCallTarget()
             Assert.False(success);
         }
 
+        /// <summary>
+        /// Tests a circular dependency target.
+        /// </summary>
+        [Fact]
+        public void TestCircularDependencyTarget()
+        {
+            string projectContents = @"
+<Project xmlns=""http://schemas.microsoft.com/developer/msbuild/2003"">
+    <Target Name=""TargetA"" AfterTargets=""Build"" DependsOnTargets=""TargetB"">
+        <Message Text=""TargetA""></Message>
+    </Target>
+    <Target Name=""TargetB"" DependsOnTargets=""TargetC"">
+        <Message Text=""TargetB""></Message>
+    </Target>
+    <Target Name=""TargetC"" DependsOnTargets=""TargetA"">
+        <Message Text=""TargetC""></Message>
+    </Target>
+</Project>
+      ";
+            string errorMessage = @"There is a circular dependency in the target dependency graph involving target ""TargetA"". Since ""TargetC"" has ""DependsOn"" dependence on ""TargetA"", the circular is ""TargetA<-TargetC<-TargetB<-TargetA"".";
+
+            StringReader reader = new StringReader(projectContents);
+            Project project = new Project(new XmlTextReader(reader), null, null);
+            project.Build(_mockLogger).ShouldBeFalse();
+            _mockLogger.ErrorCount.ShouldBe(1);
+            _mockLogger.Errors[0].Message.ShouldBe(errorMessage);
+        }
+
         /// <summary>
         /// Tests that cancel with no entries after building does not fail.
         /// </summary>

src/Build/BackEnd/Components/RequestBuilder/TargetBuilder.cs

@@ -697,16 +697,9 @@ private async Task<bool> PushTargets(IList<TargetSpecification> targets, TargetE
                     // Does this target exist in our direct parent chain, if it is a before target (since these can cause circular dependency issues)
                     if (buildReason == TargetBuiltReason.BeforeTargets || buildReason == TargetBuiltReason.DependsOn || buildReason == TargetBuiltReason.None)
                     {
-                        TargetEntry currentParent = parentTargetEntry;
-                        while (currentParent != null)
+                        if (HasCircularDependenceInTargets(parentTargetEntry, targetSpecification, out List<string> targetDependenceChain))
                         {
-                            if (String.Equals(currentParent.Name, targetSpecification.TargetName, StringComparison.OrdinalIgnoreCase))
-                            {
-                                // We are already building this target on this request. That's a circular dependency.
-                                ProjectErrorUtilities.ThrowInvalidProject(targetLocation, "CircularDependency", targetSpecification.TargetName);
-                            }
-
-                            currentParent = currentParent.ParentEntry;
+                            ProjectErrorUtilities.ThrowInvalidProject(targetLocation, "CircularDependencyInTargetGraph", targetSpecification.TargetName, parentTargetEntry.Name, buildReason, targetSpecification.TargetName, string.Join("<-", targetDependenceChain));
                         }
                     }
                     else
@@ -812,5 +805,37 @@ private void ComputeAfterTargetFailures(string[] targetNames)
                 }
             }
         }
+
+        private bool HasCircularDependenceInTargets(TargetEntry parentTargetEntry, TargetSpecification targetSpecification, out List<string> circularDependenceChain)
+        {
+            TargetEntry currentParent = parentTargetEntry;
+            circularDependenceChain = new List<string>();
+            bool hasCircularDependence = false;
+
+            while (currentParent != null)
+            {
+                if (String.Equals(currentParent.Name, targetSpecification.TargetName, StringComparison.OrdinalIgnoreCase))
+                {
+                    // We are already building this target on this request. That's a circular dependency.
+                    hasCircularDependence = true;
+
+                    // Cache the circular dependence chain only when circular dependency occurs.
+                    currentParent = parentTargetEntry;
+                    circularDependenceChain.Add(targetSpecification.TargetName);
+                    while (!String.Equals(currentParent.Name, targetSpecification.TargetName, StringComparison.OrdinalIgnoreCase))
+                    {
+                        circularDependenceChain.Add(currentParent.Name);
+                        currentParent = currentParent.ParentEntry;
+                    }
+
+                    circularDependenceChain.Add(currentParent.Name);
+                    break;
+                }
+
+                currentParent = currentParent.ParentEntry;
+            }
+
+            return hasCircularDependence;
+        }
     }
 }

src/Build/Resources/Strings.resx

@@ -269,6 +269,11 @@
     <comment>{StrBegin="MSB4006: "}UE: This message is shown when the build engine detects a target referenced in a circular manner -- a project cannot
     request a target to build itself (perhaps via a chain of other targets).</comment>
   </data>
+  <data name="CircularDependencyInTargetGraph" xml:space="preserve">
+    <value>MSB4006: There is a circular dependency in the target dependency graph involving target "{0}". Since "{1}" has "{2}" dependence on "{3}", the circular is "{4}".</value>
+    <comment>{StrBegin="MSB4006: "}UE: This message is shown when the build engine detects a target referenced in a circular manner -- a project cannot
+    request a target to build itself (perhaps via a chain of other targets).</comment>
+  </data>
   <data name="ComparisonOnNonNumericExpression" xml:space="preserve">
     <value>MSB4086: A numeric comparison was attempted on "{1}" that evaluates to "{2}" instead of a number, in condition "{0}".</value>
     <comment>{StrBegin="MSB4086: "}</comment>