-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Fix handling of repo analyzers and warnings-as-errors #42272
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.
Thanks!
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.
Besides my one question this looks great. Thanks a lot
7503ecb
to
9d45cba
Compare
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.
Thanks for doing this! I love getting to zero warnings!
I would suggest triggering the
runtime-coreclr outerloop
job to see what the coreclr Pri-1 test build looks like.
Looks like the coreclr test wrappers are failing to build now
src/mono/wasm/debugger/BrowserDebugHost/BrowserDebugHost.csproj
Outdated
Show resolved
Hide resolved
src/mono/wasm/debugger/BrowserDebugProxy/BrowserDebugProxy.csproj
Outdated
Show resolved
Hide resolved
9d45cba
to
dba6621
Compare
When we brought in the new SDK, it enabled analyzers by default (which then used our custom ruleset), but a bunch of projects (in particular tests) weren't expecting that, such that we now have thousands of warnings in the repo. This opts-out those projects. It also enables warnings-as-errors at the root level of the repo, to hopefully avoid such warning storms in the future, and to also clean up the remaining that exist. This includes a bunch of new obsoletion and platform compat warnings that are firing in the runtime tests. We may choose to run analyzers on additional projects in the future where it's currently disabled, but this gets us back to a state at least as good if not better than we were previously.
Fixes the warnings that were triggered by our rule set applying to this project. All fixes were automated.
dba6621
to
97e38d8
Compare
/azp run runtime-coreclr outerloop |
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
@BruceForstall, how do I trigger your recommended leg? |
Thanks, @jkotas. And it looks like you kicked it off; cool. |
Failures are outerloop coreclr test failures rather than build failures due to warnings-as-errors. Official build also passed. |
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.
LGTM
Fixes #42184
cc: @jkotas, @ViktorHofer, @BruceForstall