Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 13 commits into from
Dec 15, 2020
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
436c472
add verbose info for target circular dependence
Sep 4, 2020
197263e
update error message
Sep 4, 2020
5742b01
update xlf
Sep 7, 2020
0961dc0
add UT
Sep 7, 2020
eb91aa5
update error message format
Oct 16, 2020
28d28dc
resolve conflicts
Oct 16, 2020
87302fa
Apply suggestions from code review
haiyuzhu Nov 18, 2020
a21241d
Add CircularDependencyInTargetGraph with same error code to show the …
haiyuzhu Dec 5, 2020
8323608
revert changes in TaskBuilder
haiyuzhu Dec 6, 2020
791ba89
revert changes in TaskBuilder
haiyuzhu Dec 6, 2020
6335f9c
Merge branch 'master' into users/haiz/AddVerbosityForTargetCircularDe…
haiyuzhu Dec 6, 2020
0981570
revert bad changes caused by merge conflicts
haiyuzhu Dec 6, 2020
ead5b85
add type to out keywords
haiyuzhu Dec 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions src/Build.UnitTests/BackEnd/TargetBuilder_Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1254,6 +1254,31 @@ 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>
";
StringReader reader = new StringReader(projectContents);
Project project = new Project(new XmlTextReader(reader), null, null);
bool success = project.Build(_mockLogger);
Assert.False(success);
haiyuzhu marked this conversation as resolved.
Show resolved Hide resolved
}

/// <summary>
/// Tests that cancel with no entries after building does not fail.
/// </summary>
Expand Down
8 changes: 5 additions & 3 deletions src/Build/BackEnd/Components/RequestBuilder/TargetBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -677,7 +677,7 @@ private async Task<bool> PushTargets(IList<TargetSpecification> targets, TargetE
// continue so we could throw the exception.
if (_requestEntry.RequestConfiguration.ActivelyBuildingTargets.ContainsKey(targetSpecification.TargetName))
{
ProjectErrorUtilities.ThrowInvalidProject(targetLocation, "CircularDependency", targetSpecification.TargetName);
ProjectErrorUtilities.ThrowInvalidProject(targetLocation, "CircularDependencyInTargetGraph", targetSpecification.TargetName);
}
}
else
Expand All @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. 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.
  2. 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ProjectErrorUtilities.ThrowInvalidProject(targetLocation, "CircularDependencyInTargetGraph", targetSpecification.TargetName);
}
}
else
Expand All @@ -698,12 +698,14 @@ private async Task<bool> PushTargets(IList<TargetSpecification> targets, TargetE
if (buildReason == TargetBuiltReason.BeforeTargets || buildReason == TargetBuiltReason.DependsOn || buildReason == TargetBuiltReason.None)
{
TargetEntry currentParent = parentTargetEntry;
List<string> parentChain = new List<string>();
while (currentParent != null)
{
parentChain.Add(currentParent.Name);
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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you account for verbosity in any way?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

improve the format of circular target dependence error message.

}

currentParent = currentParent.ParentEntry;
Expand Down
2 changes: 1 addition & 1 deletion src/Build/BackEnd/Components/RequestBuilder/TaskBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -866,7 +866,7 @@ private async Task<WorkUnitResult> ExecuteInstantiatedTask(ITaskExecutionHost ta
else if (type == typeof(CircularDependencyException))
{
_continueOnError = ContinueOnError.ErrorAndStop;
ProjectErrorUtilities.ThrowInvalidProject(taskLoggingContext.Task.Location, "CircularDependency", taskLoggingContext.TargetLoggingContext.Target.Name);
ProjectErrorUtilities.ThrowInvalidProject(taskLoggingContext.Task.Location, "CircularDependencyInTargetGraph", taskLoggingContext.TargetLoggingContext.Target.Name);
haiyuzhu marked this conversation as resolved.
Show resolved Hide resolved
}
else if (type == typeof(InvalidProjectFileException))
{
Expand Down
7 changes: 6 additions & 1 deletion src/Build/Resources/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,11 +264,16 @@
<comment>{StrBegin="MSB4114: "}UE: This message appears if the project file contains unreasonably nested Choose elements.
LOCALIZATION: Do not localize "Choose" as it is an XML element name.</comment>
</data>
<data name="CircularDependency" xml:space="preserve">
<data name="CircularDependencyInTargetGraph" xml:space="preserve">
<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>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm convinced.

<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>
Expand Down
18 changes: 12 additions & 6 deletions src/Build/Resources/xlf/Strings.cs.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

18 changes: 12 additions & 6 deletions src/Build/Resources/xlf/Strings.de.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

18 changes: 12 additions & 6 deletions src/Build/Resources/xlf/Strings.en.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

18 changes: 12 additions & 6 deletions src/Build/Resources/xlf/Strings.es.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

18 changes: 12 additions & 6 deletions src/Build/Resources/xlf/Strings.fr.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading