Add new TargetBuiltReasons

[maridematte]

Fixes https://github.com/dotnet/msbuild/issues/9467

Context

When building targets we only specify a reason when it has direct control over the flow. So some targets have no built reason at the end of the build and can get very confusing on large builds.

Changes Made

Added options for TargetBuiltReason: Initial target, entry target, default target. These are added to TargetSpecification so they can be referenced in different points of the build.

[rainersigwald]

I believe we should test the case of explicit entry target (or implicit default target) and identical initial target - as then the same target should be executed twice - for 2 different reasons.

Not sure I understand this. A target must be executed only once, for exactly one reason--even if it is put in the target stack multiple times for multiple reasons. Right?

[JanKrivanek]

I believe we should test the case of explicit entry target (or implicit default target) and identical initial target - as then the same target should be executed twice - for 2 different reasons.

Not sure I understand this. A target must be executed only once, for exactly one reason--even if it is put in the target stack multiple times for multiple reasons. Right?

I believe for InitialTargets we do not deduplicate this and we document it so as well - but will double check (afk now though). In general - target can execute multiple times if not properly input/output attributed, right? Or maybe I'm just confused :-)

[rainersigwald]

In general - target can execute multiple times if not properly input/output attributed, right?

Within a single project instance in a single build, the engine should never run the target again, at most emitting Target "{0}" skipped. Previously built successfully. to the log.

[JanKrivanek]

Sorry - yes, you are correct - I was totally wrong on this. I still have some recollection that even for skipped we could set the BuiltReason. But I can be wrong again. @maridematte - please ignore my comment until I check this on my end with code at hand

[JanKrivanek]

Seems like TargetSkippedBuildEventArgs has BuiltReason as well: https://github.com/dotnet/msbuild/blob/main/src/Framework/TargetSkippedEventArgs.cs#L102 It's being populated from the TargetEntry populated here. It's probably not used in the viewer - as viewer doesn't seem to display built reason for skipped targets (@KirillOsenkov ?). But even despite that it might not be bad idea to populate the information properly - we just need to pass a tupple list instead of a string list into the BuildTargets

[KirillOsenkov]

Reminds me of this case I think:

[KirillOsenkov]

It is indeed pretty elegant and simpler than I thought it would be!

I filed a viewer issue to expose the TargetBuiltReason for skipped targets.

Would be nice to check if we can pass through this information for skipped targets.

Jan, not sure what you mean by passing a tuple instead of a string list, can you link to the source code?

[KirillOsenkov]

Also, be very careful here:

https://github.com/dotnet/msbuild/blob/f7f0b1924904ec7f2792c0a7805a4d0e91ac8955/src/Build/BackEnd/Components/RequestBuilder/TargetBuilder.cs#L710

We should reason about this check very carefully, because now instead of None it could be something else. If the intent here was to check for "anything else but AfterTargets", the condition needs to be updated for the new enum values.

Need a lot of careful scrutiny here, because we might accidentally introduce a very bad regression.

[KirillOsenkov]

PushTargets seems like a very non-trivial algorithm. I'd sleep better at night if it was covered by unit-tests. Wondering if we already have tests covering this method?

We need to trace the control flow in this method and carefully determine which branch of the if/else is (or should be) taken for each of the new enum values.

[JanKrivanek]

Jan, not sure what you mean by passing a tuple instead of a string list, can you link to the source code?

tl;dr;: Setting the reason should be responsibility of the code that requested the build (and hence knows the reason)

Basically the BuildTargets is an internal contract and called from a single place in code (not considering the unit tests):

https://github.com/dotnet/msbuild/blob/f25414599dcb07202a9f8bfd8125d04a6e5c14be/src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs#L1210C21-L1210C31

So it should be very easy to transfer the responsibility of deciding about the build reason from the BuildTargets method, which actually doesn't have the knowledge (and the proposal now is just best guessing) to the caller (above) who is requesting the targets to be build and hence has the exact knowledge of the reason. In such case the BuildTargets would accept e.g. (string, BuiltReason)[] targets instead of current string[] targets - which is easily accomodable on a caller side, removes the uncertainity on the callee side and is not breaking any contracts.

(but we already agreed on this offline with @maridematte)

[AR-May]

/azp run

[azure-pipelines[bot]]

Azure Pipelines successfully started running 1 pipeline(s).

[KirillOsenkov]

I'd say use named tuples everywhere and pick the same names for consistency, e.g. (string targetName, TargetBuiltReason builtReason)

Or maybe create a struct such as internal record struct TargetInfo(string Name, TargetBuiltReason BuiltReason) and use it everywhere instead (this way we can add more info to it in the future, such as ParentTarget, if needed).

[KirillOsenkov]

Otherwise I think it's good! Avoids changing any actual logic that might be a regression