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

Condition EnableSingleFileAnalyzer #91474

Closed
wants to merge 2 commits into from

Conversation

agocke
Copy link
Member

@agocke agocke commented Sep 1, 2023

With the latest SDK, a warning is generated if the analyzer is enabled on an unsupported SDK.

@ghost
Copy link

ghost commented Sep 1, 2023

Tagging subscribers to this area: @dotnet/area-meta
See info in area-owners.md if you want to be subscribed.

Issue Details

With the latest SDK, a warning is generated if the analyzer is enabled on an unsupported SDK.

Author: agocke
Assignees: -
Labels:

area-Meta

Milestone: -

@agocke agocke requested a review from sbomer September 1, 2023 18:00
@sbomer
Copy link
Member

sbomer commented Sep 1, 2023

How did you encounter this? I'm wondering why it wasn't hit in #90880.

With the latest SDK, a warning is generated if the analyzer is enabled
on an unsupported SDK.
@agocke agocke force-pushed the singlefile-analyzer branch from e39f727 to 73ea692 Compare September 1, 2023 18:47
@@ -14,7 +14,7 @@
<PropertyGroup Condition="'$(RunAnalyzers)' != 'false'">
<EnableSingleFileAnalyzer Condition="
'$(EnableSingleFileAnalyzer)' == '' And
'$(TargetFrameworkIdentifier)' == '.NETCoreApp'">true</EnableSingleFileAnalyzer>
$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net6.0'))">true</EnableSingleFileAnalyzer>
Copy link
Member

@ViktorHofer ViktorHofer Sep 1, 2023

Choose a reason for hiding this comment

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

I don't understand this change. dotnet/runtime main only builds net8.0 and net9.0 now. Why do we need this .NETCoreApp check?

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you sure? I see a lot in Microsoft.Extensions that has netstandard2.0

Copy link
Member

Choose a reason for hiding this comment

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

I'd have expected the '$(TargetFrameworkIdentifier)' == '.NETCoreApp' condition to take care of netstandard TFMs.

Copy link
Member

@ViktorHofer ViktorHofer Sep 2, 2023

Choose a reason for hiding this comment

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

Exactly. The TFI == .NETCoreApp check should already only apply to .NETCoreApp TFMs.

Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

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

Please respond to my question before merging as I don't think this is right.

@ghost ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Sep 1, 2023
@agocke
Copy link
Member Author

agocke commented Sep 25, 2023

Hmm, can't repro this anymore... I wonder if it was a stale interaction.

@agocke agocke closed this Sep 25, 2023
@agocke agocke deleted the singlefile-analyzer branch September 25, 2023 21:49
@ghost ghost locked as resolved and limited conversation to collaborators Oct 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants