[Xamarin.Android.Build.Tasks] refactor symbol files targets (#2394)

~~ _ConvertPdbFiles, _CopyPdbFiles, and _CopyMdbFiles ~~

These targets were building incrementally, but not optimally:

  - `<CopyMdbFiles/>` (duplicate of `<CopyIfChanged/>`) was copying
    the symbol files.
  - The `Outputs` of these targets were all being touched, after the
    `<CopyIfChanged/>` call, even if they didn't change.
  - These targets contained vestigial support for XBuild and was
    writing directly to `$(IntermediateOutputPath)$(CleanFile)`
    instead of adding to `@(FileWrites)`.  The file would continue to
    grow with duplicates on incremental builds.
  - Several `<ItemGroup/>` transforms were duplicated throughout
    the targets.

Changes:

  - Removed `<CopyMdbFiles/>` and use `<CopyIfChanged/>` instead.
  - Use "stamp" files as per our convention as `Outputs`.
  - Declare new `<ItemGroup/>` values where necessary, instead of
    using wildcards or repetitive transforms.
  - Verified that `@(FileWrites)` was correct.

~~ _CopyIntermediateAssemblies ~~

It appears the `Inputs` should actually be `@(ResolvedAssemblies)`,
since that is the `<ItemGroup/>` used throughout this target.

Other refactoring:

  - Fixed duplicate item transforms, by using private `<ItemGroup/>`
  - Added files to `@(FileWrites)`

~~ _PrepareAssemblies ~~

Since removing the direct writing to `$(CleanFile)` (01be8ac), I
discovered most of the targets run during `_PrepareAssemblies` were
not properly adding to `@(FileWrites)` due to the ordering of
MSBuild targets.

I added `BeforeTargets="_CleanGetCurrentAndPriorFileWrites"`, but set
it up to optionally happen only if `$(AndroidApplication)`=True.

This made any targets that run during `_PrepareAssemblies`
appropriately add to `@(FileWrites)`.

~~ _LinkAssemblies and _LinkAssembliesNoShrink ~~

Both targets need to add files to `@(FileWrites)`.

Additionally, `_LinkAssembliesNoShrink` has another problem:
`_LinkAssembliesNoShrink` should be using
`$(MonoAndroidIntermediateAssemblyDir)` for
`@(_LinkAssembliesNoShrinkFiles)`, which are any files in
`$(IntermediateOutputPath)android\assets`.

~~ _CleanGetCurrentAndPriorFileWrites ~~

I've found the only way to make sure an MSBuild target runs *before*
`IncrementalClean` is to add:

	BeforeTargets="_CleanGetCurrentAndPriorFileWrites"

I adjusted the `_AddFilesToFileWrites` target to run before the
`_CleanGetCurrentAndPriorFileWrites` target. I documented my
findings, see details here:
<dotnet/msbuild#3916>

~~ Tests ~~

`AppProjectTargetsDoNotBreak` - added checks for these targets:
`_ConvertPdbFiles`, `_CopyPdbFiles`, and `_CopyMdbFiles`.

Documentation/guides/MSBuildBestPractices.md

@@ -32,6 +32,80 @@ 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.
 
+## Item Group Transforms
+
+MSBuild has a widely used feature where you can create a one-to-one
+mapping from an `<ItemGroup/>` to a new `<ItemGroup/>`. The syntax for
+this looks like:
+
+```xml
+<ItemGroup>
+  <_DestinationFiles Include="@(_SourceFiles->'$(SomeDirectory)%(Filename)%(Extension)')" />
+</ItemGroup>
+```
+
+This takes a list of files, and creates a desired destination path for
+each file in `$(SomeDirectory)`. The `%(Filename)` and `%(Extension)`
+item metadata is used to get the filename of the source file. See the
+MSBuild documentation on [transforms][msbuild-transforms] and
+[well-known item metadata][msbuild-metadata] for more info.
+
+One thing to note here, is we shouldn't have multiple transforms
+within the same target:
+
+```xml
+<Target Name="_CopyPdbFiles"
+    Inputs="@(_ResolvedPortablePdbFiles)"
+    Outputs="$(_AndroidStampDirectory)_CopyPdbFiles.stamp"
+    DependsOnTargets="_ConvertPdbFiles">
+  <CopyIfChanged
+      SourceFiles="@(_ResolvedPortablePdbFiles)"
+      DestinationFiles="@(_ResolvedPortablePdbFiles->'$(MonoAndroidLinkerInputDir)%(Filename)%(Extension)')"
+  />
+  <Touch Files="$(_AndroidStampDirectory)_CopyPdbFiles.stamp" AlwaysCreate="True" />
+  <ItemGroup>
+    <FileWrites Include="@(_ResolvedPortablePdbFiles->'$(MonoAndroidLinkerInputDir)%(Filename)%(Extension)')" />
+  </ItemGroup>
+</Target>
+```
+
+Running this transformation twice:
+
+```
+@(_ResolvedPortablePdbFiles->'$(MonoAndroidLinkerInputDir)%(Filename)%(Extension)')
+```
+
+Would be like generating the same `string[]` twice in C#, in the same method.
+
+The target could be better written as:
+
+```xml
+<Target Name="_CopyPdbFiles"
+    Inputs="@(_ResolvedPortablePdbFiles)"
+    Outputs="$(_AndroidStampDirectory)_CopyPdbFiles.stamp"
+    DependsOnTargets="_ConvertPdbFiles">
+  <ItemGroup>
+    <_CopyPdbFilesDestinationFiles Include="@(_ResolvedPortablePdbFiles->'$(MonoAndroidLinkerInputDir)%(Filename)%(Extension)')" />
+  </ItemGroup>
+  <CopyIfChanged SourceFiles="@(_ResolvedPortablePdbFiles)" DestinationFiles="@(_CopyPdbFilesDestinationFiles)" />
+  <Touch Files="$(_AndroidStampDirectory)_CopyPdbFiles.stamp" AlwaysCreate="True" />
+  <ItemGroup>
+    <FileWrites Include="@(_CopyPdbFilesDestinationFiles)" />
+  </ItemGroup>
+</Target>
+```
+
+Additionally, if the new `@(_CopyPdbFilesDestinationFiles)` is not
+meant to be used from outside the target, it should be prefixed with
+an underscore and have a name specific to the target. The
+`@(_CopyPdbFilesDestinationFiles)` name is a reasonable length, but an
+abbreviation could be used if the name is quite long, such as:
+`@(_CPFDestinationFiles)`. `_CPF` denotes a private value within the
+`_CopyPdbFiles` target.
+
+[msbuild-transforms]: https://docs.microsoft.com/en-us/visualstudio/msbuild/msbuild-transforms
+[msbuild-metadata]: https://docs.microsoft.com/en-us/visualstudio/msbuild/msbuild-well-known-item-metadata
+
 ## Incremental Builds
 
 The MSBuild Github repo has some [documentation][msbuild] on this
@@ -240,6 +314,63 @@ We should also name the stamp file the same as the target, such as:
 Do we need `FileWrites` here? Nope. The `_AddFilesToFileWrites`
 target takes care of it, so we can't as easily mess it up:
 
+```xml
+<Target Name="_AddFilesToFileWrites">
+  <ItemGroup>
+    <FileWrites Include="$(_AndroidStampDirectory)*.stamp" />
+  </ItemGroup>
+</Target>
+```
+
+## Legacy Code and XBuild
+
+From time to time, we might find oddities in our MSBuild targets, that
+might be around for one reason or another:
+
+  - We might be doing something weird in order to support XBuild. We
+    support XBuild no longer, yay!
+  - The code just might have been around a while, and there wasn't a
+    reason to change it.
+  - There is a nuance to MSBuild we hadn't figured out yet (lol?).
+
+Take, for instance, the following example:
+
+```xml
+<WriteLinesToFile
+    File="$(IntermediateOutputPath)$(CleanFile)"
+    Lines="@(_ConvertedDebuggingFiles)"
+    Overwrite="false"
+/>
+```
+
+The intent here is to replicate what happens with the `@(FileWrites)`
+item group, by directly writing to this file. This
+`<WriteLinesToFile/>` call likely "works" in some fashion, but is not
+quite correct.
+
+A couple problems with this approach with MSBuild:
+
+  - This task won't run if the target is skipped!
+  - How do we know MSBuild isn't going to overwrite this file?
+  - On a subsequent build, this could append to the file *again*.
+
+Really, who knows what weirdness could be caused by this?
+
+For MSBuild, we should instead do:
+
+```xml
+<ItemGroup>
+  <FileWrites Include="@(_ConvertedDebuggingFiles)" />
+</ItemGroup>
+```
+
+Then we just let MSBuild and `IncrementalClean` do their thing.
+
+## IncrementalClean and `_CleanGetCurrentAndPriorFileWrites`
+
+If you have a target that needs to run before `IncrementalClean`, such
+as:
+
 ```xml
 <Target Name="_AddFilesToFileWrites" BeforeTargets="IncrementalClean">
   <ItemGroup>
@@ -248,6 +379,26 @@ target takes care of it, so we can't as easily mess it up:
 </Target>
 ```
 
+Unfortunately, due to the ordering of MSBuild's core targets. These
+files won't get added to the `FileWrites` list appropriately!
+`IncrementalClean` depends on a `_CleanGetCurrentAndPriorFileWrites`
+target which does the actual work of persisting the contents of
+`FileWrites`. The above target runs after
+`_CleanGetCurrentAndPriorFileWrites`.
+
+The only working fix I've found so far is to add:
+
+```
+BeforeTargets="_CleanGetCurrentAndPriorFileWrites"
+```
+
+In the meantime, see the following links about this problem:
+
+  * [MSBuild Github Issue #3916][msbuild_issue]
+  * [MSBuild Repro][msbuild_repro]
+
 [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
+[clean]: https://github.com/Microsoft/msbuild/issues/2408#issuecomment-321082997
+[msbuild_issue]: https://github.com/Microsoft/msbuild/issues/3916
+[msbuild_repro]: https://github.com/jonathanpeppers/MSBuildIncrementalClean

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

@@ -9,8 +9,6 @@ namespace Xamarin.Android.Tasks
 {
 	public class CollectPdbFiles : Task
 	{
-		const uint ppdb_signature = 0x424a5342;
-
 		[Required]
 		public ITaskItem[] ResolvedAssemblies { get; set; }
 
@@ -20,11 +18,8 @@ public class CollectPdbFiles : Task
 		[Output]
 		public ITaskItem[] PortablePdbFiles { get; set; }
 
-		public override bool Execute () {
-
-			Log.LogDebugMessage ("CollectPdbFiles CollectPdbFiles");
-			Log.LogDebugTaskItems ("  ResolvedAssemblies:", ResolvedAssemblies);
-
+		public override bool Execute ()
+		{
 			var pdbFiles = new List<ITaskItem> ();
 			var portablePdbFiles = new List<ITaskItem> ();
 

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

@@ -394,7 +394,7 @@ public void CheckTimestamps ([Values (true, false)] bool isRelease)
 		[Test]
 		public void BuildApplicationAndClean ([Values (false, true)] bool isRelease)
 		{
-			var proj = new XamarinAndroidApplicationProject () {
+			var proj = new XamarinFormsAndroidApplicationProject {
 				IsRelease = isRelease,
 			};
 			using (var b = CreateApkBuilder (Path.Combine ("temp", TestContext.CurrentContext.Test.Name))) {

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

@@ -320,13 +320,15 @@ public Class2 ()
 		[Test]
 		public void AppProjectTargetsDoNotBreak ()
 		{
-			var targets = new [] {
+			var targets = new List<string> {
 				"_CopyIntermediateAssemblies",
 				"_GeneratePackageManagerJava",
 				"_ResolveLibraryProjectImports",
 				"_BuildAdditionalResourcesCache",
 				"_CleanIntermediateIfNuGetsChange",
 				"_CopyConfigFiles",
+				"_CopyPdbFiles",
+				"_CopyMdbFiles",
 			};
 			var proj = new XamarinFormsAndroidApplicationProject {
 				OtherBuildItems = {
@@ -338,6 +340,11 @@ public void AppProjectTargetsDoNotBreak ()
 					}
 				}
 			};
+			if (IsWindows) {
+				//NOTE: pdb2mdb will run on Windows on the current project's symbols if DebugType=Full
+				proj.SetProperty (proj.DebugProperties, "DebugType", "Full");
+				targets.Add ("_ConvertPdbFiles");
+			}
 			using (var b = CreateApkBuilder (Path.Combine ("temp", TestName))) {
 				Assert.IsTrue (b.Build (proj), "first build should succeed");
 				foreach (var target in targets) {
@@ -350,6 +357,7 @@ public void AppProjectTargetsDoNotBreak ()
 					Path.Combine (intermediate, "..", "project.assets.json"),
 					Path.Combine (intermediate, "build.props"),
 					Path.Combine (intermediate, $"{proj.ProjectName}.dll"),
+					Path.Combine (intermediate, $"{proj.ProjectName}.pdb"),
 					Path.Combine (intermediate, "android", "assets", $"{proj.ProjectName}.dll"),
 					Path.Combine (output, $"{proj.ProjectName}.dll.config"),
 				};

src/Xamarin.Android.Build.Tasks/Xamarin.Android.Build.Tasks.csproj

@@ -118,7 +118,6 @@
     <Compile Include="Tasks\CheckForInvalidResourceFileNames.cs" />
     <Compile Include="Tasks\CollectNonEmptyDirectories.cs" />
     <Compile Include="Tasks\CollectPdbFiles.cs" />
-    <Compile Include="Tasks\CopyMdbFiles.cs" />
     <Compile Include="Tasks\Generator.cs" />
     <Compile Include="Tasks\JarToXml.cs" />
     <Compile Include="Tasks\GetJavaPlatformJar.cs" />