Skip to content
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

Linking an empty .NET MAUI app produces inconsistent binaries #86242

Closed
filipnavara opened this issue May 15, 2023 · 4 comments · Fixed by #86474
Closed

Linking an empty .NET MAUI app produces inconsistent binaries #86242

filipnavara opened this issue May 15, 2023 · 4 comments · Fixed by #86474
Labels
area-Tools-ILLink .NET linker development as well as trimming analyzers

Comments

@filipnavara
Copy link
Member

filipnavara commented May 15, 2023

Repro:

  • Create a MAUI app with dotnet new maui
  • Build it for MacCatalyst or iOS, eg. dotnet build -f net8.0-maccatalyst
  • Check the linked assemblies in obj/Debug/net8.0-maccatalyst/maccatalyst-x64/linked/.

Observations:

  • In Microsoft.Maui.Controls.dll the INavigation.PopToRootAsync() method is trimmed from the interface.
  • In Microsoft.Maui.Compatibility.dll the Microsoft.Maui.Controls.Compatibility.Platform.iOS.Platform type keeps an explicit implementation of the INavigation.PopToRootAsync() method.

Expectations:

  • Either the method is not trimmed from the interface, or all explicit implementations of the method are trimmed too.
@filipnavara filipnavara added the area-Tools-ILLink .NET linker development as well as trimming analyzers label May 15, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label May 15, 2023
@ghost
Copy link

ghost commented May 15, 2023

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

Issue Details

Repro:

  • Create a MAUI app with dotnet new maui
  • Build it for MacCatalyst or iOS, eg. dotnet build -f net8.0-maccatalyst
  • Check the linked assemblies in obj/Debug/net8.0-maccatalyst/maccatalyst-x64/linked/.

Observations:

  • In Microsoft.Maui.Controls.dll the INavigation.PopToRootAsync() method is trimmed from the interface.
  • In Microsoft.Maui.Compatibility.dll the Microsoft.Maui.Controls.Compatibility.Platform.iOS.Platform type keeps an explicit implementation of the INavigation.PopToRootAsync() method.

Expectations:

  • Either the method is not trimmed from the interface, or all explicit implementation of the method are trimmed too.
Author: filipnavara
Assignees: -
Labels:

area-Tools-ILLink

Milestone: -

@sbomer
Copy link
Member

sbomer commented May 18, 2023

I was able to reproduce the issue with the repro that you shared with @vitek-karas. Here's what's happening:

  • The custom step ManagedRegistrarStep sets the assembly action for Microsoft.Maui.Controls.Compatibility.dll to save (it calls AppBundleRewriter.SaveCurrentAssembly). On its own, this might have been enough to repro the issue because the fix Mark .overrides from marked methods in a copy assembly #82197 doesn't take into account save assemblies - we should probably fix that.
  • On top of that, MarkStep never marks the Platform implementation of INavigation.PopToRootAsync, because of the custom data DisableMarkingOfCopyAssemblies:
    if (!Context.TryGetCustomData ("DisableMarkingOfCopyAssemblies", out string? disableMarkingOfCopyAssembliesValue) ||
    So even if we fix the above, it doesn't help in this case.

I don't have full context, but it seems like the action is set to save because the registrar is injecting UnmanagedCallersOnly trampoline methods for interop. Maybe we should change the behavior of DisableMarkingOfCopyAssemblies so that it doesn't also skip marking of save assemblies. @vitek-karas thoughts?

@filipnavara
Copy link
Member Author

Thanks for analyzing it!

Before I reported the issue I made sure that it reproduces on stock runtime/linker/workloads. The repro I sent, however, used a new feature introduced in xamarin/xamarin-macios#18268. In that case the ManagedRegistrarStep is an additional step that takes place in the linker pipeline. It does generate new code and as a consequence changes the action from Copy to Save. Unfortunately that opened a small can of worms, where the behavior of the Save action is sometimes unintuitive or broken.

I suppose that on the stock workload the issue is the use of DisableMarkingOfCopyAssemblies you described. I'll try to selectively disable that and check the output.

@filipnavara
Copy link
Member Author

I suppose that on the stock workload the issue is the use of DisableMarkingOfCopyAssemblies you described. I'll try to selectively disable that and check the output.

Removing DisableMarkingOfCopyAssemblies (by setting <MarkCopiedAssemblies>true</MarkCopiedAssemblies> in the .csproj file) seems to have solved the issue even for the case with ManagedRegistrarStep.

I was not able reproduce the issue on stock workloads this time. I'm not sure if I accidentally changed something or if it's a result of Tuesday's updates (unlikely). I'll try again tomorrow.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 19, 2023
@ghost ghost removed in-pr There is an active PR which will close this issue when it is merged untriaged New issue has not been triaged by the area owner labels May 19, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Tools-ILLink .NET linker development as well as trimming analyzers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants