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

Tasks Log.HasLoggedError now respects MSBuildWarningsAsErrors #6174

Merged
merged 32 commits into from
Mar 13, 2021

Conversation

benvillalobos
Copy link
Member

@benvillalobos benvillalobos commented Feb 18, 2021

Fixes #5511

Context

Tasks have a Log.HasLoggedErrors property that does not respect warnings that were thrown as errors when listed under MSBuildWarningsAsErrors.

Changes Made

  • IBuildEngine8 has a hashset of warnings to be converted into errors.
  • ILoggingService has a method to extract warnings to be logged as errors based on the build context.
  • TaskHostConfiguration had warningsaserrors hashset added to it so that the OOPTaskHostNode can set up properly.

Testing

  • TaskReturnsTrueButLogsError_BuildShouldContinue
  • WarningsAsErrors_ExpectBuildToStopWhenTaskLogsWarningAsError
  • TestTranslationWithWarningsAsErrors
  • Confirmed this fixes the repro in the linked issue.
  • Need to test batched builds, as a TaskHost is generated per 'bucket'

Notes

Comment from Rainer in previous PR about this: #5957

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.

@benvillalobos benvillalobos marked this pull request as draft February 18, 2021 21:49
Copy link
Member

@Forgind Forgind left a comment

Choose a reason for hiding this comment

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

This looks like it should work, but it's also kinda a heavy solution. Is there something simpler? Alternatively, do you think it would make sense to add a Dictionary<string, string> with dict["warningsAsErrors"] == what WarningsAsErrors is here (post-serialization). The reason that might make sense is that it would be good to avoid having too many new IBuildEngines, and that would mean that, for any future IBE where we just need to make a certain piece of information more visible, we could add a kvp instead of adding a new ibe. What do you think?

src/Utilities/Task.cs Outdated Show resolved Hide resolved
@benvillalobos
Copy link
Member Author

@Forgind This is a great idea imo. This begs a few questions, like what is the key in this dictionary. It could be <string, string>, then the value would have to be a semicolon delimited list, then tasks are parsing more strings and therefore allocating.

Perhaps <string, object>? Where the task that needs to use, say, dictionary["WarningsAsErrors] would know that the value is a list and would perform (dictionary["WarningsAsErrors] as List).contains("msb1234") or something equivalent?

This might be complicated for the OOP task host that needs to translate these objects through the binary formatter. And we may need to special case every object. I'm slowly convincing myself <string, object> is a bad idea.

@cdmihai thoughts?

benvillalobos and others added 2 commits February 19, 2021 13:25
Co-authored-by: Forgind <Forgind@users.noreply.github.com>
@Forgind
Copy link
Member

Forgind commented Feb 19, 2021

Well, it has to translate them through the BinaryFormatter now, but that has to change eventually. I do like the reusability of object over string, but maybe as a middle ground, <string, serializable object>? That makes calling Translate on them relatively easy but avoids the excess allocations from my suggestion.

Also, if we're going this way, one thing to not do in this PR but possibly consider for the future is whether we could have a second dictionary of <string, delegate> that we could use for methods. Smaller win, in my view, for more ugliness, but that would hopefully mean we could stop making more IBuildEngines. I live for the day when you can just say `if (BuildEngine..TryGet(, out ...)) ... without needing to check whether your BuildEngine is a new enough BuildEngine. That's probably a pipe dream, though.

@cdmihai
Copy link
Contributor

cdmihai commented Feb 22, 2021

Regarding the dict<string, object> to avoid revving up the IBE interface, I don't like it :). Users would have to know the magic string and then explicitly cast the delegate object to its actual type in order to use the delegate's return type and arg types. I'd rather we add one last IBE interface that has a reference to an abstract class containing the functionality of future IBEs: IBuildEngine9.EngineServices, with EngineServices an abstract class that keeps on accumulating APIs.

The downside to the abstract class approach is that newer tasks that depend on a new EngineServices API won't be able to function as easily with older versions of msbuild that do not have that API. With the current interface numbering scheme and with the dict<string, object> scheme, tasks can query functionality at runtime. With the abstract class, tasks that really want back-compat (supporting older msbuilds) will have to use reflection on EngineServices. On the other hand, I don't think it's that common for tasks to support older engines and the few that do can use reflection.

But let's not design / implement this in this PR.

@Forgind
Copy link
Member

Forgind commented Feb 22, 2021

Currently, users need to know the exact name of property they need, which is the semantic equivalent of needing to know the exact string. To be clear, I consider Dictionary<string, delegate> as the more controversial and probably undesirable cousin of having Dictionary<string, object : Translatable> with the values specifically for things like properties. You presumably know what type they should be if you're trying to use them, just as you would need to figure out the type of a property. Letting it be object instead of string makes it more flexible if we need unusual objects later.

I do like the abstract class plan, although a simpler approach might be to have IBuildEngine9 (possibly renamed) be an abstract class itself, and have it extend IBuildEngine8 and be extended as if it were an interface. As long as we provide a default implementation for everything, it should prevent people from being broken just for extending it. Reflection is kinda slow.

Do you want to set up a meeting to talk about it?

logger.ErrorCount.ShouldBe(1);

// The build should STOP when a task logs an error, make sure ReturnFailureWithoutLoggingErrorTask doesn't run.
logger.AssertLogDoesntContain("MSB4181");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this may not work with tasks that implement ITask directly. Can you please add another test just like this one but calling a task implementing ITask directly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting. Going to brain dump this before making the test.

Someone implementing ITask wouldn't have access to a TaskLoggingHelper (like Task does) and thus have to log a warning through the BuildEngine.LogWarningEvent. The build engine would log it to the logging service as a warning and at LoggingService.RouteBuildEvent it would be translated into an error.

But I think the build would continue because it entirely depends on what the task returned right? So long as the task returns true despite logging an error, the build will complete.

Question, what context would someone implement itask vs inheriting from task?

Copy link
Contributor

@cdmihai cdmihai Feb 23, 2021

Choose a reason for hiding this comment

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

Don't really know why people would do it, but according to this GH search they do it.

So long as the task returns true despite logging an error, the build will complete.

I think MSBuild might either warn or error that the ITask instance returned true but an error was logged. Not super sure about this though, so worth writing a test to pin the behaviour :)
On the other hand, since an ITask implementation cannot know whether any errors were logged (because it does not have access to Log), it can't really condition its return value. So maybe this whole scenario does not make sense and we can ignore it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that there's no need to check this on an ITask. Something interesting about the error task. It explicitly returns false otherwise the build would continue:

// careful to return false. Otherwise the build would continue.
that's a very strange disconnect that sounds to me like it should be an issue to fix. But if that's just how msbuild has been since the beginning of time...should we consider fixing it?

Copy link
Contributor

@cdmihai cdmihai Feb 23, 2021

Choose a reason for hiding this comment

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

It comes from an era where an error was an error, so returning false made sense :)

I'd wait until somebody reports an issue and then update both Error and Warning.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, why is that wrong? A task returns false if it logged an error. The Error task is designed to log an error, hence always returns "HasLoggedAnError."

Copy link
Member Author

@benvillalobos benvillalobos Feb 23, 2021

Choose a reason for hiding this comment

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

I should have included it in the code sample, but the error task called Log.LogError and simply returns false, not Log.HasLoggedErrors.

Edit: So if the error task returned true, the build would continue despite it having logged an error. So we'd see the rest of the build but we'd get a Build Failed.

Copy link
Member

@Forgind Forgind Feb 24, 2021

Choose a reason for hiding this comment

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

Right...it has always logged an error because it logs an error right before the return statement.

Right, and people who log errors tend to want execution to stop at that point. No point continuing if something went irredeemably wrong.

@benvillalobos
Copy link
Member Author

benvillalobos commented Feb 23, 2021

I created #6183 to track discussion for making a more generalized IBuildEngine interface.

@Forgind, you created IBuildEngine7, how do you know whether that new property works in a OOP task host node? Basically, where does OOPTHN get this value if the user sets it?

I was under the impression we needed to pass that value to the OOPTaskHostNode through TaskHostConfiguration.
/cc @cdmihai for this question as well.

@Forgind
Copy link
Member

Forgind commented Feb 23, 2021

I'm slightly confused by your question, so I'll answer what I think you're asking. Let me know if I'm wrong.

Wherever I have access to the BuildEngine, I can say if be is IBuildEngine7 be7 (or the equivalent) then use be7 to access my property.

An out-of-proc TaskHost node is an IBuildEngine(7), so I can try to access properties directly without casting. A newer TH will succeed, and an older TH won't have the newer code that tries.

Did that answer your question?

@benvillalobos
Copy link
Member Author

@Forgind Yes~ish. I see how my question was confusing. IBE7 added a property that a task could set on its own. IBE8 will need information passed to it that a task can then access. From a taskhost perspective it's not difficult at all. OOPTHN is a slightly different story.

My confusion comes from the OOPTHN and how it receives this data. From what I could gather (and please correct me if I'm wrong /cc: @cdmihai or @KirillOsenkov 🙂), an OOPTHN needs to be passed initialization data in the form of a TaskHostConfigurationData. See here: https://github.com/dotnet/msbuild/blob/master/src/MSBuild/OutOfProcTaskHostNode.cs#L604. So that's how I need to give it the warnings that will be translated into errors.

Another question, do we not have tests for OOPTHN? I can't seem to find anything testing that specifically. Or is testing the translation data enough?

… build will continue. Add custom task that returns and logs what you want.
@benvillalobos benvillalobos marked this pull request as ready for review February 23, 2021 20:48
@benvillalobos
Copy link
Member Author

NTS: Test failures have to do with WarningsAsMessages. Apparently that takes precedence over WarningsAsErrors.

@cdmihai
Copy link
Contributor

cdmihai commented Feb 23, 2021

@benvillalobos, we have the same impression on how OOPTHN gets its data. But always leave room for surprise ...

…akes priority. Cache the hashset in TaskHost
@Forgind
Copy link
Member

Forgind commented Feb 23, 2021

Adding which warnings should be treated as errors to the TaskHostConfiguration sounds right to me.

…L warnings as errors.

Add tests verifying various builds and results for warnings as errors
@KirillOsenkov
Copy link
Member

Sounds good. And what do you think about replacing the HashSet<string> with ICollection<string> type?

Apologies if I'm being pedantic :)

@jeffkl
Copy link
Contributor

jeffkl commented Mar 8, 2021

Sorry I haven't had a chance to look at this sooner, let me dive into it...

@benvillalobos
Copy link
Member Author

@jeffkl To be fair, I should have pinged sooner. Just a heads up, I'm about to change IBE8 to have bool ShouldLogWarningAsError(string code) instead of the set containing the warning codes as mentioned above.

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.

Looks good

@benvillalobos
Copy link
Member Author

@KirillOsenkov (and any other reviewers familiar with the binary formatter) Requesting a specific pass over my last four commits. Having WarningsAsErrors be an ICollection required adding an overload to the ITranslator interface and I'd like to be sure that the way I did it is okay. I mostly looked at how it was done with an IList and went with it. Looks good to me and it passes tests but I'd like to be sure.

@Forgind
Copy link
Member

Forgind commented Mar 8, 2021

Looks good to me.

/// </summary>
/// <param name="warningCode">The warning code to check.</param>
/// <returns>A boolean to determine whether the warning should be treated as an error.</returns>
public bool ShouldTreatWarningAsError(string warningCode);
Copy link
Member

Choose a reason for hiding this comment

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

the public keyword is redundant here, the general convention is to avoid modifiers on interface members

Copy link
Member

@KirillOsenkov KirillOsenkov left a comment

Choose a reason for hiding this comment

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

👍

Comment on lines +14 to +17
public bool ReturnHasLoggedErrors { get; set; }

[Required]
public bool Return { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

It looks like these are always true?


public bool ShouldTreatWarningAsError(string warningCode)
{
if (_taskLoggingContext == null || WarningsAsErrors == null)
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
if (_taskLoggingContext == null || WarningsAsErrors == null)
if (WarningsAsErrors == null)

(Should be the same but looks a little cleaner to me.)

return false;
}

return WarningsAsErrors.Count == 0 || WarningsAsErrors.Contains(warningCode);
Copy link
Member

Choose a reason for hiding this comment

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

Comment?

@benvillalobos benvillalobos added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Mar 10, 2021
@benvillalobos benvillalobos merged commit 14c3714 into dotnet:master Mar 13, 2021
@benvillalobos benvillalobos deleted the warnaserr-fix branch March 13, 2021 00:39
Forgind pushed a commit that referenced this pull request Mar 31, 2021
…dErrors` (#6308)

Fixes #6306

Context
#6174 didn't properly account for warningsaserrors and warningsasmessages together. This PR makes each taskhost aware of both WarningsAsMessages and WarningsAsErrors when telling tasks whether or not an error should be treated as a warning.

Changes Made
LoggingService and TaskLoggingContext expose a GetWarningsAsMessages to each taskhost for them to store.
No changes to IBE8 API
Taskhosts first check if a warning would be treated as a message when telling tasks whether or not a warning should be treated as a warning.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HasLoggedErrors should respect MSBuildWarningsAsErrors
5 participants