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

Annotate Serilog.Settings.Configuration as not trim-compatible #371

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

agocke
Copy link

@agocke agocke commented Mar 31, 2023

Unfortunately I don't have any good news for this library. It looks like the fundamental concept, loading arbitrary types by name from arbitrary assemblies, configured by the user at run-time, is fundamentally incompatible with trimming and AOT.

This at least marks it as such. Supporting a feature set like the one provided here would likely require a source generator, or build-time code generation phase.

Unfortunately I don't have any good news for this library. It looks like the fundamental
concept, loading arbitrary types by name from arbitrary assemblies, configured by the user
at run-time, is fundamentally incompatible with trimming and AOT.

This at least marks it as such. Supporting a feature set like the one provided
here would likely require a source generator, or build-time code generation phase.
@agocke
Copy link
Author

agocke commented Apr 5, 2023

PolySharp is currently missing RequiresAssemblyFiles (Sergio0694/PolySharp#65) but the author is very responsive, so I'll wait for them to add it and then I'll pull in a new version.

@agocke
Copy link
Author

agocke commented Apr 5, 2023

I should also mention -- it's worth considering if this space should have an AOT-compatible solution. I think a source generator would probably be necessary.

<IsTrimmable>true</IsTrimmable>
<EnableAotAnalyzer>true</EnableAotAnalyzer>
<EnableSingleFileAnalyzer>true</EnableSingleFileAnalyzer>
<PolySharpIncludeRuntimeSupportedAttributes>true</PolySharpIncludeRuntimeSupportedAttributes>

Choose a reason for hiding this comment

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

Likely has a negligible impact, but if you want to minimize the size impact of PolySharp as much as possible here you can optionally get it to only polyfill the types you're actually using, instead of all available ones, which is the default. You can do so by adding the following MSBuild property here:

<PolySharpIncludeGeneratedTypes>
  System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute;
  System.Diagnostics.CodeAnalysis.RequiresDynamicCodeAttribute;
  System.Diagnostics.CodeAnalysis.RequiresAssemblyFilesAttribute;
</PolySharpIncludeGeneratedTypes>

@nblumhardt
Copy link
Member

@agocke just thinking about:

I should also mention -- it's worth considering if this space should have an AOT-compatible solution. I think a source generator would probably be necessary.

Would this have much benefit over just relying on the C# configuration API? (The C# API can still pull values from appsettings.json directly, where parameters need to vary at deployment time..)

@agocke
Copy link
Author

agocke commented May 5, 2023

This is a good point -- I was thinking that theoretically you could use a source generator to parse the file, and then construct an equivalent graph at compile time. But that would create a potentially confusing situation where people may believe they can change the file after deployment and get different behavior -- and they can't. Making this a code-only configuration system seems like the right idea.

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.

None yet

5 participants