[Xamarin.Android.Build.Tasks] <CopyIfChanged/> uses last write time (d…

…otnet#1968)

Fixes: dotnet#1962

Since 0adf1ae, I believe that resource changes on macOS have been
flaky: sometimes working, sometimes not. They have been consistently
working on Windows, however...

After writing the simplest test I could, I found that my machine on
High Sierra could *sometimes* reproduce what our QA team was seeing.
I had to add the `[Repeat]` NUnit attribute before I could get the
failure to occur regularly.

After debugging for quite some time, I noticed a mistake in the
`<CopyIfChanged/>` task: when comparing timestamps, it looks like it
is using the last *access* time of the destination file instead of
the last *write* time!

I think this is likely a typo (unintended), since switching the call
to `File.GetLastWriteTimeUtc()` fixes the problem.  Keeping the test
is a good idea, because it seems a bit scary we didn't catch this yet.

With that discovery, we audited the rest of the codebase and found
that we were using `File.GetLastAccessTimeUtc()` in several places.
We have deemed these *all* to be accidental, and have replaced all
occurrences of `File.GetLastAccessTimeUtc()` with
`File.GetLastWriteTimeUtc()`.

src/Xamarin.Android.Build.Tasks/Tasks/CopyAndConvertResources.cs

@@ -48,7 +48,7 @@ public override bool Execute ()
 					var ext = Path.GetExtension (filename);
 					var destfilename = DestinationFiles [i].ItemSpec;
 					var srcmodifiedDate = File.GetLastWriteTimeUtc (filename);
-					var dstmodifiedDate = File.Exists (destfilename) ? File.GetLastAccessTimeUtc (destfilename) : DateTime.MinValue;
+					var dstmodifiedDate = File.Exists (destfilename) ? File.GetLastWriteTimeUtc (destfilename) : DateTime.MinValue;
 					var isXml = ext == ".xml" || ext == ".axml";
 
 					Directory.CreateDirectory (Path.GetDirectoryName (destfilename));
@@ -85,7 +85,7 @@ public override bool Execute ()
 				string filename = p.Key;
 				var destfilename = p.Value;
 				var srcmodifiedDate = File.GetLastWriteTimeUtc (filename);
-				var dstmodifiedDate = File.Exists (destfilename) ? File.GetLastAccessTimeUtc (destfilename) : DateTime.MinValue;
+				var dstmodifiedDate = File.Exists (destfilename) ? File.GetLastWriteTimeUtc (destfilename) : DateTime.MinValue;
 				var tmpdest = Path.GetTempFileName ();
 				var res = Path.Combine (Path.GetDirectoryName (filename), "..");
 				MonoAndroidHelper.CopyIfChanged (filename, tmpdest);

src/Xamarin.Android.Build.Tasks/Tasks/CopyIfChanged.cs

@@ -44,7 +44,7 @@ public override bool Execute ()
 					continue;
 				var dest = DestinationFiles [i].ItemSpec;
 				var srcmodifiedDate = File.GetLastWriteTimeUtc (src);
-				var dstmodifiedDate = File.Exists (dest) ? File.GetLastAccessTimeUtc (dest) : srcmodifiedDate;
+				var dstmodifiedDate = File.Exists (dest) ? File.GetLastWriteTimeUtc (dest) : srcmodifiedDate;
 				if (dstmodifiedDate > srcmodifiedDate) {
 					Log.LogDebugMessage ($"  Skipping {src} its up to date");
 					continue;

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/AndroidUpdateResourcesTest.cs

@@ -1353,5 +1353,24 @@ public void CheckInvalidXmlInManagedResourceParser ()
 				StringAssertEx.Contains ("warning XA1000", builder.LastBuildOutput, "Build output should contain a XA1000 warning.");
 			}
 		}
+
+		//NOTE: This test was failing randomly before fixing a bug in `CopyIfChanged`.
+		//      Let's set it to run 3 times, it still completes in a reasonable time ~1.5 min.
+		[Test, Repeat(3)]
+		public void LightlyModifyLayout ()
+		{
+			var proj = new XamarinAndroidApplicationProject ();
+			using (var b = CreateApkBuilder (Path.Combine ("temp", TestName))) {
+				Assert.IsTrue (b.Build (proj), "first build should have succeeded");
+
+				//Just change something, doesn't matter
+				var layout = Path.Combine (Root, b.ProjectDirectory, "Resources", "layout", "Main.axml");
+				FileAssert.Exists (layout);
+				File.AppendAllText (layout, " ");
+
+				Assert.IsTrue (b.Build (proj), "second build should have succeeded");
+				Assert.IsFalse (b.Output.IsTargetSkipped ("_UpdateAndroidResgen"), "`_UpdateAndroidResgen` should not be skipped!");
+			}
+		}
 	}
 }

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs

@@ -905,14 +905,14 @@ public void BuildBasicApplicationCheckMdbAndPortablePdb ()
 				Assert.IsTrue (
 					b.Output.IsTargetSkipped ("_CopyMdbFiles"),
 					"the _CopyMdbFiles target should be skipped");
-				var lastTime = File.GetLastAccessTimeUtc (pdbToMdbPath);
+				var lastTime = File.GetLastWriteTimeUtc (pdbToMdbPath);
 				pdb.Timestamp = DateTime.UtcNow;
 				Assert.IsTrue (b.Build (proj, doNotCleanupOnUpdate: true), "third build failed");
 				Assert.IsFalse (
 					b.Output.IsTargetSkipped ("_CopyMdbFiles"),
 					"the _CopyMdbFiles target should not be skipped");
 				Assert.Less (lastTime,
-					File.GetLastAccessTimeUtc (pdbToMdbPath),
+					File.GetLastWriteTimeUtc (pdbToMdbPath),
 					"{0} should have been updated", pdbToMdbPath);
 			}
 		}
@@ -2275,14 +2275,14 @@ public void BuildBasicApplicationCheckPdb ()
 					b.Output.IsTargetSkipped ("_CopyMdbFiles"),
 					"the _CopyMdbFiles target should be skipped");
 				b.BuildLogFile = "build2.log";
-				var lastTime = File.GetLastAccessTimeUtc (pdbToMdbPath);
+				var lastTime = File.GetLastWriteTimeUtc (pdbToMdbPath);
 				pdb.Timestamp = DateTime.UtcNow;
 				Assert.IsTrue (b.Build (proj, doNotCleanupOnUpdate: true), "third build failed");
 				Assert.IsFalse (
 					b.Output.IsTargetSkipped ("_CopyMdbFiles"),
 					"the _CopyMdbFiles target should not be skipped");
 				Assert.Less (lastTime,
-					File.GetLastAccessTimeUtc (pdbToMdbPath),
+					File.GetLastWriteTimeUtc (pdbToMdbPath),
 					"{0} should have been updated", pdbToMdbPath);
 			}
 		}