-
Notifications
You must be signed in to change notification settings - Fork 530
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] fix System.Text.Json with default trimmer settings #8266
[Xamarin.Android.Build.Tasks] fix System.Text.Json with default trimmer settings #8266
Conversation
…er settings Fixes: dotnet/maui#16038 Changes to trimmer defaults in .NET 8 require a new `$(JsonSerializerIsReflectionEnabledByDefault)` MSBuild property to be set by default for project types that use `$(TrimMode)=partial` like when running on mobile (iOS/Android). I added a very basic System.Text.Json test, which fails with the exception: System.InvalidOperationException : JsonSerializerIsReflectionDisabled at System.Text.Json.JsonSerializerOptions.ConfigureForJsonSerializer() at System.Text.Json.JsonSerializer.GetTypeInfo(JsonSerializerOptions , Type ) at System.Text.Json.JsonSerializer.Deserialize(String , Type , JsonSerializerOptions ) at System.Text.JsonTests.JsonSerializerTest.Deserialize() at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args) at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object , BindingFlags ) Simply running the test again with the MSBuild property set allows the test to pass. This will be a good smoke test for System.Text.Json going forward. The iOS side of this is tracked here: xamarin/xamarin-macios#18057
@@ -112,6 +112,7 @@ | |||
<_AggressiveAttributeTrimming Condition="'$(_AggressiveAttributeTrimming)' == ''">true</_AggressiveAttributeTrimming> | |||
<NullabilityInfoContextSupport Condition="'$(NullabilityInfoContextSupport)' == ''">false</NullabilityInfoContextSupport> | |||
<BuiltInComInteropSupport Condition="'$(BuiltInComInteropSupport)' == ''">false</BuiltInComInteropSupport> | |||
<JsonSerializerIsReflectionEnabledByDefault Condition="'$(JsonSerializerIsReflectionEnabledByDefault)' == ''">true</JsonSerializerIsReflectionEnabledByDefault> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldthis not check '$(TrimMode)' as well? given the commit message this should be set only when '$(TrimMode)' is 'partial'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think that makes sense. It looks like TrimMode
is set earlier in this file, so this is the right place we can check it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @eerhardt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this makes sense too. If someone sets TrimMode=full
, JsonSerializer is basically guaranteed to break. So you might as well disable reflection and guide them to use the source generator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC we didn't add a similar condition in Blazor. Do we need to revisit there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blazor doesn't support TrimMode=full
at all. So I don't think we need to worry about it for .NET 8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
We can ignore the test failures, they look unrelated:
Going to merge. |
Fixes: dotnet/maui#16038
Changes to trimmer defaults in .NET 8 require a new
$(JsonSerializerIsReflectionEnabledByDefault)
MSBuild property to be set by default for project types that use$(TrimMode)=partial
like when running on mobile (iOS/Android).I added a very basic System.Text.Json test, which fails with the exception:
Simply running the test again with the MSBuild property set allows the test to pass. This will be a good smoke test for System.Text.Json going forward.
The iOS side of this is tracked here:
xamarin/xamarin-macios#18057