-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
Add CopyUpToDate ETW #6661
Add CopyUpToDate ETW #6661
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the right place for these events, to catch the "incremental build copying the same outputs to the same locations" case? I naïvely assumed it would be in DoCopyIfNecessary
.
The part I put ETW traces around looks like "the file at destination was copied from source." DoCopyIfNecessary has "files are the same size and timestamp" and "files have the same name." There's a fourth check buried in DoCopyWithRetries that does a deep file comparison. Looking at those four, the first sounded sufficient for if it's just an incremental build, and I stopped there because there deeper I try to push it, the more complicated it gets. This is a simple check with no code movement. To add the next level, I'd need three possible ending places, one of which is currently also executed when actually copying, so I would need a little extra control logic there. The last check is in a catch. I think it would be impossible to hit without capturing copies as well without substantial code changes. I can check to see if my assumption about the first being sufficient is true or just put in the change to capture the first two levels. I don't think it's worth it to try for the third. |
Sounds like a plan. |
src/Tasks/Copy.cs
Outdated
@@ -561,6 +568,10 @@ int parallelism | |||
success = false; | |||
} | |||
} | |||
else | |||
{ | |||
MSBuildEventSource.Log.CopyUpToDateStop(destItem.ItemSpec); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specify whether it was up to date or not
Progress towards #6658
Adds an up-to-date ETW for the Copy task. Also captures metadata copying just because that happens if it was up-to-date. (Should we have a check rather than always copying?)