Skip to content

Add support for analyzers from ref-packs #18501

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

Merged
merged 2 commits into from
Jun 29, 2021

Conversation

ericstj
Copy link
Member

@ericstj ericstj commented Jun 24, 2021

@ericstj ericstj requested a review from dsplaisted June 24, 2021 06:13
@ghost
Copy link

ghost commented Jun 24, 2021

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

Also add analyzers to conflict resolution.
@ericstj
Copy link
Member Author

ericstj commented Jun 24, 2021

@dsplaisted @wli3 can one of you have a look at this and give some guidance around what might be missing? I did what I think might be the minimum to enable the scenario but I expect to need to do more :)

Copy link
Member

@dsplaisted dsplaisted left a comment

Choose a reason for hiding this comment

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

Looks good. Is there any way to have test coverage of analyzer conflict resolution?

@ericstj
Copy link
Member Author

ericstj commented Jun 25, 2021

Did you have a suggestion for where to look for such tests, @dsplaisted? I saw tests of the conflict resolver itself, but nothing that tested the task level where this logic lives. I might be missing something though.

@ericstj
Copy link
Member Author

ericstj commented Jun 25, 2021

Copy link
Member

@safern safern left a comment

Choose a reason for hiding this comment

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

Looks good.

@ericstj ericstj marked this pull request as ready for review June 25, 2021 20:25
@ericstj
Copy link
Member Author

ericstj commented Jun 25, 2021

I added a test that forced inbox and OOB SDK analyzers to be used. I made it fail locally by disabling the conflict-resolution change in targets:

C:\src\dotnet\sdk\artifacts\bin\redist\Debug\dotnet\sdk\6.0.100-dev\Sdks\Microsoft.NET.Sdk\analyzers\build\Microsoft.CodeAnalysis.NetAnalyzers.targets(580,5): warning : The .NET SDK has newer analyzers with version '6.0.0' than what version '5.0.3' of 'Microsoft.CodeAnalysis.NetAnalyzers' package provides. Update or remove this package reference. [C:\src\dotnet\sdk\artifacts\tmp\Debug\AnalyzersAreC---4DE360D4\AnalyzersAreConflictResolved\AnalyzersAreConflictResolved.csproj]
CSC : error CS8034: Unable to load Analyzer assembly C:\src\dotnet\sdk\artifacts\.nuget\packages\microsoft.codeanalysis.netanalyzers\5.0.3\analyzers\dotnet\cs\Microsoft.CodeAnalysis.CSharp.NetAnalyzers.dll : Assembly with same name is already loaded [C:\src\dotnet\sdk\artifacts\tmp\Debug\AnalyzersAreC---4DE360D4\AnalyzersAreConflictResolved\AnalyzersAreConflictResolved.csproj]
CSC : error CS8034: Unable to load Analyzer assembly C:\src\dotnet\sdk\artifacts\.nuget\packages\microsoft.codeanalysis.netanalyzers\5.0.3\analyzers\dotnet\cs\Microsoft.CodeAnalysis.NetAnalyzers.dll : Assembly with same name is already loaded [C:\src\dotnet\sdk\artifacts\tmp\Debug\AnalyzersAreC---4DE360D4\AnalyzersAreConflictResolved\AnalyzersAreConflictResolved.csproj]

Then put my change back in place and it succeeds.

I think this is ready to go.

@ericstj
Copy link
Member Author

ericstj commented Jun 29, 2021

Tested this with the work I have in PR in arcade/runtime and it's working as expected. Thanks folks!

@ericstj ericstj merged commit 07768a0 into dotnet:main Jun 29, 2021
v-wuzhai pushed a commit that referenced this pull request Apr 22, 2024
… log (#18501)

Noticed in dotnet/installer#18409 that errors aren't propagated which resulted in builds not to fail.

The fix here is to use `OnError` in the correct target (but only when using the minimal console log feature) and not set `IgnoreStandardErrorWarningFormat=true` on the Exec task that invokes the repo build script.

I also cleaned targets up as those introduced unnecessary complexity and logged in cases when they shouldn't (i.e. in dotnet.proj or package-source-build.proj).

This affected runtime which errors after 30s of building because of RuntimeOS and BaseOS being passed in in-correctly. Regressed with dotnet/installer@a265140#diff-86602308e6bb519266bc2f224ea65e39589d273804d40ad0f9c6e0eea2a263dc. Fixed that in runtime.proj. Kudos to Alexander who made the fix. I just copied it in form his CI PR.
ViktorHofer added a commit that referenced this pull request May 7, 2024
… log (#18501)

Noticed in dotnet/installer#18409 that errors aren't propagated which resulted in builds not to fail.

The fix here is to use `OnError` in the correct target (but only when using the minimal console log feature) and not set `IgnoreStandardErrorWarningFormat=true` on the Exec task that invokes the repo build script.

I also cleaned targets up as those introduced unnecessary complexity and logged in cases when they shouldn't (i.e. in dotnet.proj or package-source-build.proj).

This affected runtime which errors after 30s of building because of RuntimeOS and BaseOS being passed in in-correctly. Regressed with dotnet/installer@37ddec7#diff-86602308e6bb519266bc2f224ea65e39589d273804d40ad0f9c6e0eea2a263dc. Fixed that in runtime.proj. Kudos to Alexander who made the fix. I just copied it in form his CI PR.
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.

4 participants