Skip to content

Conversation

@jpobst
Copy link
Contributor

@jpobst jpobst commented Jul 3, 2024

Context: #9043

While attempting to mark API-35 stable, we hit this error running all of the in-tree Windows smoke tests on CI:

D:\a\_work\1\s\bin\Release\dotnet\packs\Microsoft.Android.Sdk.Windows\35.0.0-ci.pr.gh9043.6\tools\
Xamarin.Android.Bindings.JavaDependencyVerification.targets(22,5): error MSB4062: The 
"Xamarin.Android.Tasks.GetMicrosoftNuGetPackagesMap" task could not be loaded from the assembly 
D:\a\_work\1\s\bin\Release\dotnet\packs\Microsoft.Android.Sdk.Windows\35.0.0-
ci.pr.gh9043.6\tools\Xamarin.Android.Build.Tasks.dll. Could not load file or assembly 'Mono.Cecil, Version=0.11.4.0, 
Culture=neutral, PublicKeyToken=50cebf1cceb9d05e'. The system cannot find the file specified. Confirm that the 
<UsingTask> declaration is correct, that the assembly and all its dependencies are available, and that the task contains a 
public class that implements Microsoft.Build.Framework.ITask. [D:\a\_work\1\a\TestRelease\07-
02_16.40.15\temp\DotNetBuildandroid-armFalseFalseFalse\UnnamedProject.csproj]

This appears to be caused by the fact that today we:

  • Build the ILLink* project(s) from runtime with their 0.11.5 Microsoft.DotNet.Cecil package.
  • Build the rest of XA with 0.11.4 Mono.Cecil package.
  • The 0.11.4 Mono.Cecil "wins" and ends up in the output directory and the sdk pack.

So long as ILLink* doesn't use any Cecil 0.11.5 API, this works, however it is risky because we don't know exactly what API ILLink* uses.

Removing our building of API-34 (by making API-35 stable), and no longer building API-35 as a preview, this order seems to get changed and the 0.11.5 Cecil "wins" and ends up in the output directory. This causes the assembly load error listed above.

We could try to fix the ordering and make 0.11.4 "win", but this leaves us vulnerable to missing some API that ILLink* needs. As such, it's better that we update the rest of XA to use the 0.11.5 version of Mono.Cecil.

This update breaks the FixAbstractMethodsStep_Explicit unit test. Specifically, this logic now returns null instead of being able to be resolved:

new MethodReference (iface_method.Name, void_type, iface).Resolve ();

It feels like this makes sense: a method name and return type doesn't seem like it would be enough to resolve, as parameters are not considered.

The fix is simply to use the existing MethodDefinition as the MethodReference, there is no reason to create a new one. This change fixes the test.

This change was pulled out of #9043 to ensure that it doesn't cause any breakage.

Note: Technically ILLink* references Microsoft.DotNet.Cecil version 0.11.4-alpha.24313.1, which implies that it is 0.11.4, however the Mono.Cecil.dll inside is versioned as 0.11.5.

@jpobst jpobst marked this pull request as ready for review July 3, 2024 19:44
@jonpryor jonpryor merged commit 54a9c8c into main Jul 3, 2024
@jonpryor jonpryor deleted the update-cecil branch July 3, 2024 20:44
grendello added a commit that referenced this pull request Jul 5, 2024
* main:
  [tests] verify trimmer warnings where appropriate (#9076)
  Bump to jbevain/cecil@8c123e1 (#9078)
  [trimming] remove `$(NullabilityInfoContextSupport)` (#9069)
  [build] Bump `$(XABuildToolsVersion)`=35 (#9071)
  [ci] Move PR build to shared pool (#8854)
  Use AsyncTask from xamarin-android-tools (#9017)
  Bump to dotnet/sdk@02c06d398a 9.0.100-preview.7.24351.1 (#9067)
  [trimming] use public `$(MetricsSupport)` property (#9068)
  [ci] Update resourceManagement.yml (#9070)
  Bump to dotnet/android-api-docs@c14203771a (#8992)
  [trimming] remove `$(_AggressiveAttributeTrimming)` by default (#9062)
@github-actions github-actions bot locked and limited conversation to collaborators Aug 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants