[Bug] Microsoft.Identity.Client doesn't support IL Trimming #3407
Assignees
Labels
Comments
|
Excellent bug. Just so happens that we are also exploring this area, as we want to make MSAL compile with NET7's AOT compiler. Here's are the notes that @pmaytak wrote on the subject. Problem statement:
Possible solutions:
One of the challenges in estimating time are the unknowns that need to be researched - what exactly needs to be excluded in JSON.NET and making sure System.Text.Json supports everything that we use from JSON.NET. Unity related work: System.Text.Json framework support: .NET API Catalog (apisof.net) |
|
Hey @Sergio0694, do you happen to be aware of any common pitfalls for the approaches above? |
|
Just want to clarify this:
This is not correct. There's nothing inherently problematic with AOT and reflection. The issue is not using AOT, the issue is the package not using proper trimming annotations to inform the linker when performing trimming. You could publish a non-trimmed fully AOT binary with no code changes, and there would be no problems whatsoever. Just to give an example, we use reflection in several places in the Microsoft Store, and we also use Essentially, removing reflection is of course nice as it helps performance and binary size, but it is not required. A first, low-risk approach should be to just enable trimming for the assembly and add the necessary annotations to eliminate trimming warnings. This should be done regardless of any other changes you might want in the future, as it's just a baseline you should start from. Then, definitely, moving to |
|
@bgavrilMS what's the minimum target framework you're looking to support? According to the support policy here:
As it'd be nice to just switch over to |
|
@Sergio0694 you're right. I think initially that was written to mean as you have to pay attention to reflection-based code when using AOT. @michael-hawker net45 is one of the targets that we still support but not supported by System.Text.Json. And also perhaps some of the older Xamarin targets might not be supported. But we're thinking deprecating support for those, so we should be fine to use System.Text.Json. One thing that's not clear to me yet, is if the source generation is only supported in net6+, or in earlier frameworks via the package also (if so, it would suit us well). |
|
@pmaytak source generators in general w/ .NET need to be built with .NET 6+, but the outputted code doesn't have to be limited to run on .NET 6. We use them in the Toolkit to interop with .NET Standard 2.0 and UWP for instance. According to the intro dev blog here, the
|
About dependencies
About Json operationsApart from the regular serialize / deserialize operations, MSAL uses a more advanced serialization technique when it comes to the token cache. MSAL will preserve unknown JSON nodes (think distributed system, where node A uses MSAL 4.1 and node B uses MSAL 4.2). As such, it might be easier to make changes to existing newtonsoft serializer. Related issue: #3413 |
|
Please also consider that the Newtonsoft.JSON dependency is extremely large. Multiple megabytes, especially when AOT is enabled (which is a must in iOS apps). Would it be possible to re-open this feature request? |
|
CC @pmaytak |
|
Working on using System.Text.Json. Got most of the tests passing again. Sending PR soon. |
|
@michael-hawker - can you please elaborate on this? MSAL is built against .NET 4.6.x, .NET Core App 2.1 and .NETStandard 2.0. The build scripts today I believe use .NET SDK 3.1 To ensure code generators work, what do we need to do? Ensure that the build machines use .NET 6 SDK ? Or add NET 6 target framework to MSAL explicitly? |
|
@bgavrilMS all you should need is for the build environment to install the .NET 6 SDK (for instance with GitHub Actions it'd be): - name: Install .NET 6 SDK
uses: actions/setup-dotnet@v1
with:
dotnet-version: '6.0.301'Though if you're still building for an older target netcore app, you may need to install both SDKs? But you don't have to change the multi-targeting unless you want to make specific optimizations for newer platforms (like .NET 6 wouldn't need the Net Core App 2.1 is out of support now, so it looks like Alex updated to 3.1 in his PR here as well |
|
One of the blockers for us (Git Credential Manager) from dropping .NET Framework on Windows (and going .NET6/7 etc) would be the ability to publish our app trimmed. Since MSAL is a hard dependency for us, this work would be really helpful for us! Would love to publish AOT compiled too in .NET 7 if MSAL can work. |
pmaytak
moved this from Estimated/Committed
to Waiting for Code Review
in MSAL Customer Trust / QM
Repository owner
moved this from Waiting for Code Review
to Fixed
in MSAL Customer Trust / QM
Sep 2, 2022
|
I believe this is still not solved. We publish an ios app with NativeAOT and get these warnings: |
|
@charlesroddie Do you target .NET 8, net8.0-ios? |
|
Yes sorry |
|
Please open a new bug on the library @charlesroddie . We've done the work to move from Newtonsoft to System.Text.Json only for our net6 and net6-windows target, we did not do it for net6-ios / net6-android targets. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
.NET6 added IL Trimming (https://devblogs.microsoft.com/dotnet/announcing-net-6/#il-trimming)
Unfortunately, MSAL.Net doesn't support it.
Which version of MSAL.NET are you using?
Microsoft.Identity.Client 4.44.0
Platform
.NET 6
What authentication flow has the issue?
Is this a new or existing app?
This is a new app or experiment.
Repro
A very simple .NET 6 app:
With:
And built with:
dotnet publish --self-contained -r win-x64 /p:Configuration=Release /p:TrimmerSingleWarn=falseWill produce dozens of warnings/errors, such as:
Expected behavior
No build errors or warnings when building a self-contained exe.
Actual behavior
Multiple errors, and very hard to workaround/fix manually.
Possible solution
Most errors seems related to JSON deserialization, and .NET6 now supports source generation for this, so it should be possible to support this.
The text was updated successfully, but these errors were encountered: