-
Notifications
You must be signed in to change notification settings - Fork 498
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
Enable a Roslyn analyzer for a subset of trim analysis warnings. #12598
base: master
Are you sure you want to change the base?
Enable a Roslyn analyzer for a subset of trim analysis warnings. #12598
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI is failing.
Yeah well not my fault :) lock files are failing as CI has updated .NET version |
It contains trimming warning about reflection usage
Can you explain a bit more this PR? Are we going to see fewer warnings (only important ones)? |
We are actually will be seeing more warning as it shows where we use reflection that is marked as not trim compatible. So this is first step, next step that can be done gradually is to eliminate those warning by removing reflection usage. |
cACK. Wait to merge this until we have a clear picture of how to implement the UI part of Mobile. We might not use anything from the Desktop version and implement everything from scratch for Mobile. Which would make it unnecessary to refactor and avoid Reflection for the sake of AOT in Fluent. |
Agree but this PR is also useful for backend part which also uses reflection and it affects the mobile and desktop targets. Something to keep in mind while evaluating this PR |
@@ -12,7 +12,7 @@ jobs: | |||
- task: UseDotNet@2 | |||
displayName: 'Install .NET' | |||
inputs: | |||
version: 8.0.x | |||
useGlobalJson: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nice.
However, I wonder what it leads to in practice. If I understand correctly, it can happen that, for example, we would test against https://github.com/dotnet/runtime/releases/tag/v8.0.2 and not against 8.0.3 because 8.0.3 can be on a newer SDK (e.g. 8.0.300).
Is it how it works or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -10,6 +10,7 @@ | |||
<PathMap>$(MSBuildProjectDirectory)\=WalletWasabi.Fluent.Generators</PathMap> | |||
<IsRoslynComponent>true</IsRoslynComponent> | |||
<EnforceExtendedAnalyzerRules>true</EnforceExtendedAnalyzerRules> | |||
<EnableTrimAnalyzer>false</EnableTrimAnalyzer> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? Directory.Build.props
contains this line.
I would say that the warnings can be disabled for the test project and for the backend project. |
Thie PR sets PublishTrimmed to enable a Roslyn analyzer that shows a limited set of analysis warnings.
This will make easier to diagnose issues with code that uses Reflection and future proof code for Native AOT compatibility.
https://learn.microsoft.com/en-us/dotnet/core/deploying/trimming/trimming-options?pivots=dotnet-8-0#roslyn-analyzer
Related #11625