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

Enabling trim mode breaks program at runtime (v2 only) #3574

Closed
Mersid opened this issue Jun 29, 2024 · 16 comments · Fixed by #3582
Closed

Enabling trim mode breaks program at runtime (v2 only) #3574

Mersid opened this issue Jun 29, 2024 · 16 comments · Fixed by #3582
Labels
Milestone

Comments

@Mersid
Copy link

Mersid commented Jun 29, 2024

Describe the bug
Enabling trim mode when publishing a project using Terminal.Gui will throw this error when run. Disabling trim mode or reverting to v1 resolves this issue:

Unhandled exception. System.InvalidOperationException: Reflection-based serialization has been disabled for this application. Either use the source generator APIs or explicitly configure the 'JsonSerializerOptions.TypeInfoResolver' property.
   at System.Text.Json.ThrowHelper.ThrowInvalidOperationException_JsonSerializerIsReflectionDisabled()
   at System.Text.Json.JsonSerializerOptions.ConfigureForJsonSerializer()
   at System.Text.Json.JsonSerializerOptions.MakeReadOnly(Boolean)
   at System.Text.Json.JsonSerializer.GetTypeInfo(JsonSerializerOptions, Type)
   at System.Text.Json.JsonSerializer.GetTypeInfo[T](JsonSerializerOptions)
   at System.Text.Json.JsonSerializer.Deserialize[TValue](Stream , JsonSerializerOptions )
   at Terminal.Gui.SettingsScope.Update(Stream, String)
   at Terminal.Gui.SettingsScope.UpdateFromResource(Assembly, String)
   at Terminal.Gui.ConfigurationManager.Reset()
   at Terminal.Gui.ConfigurationManager.get_Settings()
   at Terminal.Gui.ConfigurationManager.Load(Boolean )
   at Terminal.Gui.Application.InternalInit(ConsoleDriver , String , Boolean )
   at Terminal.Gui.Application.Init(ConsoleDriver , String )
   at Program.<Main>$(String[]) in C:\Users\Admin\Workshop\FFBatchConverter\FFBatchConverter\Source\Program.cs:line 4

To Reproduce
Steps to reproduce the behavior:

  1. Create a sample project, Something like this will suffice:
Application.Init();

try
{
	Application.Run(new Toplevel());
}
finally
{
	Application.Shutdown();
}
  1. Publish the project with trim enabled.

Expected behavior
The program should work even with trim enabled, as it did for v1.

Screenshots
image

Desktop (please complete the following information):

  • OS: Windows 10
  • Browser N/A
  • Version 23H2, OS build 22631.37374

Additional context
This link recommends setting this flag in the .csproj file:

  <JsonSerializerIsReflectionEnabledByDefault>true</JsonSerializerIsReflectionEnabledByDefault>

as a workaround. This does not work, as it will give this error message at runtime:

Terminal.Gui ConfigurationManager encountered the following errors while deserializing configuration files:
Error deserializing resource://[Terminal.Gui]/Terminal.Gui.Resources.config.json: Unknown property name "$schema".
@dodexahedron
Copy link
Collaborator

dodexahedron commented Jun 29, 2024

You would need to disable all reflection-based serializers to be able to trim. Reflection is inherently not compatible with trimming, in the majority of cases.

The reason that appears to disagree with that doc is because that doc is making an assumption that isn't valid, here - that the required metadata is being rooted in some other way, so that it's still there to resolve and use. We don't do that, either, so setting it to true is apt to make problems worse at the moment. The only option with the current v2_develop code is to not trim.

However, we do know, right now, that V2 is not yet trim-friendly (reflection being the primary culprit). It will be, before release.

We have not started dedicated work to do that, yet, but I think everyone is aware of the need to avoid obvious trim incompatibilities for new work, so the problem shouldn't be getting worse, at least. 😅

@Mersid
Copy link
Author

Mersid commented Jun 29, 2024

Hello @dodexahedron! That's good to hear! Is "disabling all reflection-based serializers" something one could reasonably do now (that is, it's exposed via an API) or will it require digging through the guts of the project?

And regarding resolving this, is the plan to move more towards .NET's source generators, or is it headed another direction?

@dodexahedron
Copy link
Collaborator

I don't think so - at least not in a way that will make TG work with trim outside of non-View types, which of course isn't all that helpful.

There are other non-serializer uses of reflection-related functionality, so it's just not really anything we can give much support to at all, until we get to addressing reflection and trim compatibility, specifically.

@dodexahedron
Copy link
Collaborator

dodexahedron commented Jun 29, 2024

Now... If you did want to dig around and fix it all, we certainly wouldn't complain. But I suspect it's going to be a non-trivial task, and likely to come with some collateral damage requiring other work to deal with.

However....

If you are ok with not trimming the TG library itself, and as long as you aren't trying to use AoT, which would require that you do trim it, you can disable trim for just TG in your project.

Publish TG in its full state, reference it, and then add this stuff to your project that references it:

<PropertyGroup>
  <TrimMode>partial</TrimMode>
</PropertyGroup>
<ItemGroup>
  <TrimmableAssembly Include="YourProgram" />
  <TrimmableAssembly Include="Other" />
  <TrimmableAssembly Include="Assemblies" />
  <TrimmableAssembly Include="Except" />
  <TrimmableAssembly Include="TG" />
</ItemGroup>

Worth a shot, anyway. Might still have gaps.

@dodexahedron
Copy link
Collaborator

dodexahedron commented Jun 29, 2024

However, I suspect some of the PInvoke that is still the DllImport form will break it anyway, because the marshaling for that is not trim-compatible. It's IL emitted via the reflection APIs at run-time, internally, so the linker doesn't know what to do about it for AoT especially, as it just looks like dead code and no-ops.

Switching them to the LibraryImport form should fix any of those, though.

@BDisp
Copy link
Collaborator

BDisp commented Jun 29, 2024

This has already been discussed in #3544. It's worth reading to find out what I've already discovered. All that remains is to find an alternative to resolve and read the configuration files. I also advise using trim on all operating systems, especially win-x64, linux-x64 and osx-x64, because I also found other problems with win-x64 related to the WriteConsole method.

@BDisp
Copy link
Collaborator

BDisp commented Jun 29, 2024

With osx-x64:

image

@Mersid
Copy link
Author

Mersid commented Jun 29, 2024

Alright, thanks! Might look into it. I suspect there's no concrete answer yet, but do you know when, roughly, Terminal.Gui v2 will be released officially? Would it be in 2025 or 2026?

@BDisp
Copy link
Collaborator

BDisp commented Jun 29, 2024

Alright, thanks! Might look into it. I suspect there's no concrete answer yet, but do you know when, roughly, Terminal.Gui v2 will be released officially? Would it be in 2025 or 2026?

Unfortunately I'm not able to debug the self-container after the publish to find out what's going on. With the Debug configuration the ConfigurationManager works well. With the Release configuration and publishing the self-container it doesn't work well and I can't even debug it to discover the cause. It seems that there is a bug in the runtime that is not allowing debugging of the release version.
I have no idea when the official version will be released but I know that a beta version will be released before then.

@dodexahedron
Copy link
Collaborator

dodexahedron commented Jun 30, 2024

There are still some usages of things like GetType(), which will cause issues too.

RuntimeType, which is what that actually returns (just up-cast to Type - a critical difference between typeof and GetType()) doesn't exist at runtime in an AoT assembly. It works in non-AoT trimmed assemblies - just not AoT.... And even that isn't a guarantee, IME, though specifics of the entire program consuming it matter, too.

That's one of the nitpicky things we have to be careful about, around Trim-compatibility: What works in Self-Contained, Framework-Dependent, and AoT are mostly overlapping but not identical circles on a venn diagram.

@dodexahedron
Copy link
Collaborator

Alright, thanks! Might look into it. I suspect there's no concrete answer yet, but do you know when, roughly, Terminal.Gui v2 will be released officially? Would it be in 2025 or 2026?

Unfortunately I'm not able to debug the self-container after the publish to find out what's going on. With the Debug configuration the ConfigurationManager works well. With the Release configuration and publishing the self-container it doesn't work well and I can't even debug it to discover the cause. It seems that there is a bug in the runtime that is not allowing debugging of the release version. I have no idea when the official version will be released but I know that a beta version will be released before then.

Did you explicitly tell the Debug config to publish trimmed, too? IIRC, the default if not explicitly specified is for Debug to not trim1, but don't quote me on that.

Footnotes

  1. The reason being that the debug symbols themselves would also be removed, if not configured to not do that, because they're "unreferenced" code.

@BDisp
Copy link
Collaborator

BDisp commented Jun 30, 2024

@dodexahedron if you create a Self-Contained project in Terminal.Gui and manage to generate a single file that can be debugged please let me know. I can only generate a single file with Publish but unfortunately I can't debug it to find out how to get around the fact that the config file settings are not being loaded and if I could debug maybe I could find an alternative to read this configuration. In the Debug and Release configuration I can debug but the config file is being properly read. With the single file the config file is not being properly loaded.

From the error below you can see that it is the Sequence field that is not being properly loaded with the config file configuration, but without being able to debug it is difficult to find a solution.

4>ILLink(0,0): Error IL1012: IL Trimmer has encountered an unexpected error. Please report the issue at https://aka.ms/report-illink
4>Fatal error in IL Linker
4>Unhandled exception. System.InvalidOperationException: Sequence contains no elements
4> at System.Linq.ThrowHelper.ThrowNoElementsException()
4> at System.Linq.Enumerable.Last[TSource](IEnumerable`1 source)
4> at Mono.Linker.MessageOrigin.ToString()
4> at Mono.Linker.MessageContainer.ToMSBuildString()
4> at Mono.Linker.MessageContainer.ToString()

@dodexahedron
Copy link
Collaborator

dodexahedron commented Jul 1, 2024

There's more than just the serialization mode and whatnot that would need to be done for CM to work without reflection dependencies. Many parts of it already naturally support source gen mode, but the default is that both source gen and runtime reflection are emitted by Roslyn at compile-time, which means it still will break, even though there's a valid path.

But even if you made CM fully trim-safe, any other code path in the library that requires use of unreferenced code will still cause a crash until all of them are addressed.

I don't really think it's worth your time to continue trying to debug that, right now, especially since I'll be addressing many of the core problems with it after some other in-progress work is done that I'm getting back to actually working on, today. I've decided to bump my priority for addressing trim problems in light of the recent uptick in people wanting to avail themselves of that, so I might even start on that next, after the build-related stuff.

As a note, though, there are also built-in analyzers that can help to catch some of the trim problems, where they fit the heuristics those analyzers use. I'll turn those on in the process of addressing that stuff and put it in a PR, early on, so everyone will at least get warnings if they add something that trips one of the rules.

@BDisp
Copy link
Collaborator

BDisp commented Jul 1, 2024

I've already managed to attach Self-contained to the debugger using win-x64. However, I cannot attach using linux-x64 and osx-x64 to the debugger, as it gives the following error. If anyone knows how to solve it, I'd be grateful in advance.

image

This is the right place to state this situation.

@BDisp
Copy link
Collaborator

BDisp commented Jul 1, 2024

I've already managed to attach Self-contained to the debugger using win-x64. However, I cannot attach using linux-x64 and osx-x64 to the debugger, as it gives the following error. If anyone knows how to solve it, I'd be grateful in advance.

image

This is the right place to state this situation.

I reported this issue on Visual Studio Developer Community (https://developercommunity.visualstudio.com/t/Failed-to-attach-to-process:-Unknown-Err/10694351).

@dodexahedron
Copy link
Collaborator

That sounds like something I vaguely remember seeing documented actually. But I am not sure and I'm gonna be busy reviewing #3571 and working on finishing up splitting the analyzers off, so probably not gonna try to hunt that up. I might be thinking of something else anyway. 🤷‍♂️

@tig tig added the bug label Jul 3, 2024
@tig tig modified the milestones: V2 Beta, v2 Release Jul 3, 2024
@tig tig closed this as completed in #3582 Jul 12, 2024
tig added a commit that referenced this issue Jul 12, 2024
Fixes #3574. Enabling trim mode breaks program at runtime (v2 only)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

4 participants