Add new TargetBuiltReasons #10092
Add new TargetBuiltReasons #10092
Conversation
There was a problem hiding this comment.
Thank you for diving into this!
It looks as a good direction. 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. The current implementation seem to not cover those cases (both executions would be attributed to initial target).
Other than that - I like the solution - simple and effective
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 :-) |
Within a single project instance in a single build, the engine should never run the target again, at most emitting |
|
Sorry - yes, you are correct - I was totally wrong on this. |
|
Seems like |
|
Reminds me of this case I think: |
| /// The target was defined as an initial target of the project. | ||
| /// </summary> | ||
| InitialTarget, | ||
|
|
There was a problem hiding this comment.
maybe remove the empty line. As a rule in C# there shouldn't be two consecutive empty lines.
| TargetEntry newEntry; | ||
| if (buildReason == TargetBuiltReason.None) | ||
| { | ||
| newEntry = new TargetEntry(_requestEntry, this as ITargetBuilderCallback, targetSpecification, baseLookup, parentTargetEntry, targetSpecification._targetBuiltReason, _componentHost, stopProcessingOnCompletion); |
There was a problem hiding this comment.
Would be nice to avoid having two lines of code both creating TargetEntry. I think it'll be better if you mutate the buildReason variable and pass it as an argument, so you can avoid duplicating lines 757 and 761.
|
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? |
|
Also, be very careful here: 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. |
|
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. |
| /// <summary> | ||
| /// The target was the default target of the project | ||
| /// </summary> | ||
| DefaultTarget, |
There was a problem hiding this comment.
Pluralization should match the xml attributes. So DefaultTargets and InitialTargets like AfterTargets.
Also EntryTargets as that can be several (/t:a;b)
There was a problem hiding this comment.
so, to clarify, all three new enum fields should be plural, right?
tl;dr;: Setting the reason should be responsibility of the code that requested the build (and hence knows the reason) Basically the So it should be very easy to transfer the responsibility of deciding about the build reason from the (but we already agreed on this offline with @maridematte) |
Fixes #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
TargetSpecificationso they can be referenced in different points of the build.