msbuild icon indicating copy to clipboard operation
msbuild copied to clipboard

Fix AfterTarget parent

Open maridematte opened this issue 9 months ago • 2 comments

Fixes https://github.com/KirillOsenkov/MSBuildStructuredLog/issues/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 avatar Apr 29 '24 21:04 maridematte

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

JanKrivanek avatar Apr 30 '24 12:04 JanKrivanek

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: https://github.com/dotnet/msbuild/blob/f7f0b1924904ec7f2792c0a7805a4d0e91ac8955/src/Build/BackEnd/Components/RequestBuilder/TargetBuilder.cs#L712-L715

KirillOsenkov avatar May 04 '24 02:05 KirillOsenkov

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

maridematte avatar May 15 '24 14:05 maridematte