Skip Updating CopyComplete Marker When Not Necessary

[benvillalobos]

Fixes #6576

Context

#6576 revealed that the .copycomplete file marker is updated even when the Copy task in _GetCopyFilesMarkedLocal doesn't actually copy anything. This can mess with incremental builds.

Changes Made

This change adds an output parameter, CopiedAtLeastOneFile to the Copy task that the Touch task is now conditioned off of.

Testing

Tested local builds

Notes

This could also be done by having an ITaskItem[] that contains all files that were actually copied. Then the touch task could check if that item were empty. I opted for the straightforward route since the ITaskItem[] solution isn't needed yet, and this implementation can easily be changed when we do need that.

[benvillalobos]

Much like rainer suggested here, I don't think this fixes some other incremental build problems in CSWinRT, but this was certainly a bug for CopyComplete in the first place.

cc @Scottj1s

[Forgind]

I don't think this is right. First, I imagine things that don't technically move anything around but create links still should have it marked as if something were copied. More importantly, the copy step itself sometimes throws an exception if the file doesn't need to be copied, so CopiedAtLeastOneFile should be set after that.

[Forgind]

Looks good to me!

[ladipro]

This could also be done by having an ITaskItem[] that contains all files that were actually copied.

Why doesn't CopiedFiles work here?

msbuild/src/Tasks/Copy.cs

Lines 136 to 140 in 65e44d9

	/// <summary>
	/// The subset of files that were successfully copied.
	/// </summary>
	[Output]
	public ITaskItem[] CopiedFiles { get; private set; }

[ladipro]

super-nit: This line is not indented properly.

[ladipro]

Why doesn't CopiedFiles work here?

Found an answer in the issue! Wondering if the name of the new parameter shouldn't be more explicit in encoding the difference between it and non-empty CopiedFiles.

[benvillalobos]

@ladipro Any suggestions? I considered ActuallyCopiedAtLeastOneFile but thought that may have been too snarky 😄

[ladipro]

Maybe SuccessfullyCopiedAtLeastOneFile?

What exactly is the difference between non-empty CopiedFiles and this new output parameter being true? That should help come up with the best name, I believe.

[Forgind]

CopiedFiles is nonempty if either it copies files (in which case this is true) or it notices it doesn't have to copy files because it's up-to-date, in which case this would be false.

[ladipro]

Thank you, how about UpdatedAtLeastOneFile then? Plus a good comment on the prop.

[ladipro]

Thank you!

[Forgind]

ChangedFileContents? UpdatedAtLeastOneFile feels to me like it's specifically overwriting, whereas this could be making a new file where there wasn't one before.

[ladipro]

WroteAtLeastOneFile? ChangedFileContents sounds like a prop to hold "contents", i.e. it's not obvious that it's a boolean. But not feeling strongly about it.

[Forgind]

It isn't a huge deal—better would've been to change CopiedFiles, but that isn't an option. I know I've also been annoyed sometimes when there was too much haggling over names 🙂

Whatever you decide, I'm fine with @benvillalobos

[benvillalobos]

I know I've also been annoyed sometimes when there was too much haggling over names

Yeah it gets particularly haggle-some when its public and therefore shouldn't be changed once it's in. UpdatedAtLeastOneFile makes slightly less sense when creating a new file. WroteAtLeastOneFile makes sense for both creating a link and creating a new file. Going with WroteAtLeastOneFile as the final name.