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

Fix AfterTarget parent #10087

Closed
wants to merge 1 commit into from
Closed

Conversation

maridematte
Copy link
Contributor

Fixes KirillOsenkov/MSBuildStructuredLog#762

Context

On a specific edge case, where a target is an after target of the build's entry target, the parent target does not show.

Using the example from the issue
image

Target1 is the entry target for the build.
Target4 is an after target from target1.
Since target1 is the entry point, the parent of target4 is registered as null.

Changes Made

When pushing after targets to the stack, we currently register the parent of the parent. The change will register just the parent.

Notes

In the code there is a comment that explicitly states that we are pushing the parent of the parent. We are not sure of the historical reason for this, but it does not make a lot of sense. This PR is to run all the tests and just check if things don't break.

@maridematte maridematte self-assigned this Apr 29, 2024
@maridematte maridematte changed the title Changed after target parent from the parent of the parent Fix AfterTarget parent Apr 29, 2024
@JanKrivanek
Copy link
Member

The reasoning and fix looks good to me.
There are some failing tests related to the AfteTargets - let's see if we can nail those as well

@KirillOsenkov
Copy link
Member

I think we need to (unfortunately) be extremely careful here as well. ParentEntry is not only used to log the parent target, but also to indicate which parent is waiting for our target to complete, before it proceeds. It encodes a causality DAG, so if we change the ParentEntry some targets might suddenly proceed when they should wait, or other such horrors.

See the references to ParentEntry:
https://ref12.io/?rightProjectId=Microsoft.Build&leftSymbolId=d3k4tcrf1uhe&file=BackEnd%5cComponents%5cRequestBuilder%5cTargetEntry.cs&line=289&leftMode=references&rightMode=file

I'd say to be safe we should separate the concept of "which target is waiting on us to complete" vs. "which target caused us to build".

Also note that this circularity check here also relies on the parentTargetEntry:

if (HasCircularDependenceInTargets(parentTargetEntry, targetSpecification, out List<string> targetDependenceChain))
{
ProjectErrorUtilities.ThrowInvalidProject(targetLocation, "CircularDependencyInTargetGraph", targetSpecification.TargetName, parentTargetEntry.Name, buildReason, targetSpecification.TargetName, string.Join("<-", targetDependenceChain));
}

@maridematte
Copy link
Contributor Author

Closing this PR, as this fix is not viable. It changes target build behaviour and creates bugs within the error logging for targets.

@maridematte maridematte deleted the afterTargets branch August 7, 2024 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AfterTargets build reason not shown
3 participants