Users/haiz/add verbosity for target circular dependence #5711

haiyuzhu · 2020-09-04T09:08:53Z

As issue #3181 mentioned, add more information for debugging target circular dependence.
For example,

    <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>

The error message will be:
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."

dnfadmin · 2020-09-04T09:09:10Z

All CLA requirements met.

dnfadmin · 2020-09-04T09:09:10Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

❌ haiyuzhu sign now
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

src/Build.UnitTests/BackEnd/TargetBuilder_Tests.cs

Forgind · 2020-09-08T21:48:57Z

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

@@ -689,7 +689,7 @@ private async Task<bool> PushTargets(IList<TargetSpecification> targets, TargetE
                        }

                        // We are already building this target on this request. That's a circular dependency.
-                        ProjectErrorUtilities.ThrowInvalidProject(targetLocation, "CircularDependency", targetSpecification.TargetName);


Would it make sense to log the parent graph of these, too? Maybe log what it currently logs at normal verbosity and log the parents at diagnostic verbosity?

I did some investigation here. I think it is hard to get the parent graph here.
Here are my reasons:

We use _requestEntry.RequestConfiguration.ActivelyBuildingTargets to store the targets are being executed. If the current target is in this dictionary, which means we can't find the circular by checking the current parentTargetEntry.

Since the target is in _requestEntry.RequestConfiguration.ActivelyBuildingTargets, this also means we have lost the previous parent information to add it to _requestEntry.RequestConfiguration.ActivelyBuildingTargets.

I'm not sure I understand the reason you gave to not use parentTargetEntry. You're saying that we already would have checked for a cycle via parentTargetEntry, so if it gets here, that way to figure out the cycle won't work? Or are you saying the parentTargetEntry is often null?

If there is no way to get the parent information, then I think this is good.

I mean "we already would have checked for a cycle via parentTargetEntry, so if it gets here, that way to figure out the cycle won't work". So I think currently we have no way to get the parent information.

Forgind · 2020-09-08T21:51:07Z

src/Build/Resources/Strings.resx

    <value>MSB4006: There is a circular dependency in the target dependency graph involving target "{0}".</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="CircularDependencyInTargetGraphWithVerbosity" xml:space="preserve">
+    <value>MSB4006: There is a circular dependency in the target dependency graph involving target "{0}". Target "{1}" has a "{2}" dependency on it, but it is depended upon by {3}.</value>


If we end up with a different error, we'd need a new error code.

My understanding is that this is the same error: Target Circular dependence. The only one reason why I add a new error message format is that the output is totally different from the old one. And I have no good idea to merge the different message output format into one. Any suggestions here?

One idea is that if we can print the dependence graph when circular dependence failer is triggered. let me do more investigation and I will get back later.

I think we don't need to add a new error code because this is a circular error of targets. I modify the key of the error message to improve the readability and merge the message format.

I'm convinced.

Forgind · 2020-09-08T21:51:52Z

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

                            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);
+                                ProjectErrorUtilities.ThrowInvalidProject(targetLocation, "CircularDependencyInTargetGraphWithVerbosity", targetSpecification.TargetName, parentTargetEntry.Name, buildReason, string.Join("<-", parentChain));


Did you account for verbosity in any way?

improve the format of circular target dependence error message.

src/Build.UnitTests/BackEnd/TargetBuilder_Tests.cs

Forgind · 2020-10-16T21:05:57Z

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

                            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);
+                                string errorMessage = $"Since \"{parentTargetEntry.Name}\" has \"{buildReason}\" dependence on \"{targetSpecification.TargetName}\", the circular is {string.Join("<-", parentChain)}.";


The reason it's better to have this sort of thing in strings (and having two separate but almost identical error messages) is that there, it gets translated into a variety of different languages. If we leave it here, everyone worldwide would see it in English.

+1, as much of the error message should exist within the resource as possible. Though I see the reason it is in its current state.

I propose we have two resources here with the same error code: ``` and CircularDependencyInTargetGraph. Same error code, different message where `CircularDependencyInTargetGraph` has most of the message listed here with many more required arguments. And `CircularDependency` remains the same.

Of course, the ThrowInvalidProject calls that pass null will no longer need to pass null, so long as they revert to the CircularDependency resource.

@Forgind , I agree with @benvillalobos 's proposal. I once did this in previous commits. Do you have any concerns?

Yeah, that's fine. I was pushing for two separate error messages in strings, but although it wasn't clear, I'm fine with them sharing an error code in this case.

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

benvillalobos · 2020-10-30T18:41:18Z

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

                            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);
+                                string errorMessage = $"Since \"{parentTargetEntry.Name}\" has \"{buildReason}\" dependence on \"{targetSpecification.TargetName}\", the circular is {string.Join("<-", parentChain)}.";


+1, as much of the error message should exist within the resource as possible. Though I see the reason it is in its current state.

I propose we have two resources here with the same error code: ``` and CircularDependencyInTargetGraph. Same error code, different message where `CircularDependencyInTargetGraph` has most of the message listed here with many more required arguments. And `CircularDependency` remains the same.

Of course, the ThrowInvalidProject calls that pass null will no longer need to pass null, so long as they revert to the CircularDependency resource.

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

Co-authored-by: Forgind <Forgind@users.noreply.github.com> Co-authored-by: Ben Villalobos <4691428+BenVillalobos@users.noreply.github.com>

…circular dependence

…pendence

Forgind

Looks good to me! Thanks for this!

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

Forgind · 2020-12-07T21:01:33Z

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

+
+            while (currentParent != null)
+            {
+                if (String.Equals(currentParent.Name, targetSpecification.TargetName, StringComparison.OrdinalIgnoreCase))


It's relevant which StringComparison you're using here, right?

Yes, correct.

Co-authored-by: Forgind <Forgind@users.noreply.github.com>

kg · 2020-12-15T11:01:17Z

Thank you very much for your contribution!

Haiyu Zhu added 2 commits September 4, 2020 14:50

add verbose info for target circular dependence

436c472

update error message

197263e

Haiyu Zhu added 2 commits September 7, 2020 11:52

update xlf

5742b01

add UT

0961dc0

haiyuzhu marked this pull request as ready for review September 7, 2020 12:29

haiyuzhu mentioned this pull request Sep 7, 2020

Circular dependency error should include more information. #3181

Open

Forgind reviewed Sep 8, 2020

View reviewed changes

Haiyu Zhu added 2 commits October 16, 2020 14:59

update error message format

eb91aa5

resolve conflicts

28d28dc

Forgind reviewed Oct 16, 2020

View reviewed changes

benvillalobos reviewed Oct 30, 2020

View reviewed changes

Forgind reviewed Oct 30, 2020

View reviewed changes

src/Build/BackEnd/Components/RequestBuilder/TargetBuilder.cs Outdated Show resolved Hide resolved

src/Build/BackEnd/Components/RequestBuilder/TargetBuilder.cs Outdated Show resolved Hide resolved

haiyuzhu and others added 6 commits November 18, 2020 13:05

Apply suggestions from code review

87302fa

Co-authored-by: Forgind <Forgind@users.noreply.github.com> Co-authored-by: Ben Villalobos <4691428+BenVillalobos@users.noreply.github.com>

Add CircularDependencyInTargetGraph with same error code to show the …

a21241d

…circular dependence

revert changes in TaskBuilder

8323608

revert changes in TaskBuilder

791ba89

Merge branch 'master' into users/haiz/AddVerbosityForTargetCircularDe…

6335f9c

…pendence

revert bad changes caused by merge conflicts

0981570

haiyuzhu requested review from benvillalobos and Forgind December 6, 2020 10:36

Forgind approved these changes Dec 7, 2020

View reviewed changes

add type to out keywords

ead5b85

Co-authored-by: Forgind <Forgind@users.noreply.github.com>

kg merged commit 743c545 into dotnet:master Dec 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Users/haiz/add verbosity for target circular dependence #5711

Users/haiz/add verbosity for target circular dependence #5711

haiyuzhu commented Sep 4, 2020 •

edited

Loading

dnfadmin commented Sep 4, 2020 •

edited

Loading

dnfadmin commented Sep 4, 2020

Forgind Sep 8, 2020

haiyuzhu Oct 10, 2020

Forgind Oct 13, 2020

haiyuzhu Oct 16, 2020

Forgind Sep 8, 2020

haiyuzhu Sep 12, 2020

haiyuzhu Sep 12, 2020

haiyuzhu Oct 16, 2020

Forgind Oct 16, 2020

Forgind Sep 8, 2020

haiyuzhu Oct 16, 2020

Forgind Oct 16, 2020

benvillalobos Oct 30, 2020 •

edited

Loading

haiyuzhu Nov 18, 2020 •

edited

Loading

Forgind Nov 18, 2020

benvillalobos Oct 30, 2020 •

edited

Loading

Forgind left a comment

Forgind Dec 7, 2020

haiyuzhu Dec 8, 2020

kg commented Dec 15, 2020

Users/haiz/add verbosity for target circular dependence #5711

Users/haiz/add verbosity for target circular dependence #5711

Conversation

haiyuzhu commented Sep 4, 2020 • edited Loading

dnfadmin commented Sep 4, 2020 • edited Loading

dnfadmin commented Sep 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benvillalobos Oct 30, 2020 • edited Loading

Choose a reason for hiding this comment

haiyuzhu Nov 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benvillalobos Oct 30, 2020 • edited Loading

Choose a reason for hiding this comment

Forgind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kg commented Dec 15, 2020

haiyuzhu commented Sep 4, 2020 •

edited

Loading

dnfadmin commented Sep 4, 2020 •

edited

Loading

benvillalobos Oct 30, 2020 •

edited

Loading

haiyuzhu Nov 18, 2020 •

edited

Loading

benvillalobos Oct 30, 2020 •

edited

Loading