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

Fixing nowarn flakiness - warnings config is stored by compound key #9162

Merged
merged 1 commit into from
Aug 24, 2023

Conversation

rokonec
Copy link
Member

@rokonec rokonec commented Aug 24, 2023

Fixes #9133

Context

NoWarn has become flaky in all build configurations and frequently not suppresing given warnings as errors.

During analysis I have found out our LoggingService is storing suppressed warning codes list by key which was weakly hashed with lots of possible collisions in expected input ranges.

When any project finishes its Warning configuration is removed from LogginService warnign configs dictionary, if just finished project have had collision key it removed the "other-guy" data.

Changes Made

Changed to compound key as readonly record struct.

Since we have smallish numbers of warnings during build I do not believe it have measurable perf degradation.

Testing

  • I have manually replied captured failing /bl to verify that there was indeed hash collision for this particular project warnings config key.
  • Existing PR gate
  • Local testing

Notes

Every time junior developer have had come to me with possible root cause "What if it is hash collision" I hushed them with automatic "Oh. please".
But this time it really is hash collision :-)

@rokonec rokonec self-assigned this Aug 24, 2023
Copy link
Contributor

@jeffkl jeffkl left a comment

Choose a reason for hiding this comment

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

🎉

Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

:shipit:

@rokonec rokonec merged commit 415ba40 into dotnet:main Aug 24, 2023
@rokonec rokonec deleted the rokonec/nowarn-flaky branch August 24, 2023 13:49
@kasperk81
Copy link
Contributor

is it too late to backport this to .net 8?

@kasperk81
Copy link
Contributor

nvm it made it to .net 8 dotnet/sdk#34908

@rainersigwald
Copy link
Member

@kasperk81 yeah, we'll be in .NET 8 for a while yet, until we branch for 17.9. As a "tooling" repo that ships to Visual Studio and .NET we have slightly different timelines from dotnet/runtime and other just-.NET repos.

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.

[Bug]: NoWarn randomly get ignored in build
6 participants