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

ILLInk fix the usage of treatWarningsAsErrors #45134

Open
SimaTian opened this issue Nov 26, 2024 · 6 comments
Open

ILLInk fix the usage of treatWarningsAsErrors #45134

SimaTian opened this issue Nov 26, 2024 · 6 comments
Labels
Area-ILLink untriaged Request triage from a team member

Comments

@SimaTian
Copy link
Contributor

Describe the bug

The PR 10942 in msbuild has changed the way WarningsAsErrors are treated. Unfortunately this is incompatible with the way ILLink currenctly uses them.
We need to decide how to handle this case since the MSBuild team wants this change for the sake of consistency even though it is more breaking than expected.

To Reproduce

Enable the ILLink_can_treat_warnings_not_as_errors, ILLink_can_treat_warnings_as_errors tests within the GivenThatWeWantToRunILLink.cs test file.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-ILLink untriaged Request triage from a team member labels Nov 26, 2024
Copy link
Contributor

@dotnet/illink a new issue has been filed in the ILLink area, please triage

@vitek-karas
Copy link
Member

Could you please describe in more details what changed and what part is incompatible with the ILLink handling? The PR on MSBuild is not saying the details either. Maybe there's some doc which I didn't find?

@SimaTian
Copy link
Contributor Author

SimaTian commented Nov 26, 2024

We've moved the TreatWarningsAsErrors etc. to be handled via MSBuild engine directly. Not yet merged into the sdk, the PR for the update is here: .
ILLink currently uses the fact that in some cases TreatWarningsAsErrors was ignored(or more precisely not propagated) to overload the property since this PR I think.
This will cause some issues since the property /p:WarningsAsErrors=IL2075 will now be recognized by MSBuild and it will result in an error instead of warning (which was what happened up until now)

Before this change, this was an internal inconsistency within build that we were partially solving elsewhere.

@jtschuster
Copy link
Member

I'm not sure I understand. Should the trimmer produce the warnings as warnings regardless of TreatWarningsAsErrors and WarningsAsErrors and MSBuild will report them as errors / silence them?

@vitek-karas
Copy link
Member

Should the trimmer produce the warnings as warnings regardless of TreatWarningsAsErrors and WarningsAsErrors and MSBuild will report them as errors / silence them?

Even if that's the case, how would ILLink task know what to do - would it have to act differently based on the MSBuild version? Do we have cases like that, where tasks change their behavior based on MSBuild version?

Side note - the ILLink task behavior has been modeled by the Roslyn's behavior in this area, are we making changes to Roslyn or the Csc task related to this? Would be interesting how that was done.

@vitek-karas
Copy link
Member

Also - ilc has some handling related to this as well. It's possible we don't have direct test coverage though - so as part of this we should also make sure that ilc works as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-ILLink untriaged Request triage from a team member
Projects
None yet
Development

No branches or pull requests

3 participants