New diagnostic proposal: erroring on CopyToOutputDirectory="Always"

[NickCraver]

One of the common pain points I've hit is builds re-running when they don't need to and a leading cause of this is CopyToOutputDirectory="Always" set somewhere on a Compile/None/EmbeddedResource/etc.

These are already collected up in GetCopyToOutputDirectoryItems so I propose we add a diagnostic (opt-in) to let people error on this in the base SDK (Microsoft.CurrentVersion.targets). Here's my rudamentary attempt at such a Task working locally:

<Target Name="ComplainAboutCopyAlways" 
        AfterTargets="_GetCopyToOutputDirectoryItemsFromThisProject"
        BeforeTargets="GetCopyToPublishDirectoryItems">
  <CallTarget Targets="_GetCopyToOutputDirectoryItemsFromThisProject">
    <Output TaskParameter="TargetOutputs" ItemName="_ThisProjectItemsToCopyToOutputDirectory" />
  </CallTarget>
  
  <!-- Note: due to this being an error, it only errors on the first...not sure if we can make it error on many correctly... -->
  <Error Text="Item '%(_ThisProjectItemsToCopyToOutputDirectory.TargetPath)' set as CopyToOutputDirectory=&quot;Always&quot;, use CopyToOutputDirectory=&quot;PreserveNewest&quot; instead." 
          Condition="'%(_ThisProjectItemsToCopyToOutputDirectory.CopyToOutputDirectory)'=='Always'" />
</Target>

This produces build output when violations are detected:

C:\path\Directory.Build.targets(382,5): error : Item 'AlwaysCopiedFile.Txt' set as CopyToOutputDirectory="Always", use CopyToOutputDirectory="PreserveNewest" instead. [C:\path\Test.csproj]

I think this would be very useful in the base targets, opted in via some new variable (adding a condition to my above example) - thoughts on this? I'd be happy to PR it if this is amenable we get a good variable and error message (I just tried to make an initial stab). Thanks!

/cc @baronfel @rainersigwald

[rainersigwald]

I support this: CopyAlways is a footgun.

The error message actually might be a bit annoying, because it should ideally be localized. SDK has a task to emit localized errors, but core MSBuild doesn't at the moment. That might drive the implementation into the Copy task, maybe?

Also cc @drewnoakes, who recently made this less bad for IDE-driven builds on SDK-style projects in dotnet/project-system#7963.

[baronfel]

Independently of the opt-in property, we could talk about a time frame to opt this on by default in the SDK, and/or to tie this into the much-anticipated 'strict mode'. It might also be necessary to override the logic on an item by item basis (e.g. some kind of documented AllowCopyAlways="true" metadata that also gets checked here). I think this is a better alternative than marking the entire check as NoWarn.

[drewnoakes]

For the change in VS (dotnet/project-system#7963), we were motivated to avoid calling MSBuild altogether.

Given here we're already running an MSBuild build, I'd like to understand what causes the perf impact for copy-always items.

If it's that the copy touches the destination's timestamp, and that can then trigger downstream work that would otherwise be avoidable, then perhaps the approach taken in the project system would be interesting in MSBuild too.

In that PR we downgrade CopyToOutputDirectory="Always" from copy no matter what to copy if the source and destination differ in timestamp or file size.

Telemetry shows that only 0.5% of the builds previously scheduled due to a copy-always item actually needed to copy that item.

---

A legitimate use case for Always was identified by @davkean. Consider a data file that can be modified by the application during debugging, which is to be returned to some default state during the next build.

[NickCraver]

@drewnoakes The perf issue is that is triggers a re-run of targets that could be skipped - yep. If a root level project has this for example, all downstream projects will be re-built instead of targets skipped as up to date. I think CopyToOutputDirectory="IfDifferent" or some such is totally valid, but of much greater scope to implement. I agree with the use case, but also in most projects you don't have that case and opting in is a good thing to have, I think. Or, allow people to opt-out in strict...or some combo.

[drewnoakes]

Could MSBuild just treat Always as IfDifferent? This should address the perf issue without breaking the only known use case for Always I've seen.

"Fixing" this would be less friction for the user than a warning, if it's safe for us to change the behaviour. We're generally more willing to favour performance over correctness in VS than in MSBuild, hence making the change in VS first. Since shipping this change on the VS side in 17.2, we haven't heard a single complaint. We should at least discuss making the same change in MSBuild itself.

[rainersigwald]

Could MSBuild just treat Always as IfDifferent? This should address the perf issue without breaking the only known use case for Always I've seen.

We definitely can. The existing targets explicitly disable the Copy tasks's built-in support for that with a comment that I find . . . unconvincing

msbuild/src/Tasks/Microsoft.Common.CurrentVersion.targets

Lines 5125 to 5131 in f1dae6a

	<!--
	Not using SkipUnchangedFiles="true" because the application may want to change
	one of these files and not have an incremental build replace it.
	-->
	<Copy
	SourceFiles = "@(_SourceItemsToCopyToOutputDirectoryAlways)"
	DestinationFiles = "@(_SourceItemsToCopyToOutputDirectoryAlways->'$(OutDir)%(TargetPath)')"

Spelunking through ancient history, this has been there since 2004-12-23 17:43:33, in a commit entitled DCR work that introduced CopyOutOfDateSourceItemsToOutputDirectoryAlways, presumably via copy-paste from the other copy invocation (where that sorta makes sense).

Since it has been there for so long fixing it is scary, but I also can't think of a reasonable reason to preserve the existing behavior. I'm willing to fix it under a change wave.

That said, this shouldn't cause any rebuild cascades, because the copy will preserve the timestamp of the source file, so the only time cost here should be the actual copy. @NickCraver do you have an example log where you're seeing otherwise?

❯ dir S:\repro\dotnet\msbuild\issues\7654\Depended

    Directory: S:\repro\dotnet\msbuild\issues\7654\Depended

Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
d----           5/31/2022  9:50 AM                bin
d----           5/31/2022  9:50 AM                obj
-a---           5/31/2022  9:49 AM             53 Class1.cs
-a---           5/31/2022  9:51 AM            357 Depended.csproj
-a---           5/25/2022 10:31 AM              3 TextFile1.txt


❯ dir S:\repro\dotnet\msbuild\issues\7654\Depended\bin\Debug\net7.0

    Directory: S:\repro\dotnet\msbuild\issues\7654\Depended\bin\Debug\net7.0

Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
-a---           5/31/2022  9:51 AM            416 Depended.deps.json
-a---           5/31/2022  9:51 AM           3584 Depended.dll
-a---           5/31/2022  9:51 AM          10272 Depended.pdb
-a---           5/25/2022 10:31 AM              3 TextFile1.txt


❯ dir S:\repro\dotnet\msbuild\issues\7654\Depends\bin\Debug\net7.0

    Directory: S:\repro\dotnet\msbuild\issues\7654\Depends\bin\Debug\net7.0

Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
-a---           5/31/2022  9:51 AM           3584 Depended.dll
-a---           5/31/2022  9:51 AM          10272 Depended.pdb
-a---           5/31/2022  9:51 AM            691 Depends.deps.json
-a---           5/31/2022  9:51 AM           4608 Depends.dll
-a---           5/31/2022  9:51 AM         147968 Depends.exe
-a---           5/31/2022  9:51 AM          10452 Depends.pdb
-a---           5/31/2022  9:51 AM            165 Depends.runtimeconfig.json
-a---           5/25/2022 10:31 AM              3 TextFile1.txt

[KirillOsenkov]

I can't remember if we have a build check/analyzer for this, but if we don't we probably should. @JanKrivanek @YuliiaKovalova

[JanKrivanek]

We do not and we should.
Should be easy to add once we have the OM for items with metadata: #10932. So adding this to the list and bumping up. Thanks!