[Xamarin.Android.Build.Tasks] incremental build improvements

Documentation/README.md

@@ -19,6 +19,7 @@
 
   * [Build Process](guides/BuildProcess.md)
   * [`.axml` CodeBehind Support](guides/LayoutCodeBehind.md)
+  * [MSBuild Best Practices](guides/MSBuildBestPractices.md)
 
 
 # Building from Source

Documentation/guides/MSBuildBestPractices.md

@@ -0,0 +1,191 @@
+# MSBuild Best Practices
+
+This guide is a work-in-progress, but really has two main goals:
+- What are good MSBuild practices, in general?
+- What are good MSBuild practice in relation to what we already have
+  going on in Xamarin.Android MSBuild targets?
+
+## Naming
+
+MSBuild targets, properties, and item groups are prefixed with an
+underscore, unless they are considered a public-facing API. The reason
+for this convention is that MSBuild does not have its own concepts of
+visibility (public vs private). Hence everything in MSBuild is
+basically a "global variable". Prefixing with an underscore is a
+subtle hint to the consumer, "we might rename this thing!".
+
+So for example:
+
+`$(AndroidEnableProguard)` - is a public property that developer's
+use to enable proguard (Java code shrinking) in their applications.
+
+`@(ProguardConfiguration)` - is a public item group (or build action)
+developers use to add additional proguard configuration to their
+projects.
+
+`SignAndroidPackage` - is a well-known (and widely used) public
+MSBuild target used for producing an APK.
+
+If we removed any of these, this would be the same as creating
+breaking API changes. We should be careful when adding new public
+properties, targets, etc., as we would likely need to support them
+into oblivion. However, we might choose to "safely" deprecate them
+in a way that makes sense.
+
+## Incremental Builds
+
+The MSBuild Github repo has some [documentation][msbuild] on this
+subject, but there isn't a lot of content out there on how things
+should be done in MSBuild targets so they build *incrementally*:
+things aren't "rebuilding" all the time.
+
+First let's look at what a simple MSBuild target looks like that does
+*not* build incrementally. Let's assume we have a simple target that
+compiles Java code to a `jar` file:
+
+```xml
+<Target Name="_CompileJava">
+  <Javac InputFiles="@(_JavaFiles)" OutputFile="$(_JarFile)" />
+</Target>
+```
+
+Here I've made up a fictional `<Javac/>` MSBuild task that takes
+`*.java` files and runs `javac` to produce a `jar` file. I'm assuming
+an item group of `@(_JavaFiles)` exist for simplicity, and the
+`$(_JarFile)` property is set to a valid path.
+
+So if we ran this target, it would run every time: the target isn't
+setup to build incrementally *at all*. To do this properly, MSBuild
+has the concept of `Inputs` and `Outputs` of a target. If the
+timestamps of the `Outputs` are all *newer* than the `Inputs`, MSBuild
+will skip the target completely.
+
+```xml
+<Target Name="_CompileJava"
+    Inputs="@(_JavaFiles)"
+    Outputs="$(_JarFile)">
+  <Javac InputFiles="@(_JavaFiles)" OutputFile="$(_JarFile)" />
+</Target>
+```
+
+Now with this change, the `_CompileJava` target will be skipped unless
+a Java file has been changed (its timestamp altered).
+
+## Stamp Files
+
+So let's look at another example:
+
+```xml
+<Target Name="_GenerateJavaSources">
+  <GenerateJava 
+      InputFiles="@(_AssemblyFiles)"
+      OutputDirectory="$(_JavaIntermediateDirectory)"
+  />
+</Target>
+```
+
+In this case, a `GenerateJava` task will look at a set of .NET
+assemblies and generate Java source code in an output directory. We
+have no idea what files will be output from this MSBuild task *ahead
+of time*, so what are the `Outputs`? How can it possibly build
+incrementally?
+
+Here is a good case to use a "stamp" file (also called a "flag" file).
+We can create a 0-byte file on disk, which is merely used as a
+timestamp for incremental builds.
+
+```xml
+<Target Name="_GenerateJavaSources"
+    Inputs="@(_AssemblyFiles)"
+    Outputs="$(_GenerateJavaStamp)">
+  <GenerateJava 
+      InputFiles="@(_AssemblyFiles)"
+      OutputDirectory="$(_JavaIntermediateDirectory)"
+  />
+  <Touch Files="$(_GenerateJavaStamp)" AlwaysCreate="True" />
+</Target>
+```
+
+Here we assume that `$(_GenerateJavaStamp)` is a path to a file such
+as `obj\Debug\GenerateJava.stamp`. We can use the built-in `<Touch/>`
+MSBuild task to create a 0-byte file and update its timestamp. Now the
+`_GenerateJavaSources` target will only run on subsequent runs if an
+assembly file is *newer* than the stamp file.
+
+Other times it is good to use "stamp" files:
+- You have files that are not *always* updated, such as mentioned in
+  [Github #2247][github_issue]. Since you can't on a file being
+  updated, it might be desirable to run the `<Touch />` command within
+  the target or use a stamp file.
+- The outputs are *many* files. Since MSBuild has to hit the disk to
+  read timestamps of all these files, it may be a performance
+  improvement to use a stamp file. Profile build times before and
+  after to be sure.
+
+## FileWrites and IncrementalClean
+
+Generally, the place to put intermediate files during a build is
+`$(IntermediateOutputPath)`, which is by default set to
+`obj\$(Configuration)\` and always has a trailing slash.
+
+MSBuild has a target, named `IncrementalClean`, that might be the bane
+of our existance...
+
+First, I would read up on the [Clean target][clean] and understand how
+`Clean` really works within MSBuild. `Clean` in short, should delete
+any file produced by a previous build. It does *not* simply delete
+`bin` and `obj`.
+
+`IncrementalClean` has the job of deleting "extra files" that might be
+hanging out in `$(IntermediateOutputPath)`. So it might happily go
+delete your stamp file, and completely break your incremental build!
+
+The way to properly solve this problem, is to add intermediate files
+to the `FileWrites` item group. And so our previous target would
+properly be written as:
+
+```xml
+<Target Name="_GenerateJavaSources"
+    Inputs="@(_AssemblyFiles)"
+    Outputs="$(_GenerateJavaStamp)">
+  <GenerateJava 
+      InputFiles="@(_AssemblyFiles)"
+      OutputDirectory="$(_JavaIntermediateDirectory)"
+  />
+  <Touch Files="$(_GenerateJavaStamp)" AlwaysCreate="True" />
+  <ItemGroup>
+    <FileWrites Include="$(_GenerateJavaStamp)" />
+  </ItemGroup>
+</Target>
+```
+
+# Best Practices for Xamarin.Android MSBuild targets
+
+## Stamp Files
+
+From now on, we should try to put new stamp files in
+`$(IntermediateOutputPath)stamp\` using the
+`$(_AndroidStampDirectory)` property. This way we can be sure they are
+deleted on `Clean` with a single call:
+
+```xml
+<RemoveDirFixed Directories="$(_AndroidStampDirectory)" Condition="Exists ('$(_AndroidStampDirectory)')" />
+```
+
+We should also name the stamp file the same as the target, such as:
+
+```xml
+<Target Name="_ResolveLibraryProjectImports"
+    Inputs="..."
+    Outputs="$(_AndroidStampDirectory)_ResolveLibraryProjectImports.stamp">
+  <!-- ... -->
+  <Touch Files="$(_AndroidStampDirectory)_ResolveLibraryProjectImports.stamp" AlwaysCreate="True" />
+  <ItemGroup>
+    <FileWrites Include="$(_AndroidStampDirectory)_ResolveLibraryProjectImports.stamp" />
+  </ItemGroup>
+</Target>
+```
+
+[msbuild]: https://github.com/Microsoft/msbuild/blob/master/documentation/wiki/Rebuilding-when-nothing-changed.md
+[github_issue]: https://github.com/xamarin/xamarin-android/issues/2247
+[clean]: https://github.com/Microsoft/msbuild/issues/2408#issuecomment-321082997

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

@@ -319,27 +319,45 @@ public Class2 ()
 			}
 		}
 
+		//https://github.com/xamarin/xamarin-android/issues/2247
 		[Test]
-		public void CopyIntermediateAssemblies ()
+		public void Issue2247 ()
 		{
-			var target = "_CopyIntermediateAssemblies";
+			var targets = new [] {
+				"_CopyIntermediateAssemblies",
+				"_GeneratePackageManagerJava",
+				"_ResolveLibraryProjectImports",
+			};
 			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!");
+				foreach (var target in targets) {
+					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);
+				var intermediate = Path.Combine (Root, b.ProjectDirectory, proj.IntermediateOutputPath);
+				var filesToTouch = new [] {
+					Path.Combine (intermediate, "build.props"),
+					Path.Combine (intermediate, proj.ProjectName + ".dll"),
+					Path.Combine (intermediate, "android", "assets", proj.ProjectName + ".dll"),
+				};
+				foreach (var file in filesToTouch) {
+					FileAssert.Exists (file);
+					File.SetLastWriteTimeUtc (file, DateTime.UtcNow);
+					File.SetLastAccessTimeUtc (file, DateTime.UtcNow);
+				}
 
-				//NOTE: second build, target will run because inputs changed
+				//NOTE: second build, targets 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!");
+				foreach (var target in targets) {
+					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
+				//NOTE: third build, targets 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!");
+				foreach (var target in targets) {
+					Assert.IsTrue (b.Output.IsTargetSkipped (target), $"`{target}` should be skipped on third build!");
+				}
 			}
 		}
 	}

src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets

@@ -276,6 +276,7 @@ Copyright (C) 2011-2012 Xamarin. All rights reserved.
 	<_AndroidNuGetStampFile>$(IntermediateOutputPath)$(MSBuildProjectName).nuget.stamp</_AndroidNuGetStampFile>
 	<_AndroidIntermediateDesignTimeBuildDirectory>$(IntermediateOutputPath)designtime\</_AndroidIntermediateDesignTimeBuildDirectory>
 	<_AndroidLibraryFlatArchivesDirectory>$(IntermediateOutputPath)flata\</_AndroidLibraryFlatArchivesDirectory>
+	<_AndroidStampDirectory>$(IntermediateOutputPath)stamp\</_AndroidStampDirectory>
 
 	<!-- $(EnableProguard) is an obsolete property that should be removed at some stage. -->
 	<AndroidEnableProguard Condition="'$(AndroidEnableProguard)'==''">$(EnableProguard)</AndroidEnableProguard>
@@ -1192,6 +1193,7 @@ because xbuild doesn't support framework reference assemblies.
 		<_PropertyCacheItems Include="AdbOptions=$(AdbOptions)" />
 	</ItemGroup>
 	<MakeDir Directories="$(IntermediateOutputPath)" Condition="!Exists('$(IntermediateOutputPath)')" />
+	<MakeDir Directories="$(_AndroidStampDirectory)" Condition="!Exists('$(_AndroidStampDirectory)')" />
 	<WriteLinesToFile
 			File="$(_AndroidBuildPropertiesCache)"
 			Lines="@(_PropertyCacheItems)"
@@ -1304,7 +1306,7 @@ because xbuild doesn't support framework reference assemblies.
 
 <Target Name="_ResolveLibraryProjectImports"
 		Inputs="$(ProjectAssetsFile);$(MSBuildProjectFullPath);@(ReferencePath);@(ReferenceDependencyPaths);$(_AndroidBuildPropertiesCache)"
-		Outputs="$(_AndroidLibraryProjectImportsCache)">
+		Outputs="$(_AndroidStampDirectory)_ResolveLibraryProjectImports.stamp">
 	<ResolveLibraryProjectImports
 		ContinueOnError="$(DesignTimeBuild)"
 		CacheFile="$(_AndroidLibraryProjectImportsCache)"
@@ -1317,6 +1319,10 @@ because xbuild doesn't support framework reference assemblies.
 		AssemblyIdentityMapFile="$(_AndroidLibrayProjectAssemblyMapFile)"
 		OutputImportDirectory="$(_AndroidLibrayProjectIntermediatePath)">
 	</ResolveLibraryProjectImports>
+	<Touch Files="$(_AndroidStampDirectory)_ResolveLibraryProjectImports.stamp" AlwaysCreate="True" />
+	<ItemGroup>
+		<FileWrites Include="$(_AndroidStampDirectory)_ResolveLibraryProjectImports.stamp" />
+	</ItemGroup>
 </Target>
 
 <Target Name="_CollectLibraryResourceDirectories"
@@ -2285,7 +2291,7 @@ because xbuild doesn't support framework reference assemblies.
 <Target Name="_GeneratePackageManagerJava"
   DependsOnTargets="_GetAddOnPlatformLibraries;_AddStaticResources;$(_AfterAddStaticResources);_PrepareAssemblies"
   Inputs="$(MSBuildAllProjects);@(_ResolvedAssemblies);@(_ResolvedUserAssemblies);$(MSBuildProjectFile);$(_AndroidBuildPropertiesCache)"
-  Outputs="$(IntermediateOutputPath)android\src\mono\MonoPackageManager.java">
+  Outputs="$(_AndroidStampDirectory)_GeneratePackageManagerJava.stamp">
   <!-- Create java needed for Mono runtime -->
   <GeneratePackageManagerJava
     ResolvedAssemblies="@(_ResolvedAssemblies)"
@@ -2295,6 +2301,10 @@ because xbuild doesn't support framework reference assemblies.
     UseSharedRuntime="$(AndroidUseSharedRuntime)"
     TargetFrameworkVersion="$(TargetFrameworkVersion)" 
     Manifest="$(IntermediateOutputPath)android\AndroidManifest.xml" />
+  <Touch Files="$(_AndroidStampDirectory)_GeneratePackageManagerJava.stamp" AlwaysCreate="True" />
+  <ItemGroup>
+    <FileWrites Include="$(_AndroidStampDirectory)_GeneratePackageManagerJava.stamp" />
+  </ItemGroup>
 </Target>
 
 <PropertyGroup>
@@ -3089,6 +3099,7 @@ because xbuild doesn't support framework reference assemblies.
 	<RemoveDirFixed Directories="$(MonoAndroidIntermediateResourceCache)" Condition="Exists ('$(MonoAndroidIntermediateResourceCache)')" />
 	<RemoveDirFixed Directories="$(_AndroidAotBinDirectory)" Condition="Exists ('$(_AndroidAotBinDirectory)')" />
 	<RemoveDirFixed Directories="$(_AndroidLibraryFlatArchivesDirectory)" Condition="Exists ('$(_AndroidLibraryFlatArchivesDirectory)')" />
+	<RemoveDirFixed Directories="$(_AndroidStampDirectory)" Condition="Exists ('$(_AndroidStampDirectory)')" />
 	<Delete Files="$(IntermediateOutputPath)_dex_stamp" />
  	<Delete Files="$(MonoAndroidIntermediate)R.cs.flag" />
 	<Delete Files="$(MonoAndroidIntermediate)acw-map.txt" />