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.

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

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

Merged
29 changes: 29 additions & 0 deletions src/Build.UnitTests/BackEnd/TargetBuilder_Tests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1254,6 +1254,35 @@ 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);
bool success = project.Build(_mockLogger);
Assert.False(success);
haiyuzhu marked this conversation as resolved.
Show resolved Hide resolved
Assert.Equal<int>(1, _mockLogger.ErrorCount);
Assert.Equal(errorMessage, _mockLogger.Errors[0].Message);
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
9 changes: 6 additions & 3 deletions src/Build/BackEnd/Components/RequestBuilder/TargetBuilder.cs
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, null);
haiyuzhu marked this conversation as resolved.
Show resolved Hide resolved
}
}
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, null);
}
}
else
Expand All @@ -698,12 +698,15 @@ 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>() { targetSpecification.TargetName };
haiyuzhu marked this conversation as resolved.
Show resolved Hide resolved
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);
string errorMessage = $"Since \"{parentTargetEntry.Name}\" has \"{buildReason}\" dependence on \"{targetSpecification.TargetName}\", the circular is {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.

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.

Copy link
Member

@benvillalobos benvillalobos Oct 30, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@haiyuzhu haiyuzhu Nov 18, 2020

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

ProjectErrorUtilities.ThrowInvalidProject(targetLocation, "CircularDependencyInTargetGraph", targetSpecification.TargetName, errorMessage);
}

currentParent = currentParent.ParentEntry;
Expand Down
2 changes: 1 addition & 1 deletion src/Build/BackEnd/Components/RequestBuilder/TaskBuilder.cs
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
58 changes: 29 additions & 29 deletions src/Build/Resources/Strings.resx
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
<?xml version="1.0" encoding="utf-8"?>
<root>
<!--
Microsoft ResX Schema
<!--
Microsoft ResX Schema

Version 2.0
The primary goals of this format is to allow a simple XML format
that is mostly human readable. The generation and parsing of the
various data types are done through the TypeConverter classes

The primary goals of this format is to allow a simple XML format
that is mostly human readable. The generation and parsing of the
various data types are done through the TypeConverter classes
associated with the data types.

Example:

... ado.net/XML headers & schema ...
<resheader name="resmimetype">text/microsoft-resx</resheader>
<resheader name="version">2.0</resheader>
Expand All @@ -26,36 +26,36 @@
<value>[base64 mime encoded string representing a byte array form of the .NET Framework object]</value>
<comment>This is a comment</comment>
</data>
There are any number of "resheader" rows that contain simple

There are any number of "resheader" rows that contain simple
name/value pairs.
Each data row contains a name, and value. The row also contains a
type or mimetype. Type corresponds to a .NET class that support
text/value conversion through the TypeConverter architecture.
Classes that don't support this are serialized and stored with the

Each data row contains a name, and value. The row also contains a
type or mimetype. Type corresponds to a .NET class that support
text/value conversion through the TypeConverter architecture.
Classes that don't support this are serialized and stored with the
mimetype set.
The mimetype is used for serialized objects, and tells the
ResXResourceReader how to depersist the object. This is currently not

The mimetype is used for serialized objects, and tells the
ResXResourceReader how to depersist the object. This is currently not
extensible. For a given mimetype the value must be set accordingly:
Note - application/x-microsoft.net.object.binary.base64 is the format
that the ResXResourceWriter will generate, however the reader can

Note - application/x-microsoft.net.object.binary.base64 is the format
that the ResXResourceWriter will generate, however the reader can
read any of the formats listed below.

mimetype: application/x-microsoft.net.object.binary.base64
value : The object must be serialized with
value : The object must be serialized with
: System.Runtime.Serialization.Formatters.Binary.BinaryFormatter
: and then encoded with base64 encoding.

mimetype: application/x-microsoft.net.object.soap.base64
value : The object must be serialized with
value : The object must be serialized with
: System.Runtime.Serialization.Formatters.Soap.SoapFormatter
: and then encoded with base64 encoding.

mimetype: application/x-microsoft.net.object.bytearray.base64
value : The object must be serialized into a byte array
value : The object must be serialized into a byte array
: using a System.ComponentModel.TypeConverter
: and then encoded with base64 encoding.
-->
Expand Down Expand Up @@ -264,8 +264,8 @@
<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">
<value>MSB4006: There is a circular dependency in the target dependency graph involving target "{0}".</value>
<data name="CircularDependencyInTargetGraph" xml:space="preserve">
<value>MSB4006: There is a circular dependency in the target dependency graph involving target "{0}". {1}</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>
Expand Down
12 changes: 6 additions & 6 deletions src/Build/Resources/xlf/Strings.cs.xlf

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

12 changes: 6 additions & 6 deletions src/Build/Resources/xlf/Strings.de.xlf

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

12 changes: 6 additions & 6 deletions src/Build/Resources/xlf/Strings.en.xlf

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

12 changes: 6 additions & 6 deletions src/Build/Resources/xlf/Strings.es.xlf

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

12 changes: 6 additions & 6 deletions src/Build/Resources/xlf/Strings.fr.xlf

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

Loading