From c2aadbac7498599990dbb780abe88b534e85e433 Mon Sep 17 00:00:00 2001 From: Jonathan Peppers Date: Mon, 1 Jul 2019 15:56:50 -0500 Subject: [PATCH] [Xamarin.Android.Build.Tasks] remove Distinct() from (#3296) The `` task has some logic that needs to look at the item metadata of each `@(Reference)`: * `%(AndroidSkipResourceExtraction)` to skip unzipping anything * `%(AndroidSkipResourceProcessing)` to skip `` To make matters weird, we had some LINQ: foreach (var assemblyPath in Assemblies .Select (a => a.ItemSpec) .Distinct ()) { And we would basically lose the metadata information... To handle this problem, we had two `HashSet`s we used to lookup the values: assembliesToSkipCaseFixup = new HashSet (StringComparer.OrdinalIgnoreCase); assembliestoSkipExtraction = new HashSet (StringComparer.OrdinalIgnoreCase); bool metaDataValue; foreach (var asm in Assemblies) { if (bool.TryParse (asm.GetMetadata (AndroidSkipResourceProcessing), out metaDataValue) && metaDataValue) assembliesToSkipCaseFixup.Add (asm.ItemSpec); if (bool.TryParse (asm.GetMetadata (GetAdditionalResourcesFromAssemblies.AndroidSkipResourceExtraction), out metaDataValue) && metaDataValue) assembliestoSkipExtraction.Add (asm.ItemSpec); } I added a test to see what happens when we use multiple `@(Reference)` such as: * `` * `packages.config` and a regular `` * `` with a full path It turns out there are MSBuild targets that disambiguate duplicate assembly references: https://github.com/NuGet/NuGet.BuildTasks/blob/640c8e13a9b7ab6e86264a296638fbf3cc016ad1/src/Microsoft.NuGet.Build.Tasks/Microsoft.NuGet.targets#L186-L207 The `ResolveNuGetPackageAssets` does a `Remove` against duplicate `@(Reference)`, even if there are no NuGet packages present in the project. It seems safe for us to just drop the `Distinct()` and the `HashSet`s. The code is a bit simpler after doing this. I'm also seeing a small ~25ms performance improvement with this change. --- .../Tasks/ResolveLibraryProjectImports.cs | 24 +++++-------------- .../Xamarin.Android.Build.Tests/BuildTest.cs | 23 ++++++++++++++++++ 2 files changed, 29 insertions(+), 18 deletions(-) diff --git a/src/Xamarin.Android.Build.Tasks/Tasks/ResolveLibraryProjectImports.cs b/src/Xamarin.Android.Build.Tasks/Tasks/ResolveLibraryProjectImports.cs index 402fcad2e2c..a9f18cd4fce 100644 --- a/src/Xamarin.Android.Build.Tasks/Tasks/ResolveLibraryProjectImports.cs +++ b/src/Xamarin.Android.Build.Tasks/Tasks/ResolveLibraryProjectImports.cs @@ -67,7 +67,6 @@ public class ResolveLibraryProjectImports : Task }; AssemblyIdentityMap assemblyMap = new AssemblyIdentityMap(); - HashSet assembliesToSkipCaseFixup, assembliestoSkipExtraction; public ResolveLibraryProjectImports () { @@ -83,16 +82,6 @@ public override bool Execute () var resolvedEnvironmentFiles = new List (); assemblyMap.Load (AssemblyIdentityMapFile); - assembliesToSkipCaseFixup = new HashSet (StringComparer.OrdinalIgnoreCase); - assembliestoSkipExtraction = new HashSet (StringComparer.OrdinalIgnoreCase); - bool metaDataValue; - foreach (var asm in Assemblies) { - if (bool.TryParse (asm.GetMetadata (AndroidSkipResourceProcessing), out metaDataValue) && metaDataValue) - assembliesToSkipCaseFixup.Add (asm.ItemSpec); - if (bool.TryParse (asm.GetMetadata (GetAdditionalResourcesFromAssemblies.AndroidSkipResourceExtraction), out metaDataValue) && metaDataValue) - assembliestoSkipExtraction.Add (asm.ItemSpec); - } - using (var resolver = new DirectoryAssemblyResolver (this.CreateTaskLogger (), loadDebugSymbols: false)) { try { Extract (resolver, jars, resolvedResourceDirectories, resolvedAssetDirectories, resolvedEnvironmentFiles); @@ -170,10 +159,9 @@ void Extract ( foreach (var assembly in Assemblies) res.Load (assembly.ItemSpec); - // FIXME: reorder references by import priority (not sure how to do that yet) - foreach (var assemblyPath in Assemblies - .Select (a => a.ItemSpec) - .Distinct ()) { + bool skip; + foreach (var assemblyItem in Assemblies) { + var assemblyPath = assemblyItem.ItemSpec; var fileName = Path.GetFileName (assemblyPath); if (MonoAndroidHelper.IsFrameworkAssembly (fileName) && !MonoAndroidHelper.FrameworkEmbeddedJarLookupTargets.Contains (fileName) && @@ -185,7 +173,7 @@ void Extract ( Log.LogDebugMessage ($"Skipping non-existent dependency '{assemblyPath}' during a design-time build."); continue; } - if (assembliestoSkipExtraction.Contains (assemblyPath)) { + if (bool.TryParse (assemblyItem.GetMetadata (GetAdditionalResourcesFromAssemblies.AndroidSkipResourceExtraction), out skip) && skip) { Log.LogDebugMessage ("Skipping resource extraction for '{0}' .", assemblyPath); continue; } @@ -216,7 +204,7 @@ void Extract ( var taskItem = new TaskItem (Path.GetFullPath (resDir), new Dictionary { { OriginalFile, assemblyPath }, }); - if (assembliesToSkipCaseFixup.Contains (assemblyPath)) + if (bool.TryParse (assemblyItem.GetMetadata (AndroidSkipResourceProcessing), out skip) && skip) taskItem.SetMetadata (AndroidSkipResourceProcessing, "True"); resolvedResourceDirectories.Add (taskItem); } @@ -320,7 +308,7 @@ void Extract ( var taskItem = new TaskItem (Path.GetFullPath (resDir), new Dictionary { { OriginalFile, assemblyPath } }); - if (assembliesToSkipCaseFixup.Contains (assemblyPath)) + if (bool.TryParse (assemblyItem.GetMetadata (AndroidSkipResourceProcessing), out skip) && skip) taskItem.SetMetadata (AndroidSkipResourceProcessing, "True"); resolvedResourceDirectories.Add (taskItem); } diff --git a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs index 2190670d9a0..159ff0d9edc 100644 --- a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs +++ b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs @@ -134,6 +134,29 @@ public void BuildBasicApplicationAppCompat ([Values (true, false)] bool usePacka } } + [Test] + public void DuplicateReferences () + { + var proj = new XamarinAndroidApplicationProject (); + proj.MainActivity = proj.DefaultMainActivity.Replace ("public class MainActivity : Activity", "public class MainActivity : Android.Support.V7.App.AppCompatActivity"); + var package = KnownPackages.SupportV7AppCompat_27_0_2_1; + var fullPath = Path.GetFullPath (Path.Combine (Root, "temp", "packages", $"{package.Id}.{package.Version}", "lib", package.TargetFramework, $"{package.Id}.dll")); + proj.PackageReferences.Add (package); + proj.Packages.Add (package); + proj.References.Add (new BuildItem.Reference (package.Id) { + MetadataValues = "HintPath=" + fullPath, + }); + using (var b = CreateApkBuilder (Path.Combine ("temp", TestName))) { + Assert.IsTrue (b.Build (proj), "first build should have succeeded."); + + // Remove NuGet packages, but leave References + proj.PackageReferences.Clear (); + proj.Packages.Clear (); + + Assert.IsTrue (b.Build (proj), "second build should have succeeded."); + } + } + [Test] public void BuildXamarinFormsMapsApplication () {