Skip to content

HasLoggedError Respects MSBuildWarningsAsErrors#5957

Closed
benvillalobos wants to merge 5 commits intodotnet:masterfrom
benvillalobos:hasloggedwarning-respect-warningsaserrors
Closed

HasLoggedError Respects MSBuildWarningsAsErrors#5957
benvillalobos wants to merge 5 commits intodotnet:masterfrom
benvillalobos:hasloggedwarning-respect-warningsaserrors

Conversation

@benvillalobos
Copy link
Copy Markdown
Member

@benvillalobos benvillalobos commented Dec 10, 2020 •
edited
Loading

Fixes #5511

Context

Custom tasks have a Log.HasLoggedErrors property that does not respect thrown warnings that are listed under MSBuildWarningsAsErrors.

The Solution

Before logging the warning, check if the warning code is listed under MSBuildWarningsAsErrors. If so, log as an error instead.

Notes

Some comments I wrote as I was investigating this, in all its unformatted glory

            // LoggingContext has no idea if LoggingService actually interpreted the warning as an error, and logged it as so.
            // It's a problem because TaskBuilder checks _THIS FILE'S _HASLOGGEDERRORS_ boolean to see if it logged errors.
            // But loggingservice does not expose this info!

            //doesn't work if the submission had already logged an error. unless we count the number of logged errors.

            // Another problem is you need the warning code. Okay we can get that from resourceutilities.

            // The fix: Have LoggingContext see if what we were about to log as an warning is actually an error.
            // Prev code path: LogWarning -> _loggingService.LogWarning -> LoggingService.RouteBuildEvent "Oh should we ACTUALLY log it as an error? -> Replace with error args.
            // New code path: LogWarning "oh wait actually log it as an error" -> _loggingService.LogWarning -> RouteBuildEvent -> LoggingService.RouteBuildEvent -> log.

To-Do

  • See if this works locally
  • Tests

@benvillalobos
Copy link
Copy Markdown
Member Author

benvillalobos commented Dec 23, 2020

Note to self: Look over warningsaserror tests and figure out when the collections I'm checking aren't null.

It looks like LoggingContext should be able to determine on its own that a warning its about to log should actually be an error.

@benvillalobos benvillalobos changed the title HasLoggedError Respects MSBuildWarningsAsErorrs HasLoggedError Respects MSBuildWarningsAsErors Jan 7, 2021
@benvillalobos benvillalobos force-pushed the hasloggedwarning-respect-warningsaserrors branch from 270c9f3 to d1acd05 Compare January 11, 2021 20:18
@benvillalobos benvillalobos changed the title HasLoggedError Respects MSBuildWarningsAsErors HasLoggedError Respects MSBuildWarningsAsErrors Jan 12, 2021
@benvillalobos
Copy link
Copy Markdown
Member Author

benvillalobos commented Jan 12, 2021

Correctly fixing this is proving to be quite difficult.

The warnings in the repro are logging from a Task, which logs to a TaskLoggingHelper, which logs a warning through a BuildEngine. The BuildEngine (through TaskHost.cs) tells a loggingcontext's LoggingService that it's logged a warning. The problem is a TaskLoggingHelper has no idea that when it logs a warning it's actually being logged as an error.

Funny thing is this problem also exists for LoggingContext. I solved that by exposing the ShouldLogWarningAsError method through the ILoggingService interface. Once again, I can't easily see a way to do this for a TaskLoggingHelper.

Thinking out loud: could we create an IBuildEngine8 that had an event called LoggedError, that a tasklogginghelper can subscribe to and set its own HasLoggedError to true?

Other questions I've thought up while investigating this:
LoggingService is used by many logging contexts, but is this one loggingservice per project? per build?

Have Tasks never correctly respected Log.HasLoggedError when we throw a warning and that warning is included in MSBuildWarningsAsErrors?

Note to self: see the PR that introduced it: #1928

@rainersigwald
Copy link
Copy Markdown
Member

rainersigwald commented Jan 12, 2021

The PR that introduced warning-as-error is #1355--#1928 extended it to allow the project level properties.

Since that's done in the logging infrastructure rather than at the point of logging, I think that's the problem. Unfortunately I don't know if there's an easy way to move it. Can you investigate that angle? Is the warning-as-errors stuff available in TaskHost and if not how hard would it be to get it there?

I don't think we should attack the problem for TaskLoggingHelper alone--if you attack it at the IBuildEngine API layer, it'll work for everything, not just tasks that use the helper classes.

@rainersigwald rainersigwald marked this pull request as ready for review January 12, 2021 16:39
@rainersigwald rainersigwald marked this pull request as draft January 12, 2021 16:39
@rainersigwald
Copy link
Copy Markdown
Member

rainersigwald commented Jan 12, 2021

Sorry, undrafted by accident.

@benvillalobos benvillalobos mentioned this pull request Jan 15, 2021
Closed
2 tasks
@benvillalobos
Copy link
Copy Markdown
Member Author

benvillalobos commented Jan 15, 2021

Closing in favor of: #6040

@benvillalobos benvillalobos mentioned this pull request Feb 18, 2021
Merged
2 tasks
@benvillalobos benvillalobos mentioned this pull request Jun 28, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

HasLoggedErrors should respect MSBuildWarningsAsErrors

2 participants

@benvillalobos @rainersigwald