Skip to content

Warn by default - #1372

Merged
mateoatr merged 3 commits into
dotnet:masterfrom
mateoatr:alwayswarn
Aug 4, 2020
Merged

Warn by default#1372
mateoatr merged 3 commits into
dotnet:masterfrom
mateoatr:alwayswarn

Conversation

@mateoatr

Copy link
Copy Markdown
Contributor

Log warnings and errors regardless of the --verbose option.

Dependent upon dotnet/runtime#39133.

@mateoatr
mateoatr requested a review from marek-safar as a code owner July 21, 2020 04:19
@mateoatr mateoatr self-assigned this Jul 21, 2020

@vitek-karas vitek-karas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you please add tests validating the behavior of the various messages? It might need some infra work though.

@sbomer sbomer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM if you add a test for the default behavior

@sbomer

sbomer commented Jul 30, 2020

Copy link
Copy Markdown
Member

@layomia added bulk suppressions for warnings in runtime (dotnet/runtime#40106), so I think this is unblocked.

@sbomer

sbomer commented Jul 31, 2020

Copy link
Copy Markdown
Member

For testing, maybe it would be enough to remove --verbose from the default test options: https://github.com/mono/linker/blob/7c9b806037e88df7eb40a8151808730133676668/test/Mono.Linker.Tests/TestCasesRunner/TestCaseMetadaProvider.cs#L90 and add it explicitly for any tests that check informational messages?

@mateoatr

Copy link
Copy Markdown
Contributor Author

For testing, maybe it would be enough to remove --verbose from the default test options:

https://github.com/mono/linker/blob/7c9b806037e88df7eb40a8151808730133676668/test/Mono.Linker.Tests/TestCasesRunner/TestCaseMetadaProvider.cs#L90

and add it explicitly for any tests that check informational messages?

Yes, I think this should be enough, although I think this could impact #1385. @vitek-karas should I wait until the warnings cleanup is merged?

Comment thread src/linker/Linker.Steps/BlacklistStep.cs
@mateoatr
mateoatr merged commit a3d706a into dotnet:master Aug 4, 2020
tkapin pushed a commit to tkapin/runtime that referenced this pull request Jan 31, 2023
* Always log warnings and errors

Commit migrated from dotnet/linker@a3d706a
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.

5 participants