Remove inputs which are not files from CsWinRTGenerateProjection target.

[Jeremy-Price]

The msbuild documentation states that the Inputs property of a target must be a list of files. However, $(CsWinRTExcludes);$(CsWinRTIncludes);$(CsWinRTExcludesPrivate);$(CsWinRTIncludesPrivate);$(CsWinRTFilters);$(CsWinRTWindowsMetadata) are not files, so the target does not run incrementally as the non-files are always absent.

I think it's just safe to just remove these, if they are changed then a project file or targets file will have changed, which should trigger the correct incremental build behavior.

[manodasanW]

Going to trigger a build and see what happens in local incremental scenarios.

[jevansaks]

I don't have permission to leave a review but I approve this change. 😁

[jlaanstra]

I don't think we should remove the inputs, but probably need to specify differently how they are consumed as inputs, for example by hashing them and writing to a file and then use the file as input. This is how CoreGenerateAssemblyInfo in the .NET SDK does this.

[Jeremy-Price]

@jlaanstra is it not sufficient that the inputs ultimately came from a csproj/props/targets? Or can they flow in from computed results too?

[jevansaks]

If you change those properties, you must have changed your csproj and those will cause a rebuild of your project... right?

[jlaanstra]

If you change those properties, you must have changed your csproj and those will cause a rebuild of your project... right?

Nothing prevents these properties to be defined dynamically without requiring changes to csproj/props/targets.

[jlaanstra]

@Jeremy-Price do you want to take a stab at the CoreGenerateAssemblyInfo approach? Like this https://github.com/dotnet/sdk/blob/ba45654d71b1da7cc139285b312e2f152febc008/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.GenerateAssemblyInfo.targets#L171

[Jeremy-Price]

Sure, I'll take a look at it. I am not an msbuild expert though so I may need some help if I get lost.

[Jeremy-Price]

Speaking of not being an msbuild expert--is there no way to insist that a property value comes from a source file? Or detect if it doesn't? Or put the values into a real file if they aren't backed by one already, and use that as an input? If anything can change like this, it seems that every build target is potentially in trouble if it doesn't do something similar.

[manodasanW]

One other thought I had that is similar to the hashing approach is, we instead generate the cswinrt.rsp in a separate target from where we call cswinrt.exe. And the target which calls cswinrt.exe has the rsp file as an input so it only runs if that changes. Not sure if the hashing approach is better than this or is equivalent.

[jevansaks]

This approach is pretty uncommon. Most targets don't do this. Who is dynamically changing these inputs that the ROI of doing this work is worth it?

[Jeremy-Price]

Is it possible for us to just document (or enforce?) that these properties must only be provided in csproj/props/targets files?

Would changing these dynamically even work with all the other parts of the build process?

[jlaanstra]

One other thought I had that is similar to the hashing approach is, we instead generate the cswinrt.rsp in a separate target from where we call cswinrt.exe. And the target which calls cswinrt.exe has the rsp file as an input so it only runs if that changes. Not sure if the hashing approach is better than this or is equivalent.

We currently use cswinrt.rsp as the output marker so we'd need to find a different output marker.

[jlaanstra]

Totally missed the use of MSBuildAllProjects. That would indeed probably make this work reasonably well.

[nagya]

I can see why someone might include "dynamic" stuff in the generated AssemblyInfo.cs (time of build, git commit hash, etc), making "perfect dependency tracking" a necessity in CoreGenerateAssemblyInfo. However, going for perfect dependency tracking is the exception, not the norm in MSBuild targets. For example, if I define CheckForOverflowUnderflow based on the time of day, that'll break my incremental builds.

A good parallel for the dependency of CsWinRTGenerateProjection on the value of CsWinRTExcludes, would be the dependency of CoreCompile on the value of CheckForOverflowUnderflow. It's also an imperfect dependency (it doesn't work if I define CheckForOverflowUnderflow based on the time of day, or override it on the command line), but it's considered good enough, that's ensured by mainly two things:

• CoreCompile depends on MSBuildAllProjects
• the output of CoreCompile goes to a directory that includes Configuration, just in case CheckForOverflowUnderflow depends on that (i.e. varies in Debug/Release)

So I think as long as CsWinRTGeneratedFilesDir includes Configuration, dependency tracking here is as good as in CoreCompile.

[manodasanW]

Did some basic testing of a couple incremental scenarios. Things seem to be working as expected and MSBuildAllProjects is indeed being detected as changed when you change like a csproj to adjust the namespaces to project.