Skip to content

Commit

Permalink
[Xamarin.Android.Build.Tasks] remove Distinct() from <ResolveLibraryP…
Browse files Browse the repository at this point in the history
…rojectImports/>

The `<ResolveLibraryProjectImports/>` task has some logic that needs
to look at the item metadata of each `@(Reference)`:

* `%(AndroidSkipResourceExtraction)` to skip unzipping anything
* `%(AndroidSkipResourceProcessing)` to skip `<ConvertResourcesCases/>`

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<string> (StringComparer.OrdinalIgnoreCase);
    assembliestoSkipExtraction = new HashSet<string> (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:

* `<PackageReference/>`
* `packages.config` and a regular `<Reference/>`
* `<Reference/>` 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.
  • Loading branch information
jonathanpeppers committed Jun 27, 2019
1 parent 27b5dd2 commit 6d6b907
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 18 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ public class ResolveLibraryProjectImports : Task
};

AssemblyIdentityMap assemblyMap = new AssemblyIdentityMap();
HashSet<string> assembliesToSkipCaseFixup, assembliestoSkipExtraction;

public ResolveLibraryProjectImports ()
{
Expand All @@ -83,16 +82,6 @@ public override bool Execute ()
var resolvedEnvironmentFiles = new List<ITaskItem> ();

assemblyMap.Load (AssemblyIdentityMapFile);
assembliesToSkipCaseFixup = new HashSet<string> (StringComparer.OrdinalIgnoreCase);
assembliestoSkipExtraction = new HashSet<string> (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);
Expand Down Expand Up @@ -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) &&
Expand All @@ -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;
}
Expand Down Expand Up @@ -216,7 +204,7 @@ void Extract (
var taskItem = new TaskItem (Path.GetFullPath (resDir), new Dictionary<string, string> {
{ OriginalFile, assemblyPath },
});
if (assembliesToSkipCaseFixup.Contains (assemblyPath))
if (bool.TryParse (assemblyItem.GetMetadata (AndroidSkipResourceProcessing), out skip) && skip)
taskItem.SetMetadata (AndroidSkipResourceProcessing, "True");
resolvedResourceDirectories.Add (taskItem);
}
Expand Down Expand Up @@ -320,7 +308,7 @@ void Extract (
var taskItem = new TaskItem (Path.GetFullPath (resDir), new Dictionary<string, string> {
{ OriginalFile, assemblyPath }
});
if (assembliesToSkipCaseFixup.Contains (assemblyPath))
if (bool.TryParse (assemblyItem.GetMetadata (AndroidSkipResourceProcessing), out skip) && skip)
taskItem.SetMetadata (AndroidSkipResourceProcessing, "True");
resolvedResourceDirectories.Add (taskItem);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 ()
{
Expand Down

0 comments on commit 6d6b907

Please sign in to comment.