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

Fixes #3109. AOT support with .Net 8. #3638

Merged
merged 17 commits into from
Aug 6, 2024

Conversation

BDisp
Copy link
Collaborator

@BDisp BDisp commented Aug 4, 2024

Fixes

Proposed Changes/Todos

  • Add a native AOT project.
  • Adapt Text.Json to work properly.
  • Add in AutoInitShutdownAttribute attribute the possibility to have a different configuration in the config.json and with the unit tests. This avoid to force the config.json having the same configuration as the unit test.
  • Fix bug with the color formatting which throw an exception for the Red color.

Pull Request checklist:

  • I've named my PR in the form of "Fixes #issue. Terse description."
  • My code follows the style guidelines of Terminal.Gui - if you use Visual Studio, hit CTRL-K-D to automatically reformat your files before committing.
  • My code follows the Terminal.Gui library design guidelines
  • I ran dotnet test before commit
  • I have made corresponding changes to the API documentation (using /// style comments)
  • My changes generate no new warnings
  • I have checked my code and corrected any poor grammar or misspellings
  • I conducted basic QA to assure all features are working

@BDisp BDisp requested a review from tig as a code owner August 4, 2024 00:47
Copy link
Collaborator

@tig tig left a comment

Choose a reason for hiding this comment

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

LGTM. I may have broken something with my fixing of merge conflicts though!

@BDisp BDisp marked this pull request as draft August 5, 2024 16:27
@BDisp BDisp marked this pull request as ready for review August 5, 2024 17:41
@BDisp
Copy link
Collaborator Author

BDisp commented Aug 5, 2024

Considerations to take an account when working with Text.Json and native AOT:

  • When creating new enum types or an already existed one, that are used on properties using CM, this statements are mandatory:
using System.Text.Json.Serialization;

namespace Terminal.Gui;

[JsonConverter (typeof (JsonStringEnumConverter<MyEnum>))]
public enum MyEnum
{}
  • Add them into SourceGenerationContext class:
...
[JsonSerializable (typeof (MyEnum))]
  • In all properties that uses these enum types in the CM, this statements are mandatory:
[SerializableConfigurationProperty (Scope = typeof (MyScope))]
public static MyEnum DefaultMyEnum { get; set; } = MyEnum.{Wathever}; // Default is set in config.json
  • Without theses procedures the executable native AOT file, will silently throwing exceptions and showing a default white/black and with all empty configurations.

@tig
Copy link
Collaborator

tig commented Aug 5, 2024

Considerations to take an account when working with Text.Json and native AOT:

  • When creating new enum types or an already existed one, that are used on properties using CM, this statements are mandatory:
using System.Text.Json.Serialization;

namespace Terminal.Gui;

[JsonConverter (typeof (JsonStringEnumConverter<MyEnum>))]
public enum MyEnum
{}
  • Add them into SourceGenerationContext class:
...
[JsonSerializable (typeof (MyEnum))]
  • In all properties that uses these enum types in the CM, this statements are mandatory:
[SerializableConfigurationProperty (Scope = typeof (MyScope))]
public static MyEnum DefaultMyEnum { get; set; } = MyEnum.{Wathever}; // Default is set in config.json
  • Without theses procedures the executable native AOT file, will silently throwing exceptions and showing a default white/black and with all empty configurations.

Could you add a unit test that verifies all these conditions are true for any enum defined in the libarary?

@BDisp
Copy link
Collaborator Author

BDisp commented Aug 5, 2024

Could you add a unit test that verifies all these conditions are true for any enum defined in the libarary?

If you know how to can run unit tests from a executable, of course. The problem is that the unit tests are tested with al the library on the bin/Debug/net8.0 folder, where no exception is throwed and the published executable is a trimmed file with the necessary library embedded. I only can test it when I run the executable file.

@tig
Copy link
Collaborator

tig commented Aug 5, 2024

Could you add a unit test that verifies all these conditions are true for any enum defined in the libarary?

If you know how to can run unit tests from a executable, of course. The problem is that the unit tests are tested with al the library on the bin/Debug/net8.0 folder, where no exception is throwed and the published executable is a trimmed file with the necessary library embedded. I only can test it when I run the executable file.

I must not have made myself clear:

  • You put forth a bunch of "rules above". Those rules require attributing enums with stuff.
  • It is possible to use reflection to examine every enum in Terminal.Gui to see if they have those attributes

It should be possible to build a unit test that verifies that any newly added enums have those attributes.

@BDisp
Copy link
Collaborator Author

BDisp commented Aug 5, 2024

I must not have made myself clear:

  • You put forth a bunch of "rules above". Those rules require attributing enums with stuff.
  • It is possible to use reflection to examine every enum in Terminal.Gui to see if they have those attributes

But these attributes are only needed to be added if they are used on Text.Json.Serialization. Enums that aren't used on Text.Json.Serialization don't need to be added.

It should be possible to build a unit test that verifies that any newly added enums have those attributes.

Only if they are used on Text.Json.Serialization. But if they are added to the both places, SourceGenerationContext as [JsonSerializable (typeof (MyEnum))] and on each property which use them as [JsonConverter (typeof (JsonStringEnumConverter<MyEnum>))], also throws silently an exception when a published executable native AOT file runs.
I really haven't idea how to do that. Can you help?

@BDisp
Copy link
Collaborator Author

BDisp commented Aug 5, 2024

@tig I changed the NativeAot project to ThrowOnJsonErrors. Try to add [JsonConverter (typeof (JsonStringEnumConverter<Alignment>))] to the Dialog.DefaultButtonAlignment, publish and run it from a command line. You'll see the app crashing with an exception. I think this way is better than providing a with/black window with empty configuration.
Here the output:

Unhandled Exception: System.Text.Json.JsonException: Error reading property "Themes" of type "Dictionary`2".
 ---> System.NotSupportedException: 'Terminal.Gui.ScopeJsonConverter`1+ReadHelper`1[Terminal.Gui.ThemeScope,Terminal.Gui.Alignment]' is missing native code or metadata. This can happen for code that is not compatible with trimming or AOT. Inspect and fix trimming and AOT related warnings that were generated when the app was published. For more information see https://aka.ms/nativeaot-compatibility Path: $ | LineNumber: 1 | BytePositionInLine: 46.
 ---> System.NotSupportedException: 'Terminal.Gui.ScopeJsonConverter`1+ReadHelper`1[Terminal.Gui.ThemeScope,Terminal.Gui.Alignment]' is missing native code or metadata. This can happen for code that is not compatible with trimming or AOT. Inspect and fix trimming and AOT related warnings that were generated when the app was published. For more information see https://aka.ms/nativeaot-compatibility
   at System.Reflection.Runtime.General.TypeUnifier.WithVerifiedTypeHandle(RuntimeConstructedGenericTypeInfo, RuntimeTypeInfo[]) + 0x11b
   at System.Reflection.Runtime.General.TypeUnifier.GetConstructedGenericTypeWithTypeHandle(RuntimeTypeInfo, RuntimeTypeInfo[]) + 0x3b
   at System.Reflection.Runtime.TypeInfos.RuntimeTypeInfo.MakeGenericType(Type[]) + 0x3f1
   at Terminal.Gui.ScopeJsonConverter`1.Read(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options) + 0x6df
   at System.Text.Json.Serialization.JsonConverter`1.TryRead(Utf8JsonReader&, Type, JsonSerializerOptions, ReadStack&, T&, Boolean&) + 0x3b4
   at System.Text.Json.Serialization.JsonConverter`1.ReadCore(Utf8JsonReader&, JsonSerializerOptions, ReadStack&) + 0x28b
   --- End of inner exception stack trace ---
   at System.Text.Json.ThrowHelper.ThrowNotSupportedException(ReadStack&, Utf8JsonReader&, NotSupportedException) + 0x2e7
   at System.Text.Json.Serialization.JsonConverter`1.ReadCore(Utf8JsonReader&, JsonSerializerOptions, ReadStack&) + 0x681
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo`1.Deserialize(Utf8JsonReader&, ReadStack&) + 0x61
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo`1.DeserializeAsObject(Utf8JsonReader&, ReadStack&) + 0x2e
   at System.Text.Json.JsonSerializer.ReadAsObject(Utf8JsonReader&, JsonTypeInfo) + 0x185
   at System.Text.Json.JsonSerializer.Deserialize(Utf8JsonReader&, Type, JsonSerializerContext) + 0x70
   at Terminal.Gui.DictionaryJsonConverter`1.Read(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions opti   at Terminal.Gui.ScopeJsonConverter`1.ReadHelper`1.Read(Utf8JsonReader& reader, Type type, JsonSerializerOptions options) + 0x4f
   at Terminal.Gui.ScopeJsonConverter`1.Read(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options) + 0x7e0
   --- End of inner exception stack trace ---
   at Terminal.Gui.ScopeJsonConverter`1.Read(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options) + 0xc19
   at System.Text.Json.Serialization.JsonConverter`1.TryRead(Utf8JsonReader&, Type, JsonSerializerOptions, ReadStack&, T&, Boolean&) + 0x3b4
   at System.Text.Json.Serialization.JsonConverter`1.ReadCore(Utf8JsonReader&, JsonSerializerOptions, ReadStack&) + 0x60a
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo`1.ContinueDeserialize(ReadBufferState&, JsonReaderState&, ReadStack&) + 0x187
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo`1.Deserialize(Stream) + 0x114
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo`1.DeserializeAsObject(Stream) + 0x26
   at System.Text.Json.JsonSerializer.Deserialize(Stream, Type, JsonSerializerOptions) + 0x82
   at Terminal.Gui.SettingsScope.Update(Stream stream, String source) + 0x1a3
   at Terminal.Gui.SettingsScope.UpdateFromResource(Assembly assembly, String resourceName) + 0x319
   at Terminal.Gui.ConfigurationManager.Reset() + 0x1e3
   at Terminal.Gui.ConfigurationManager.get_Settings() + 0x48
   at Terminal.Gui.ConfigurationManager.Load(Boolean reset) + 0x1de
   at Terminal.Gui.Application.InternalInit(ConsoleDriver driver, String driverName, Boolean calledViaRunT) + 0x143
   at Terminal.Gui.Application.Init(ConsoleDriver driver, String driverName) + 0x3f
   at NativeAot.Program.Main(String[] args) + 0x5d
   at NativeAot!<BaseAddress>+0x963dc7
   at NativeAot!<BaseAddress>+0x963e55

@tig
Copy link
Collaborator

tig commented Aug 5, 2024

@tig I changed the NativeAot project to ThrowOnJsonErrors. Try to add [JsonConverter (typeof (JsonStringEnumConverter<Alignment>))] to the Dialog.DefaultButtonAlignment, publish and run it from a command line. You'll see the app crashing with an exception. I think this way is better than providing a with/black window with empty configuration.

So you think it's better to let a developer have to get all the way to publishing the NaviteAot project to discover they missed following instructions than having a test in ./UnitTests that makes it clear as soon as they intoduce an enum and try to test?

@BDisp
Copy link
Collaborator Author

BDisp commented Aug 5, 2024

So you think it's better to let a developer have to get all the way to publishing the NaviteAot project to discover they missed following instructions than having a test in ./UnitTests that makes it clear as soon as they intoduce an enum and try to test?

The user doesn't have to worry about that. Must be us, as maintainer ensuring the NativeAot runs without throwing any exception and that was what I did by fixing the necessary to the project running without any issue. Thus, on every new package we need to publish the NativeAot to check if it run successfully, ensuring that any developer that want to use a native AOT project runs without any issues related with Terminal.Gui. I understand that an unit test will be great but it's not very easy as you think. You need to use compile code generation, publish and check for any throwed exception. But that stuff isn't very easy to do for a newbie :-)

@tig
Copy link
Collaborator

tig commented Aug 5, 2024

So you think it's better to let a developer have to get all the way to publishing the NaviteAot project to discover they missed following instructions than having a test in ./UnitTests that makes it clear as soon as they intoduce an enum and try to test?

The user doesn't have to worry about that. Must be us, as maintainer ensuring the NativeAot runs without throwing any exception and that was what I did by fixing the necessary to the project running without any issue. Thus, on every new package we need to publish the NativeAot to check if it run successfully, ensuring that any developer that want to use a native AOT project runs without any issues related with Terminal.Gui. I understand that an unit test will be great but it's not very easy as you think. You need to use compile code generation, publish and check for any throwed exception. But that stuff isn't very easy to do for a newbie :-)

First, we have a bunch of unit tests that are for US. E.g. all the Disposed stuff. It has saved countless hours of OUR time.

Second, I don't think it's that hard to make a unit test that does what I suggest. No, I'm not going to do it just to prove it.

I guarantee, though, that at some point in the future one of us maintainers is going to forget to properly attribute an enum and the only way we'll find out is because someone ELSE tries to publish a Native AOT app. When that happens that will be OUR fault, for not taking the time to build as much testing in as we can.

@BDisp
Copy link
Collaborator Author

BDisp commented Aug 6, 2024

Well you can try to do yourself because it's beyond of my limits.

@tig
Copy link
Collaborator

tig commented Aug 6, 2024

Well you can try to do yourself because it's beyond of my limits.

I'm sorry you feel that way. It would be great if you would reconsider.

@tig
Copy link
Collaborator

tig commented Aug 6, 2024

image

@BDisp
Copy link
Collaborator Author

BDisp commented Aug 6, 2024

I don't have Copilot but I'll try do it.

@tig
Copy link
Collaborator

tig commented Aug 6, 2024

I could have asked ChatGPT or any other of a dozen free dev AIs.

@BDisp
Copy link
Collaborator Author

BDisp commented Aug 6, 2024

Done @tig.

Explanation:

1. Assembly and Types Inspection:

  • The assembly variable retrieves the assembly containing the SourceGenerationContext class.
  • The code loops through all types in this assembly to gather properties decorated with the SerializableConfigurationProperty attribute.

2. Collecting Property Types:

  • The test collects distinct property types that are decorated with SerializableConfigurationProperty.

3. Get Registered Types:

  • The GetRegisteredTypes method uses reflection to inspect the JsonSerializerContext derived class and find all types registered for serialization.
  • It looks for properties of type JsonTypeInfo<T> to determine the registered types.

4. Assertions for Properties:

  • Ensure each property type is included in the registered types.
  • Verify that no properties have the generic JsonStringEnumConverter<> attribute.

5. Identifying Classes with ScopeJsonConverter<>:

  • The test finds all classes in the assembly that are decorated with the [JsonConverter(typeof(ScopeJsonConverter<>))] attribute.
  • It checks to ensure these classes are also registered in the JsonSerializerContext.

This comprehensive approach ensures that all relevant types are checked and validated, maintaining the robustness and flexibility of the test.

@tig
Copy link
Collaborator

tig commented Aug 6, 2024

Ready to merge?

@BDisp
Copy link
Collaborator Author

BDisp commented Aug 6, 2024

Ready to merge?

Yes. Thanks.

@BDisp BDisp marked this pull request as draft August 6, 2024 15:42
@BDisp
Copy link
Collaborator Author

BDisp commented Aug 6, 2024

@tig I don't know if the test will not fail it another derived from type of JsonConverterAttribute than typeof (ScopeJsonConverter<>) that you can use in the future. Do you want I prevent that situations in this PR?

@tig
Copy link
Collaborator

tig commented Aug 6, 2024

@tig I don't know if the test will not fail it another derived from type of JsonConverterAttribute than typeof (ScopeJsonConverter<>) that you can use in the future. Do you want I prevent that situations in this PR?

Nah. That seems rare enough that we're probably ok until it becomes a problem.

@BDisp BDisp marked this pull request as ready for review August 6, 2024 16:04
@BDisp BDisp marked this pull request as draft August 6, 2024 16:37
@BDisp
Copy link
Collaborator Author

BDisp commented Aug 6, 2024

Sorry @tig al least I learn that is impossible to test from all JsonConverter<> derived classes because we need the types passed in the generic at runtime and not at compile time. So, DictionaryJsonConverter the T type will be only know at run time.
Ready for review, thanks.

@BDisp BDisp marked this pull request as ready for review August 6, 2024 17:49
@tig tig merged commit 63e75b7 into gui-cs:v2_develop Aug 6, 2024
3 checks passed
@BDisp BDisp deleted the v2_3109_native-aot-support branch August 6, 2024 18:07
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.

AOT support with .Net 8?
2 participants