-
Notifications
You must be signed in to change notification settings - Fork 528
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Xamarin.Android.Build.Tasks] remove Distinct() from <ResolveLibraryProjectImports/> #3296
[Xamarin.Android.Build.Tasks] remove Distinct() from <ResolveLibraryProjectImports/> #3296
Conversation
@jonathanpeppers this needs rebasing I think. We are getting the packaging error because the CI system is not building the x86_64 native libraries. |
1dc7e2f
to
6d6b907
Compare
This one failed in a weird way:
Going to restart. |
…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.
6d6b907
to
3f12245
Compare
Failing Tests are
They don't look related though :/ |
Indeed, they don't look related. They are disturbing; from
Unfortunately we don't normally package up the test The Kicking off a rebuild: https://jenkins.mono-project.com/job/xamarin-android-pr-pipeline-release/1518/ |
The
<ResolveLibraryProjectImports/>
task has some logic that needsto 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:
And we would basically lose the metadata information...
To handle this problem, we had two
HashSet
's we used to lookup thevalues:
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 pathIt 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 aRemove
against duplicate@(Reference)
, even if there are no NuGet packages present in theproject.
It seems safe for us to just drop the
Distinct()
and theHashSet
's. The code is a bit simpler after doing this.I'm also seeing a small ~25ms performance improvement with this change.