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

TargetBuiltReason should have a field for InitialTargets #9467

Open
KirillOsenkov opened this issue Nov 29, 2023 · 9 comments · May be fixed by #10092
Open

TargetBuiltReason should have a field for InitialTargets #9467

KirillOsenkov opened this issue Nov 29, 2023 · 9 comments · May be fixed by #10092
Assignees
@maridematte
Labels
Area: Debuggability Issues impacting the diagnosability of builds, including logging and clearer error messages. Area: Logging backlog triaged

Comments

@KirillOsenkov
Copy link
Member

Currently we liberally use TargetBuiltReason.None
https://source.dot.net/#Microsoft.Build.Framework/TargetBuiltReason.cs,e71ba02076cc1555,references

Specifically we use None for InitialTargets, so it's hard to tell why a target was built:
image

Need to be careful about this check though:

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

Line 710 in f7f0b19

if (buildReason == TargetBuiltReason.BeforeTargets || buildReason == TargetBuiltReason.DependsOn || buildReason == TargetBuiltReason.None)

@KirillOsenkov KirillOsenkov added Area: Debuggability Issues impacting the diagnosability of builds, including logging and clearer error messages. Area: Logging labels Nov 29, 2023
@KirillOsenkov
Copy link
Member Author

I do notice that the message for the TargetStartedEventArgs includes the (entry point) string

image

So there's some indication already.

@AR-May AR-May added the triaged label Feb 7, 2024
@JanKrivanek
Copy link
Member

@maridematte pls check if this is caused by same problem or unrelated: KirillOsenkov/MSBuildStructuredLog#762

@maridematte
Copy link
Contributor

Investigation results:

We currently assign the TargetBuiltReason when pushing targets to the stack

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

Line 162 in cd64b7b

await PushTargets(targets, null, baseLookup, false, false, TargetBuiltReason.None);

During this process we define if they're being pushed because of another target and assign either BeforeTargets, DependsOn or AfterTargets. However at that step we only have access to a list of targets to be built, but not which ones are the initial targets or the entry points for the build.
The list of targets to build passed is a mix of initial targets, and the rest of the targets all joined together

msbuild/src/Build/BackEnd/Shared/BuildRequestConfiguration.cs

Line 758 in cd64b7b

var allTargets = new List<string>(initialTargets.Count + nonInitialTargets.Count);

To make the change requested here we would have to partially rewrite how targets are ordered to build, or do some pretty hacky stuff on the code and change all the tests to match, and we do not have the capacity for such a big change at the moment. Moving this to backlog for now.

@KirillOsenkov
Copy link
Member Author

yes this is definitely very low priority. I understand why it would be hard to implement given the current implementation.

Perhaps instead we should just log which targets are initial at the beginning.

@JanKrivanek
Copy link
Member

Or could we reconstruct the initial targets from the BuildRequestEntry passed to that method? (entry.RequestConfiguration.ProjectInitialTargets)

That should not require any contract change.

@JanKrivanek
Copy link
Member

Btw. is the AfterTargets BuildReason populated as well?

I'm wondering whether the KirillOsenkov/MSBuildStructuredLog#762 is an MSBuild issue or Viewer issue.

@rainersigwald
Copy link
Member

It looks like we populate it

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

Line 432 in cd64b7b

bool didPushTargets = await PushTargets(afterTargets, currentTargetEntry.ParentEntry, currentTargetEntry.Lookup, currentTargetEntry.ErrorTarget, currentTargetEntry.StopProcessingOnCompletion, TargetBuiltReason.AfterTargets);

but I wouldn't guarantee it is correct on the MSBuild side.

@KirillOsenkov
Copy link
Member Author

I took a look and MSBuild is not setting the ParentTarget for AfterTargets for that particular case:
KirillOsenkov/MSBuildStructuredLog#762 (comment)

@KirillOsenkov
Copy link
Member Author

Would be interesting to get to the bottom of this. We can use this bug or file a new one.

@maridematte maridematte linked a pull request Apr 30, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@maridematte maridematte

Labels
Area: Debuggability Issues impacting the diagnosability of builds, including logging and clearer error messages. Area: Logging backlog triaged
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add new TargetBuiltReasons maridematte/msbuild
5 participants
@KirillOsenkov @rainersigwald @JanKrivanek @maridematte @AR-May