Fix issue where targets are executed after an error. #2133
Conversation
There was a problem hiding this comment.
I'm not saying this is wrong, but I'm not sure it's right yet.
What's the timeline of events in the problem case? Specifically, when is the ActionCode of the skipped-by-condition target considered? (I'm a bit concerned that this doesn't update it in time in all cases.)
| taskBuilder.FailTaskNumber = 1; | ||
|
|
||
| IConfigCache cache = (IConfigCache)_host.GetComponent(BuildComponentType.ConfigCache); | ||
| BuildRequestEntry entry = new BuildRequestEntry(CreateNewBuildRequest(1, new[] { "Build", "ProduceError1", "_Error1" }), cache[1]); |
There was a problem hiding this comment.
Do you need to explicitly request all three?
| @@ -748,7 +748,20 @@ internal void EnterLegacyCallTargetScope(Lookup lookup) | |||
| /// </summary> | |||
| internal void MarkForError() | |||
| { | |||
| ErrorUtilities.VerifyThrow(_state != TargetEntryState.Completed, "State must not be Completed. State is {0}.", _state); | |||
| // In the case that the Target was skipped due to a condition but a dependent target failed subsequent execution | |||
| // must stop. Detect that case and set the WorkUnitActionCode to stop and mark the TargetEntry for ErrorExecution. | |||
There was a problem hiding this comment.
So:
- We're in this method because a dependent target failed
- But this target is already marked completed/skipped/continue
And therefore we want to update the current status to completed/skipped/stop?
| if (topEntry.ParentEntry != null && topEntry.ParentEntry.State != TargetEntryState.Completed) | ||
| { | ||
| topEntry.ParentEntry.MarkForError(); | ||
| } | ||
| else if (topEntry.ParentEntry != null && topEntry.ParentEntry.Result?.ResultCode == TargetResultCode.Skipped) |
There was a problem hiding this comment.
topEntry.ParentEntry?.Result?.ResultCode == TargetResultCode.Skipped| internal void MarkForStop() | ||
| { | ||
| ErrorUtilities.VerifyThrow(_state == TargetEntryState.Completed, "State must be Completed. State is {0}.", _state); | ||
| ErrorUtilities.VerifyThrow(_targetResult.ResultCode == TargetResultCode.Skipped, "State must be Completed. State is {0}.", _state); |
There was a problem hiding this comment.
copypasta: state/resultcode
This issue occurs when two targets are scheduled to be built (e.g. ProduceError1 and ProduceError2) and the condition of the first target prevents it from executing and a dependent target (BeforeTargets='ProduceError1') fails. This change prevents ProduceError2 from executing by marking the parent of the failure (ProduceError1) with the Stop WorkUnitActionCode. Fixes dotnet#2116 Update
AndyGerlicher
force-pushed
the
toomanyerrors
branch
from
c6375d3 to
9132c10
Compare
|
@dotnet-bot test OSX Build for CoreCLR please |
|
@AndyGerlicher Thanks a bunch for fixing this! I've verified that it fixes a lot of the issues with extra errors in the .NET SDK. How will this flow into a release? Does it need to go into a 15.3 branch in order to insert it into the CLI and VS? |
|
@dsplaisted This went to |
First attempt, looking for feedback. It works and fixes the error, but it scares me a bit!
This issue occurs when two targets are scheduled to be built (e.g.
ProduceError1 and ProduceError2) and the condition of the first target
causes it to not execute and a dependent target
(BeforeTargets='ProduceError1') fails. In this example, ProduceError2
still executes.
Fixes #2116