[trimming] TrimMode=full in debug mode, should enable analyzers
#9320
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jonathanpeppers
commented
Sep 24, 2024
Comment on lines
-272
to
273
| [TestCase ("TrimMode=full", new string [0], false)] | ||
| [TestCase ("TrimMode=full", new string [] { "IL2055" }, false, 1)] | ||
| [TestCase ("TrimMode=full", new string [] { "IL2055" }, true, 2)] |
There was a problem hiding this comment.
Note that you will get 2x warnings in Release, one from the trimmer and one from the Roslyn analyzer. This is "by design", and we shouldn't try to hide them or anything.
This comment was marked as outdated.
This comment was marked as outdated.
jonathanpeppers
requested review from
dellis1972 and
jonpryor
as code owners
jonathanpeppers
force-pushed
the
TrimModeFullDebug
branch
from
d62de7d to
a622ac2
Compare
For AOT scenarios, projects would set:
<PublishAot>true</PublishAot>
For both `Debug` and `Release`, this allows you to get the same set of
analyzers in both configurations. You'd want to see the same warnings
for both.
For Android, the project template is currently doing:
<PropertyGroup Condition="'$(Configuration)' == 'Release'">
<TrimMode>Full</TrimMode>
</PropertyGroup>
But this would result in a *different* set of warnings between `Debug`
and `Release` mode!
Instead, we can do:
<PropertyGroup>
<TrimMode>Full</TrimMode>
</PropertyGroup>
And then add a new default, such as:
<EnableTrimAnalyzer Condition="'$(EnableTrimAnalyzer)' == '' and '$(TrimMode)' == 'full'">true</EnableTrimAnalyzer>
`TrimMode=Full` does *not* enable the trimmer, so other defaults in
`Debug` should remain unchanged.
So, the new behavior is:
* `Configuration=Debug`
* `PublishTrimmed=false` (default, no trimmer)
* `TrimMode=Full` (project template)
* `EnableTrimAnalyzer=true` (new default)
* You get the same warnings in `Debug` and `Release` mode.
I also reworded the commend in the project template slightly, to
mention it enables analyzers.
jonathanpeppers
force-pushed
the
TrimModeFullDebug
branch
from
a622ac2 to
5ff483a
Compare
|
Draft commit message: For AOT scenarios, projects would set:
<PublishAot>true</PublishAot>
For both `Debug` and `Release`, this allows you to get the same set
of analyzers in both configurations. You'd want to see the same
warnings for both.
For Android, the project template is currently doing:
<PropertyGroup Condition="'$(Configuration)' == 'Release'">
<TrimMode>Full</TrimMode>
</PropertyGroup>
but this would result in a *different* set of warnings between
`Debug` and `Release` configuration builds!
Instead, we can do:
<PropertyGroup>
<TrimMode>Full</TrimMode>
</PropertyGroup>
And then add a new default, such as:
<EnableTrimAnalyzer
Condition=" '$(EnableTrimAnalyzer)' == '' and '$(TrimMode)' == 'full' "
>true</EnableTrimAnalyzer>
`$(TrimMode)=Full` does *not* enable the trimmer, so other defaults
in the `Debug` configuration build should remain unchanged.
So, the new behavior is:
* `$(Configuration)=Debug`
* `$(PublishTrimmed)=false` (default, no trimmer)
* `$(TrimMode)=Full` (project template)
* `$(EnableTrimAnalyzer)=true` (new default)
* You get the same set of warnings in `Debug` and `Release`
configuration builds.
I also reworded the comment in the project template slightly, to
mention it enables analyzers.@jonathanpeppers : one question about that commit message: you say:
"new behavior" implies a change in behavior. The default |
|
I think I just missed the word “for” like: For Configuration=Debug |
dellis1972
approved these changes
Sep 26, 2024
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For NativeAOT scenarios, projects would set:
For both
DebugandRelease, this allows you to get the same set of analyzers in both configurations. You'd want to see the same warnings for both.For Android, the project template is currently doing:
But this would result in a different set of warnings between
DebugandReleasemode!Instead, we can do:
And then add a new default, such as:
TrimMode=Fulldoes not enable the trimmer, so other defaults inDebugshould remain unchanged.So, the new behavior is:
Configuration=DebugPublishTrimmed=false(default, no trimmer)TrimMode=Full(project template)EnableTrimAnalyzer=true(new default)DebugandReleasemode.I also reworded the commend in the project template slightly, to mention it enables analyzers.