feat: Enable AOT-compatibility#96
Conversation
… by using generic attributes
also return IList instead of IEnumerable everywhere for consistency
|
FYI: I will need a few days for the review. I'm pretty busy this week. |
|
I'll do the review this weekend. |
|
No worries. This currently uses a preview version of xunit aot. Stable release should be soon. |
HofmeisterAn
left a comment
There was a problem hiding this comment.
Thanks, the PR looks good! I just have a few questions so I can understand a few things better.
| } No newline at end of file | ||
| } | ||
|
|
||
| // extended types that are not generated by source generator |
There was a problem hiding this comment.
question: Do you think we can generate these? What about a separate C# generator or validation tool for the API 😬?
There was a problem hiding this comment.
I am currently not sure what the best way is to automate this.
There was a problem hiding this comment.
Yes, difficult... What do you think about a test that uses reflection to check whether the return and parameter types of the API (interface) implementations are part of the context?
There was a problem hiding this comment.
Or maybe we can write an analyser. Never done that from scratch but I can try. We need to now the generic type of MakeRequestAsync.
Not sure if a source generator would work because STJ also uses a source generator.
There was a problem hiding this comment.
We need to now the generic type of MakeRequestAsync.
Yea, that's right. My idea e.g. fails with (PluginInstallParameters vs. IList<PluginPrivilege>):
Docker.DotNet/src/Docker.DotNet/Endpoints/PluginOperations.cs
Lines 43 to 63 in 2f9b40f
Probably we need:
- MakeRequestAsync
- JsonRequestContent
- MonitorStreamForMessagesAsync
But we can also handle that in a follow-up PR if that's more convenient.
| // PluginOperations.ListPluginsAsync | ||
| [JsonSerializable(typeof(Plugin[]))] | ||
| // PluginOperations.GetPrivilegesAsync | ||
| [JsonSerializable(typeof(PluginPrivilege[]))] |
There was a problem hiding this comment.
I have one more topic, but aside from the open discussions, the PR looks good. We can also address some of those in a follow-up PR if you prefer.
PluginPrivilege[] covers the array response, but InstallPluginAsync and UpgradePluginAsync serialize an IList<PluginPrivilege>.
IList<PluginPrivilege> is a different closed type from PluginPrivilege[], so it needs its own JsonSerializable entry in the combined serializer context, right?
I belive the analyzer is a good idea 💡.
I think these are missing too (or we change them to arrays too):
// PluginOperations.InstallPluginAsync, UpgradePluginAsync
[JsonSerializable(typeof(IList<PluginPrivilege>))]
// PluginOperations.ConfigurePluginAsync
[JsonSerializable(typeof(IList<string>))]
// SecretsOperations.ListAsync
[JsonSerializable(typeof(IList<Secret>))]
// TasksOperations.ListAsync
[JsonSerializable(typeof(IList<TaskResponse>))]…zer / new JsonRequestContent


IsAotCompatibleDockerModelsJsonSerializerContextwith all docker API models which have JSON parameters, so query string only models are not added for JsonSerializationDockerExtendedJsonSerializerContextwhich includes types directly used by *Operations (this can be removed / merged with DockerModelsJsonSerializerContext when STJ source generator fails when defining the serializer context in 2 partial classes dotnet/runtime#99669 is available)DynamicallyAccessedMembersattributesxunit.v3.aot.mtp-v24.0.0-pre.33which is still in preview but required to actually test AOT.xunit.v3.mtp-v24.0.0-pre.33for .NET 8.0 because xunit AOT only works from .NET 9 onwards.IEnumerableresponses to beIListfor consistency (breaking change)QueryStringParameterto reduce reflectionfixes #88