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

Address Trim warnings and prevent future regressions #3841

Merged
merged 41 commits into from
Dec 18, 2024
Merged

Conversation

jamescrosswell
Copy link
Collaborator

@jamescrosswell jamescrosswell commented Dec 11, 2024

Resolves #3808:

Summary

To get useful trim analysis when building libraries we need a test app that shows all warnings.

In this PR we've added test/Sentry.TrimTest/Sentry.TrimTest.csproj to serve as the test app that will produce said warnings for most of the sentry packages. We've also added test/Sentry.MauiTrimTest/Sentry.MauiTrimTest.csproj to detect trim warnings for code targeting android and iOS (in the Sentry.Maui and Sentry.Bindings.* packages).

Additionally, a new job has been added to CI to publish those two test apps. If the publish attempts succeed without error, we should be trim compatible.

Note to Reviewers

This looks like a 9k line PR. However, most of the "additions" are in test/Sentry.MauiTrimTest/Resources/Fonts/FluentUI.cs which is just part of the MAUI template (and can be ignored).

Merge Note

This PR should also be merged in the Ben.Demystifier submodule when this PR is accepted/merged:

Sentry.Maui trimming limitations

We're using reflection to read the values of some internal properties on event args (so we can drop breadcrumbs). When apps are trimmed, we're not able to generate these breadcrumbs... so we don't have 100% functional equivalence for the SDK when MAUI apps are trimmed.

See:

Sentry.Maui trim analyser issues

There is currently a bug in System.Text.Json that prevents us from publishing AOT applications to iOS:

So we can't check for AOT warnings on iOS (only trim analyser warnings for the time being).

Packages incompatible with trimming

At this time, the following packages are not compatible with trimming:

<!-- The following packages are deliberately excluded from the TrimTest app because they do not support trimming -->
<!-- Sentry.AspNetCore.Blazor.WebAssembly: produces trim warnings... not yet sure how to resolve these -->
<!-- Sentry.Azure.Functions.Worker: `Microsoft.Azure.Functions.Worker` produces [trim warnings](https://github.com/Azure/azure-functions-dotnet-worker/issues/2899) -->
<!-- Sentry.Hangfire: Hangfire.Core produces trim warnings and relies on Newtonsoft.Json -->
<!-- Sentry.Log4Net: Legacy - produces trim warnings -->
<!-- Sentry.Profiling: FastSerialization and DynamicTraceEventParser produce trim warnings -->

Of those, Sentry.Profiling and Sentry.AspNetCore.Blazor.WebAssembly are currently the only two that we might be able to address... We should create separate issues for these if we want to trimming compatibility for them.

@jamescrosswell
Copy link
Collaborator Author

jamescrosswell commented Dec 13, 2024

Sentry.Profiling

There are a few trim warnings for Sentry.Profiling that are no trivial to resolve:

/src/FastSerialization/FastSerialization.cs(2017,21): Trim analysis error IL2072: FastSerialization.Deserializer.<>c__DisplayClass68_0.<GetFactory>b__0(): 'type' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicParameterlessConstructor' in call to 'System.Activator.CreateInstance(Type)'. The return value of method 'System.Func<T,TResult>.Invoke(Func<T,TResult>.T)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
/src/FastSerialization/FastSerialization.cs(1995,17): Trim analysis error IL2057: FastSerialization.Deserializer.GetFactory(String): Unrecognized value passed to the parameter 'typeName' of method 'System.Type.GetType(String)'. It's not possible to guarantee the availability of the target type.
/src/TraceEvent/DynamicTraceEventParser.cs(957,17): Trim analysis error IL2067: Microsoft.Diagnostics.Tracing.Parsers.DynamicTraceEventData.GetDefaultValueByType(Type): 'type' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicParameterlessConstructor' in call to 'System.Activator.CreateInstance(Type)'. The parameter 'type' of method 'Microsoft.Diagnostics.Tracing.Parsers.DynamicTraceEventData.GetDefaultValueByType(Type)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
/src/TraceEvent/DynamicTraceEventParser.cs(1674,21): Trim analysis error IL2057: Microsoft.Diagnostics.Tracing.Parsers.DynamicTraceEventData.PayloadFetch.FromStream(Deserializer): Unrecognized value passed to the parameter 'typeName' of method 'System.Type.GetType(String)'. It's not possible to guarantee the availability of the target type.

The warning on line 1995 of FastSerialization.cs relates to this:

type = Type.GetType(fullName);

and the related error on line 2017 relates to this code:

return (IFastSerializable)Activator.CreateInstance(type);

The code essentially seems to be trying to construct arbitrary types... which will definitely mess with trimming.

The conventional solution to this would be to use System.Text.Json and provide a custom JsonSerializerContext... so basically it would be to use different serializers (that are trim compatible).

@vaind I think you're the resident expert on all things profiling. Do you have any context for why the profiling module uses FastSerialization instead of System.Text.Json? Could we potentially swap the serialization out and replace this with System.Text.Json or, alternatively, is there maybe updated code for FastSerialization that is trim compatible?

@jamescrosswell
Copy link
Collaborator Author

Sentry.AspNetCore.Blazor.WebAssembly

Trying to include Sentry.AspNetCore.Blazor.WebAssembly in the TrimTest app results in a couple of trim warnings but nothing specific (no line numbers of specific piece of code that we could call the culprit)... so it's not really actionable. Might need to do a bit of digging to see if Trimming/AOT is possible with WASM:

    ILLink : Trim analysis error IL2111: Microsoft.JSInterop.Infrastructure.DotNetDispatcher.GetCachedMethodInfo(IDotNetObjectReference, String): Method 'Microsoft.JSInterop.Infrastructure.DotNetDispatcher.<GetCachedMethodInfo>g__ScanTypeForCallableMethods|16_0(Type)' with parameters or return value with `DynamicallyAccessedMembersAttribute` is accessed via reflection. Trimmer can't guarantee availability of the requirements of the method.
    ILLink : Trim analysis error IL2065: Microsoft.JSInterop.Infrastructure.DotNetDispatcher.ScanAssemblyForCallableMethods(DotNetDispatcher.AssemblyKey): Value passed to implicit 'this' parameter of method 'System.Type.GetMethods(BindingFlags)' can not be statically determined and may not meet 'DynamicallyAccessedMembersAttribute' requirements.

@vaind
Copy link
Collaborator

vaind commented Dec 13, 2024

@vaind I think you're the resident expert on all things profiling. Do you have any context for why the profiling module uses FastSerialization instead of System.Text.Json? Could we potentially swap the serialization out and replace this with System.Text.Json or, alternatively, is there maybe updated code for FastSerialization that is trim compatible?

These all belong to perfview, we just import some of the projects from there. Not sure how we could proceed here, yet. Do we have any other 3rd party code included that has these warnings?

@jamescrosswell
Copy link
Collaborator Author

These all belong to perfview, we just import some of the projects from there. Not sure how we could proceed here, yet. Do we have any other 3rd party code included that has these warnings?

Not code no. Hangfire.Core (which is a dependency of Sentry.Hangfire) uses Newtonsoft.Json... In practice, this means the current version of Hangfire can't be used in AOT Compiled applications (theoretically it could, but only with trimming disabled - which would make the resulting binaries absolutely massive).

The only other code that we've used that had these trim warnings was our own code, which we updated for the 4.0.0 release to use the System.Text.Json source generators instead of reflection for serialisation. That's the reason I asked if it might be possible to swap FastSerialization for System.Text.Json... Creating source generated serialisers is non-trivial so there would have to be a very compelling reason to re-do all of that work for FastSerialization when it's already been done for System.Text.Json.

@jamescrosswell jamescrosswell marked this pull request as ready for review December 16, 2024 03:04
@jamescrosswell jamescrosswell requested a review from vaind December 16, 2024 03:05
@jamescrosswell jamescrosswell marked this pull request as ready for review December 17, 2024 10:49

trim-analysis:
name: Trim analysis
runs-on: macos-15
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this be latest or we need to pin?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We pin to get the correct version of Xcode. As of about a month ago, the GitHub macos runners are associated with specific major versions of Xcode.

  • macos-14 has Xcode 15.x installed
  • macos-15 has Xcode 16.x installed

I've basically started trying to pin everything (specific runner version, .NET workload versions via the ios18.0 or android35.0 monkiers etc.) since it gets us closer to deterministic builds. If we don't do this, too much changes arbitrarily between different runs against the same commit and CI fails arbitrarily.

- name: Install Android SDKs
if: runner.os == 'macOS'
run: |
dotnet build src/Sentry/Sentry.csproj -t:InstallAndroidDependencies -f:net8.0-android34.0 -p:AcceptAndroidSDKLicenses=True -p:AndroidSdkPath="/usr/local/lib/android/sdk/"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to set the specific android version here? I imagine it might go out of sync when we bump .NET for example

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What we're passing here is the target framework net8.0-android34.0. That has to match one of the target frameworks in the csproj file and we pin android34.0 as the oldest possible set of APIs that can be used with net8.0 (for net9.0 it's net9.0-android35.0).

The main reason we're doing this is to ensure the android34.0 SDK gets installed as this is missing from the new GitHub runners. If we remove this, the build fails later on in the workflow.

.github/workflows/build.yml Outdated Show resolved Hide resolved
run: dotnet publish test/Sentry.TrimTest/Sentry.TrimTest.csproj -c Release -r osx-arm64

- name: Publish Test app (Android)
run: dotnet publish test/Sentry.MauiTrimTest/Sentry.MauiTrimTest.csproj -c Release -f net9.0-android35.0 -r android-arm64
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we use Android 35 here and 34 above. Is that by default? We could set this as vars at the top of the workflow at least?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

34.0 is used abote with the t:InstallAndroidDependencies parameter to install some missing dependencies. For the trim tests, I've used net9.0 as there are some new analysers in net9.0 (i.e. it picks up more issues).

So yeah, unfortunately we want to use two separate target frameworks for the two different build steps.

@jamescrosswell jamescrosswell merged commit a421af5 into main Dec 18, 2024
22 checks passed
@jamescrosswell jamescrosswell deleted the trim-warnings branch December 18, 2024 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to publish AOT compiled net9.0-ios apps
4 participants