Some X.A targets do not respect Inputs/Outputs timestamp invariant

[garuma]

I'm debugging an incremental compilation for a project of ours and I have identified at least 3 spots where X.A targets are using CopyIfChanged or a variant-thereof and thus breaking inputs/outputs invariant.

What I mean by this is that you have plenty of scenarios where some target Inputs are newer than Outputs causing the target to run yet those updated Inputs will end up generating the same Outputs content. Because of the logic of CopyIfChanged, this mean that no actual change is committed to Outputs and thus when the target finishes executing you have broken the invariant that Outputs always have to be newer that your Inputs.

Three places where I have seen this (there might be more):

• _CopyIntermediateAssemblies (Outputs are also wrong for this one, they lack the satellites copy)
• _GeneratePackageManagerJava
• _ResolveLibraryProjectImports

[garuma]

Thinking about it more, I don’t know if there is any way you can salvage that CopyIfChanged logic, it just doesn't play well with MBuild.

Maybe what can be done is that the heavy targets output an intermediary file that’s always written to and then a second target copy that intermediate file to the real location if it has changed. That way the heavy task will respect incrementality and not be re-run all the time while the CopyIfChanged based target will be the one that runs all the time but at least it won’t cost anything

[jonathanpeppers]

@garuma if these targets are running all the time, and taking > 0ms, we should look into this.

The immediate idea that comes to mind is to use a "stamp" file:

<Target Name="_ResolveLibraryProjectImports" Inputs="..." Outputs="$(IntermediateOutputPath)libraryprojectimports.stamp">
  <!--Whatever the target does-->
  <Touch Files="$(IntermediateOutputPath)libraryprojectimports.stamp" AlwaysCreate="True" />
  <ItemGroup>
    <FileWrites Include="$(IntermediateOutputPath)libraryprojectimports.stamp" />
  </ItemGroup>
</Target

We would also need to verify the inputs are correct.

@garuma is there an easy way I can reproduce this? or can you attach binlogs?

[garuma]

@jonathanpeppers yeah making everything do stamps might be the easiest things considering it's already done throughout.

I had started writing a skeleton of unit-test showing the issue but basically the gist of it is to start touch-ing random input files (e.g. built dlls, NuGet dlls, referenced binding projects stuff...) and execute MSBuild multiple time to see targets being constantly re-run.

[jonathanpeppers]

@garuma I can reproduce this issue for _CopyIntermediateAssemblies with the following test:

[Test]
public void CopyIntermediateAssemblies ()
{
	var target = "_CopyIntermediateAssemblies";
	var proj = new XamarinFormsAndroidApplicationProject ();
	using (var b = CreateApkBuilder (Path.Combine ("temp", TestName))) {
		Assert.IsTrue (b.Build (proj), "first build should succeed");
		Assert.IsFalse (b.Output.IsTargetSkipped (target), $"`{target}` should *not* be skipped!");

		var assembly = Path.Combine (Root, b.ProjectDirectory, proj.IntermediateOutputPath, proj.ProjectName + ".dll");
		FileAssert.Exists (assembly);
		File.SetLastWriteTimeUtc (assembly, DateTime.UtcNow);
		File.SetLastAccessTimeUtc (assembly, DateTime.UtcNow);

		//NOTE: second build, target will run because inputs changed
		Assert.IsTrue (b.Build (proj, doNotCleanupOnUpdate: true, saveProject: false), "second build should succeed");
		Assert.IsFalse (b.Output.IsTargetSkipped (target), $"`{target}` should *not* be skipped on second build!");

		//NOTE: third build, it should certainly *not* run! there are no changes
		Assert.IsTrue (b.Build (proj, doNotCleanupOnUpdate: true, saveProject: false), "third build should succeed");
		Assert.IsTrue (b.Output.IsTargetSkipped (target), $"`{target}` should be skipped on third build!");
	}
}

This is going to certainly impact incremental build times... I will probably investigate each target separately and create a PR for each.

[garuma]

For reference, we also logged an enhancement in MSBuild itself to potentially surface those type of issues more easily: dotnet/msbuild#3817