Add TargetSkipReason and OriginalBuildEventContext to TargetSkippedEventArgs #6402
Conversation
…entArgs We weren't logging TargetSkippedEventArgs in two cases: when the target was satisfied from cache (previously built), or when the outputs were up-to-date. We were logging simple messages. Switch to logging TargetSkippedEventArgs in these cases as well. Add a new TargetSkipReason enum to indicate why the target was skipped. Store and serialize it for node packet translator and binary logger. When logging a TargetSkippedEventArgs because a target was built previously it is also useful to find the original target invocation (e.g. to see the target outputs). Add OriginalBuildEventContext to TargetResult and ensure it is translated properly. Add OriginalBuildEventContext on TargetSkippedEventArgs and serialize that as well (both node packet translator and binary logger). Note that if the target didn't build because the Condition was false, the OriginalBuildEventContext will be a project context, not a target context. We have to increment the binlog file format version to 14 to add the two new fields. Implement BuildEventContext.ToString() for easier debugging. Add an internal setter for Importance on BuildMessageEventArgs. We were missing unit-tests for node packet translation of TargetSkippedEventArgs. Add test. Fixes #5475
|
The viewer was updated to file format version 14 and to take advantage of this new information:
|
| public enum TargetSkipReason | ||
| { | ||
| /// <summary> | ||
| /// The target was not skipped or the skip reason was unknown. |
There was a problem hiding this comment.
A target that was not skipped should never issue a TargetSkippedEvent right? So I'm confused how None can represent a target that was not skipped.
There was a problem hiding this comment.
Yes in theory None should never be used. But I decided to leave a "null" value just in case... Maybe it should be called Invalid?
There was a problem hiding this comment.
What about just deleting it altogether? Or asserting that it's never None?
There was a problem hiding this comment.
Yes I'm guessing I'll just delete it. Felt like if we didn't have a "null" value we could paint ourselves in the corner, e.g. when reading 0 from a deserializer it would map to an invalid value, thus signaling to us that it's an error, but if we don't have a default value and 0 will just map the first field, then we'll never know it was bad. I'm just hypothesizing here.
There was a problem hiding this comment.
This is the scenario I'm talking about here:
For older versions we need some default value that represents "null" and I don't want to make it nullable because it'd increase bloat.
There was a problem hiding this comment.
Feels like we do need a value for None/Unknown/Invalid/Unspecified
There was a problem hiding this comment.
Yea, and always error via ErrorUtilities.VerifyThrow when a None value flows into a field via the TaskSkippedEventArgs constructor / setter? It would be nice for something to crash somewhere whenever the null symbol starts to flow around.
Forgind
added
the
merge-when-branch-open
We weren't logging TargetSkippedEventArgs in two cases: when the target was satisfied from cache (previously built), or when the outputs were up-to-date. We were logging simple messages. Switch to logging TargetSkippedEventArgs in these cases as well.
Add a new TargetSkipReason enum to indicate why the target was skipped. Store and serialize it for node packet translator and binary logger.
When logging a TargetSkippedEventArgs because a target was built previously it is also useful to find the original target invocation (e.g. to see the target outputs). Add OriginalBuildEventContext to TargetResult and ensure it is translated properly. Add OriginalBuildEventContext on TargetSkippedEventArgs and serialize that as well (both node packet translator and binary logger).
Note that if the target didn't build because the Condition was false, the OriginalBuildEventContext will be a project context, not a target context.
We have to increment the binlog file format version to 14 to add the two new fields.
Implement BuildEventContext.ToString() for easier debugging.
Add an internal setter for Importance on BuildMessageEventArgs.
We were missing unit-tests for node packet translation of TargetSkippedEventArgs. Add test.
Fixes #5475