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

Enable analyzers for mono/netcore #34052

Merged
merged 2 commits into from
Mar 25, 2020

Conversation

stephentoub
Copy link
Member

cc: @akoeplinger, @marek-safar

This is mostly just running auto-fixers for the various violated rules. In just a few places, I manually intervened.

@stephentoub stephentoub force-pushed the enablemonoanalyzers branch from ce71b37 to 3dc2ad8 Compare March 25, 2020 03:26
@danmoseley
Copy link
Member

src\libraries\Common\src\Interop\Windows\User32\Interop.GetProcessWindowStation.cs(12,9): error BCL0015: (NETCORE_ENGINEERING_TELEMETRY=Build) user32.dll!GetProcessWindowStation is not supported on one\more targeted platforms

I get the same locally.

I pushed a change to disable it pending #33867

Copy link
Member

@akoeplinger akoeplinger left a comment

Choose a reason for hiding this comment

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

LGTM with one question.

@akoeplinger
Copy link
Member

The test failure is the known issue: #34045

@akoeplinger akoeplinger merged commit 7302fa4 into dotnet:master Mar 25, 2020
@akoeplinger
Copy link
Member

@stephentoub by the way how did you run these auto-fixers?

@stephentoub stephentoub deleted the enablemonoanalyzers branch March 25, 2020 12:38
@stephentoub
Copy link
Member Author

@stephentoub by the way how did you run these auto-fixers?

Opened the solution in VS, and used Build | Run Code Analysis on Solution to force all of the analyzers to run promptly. Then for each category, selected one of the warnings from the error list, triggered its fixer from the lightbulb (ctrl-period), and selected Solution when determining the scope to which to apply it. Repeated that for each kind of warning. Then also hacked the .csproj with manually specifying TargetsLinux/OSX/Windows and repeated the process, to cover files only in specific builds.

On that note, how do I build mono's corelib for Linux and macOS? I was able to specify a property on the command-line, and it output the assembly into the relevant location, but it appeared to still be building the Windows source code.

@akoeplinger
Copy link
Member

On that note, how do I build mono's corelib for Linux and macOS? I was able to specify a property on the command-line, and it output the assembly into the relevant location, but it appeared to still be building the Windows source code.

As far as I know everything should use the TargetOS msbuild property, same as CoreCLR. What property did you pass?

@stephentoub
Copy link
Member Author

As far as I know everything should use the TargetOS msbuild property, same as CoreCLR. What property did you pass?

That appears to work. No idea what I was doing differently before :)

@danmoseley
Copy link
Member

triggered its fixer from the lightbulb (ctrl-period), and selected Solution when determining the scope to which to apply it. Repeated that for each kind of warning.

From a code review point of view, consider making a commit after each fixer. That way I can do only brief review of eg "remove unused usings" and focus on other stuff.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
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.

5 participants