[Xamarin.Android.Build.Tasks] ignore mdbs and non-portable pdbs

Fixes: #2230

We currently spend some time during builds converting symbol files if
`DebugType=Full` or `DebugType=PdbOnly` is used.

These are older symbol formats, and we have already done the work to
use `DebugType=Portable` everywhere in the Xamarin templates.

My thought is to just remove support for these symbols by default
unless `$(AndroidLegacySymbols)` is set to `True`. This way if a
problem comes up, developers can opt-in to the old behavior.

Eventually we may phase out support completely, but it may not matter
to leave it in.

I updated various tests here to make sure we continue to test both
`Full` and `Portable` in `DebuggingTest.cs`.

~~ XA0122 ~~

I added a new warning, for cases where a PCL `<ProjectReference/>`
have `DebugType=Full` set. This seems like the only likely cause a
user could hit an issue. I added a test for this scenario.

Other changes:

* I found the `_CollectMdbFiles` MSBuild target is completely unused.
  It should have been removed in 2ea31e6, but we can remove it now.
* I found a place `$(AllowedReferenceRelatedFileExtensions)` was
  adding duplicate values. We should have removed this in e390702.

Documentation/guides/BuildProcess.md

@@ -498,6 +498,14 @@ when packaging Release applications.
 
     Added in Xamarin.Android 6.1.
 
+-   **AndroidLegacySymbols** – If set to `True`, opts back into
+    supporting `DebugType=Full` and `DebugType=PdbOnly`. These are
+    Windows-specific symbol formats and `DebugType=Portable` is
+    generally preferred. See the [msbuild documentation][msbuild_common]
+    for details.
+
+    [msbuild_common]: https://docs.microsoft.com/en-us/visualstudio/msbuild/common-msbuild-project-properties
+
 -   **AndroidLinkMode** – Specifies which type of
     [linking](~/android/deploy-test/linker.md) should be
     performed on assemblies contained within the Android package. Only

Documentation/guides/messages/xa0122.md

@@ -0,0 +1,50 @@
+---
+title: Xamarin.Android warning XA0122
+description: XA0122 warning code
+ms.date: 11/01/2019
+---
+# Xamarin.Android warning XA0122
+
+## Issue
+
+In Xamarin.Android 10.2, Windows-specific symbol formats were
+deprecated. These are specified by the `$(DebugType)` MSBuild property
+in `.csproj` files, such as:
+
+* `<DebugType>full</DebugType>`
+* `<DebugType>pdbonly</DebugType>`
+
+Xamarin.Android's build system has to convert these symbols to a
+format supported by the Mono runtime. This hurts build performance.
+
+Since these symbol formats are now deprecated, debugging and
+breakpoints will not work for any referenced projects using
+`DebugType=full` or `DebugType=pdbonly`.
+
+See the [msbuild documentation][msbuild_common] for details about
+`$(DebugType)`.
+
+[msbuild_common]: https://docs.microsoft.com/en-us/visualstudio/msbuild/common-msbuild-project-properties
+
+## Solution
+
+Any projects referenced by a Xamarin.Android project should use:
+
+```xml
+<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' ">
+  <!-- ... -->
+  <DebugType>portable</DebugType>
+</PropertyGroup>
+```
+
+Alternatively, you could set `$(AndroidLegacySymbols)`:
+
+```xml
+<PropertyGroup>
+  <!-- ... -->
+  <AndroidLegacySymbols>True</AndroidLegacySymbols>
+</PropertyGroup>
+```
+
+This will restore the legacy symbol behavior, at the cost of longer
+build times.

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

@@ -20,6 +20,8 @@ public class CollectPdbFiles : AndroidTask
 		[Output]
 		public ITaskItem[] PortablePdbFiles { get; set; }
 
+		public bool LegacySymbols { get; set; }
+
 		public override bool RunTask ()
 		{
 			var pdbFiles = new List<ITaskItem> ();
@@ -35,6 +37,14 @@ public override bool RunTask ()
 					portablePdbFiles.Add (file);
 				} else {
 					pdbFiles.Add (file);
+					if (!LegacySymbols) {
+						// Warn for <ProjectReference/> where a non-portable pdb was found
+						var project = file.GetMetadata ("MSBuildSourceProjectFile");
+						if (!string.IsNullOrEmpty (project)) {
+							Log.LogCodedWarning ("XA0122", project, lineNumber: 0,
+								message: $"{project} is generating legacy symbols that disables debugging for this project. Use 'DebugType=portable' instead.");
+						}
+					}
 				}
 			}
 

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

@@ -56,6 +56,8 @@ public class LinkAssemblies : AndroidTask, ML.ILogger
 
 		public bool Deterministic { get; set; }
 
+		public bool LegacySymbols { get; set; }
+
 		IEnumerable<AssemblyDefinition> GetRetainAssemblies (DirectoryAssemblyResolver res)
 		{
 			List<AssemblyDefinition> retainList = null;
@@ -133,7 +135,7 @@ bool Execute (DirectoryAssemblyResolver res)
 					if (!MonoAndroidHelper.IsForceRetainedAssembly (filename))
 						continue;
 
-					MonoAndroidHelper.CopyAssemblyAndSymbols (copysrc, assemblyDestination);
+					MonoAndroidHelper.CopyAssemblyAndSymbols (copysrc, assemblyDestination, LegacySymbols);
 				}
 			} catch (ResolutionException ex) {
 				Diagnostic.Error (2006, ex, "Could not resolve reference to '{0}' (defined in assembly '{1}') with scope '{2}'. When the scope is different from the defining assembly, it usually means that the type is forwarded.", ex.Member, ex.Member.Module.Assembly, ex.Scope);

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

@@ -28,6 +28,8 @@ public class LinkAssembliesNoShrink : AndroidTask
 
 		public bool Deterministic { get; set; }
 
+		public bool LegacySymbols { get; set; }
+
 		public override bool RunTask ()
 		{
 			if (SourceFiles.Length != DestinationFiles.Length)
@@ -65,7 +67,7 @@ public override bool RunTask ()
 						}
 					}
 
-					if (MonoAndroidHelper.CopyAssemblyAndSymbols (source.ItemSpec, destination.ItemSpec)) {
+					if (MonoAndroidHelper.CopyAssemblyAndSymbols (source.ItemSpec, destination.ItemSpec, LegacySymbols)) {
 						Log.LogDebugMessage ($"Copied: {destination.ItemSpec}");
 					} else {
 						Log.LogDebugMessage ($"Skipped unchanged file: {destination.ItemSpec}");

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

@@ -40,6 +40,7 @@ public class ResolveAssemblies : AndroidAsyncTask
 
 		public string I18nAssemblies { get; set; }
 		public string LinkMode { get; set; }
+		public bool LegacySymbols { get; set; }
 
 		// The user's assemblies, and all referenced assemblies
 		[Output]
@@ -137,7 +138,7 @@ void Execute (MetadataResolver resolver)
 			foreach (var assembly in assemblies.Values) {
 				var mdb = assembly + ".mdb";
 				var pdb = Path.ChangeExtension (assembly.ItemSpec, "pdb");
-				if (File.Exists (mdb))
+				if (LegacySymbols && File.Exists (mdb))
 					resolvedSymbols.Add (new TaskItem (mdb));
 				if (File.Exists (pdb) && Files.IsPortablePdb (pdb))
 					resolvedSymbols.Add (new TaskItem (pdb));

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

@@ -1419,7 +1419,9 @@ public Class2 ()
 		[Test]
 		public void BuildBasicApplicationCheckMdbAndPortablePdb ()
 		{
-			var proj = new XamarinAndroidApplicationProject ();
+			var proj = new XamarinAndroidApplicationProject {
+				AndroidLegacySymbols = true,
+			};
 			using (var b = CreateApkBuilder ("temp/BuildBasicApplicationCheckMdbAndPortablePdb")) {
 				b.Verbosity = LoggerVerbosity.Diagnostic;
 				var reference = new BuildItem.Reference ("PdbTestLibrary.dll") {
@@ -2926,6 +2928,7 @@ public void BuildBasicApplicationCheckPdb ()
 			var proj = new XamarinAndroidApplicationProject {
 				EmbedAssembliesIntoApk = true,
 				AndroidUseSharedRuntime = false,
+				AndroidLegacySymbols = true,
 			};
 			proj.SetProperty (proj.ActiveConfigurationProperties, "DebugType", "portable");
 			using (var b = CreateApkBuilder ("temp/BuildBasicApplicationCheckPdb", false, false)) {
@@ -2991,6 +2994,48 @@ public void BuildBasicApplicationCheckPdb ()
 			}
 		}
 
+		[Test]
+		public void WarningForDebugTypeFull ()
+		{
+			if (IsWindows) {
+				Assert.Ignore ("This test is only applicable on Windows.");
+				return;
+			}
+
+			var path = Path.Combine ("temp", TestName);
+			var lib = new XamarinPCLProject {
+				ProjectName = "MyPCL",
+				Sources = {
+					new BuildItem.Source ("Foo.cs") {
+						TextContent = () =>
+@"public class Foo
+{
+	public Foo ()
+	{
+	}
+}",
+					}
+				}
+			};
+			lib.SetProperty (lib.DebugProperties, "DebugType", "Full");
+			var app = new XamarinFormsAndroidApplicationProject {
+				ProjectName = "MyApp",
+				Sources = {
+					new BuildItem.Source ("Bar.cs") {
+						TextContent = () => "public class Bar : Foo { }",
+					}
+				}
+			};
+			app.References.Add (new BuildItem.ProjectReference ($"..\\{lib.ProjectName}\\{lib.ProjectName}.csproj", lib.ProjectName, lib.ProjectGuid));
+
+			using (var libBuilder = CreateDllBuilder (Path.Combine (path, lib.ProjectName)))
+			using (var appBuilder = CreateApkBuilder (Path.Combine (path, app.ProjectName))) {
+				Assert.True (libBuilder.Build (lib), "Library should have built.");
+				Assert.True (appBuilder.Build (app), "App should have built.");
+				Assert.IsTrue (StringAssertEx.ContainsText (appBuilder.LastBuildOutput, "XA0122"), "Output should contain XA0122 warnings");
+			}
+		}
+
 		[Test]
 		public void BuildInDesignTimeMode ([Values(false, true)] bool useManagedParser)
 		{

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

@@ -365,6 +365,7 @@ 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");
+				proj.AndroidLegacySymbols = true;
 				targets.Add ("_ConvertPdbFiles");
 			}
 			using (var b = CreateApkBuilder (Path.Combine ("temp", TestName))) {

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Android/KnownProperties.cs

@@ -12,7 +12,7 @@ public static class KnownProperties
 		public const string EmbedAssembliesIntoApk = "EmbedAssembliesIntoApk";
 		public const string AndroidUseLatestPlatformSdk = "AndroidUseLatestPlatformSdk";
 		public const string AndroidCreatePackagePerAbi = "AndroidCreatePackagePerAbi";
-
+		public const string AndroidLegacySymbols = "AndroidLegacySymbols";
 		public const string AndroidSupportedAbis = "AndroidSupportedAbis";
 
 		public const string Deterministic = "Deterministic";

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Android/XamarinAndroidApplicationProject.cs

@@ -106,6 +106,11 @@ public bool EmbedAssembliesIntoApk {
 			set { SetProperty (KnownProperties.EmbedAssembliesIntoApk, value.ToString ()); }
 		}
 
+		public bool AndroidLegacySymbols {
+			get { return string.Equals (GetProperty (KnownProperties.AndroidLegacySymbols), "True", StringComparison.OrdinalIgnoreCase); }
+			set { SetProperty (KnownProperties.AndroidLegacySymbols, value.ToString ()); }
+		}
+
 		public string DexTool {
 			get { return GetProperty (KnownProperties.AndroidDexTool); }
 			set { SetProperty (KnownProperties.AndroidDexTool, value); }

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Common/DotNetStandard.cs

@@ -18,7 +18,7 @@ public DotNetStandard ()
 			ProjectName = "UnnamedProject";
 			Sources = new List<BuildItem> ();
 			OtherBuildItems = new List<BuildItem> ();
-			SetProperty (CommonProperties, "DebugType", "full");
+			SetProperty (CommonProperties, "DebugType", "portable");
 			ItemGroupList.Add (Sources);
 			ItemGroupList.Add (OtherBuildItems);
 			Language = XamarinAndroidProjectLanguage.CSharp;

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Common/DotNetXamarinProject.cs

@@ -34,7 +34,7 @@ protected DotNetXamarinProject (string debugConfigurationName = "Debug", string
 			SetProperty ("BaseIntermediateOutputPath", "obj\\", " '$(BaseIntermediateOutputPath)' == '' ");
 
 			SetProperty (DebugProperties, "DebugSymbols", "true");
-			SetProperty (DebugProperties, "DebugType", "full");
+			SetProperty (DebugProperties, "DebugType", "portable");
 			SetProperty (DebugProperties, "Optimize", "false");
 			SetProperty (DebugProperties, KnownProperties.OutputPath, Path.Combine ("bin", debugConfigurationName));
 			SetProperty (DebugProperties, "DefineConstants", "DEBUG;");

src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.cs

@@ -389,13 +389,15 @@ public static void SetDirectoryWriteable (string directory)
 			}
 		}
 
-		public static bool CopyAssemblyAndSymbols (string source, string destination)
+		public static bool CopyAssemblyAndSymbols (string source, string destination, bool includeLegacySymbols)
 		{
 			bool changed = CopyIfChanged (source, destination);
-			var mdb = source + ".mdb";
-			if (File.Exists (mdb)) {
-				var mdbDestination = destination + ".mdb";
-				CopyIfChanged (mdb, mdbDestination);
+			if (includeLegacySymbols) {
+				var mdb = source + ".mdb";
+				if (File.Exists (mdb)) {
+					var mdbDestination = destination + ".mdb";
+					CopyIfChanged (mdb, mdbDestination);
+				}
 			}
 			var pdb = Path.ChangeExtension (source, "pdb");
 			if (File.Exists (pdb) && Files.IsPortablePdb (pdb)) {
@@ -475,6 +477,11 @@ public static string HashBytes (byte[] bytes)
 			return Files.HashBytes (bytes);
 		}
 
+		public static string HashString (string s)
+		{
+			return Files.HashString (s);
+		}
+
 		public static bool HasFileChanged (string source, string destination)
 		{
 			return Files.HasFileChanged (source, destination);

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

@@ -45,6 +45,7 @@ Copyright (C) 2011-2012 Xamarin. All rights reserved.
     <PropertyGroup>
         <DebugSymbols Condition=" '$(DebugType)' == 'None' ">true</DebugSymbols>
         <DebugType Condition=" '$(DebugType)' == 'None' Or '$(DebugType)' == '' ">portable</DebugType>
+        <DebugType Condition="('$(DebugType)' == 'Full' Or '$(DebugType)' == 'PdbOnly') And '$(AndroidLegacySymbols)' != 'True' ">portable</DebugType>
     </PropertyGroup>
     <Import Project="Xamarin.Android.DefaultOutputPaths.targets" Condition="'$(IsXBuild)' != 'true' and '$(EnableDefaultOutputPaths)' == 'true'" />
     <Import Project="$(MSBuildBinPath)\Microsoft.CSharp.targets" />

src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.props.in

@@ -12,6 +12,7 @@
 		<AndroidVersionCodePattern Condition=" '$(AndroidUseLegacyVersionCode)' != 'True' And '$(AndroidVersionCodePattern)' == '' ">{abi}{versionCode:D5}</AndroidVersionCodePattern>
 		<AndroidResourceGeneratorTargetName>UpdateGeneratedFiles</AndroidResourceGeneratorTargetName>
 		<AndroidUseAapt2 Condition=" '$(AndroidUseAapt2)' == '' ">True</AndroidUseAapt2>
+		<AndroidLegacySymbols Condition=" '$(AndroidLegacySymbols)' == '' ">False</AndroidLegacySymbols>
 		<AndroidPackageNamingPolicy Condition=" '$(AndroidPackageNamingPolicy)' == '' ">LowercaseCrc64</AndroidPackageNamingPolicy>
 		<AndroidUseManagedDesignTimeResourceGenerator Condition=" '$(AndroidUseManagedDesignTimeResourceGenerator)' == '' And '$(OS)' != 'Windows_NT' ">False</AndroidUseManagedDesignTimeResourceGenerator>
 		<AndroidSdkBuildToolsVersion Condition="'$(AndroidSdkBuildToolsVersion)' == ''">28.0.3</AndroidSdkBuildToolsVersion>

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

@@ -385,6 +385,9 @@ Copyright (C) 2011-2012 Xamarin. All rights reserved.
 		.pdb;
 		.xml;
 		.dll.config;
+	</AllowedReferenceRelatedFileExtensions>
+	<AllowedReferenceRelatedFileExtensions Condition=" '$(AndroidLegacySymbols)' == 'True' ">
+		$(AllowedReferenceRelatedFileExtensions);
 		.dll.mdb;
 	</AllowedReferenceRelatedFileExtensions>
 </PropertyGroup>
@@ -461,15 +464,6 @@ Copyright (C) 2011-2012 Xamarin. All rights reserved.
 	</CreateProperty>
 </Target>
 
-<!-- When looking for related files to copy, look for Mono debugging files as well -->
-<PropertyGroup>
-	<AllowedReferenceRelatedFileExtensions>
-		$(AllowedReferenceRelatedFileExtensions);
-		.dll.mdb;
-		.exe.mdb
-	</AllowedReferenceRelatedFileExtensions>
-</PropertyGroup>
-
 <Target Name="_SetupDesignTimeBuildForBuild">
 	<PropertyGroup>
 		<DesignTimeBuild Condition=" '$(DesignTimeBuild)' == '' ">false</DesignTimeBuild>
@@ -1814,6 +1808,7 @@ because xbuild doesn't support framework reference assemblies.
 		Assemblies="@(FilteredAssemblies)"
 		I18nAssemblies="$(MandroidI18n)"
 		LinkMode="$(AndroidLinkMode)"
+		LegacySymbols="$(AndroidLegacySymbols)"
 		ProjectFile="$(MSBuildProjectFullPath)"
 		TargetFrameworkVersion="$(TargetFrameworkVersion)"
 		ProjectAssetFile="$(ProjectLockFile)"
@@ -1947,32 +1942,29 @@ because xbuild doesn't support framework reference assemblies.
 	<Touch Files="$(_AndroidStampDirectory)_CopyConfigFiles.stamp" AlwaysCreate="True" />
 </Target>
 
-<Target Name="_CollectMdbFiles"
-		DependsOnTargets="_CollectPdbFiles">
-	<GetFilesThatExist
-			Files="@(ResolvedAssemblies->'%(RootDir)%(Directory)%(Filename)%(Extension).mdb')"
-			IgnoreFiles="@(_ResolvedPortablePdbFiles->'%(RootDir)%(Directory)%(Filename).dll.mdb')"
-	>
-		<Output TaskParameter="FilesThatExist" ItemName="_ResolvedMdbFiles" />
-	</GetFilesThatExist>
-</Target>
-
 <Target Name="_CollectPdbFiles">
 	<CollectPdbFiles
+			LegacySymbols="$(AndroidLegacySymbols)"
 			ResolvedAssemblies="@(ResolvedAssemblies->'%(RootDir)%(Directory)%(Filename).pdb')">
 		<Output TaskParameter="PdbFiles" ItemName="_ResolvedPdbFiles" />
 		<Output TaskParameter="PortablePdbFiles" ItemName="_ResolvedPortablePdbFiles" />
 	</CollectPdbFiles>
 	<ItemGroup>
-		<_ConvertedMdbFiles Include="@(_ResolvedPdbFiles->'%(RootDir)%(Directory)%(Filename).dll.mdb')" />
+		<_ConvertedMdbFiles
+				Condition=" '$(AndroidLegacySymbols)' == 'True' "
+				Include="@(_ResolvedPdbFiles->'%(RootDir)%(Directory)%(Filename).dll.mdb')"
+		/>
 	</ItemGroup>
 </Target>
 
 <Target Name="_ConvertPdbFiles"
 		Inputs="@(_ResolvedPdbFiles)"
 		Outputs="@(_ConvertedMdbFiles)"
 		DependsOnTargets="_CollectPdbFiles">
-	<ConvertDebuggingFiles Files="@(_ResolvedPdbFiles)" />
+	<ConvertDebuggingFiles
+			Condition=" '$(AndroidLegacySymbols)' == 'True' "
+			Files="@(_ResolvedPdbFiles)"
+	/>
 	<Touch Files="@(_ConvertedMdbFiles)" />
 	<ItemGroup>
 		<FileWrites Include="@(_ConvertedMdbFiles)" />
@@ -2023,6 +2015,7 @@ because xbuild doesn't support framework reference assemblies.
       SourceFiles="@(ResolvedAssemblies)"
       DestinationFiles="@(ResolvedAssemblies->'$(MonoAndroidIntermediateAssemblyDir)%(Filename)%(Extension)')"
       Deterministic="$(Deterministic)"
+      LegacySymbols="$(AndroidLegacySymbols)"
   />
   <ItemGroup>
     <FileWrites Include="$(MonoAndroidIntermediateAssemblyDir)*" />
@@ -2067,6 +2060,7 @@ because xbuild doesn't support framework reference assemblies.
       PreserveJniMarshalMethods="$(AndroidGenerateJniMarshalMethods)"
       EnableProguard="$(AndroidEnableProguard)"
       Deterministic="$(Deterministic)"
+      LegacySymbols="$(AndroidLegacySymbols)"
       DumpDependencies="$(LinkerDumpDependencies)"
       ResolvedAssemblies="@(_AssembliesToLink)"
       HttpClientHandlerType="$(AndroidHttpClientHandlerType)"