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.

Sign up for GitHub

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

Jump to bottom

MAUI sdk should default the JsonSerializerIsReflectionEnabledByDefault property to true #16038

Closed
eiriktsarpalis opened this issue Jul 7, 2023 · 5 comments · Fixed by dotnet/android#8266
Closed

MAUI sdk should default the JsonSerializerIsReflectionEnabledByDefault property to true #16038

eiriktsarpalis opened this issue Jul 7, 2023 · 5 comments · Fixed by dotnet/android#8266
Assignees
@jonathanpeppers
Labels
external partner/android Issues for the Android SDK partner/macios Issues for the Mac / iOS SDK t/bug Something isn't working
Milestone
.NET 8 GA

Comments

@eiriktsarpalis
Copy link
Member

We are about to merge dotnet/runtime#88480 which once active will turn off System.Text.Json reflection-based serialization for projects with PublishTrimmed=true. Projects that use both trimming and reflection serialization will need to explicitly set the JsonSerializerIsReflectionEnabledByDefault property to true. For reference, here's the PR enabling this for blazor wasm targets.
@akoeplinger
Copy link
Member

I wonder if this should be set at the iOS/Android SDK level rather than MAUI since it'll affect those projects as well.

/cc @Redth

@samhouts samhouts added the t/bug Something isn't working label Jul 31, 2023
@akoeplinger
Copy link
Member

akoeplinger commented Aug 1, 2023 •
edited
Loading

Just noticed that we probably don't want this for the NativeAOT iOS scenario since it should disable reflection there, like for desktop NativeAOT.

@eiriktsarpalis eiriktsarpalis mentioned this issue Aug 11, 2023
Closed
@eiriktsarpalis
Copy link
Member Author

Is this on track for .NET 8 completion? We just received a bug report which seems to suggest that a basic scenario is broken at the moment.

@jonathanpeppers jonathanpeppers self-assigned this Aug 11, 2023
@jonathanpeppers jonathanpeppers added this to the .NET 8 GA milestone Aug 11, 2023
@jonathanpeppers
Copy link
Member

@rolfbjarne can you do the xamarin-macios side of this?

We should also add a runtime/on-device test that does something like System.Text.Json.JsonSerializer.Serialize(42) or a basic .json blob.

@rolfbjarne rolfbjarne mentioned this issue Aug 11, 2023
Closed
@rolfbjarne
Copy link
Member

@rolfbjarne can you do the xamarin-macios side of this?

We should also add a runtime/on-device test that does something like System.Text.Json.JsonSerializer.Serialize(42) or a basic .json blob.

Sure, tracked here: xamarin/xamarin-macios#18057

jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this issue Aug 11, 2023
@jonathanpeppers
[Xamarin.Android.Build.Tasks] fix System.Text.Json with default trimm…
658bb80 
…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
@jonathanpeppers jonathanpeppers mentioned this issue Aug 11, 2023
Merged
@samhouts samhouts added external partner/macios Issues for the Mac / iOS SDK partner/android Issues for the Android SDK labels Aug 14, 2023
jonathanpeppers added a commit to dotnet/android that referenced this issue Aug 14, 2023
@jonathanpeppers
[Xamarin.Android.Build.Tasks] fix System.Text.Json with default trimm…
05f79c0 
…er settings (#8266)

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
@ghost ghost locked as resolved and limited conversation to collaborators Sep 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@jonathanpeppers jonathanpeppers

Labels
external partner/android Issues for the Android SDK partner/macios Issues for the Mac / iOS SDK t/bug Something isn't working
Projects
None yet
Milestone
.NET 8 GA
5 participants
@rolfbjarne @jonathanpeppers @akoeplinger @eiriktsarpalis @samhouts