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

iOS Release can't build with CosmosDB #101967

Closed
mdbill opened this issue Apr 5, 2024 · 14 comments
Closed

iOS Release can't build with CosmosDB #101967

mdbill opened this issue Apr 5, 2024 · 14 comments
Assignees
Labels
area-Infrastructure-mono in-pr There is an active PR which will close this issue when it is merged os-ios Apple iOS
Milestone

Comments

@mdbill
Copy link

mdbill commented Apr 5, 2024

Description

Attempt to build an ipa file (Release) fails.

error : ILStrip failed for C:/Users/xxx/.nuget/packages/microsoft.azure.
cosmos/3.38.1/runtimes/win-x64/native/Microsoft.Azure.Cosmos.ServiceInterop.dll: The image is not a managed assembly

Oddly, it's a windows-only DLL. Worked in Xamarin.Forms. Works on Android. Works on iOS Debug/Simulator

See:
Azure/azure-cosmos-dotnet-v3#3171

Steps to Reproduce

build an ipa file (Release) of the MAUI template with a reference to the Cosmos nuget. I used 3.38.1.

Link to public reproduction project repository

No response

Version with bug

8.0.10 SR3

Is this a regression from previous behavior?

Yes, this used to work in Xamarin.Forms

Last version that worked well

Unknown/Other

Affected platforms

iOS

Affected platform versions

No response

Did you find any workaround?

The Cosmos group said this DLL is not needed and will work fine if missing, so I just need to get past the build.

Relevant log output

No response

@drasticactions
Copy link

drasticactions commented Apr 6, 2024

Your issue is unrelated to the MAUI UI project and this repo, it's a runtime issue. I'm not unsure if it's an issue with ILStrip trying to strip an assembly it should not be or with the Cosmos library including that assembly if it shouldn't be, but I believe this is something that would need to go to runtime or to Cosmos. It must be addressed elsewhere.

If you want to work around it, you can enable the Mono Interpreter with <UseInterpreter> in your csproj, and that should disable the stripping.

スクリーンショット 2024-04-06 23 00 21

To show my point, I wrote an example app in .NET iOS app with no MAUI tooling. Enabling the interpreter in Release mode and publishing it gets me the IPA, disabling it throws the same exception you're getting in your MAUI app. Remember that a MAUI app is really a .NET platform app with an abstracted UI layer library on top and some extra tooling.

If you see library errors like this, try deploying it with a straight .NET app first without the MAUI UI layer. If it works there and doesn't work with the MAUI Template, there could be an issue in this repo. But if it doesn't work with a .NET app at all, it's probably something more fundamental.

@dalexsoto I'm not sure where this issue should go, would you know?

e. Also to note, it "worked" in Xamarin.Forms because it's based on older Mono tooling. The underlying .NET runtime used by MAUI is similar, but not the same. The UI layer has nothing to do with your issue.

@mdbill
Copy link
Author

mdbill commented Apr 12, 2024

Thanks for the help.
How would:
<EnableAssemblyILStripping>false</EnableAssemblyILStripping>
compare to <UseInterpreter>? Is one less of an effect than the other? Is there a way to focus the effect only on the Cosmos dll?
Is it really OK to do either of these for production?

@mdbill
Copy link
Author

mdbill commented Apr 22, 2024

I realized my tags weren't wrapped as code and parts of my previous post weren't visible and made no sense. Can anyone comment now?

@PureWeen PureWeen transferred this issue from dotnet/maui May 7, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label May 7, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Copy link
Contributor

Tagging subscribers to this area: @directhex
See info in area-owners.md if you want to be subscribed.

@ViktorHofer
Copy link
Member

@akoeplinger do you have an area label for ILStrip issues?

@akoeplinger
Copy link
Member

akoeplinger commented May 7, 2024

hmm we don't have one for ILStrip. @vitek-karas should we add one or use area-Tools-ILLink?

To the issue: we need to add a check and skip non-managed DLLs in ILStrip (e.g. by catching ImageFormatException). That said, I'm not sure if that'll be enough since looking at the xamarin-macios task I think it relies on ILStrip writing the stripped file to the output path and just skipping would "remove" the assembly. Maybe we can avoid passing in non-managed assemblies to the task instead. /cc @rolfbjarne

@mdbill I'd recommend EnableAssemblyILStripping=false as a workaround since that'll still AOT compile your app, it just won't remove the (now unused) IL from the .dll's.

@akoeplinger akoeplinger added the os-ios Apple iOS label May 7, 2024
@akoeplinger akoeplinger added this to the 9.0.0 milestone May 7, 2024
@akoeplinger akoeplinger removed the untriaged New issue has not been triaged by the area owner label May 7, 2024
@vitek-karas
Copy link
Member

We can probably leave it in a mono area either as infra or as AOT code gen related.

Where is ILStrip used - is it only from mono AOT compiler on both Android/iOS?

@jkurdek could you please take a quick look?

@akoeplinger
Copy link
Member

Where is ILStrip used - is it only from mono AOT compiler on both Android/iOS?

There is a bit of a complex history here :)

Originally ILStrip was only used by xamarin-macios to remove IL since they do "full" AOT and don't need the IL anymore. It trims method bodies from all methods in an assembly unconditionally. It uses a very old version of Cecil that keeps the metadata tokens the same when removing method bodies which is required so the mono runtime can resolve methods in the AOT image from a stripped .dll but newer Cecil versions automatically regenerate the metadata token. We imported that copy of Cecil into runtime-assets and compile it into the task directly (which is kinda bad: #60273).

Some time ago Fan added a way to trim only specific methods (TrimIndividualMethods flag in the task), this was so that xamarin-android can use ILStrip as well since FullAOT/stripping everything isn't realistic there. The way it works is that the mono aot compiler writes a list of methods it AOTd into a file and that file is passed to ILStrip so it only trims those methods. This part of ILStrip uses System.Reflection.Metadata instead of Cecil and "solves" the metadata token issue by just zero-ing the method body bytes.

I assume only the first one used by xamarin-macios is affected.

@rolfbjarne
Copy link
Member

@mdbill I'd recommend EnableAssemblyILStripping=false as a workaround since that'll still AOT compile your app, it just won't remove the (now unused) IL from the .dll's.

Agreed, this is the best option (pretty much the only downside is a bigger app).

To the issue: we need to add a check and skip non-managed DLLs in ILStrip (e.g. by catching ImageFormatException). That said, I'm not sure if that'll be enough since looking at the xamarin-macios task I think it relies on ILStrip writing the stripped file to the output path and just skipping would "remove" the assembly. Maybe we can avoid passing in non-managed assemblies to the task instead. /cc @rolfbjarne

We could adjust our ILStrip task subclass to filter out non-managed assemblies:

https://github.com/xamarin/xamarin-macios/blob/6d137cbeb228818ff1f3d578114e4027402e347d/msbuild/Xamarin.MacDev.Tasks/Tasks/ILStrip.cs#L26-L27

That said, to me it feels like the best option would be to add a property to the ILTask that allows the user to decide what to do with non-managed assemblies (throw/warn/ignore) - just from a simple performance perspective it's best to open and read the file(s) in question only once, and ILTask already opens and reads these files.

@akoeplinger
Copy link
Member

We could also populate the UpdatedAssemblies output which is currently only updated by the TrimIndividualMethods codepath:

/// <summary>
/// Contains the updated list of assemblies comparing to the input variable Assemblies.
/// Replaced the trimmed ones with their new location.
///
/// Added two metadata for trimmed items:
/// - UntrimmedAssemblyFilePath
/// - ILStripped: set to true to indicate this item is trimmed
/// </summary>
[Output]
public ITaskItem[]? UpdatedAssemblies { get; set; }

Only downside is that this existed in a different form in 8.0 so backporting wouldn't be straighforward.

Sidenote: it is kinda weird that a file from runtimes/win-x64 gets included in the build, we should investigate that separately.

@jkurdek
Copy link
Member

jkurdek commented May 13, 2024

That said, I'm not sure if that'll be enough since looking at the [xamarin-macios task](https://github.com/xamarin/xamarin- macios/blob/6d137cbeb228818ff1f3d578114e4027402e347d/dotnet/targets/Xamarin.Shared.Sdk.targets#L894) I think it relies on ILStrip writing the stripped file to the output path and just skipping would "remove" the assembly.

Yeah, just handling the error in ILStrip.cs/StripAssembly doesn't look enough. The xamarin-macios task would still write it information about the assembly to the output, yet the assembly would not be written if skipped.

We could also populate the UpdatedAssemblies output which is currently only updated by the TrimIndividualMethods codepath

This looks hopeful. You mean to create StrippedAssemblies in xamarin-macios using UpdatedAssemblies right?

@akoeplinger
Copy link
Member

This looks hopeful. You mean to create StrippedAssemblies in xamarin-macios using UpdatedAssemblies right?

Yep.

@jkurdek
Copy link
Member

jkurdek commented Oct 3, 2024

Fixed in .NET 9

@jkurdek jkurdek closed this as completed Oct 3, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Nov 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure-mono in-pr There is an active PR which will close this issue when it is merged os-ios Apple iOS
Projects
None yet
Development

No branches or pull requests

7 participants