Tasks Log.HasLoggedError now respects MSBuildWarningsAsErrors
#6174
Conversation
…checks BuildEngine.WarningsAsErrors before logging warnings. TaskHost calls GetWarningsAsErrors from ILoggingService
…t knows what warningsaserrors are.
There was a problem hiding this comment.
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?
|
@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? |
Co-authored-by: Forgind <Forgind@users.noreply.github.com>
|
Well, it has to translate them through the BinaryFormatter now, but that has to change eventually. I do like the reusability of 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. |
…errors. Add Task that logs a warning and returns !Log.HasLoggedErrors (as standard tasks tend to do)
|
Regarding the The downside to the abstract class approach is that newer tasks that depend on a new But let's not design / implement this in this PR. |
|
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"); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
Line 47 in f0eebf2
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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."
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
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 |
|
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 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? |
|
@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 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.
|
NTS: Test failures have to do with |
|
@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
|
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
|
Sounds good. And what do you think about replacing the Apologies if I'm being pedantic :) |
|
Sorry I haven't had a chance to look at this sooner, let me dive into it... |
|
@jeffkl To be fair, I should have pinged sooner. Just a heads up, I'm about to change IBE8 to have |
|
@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. |
|
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); |
There was a problem hiding this comment.
the public keyword is redundant here, the general convention is to avoid modifiers on interface members
| public bool ReturnHasLoggedErrors { get; set; } | ||
|
|
||
| [Required] | ||
| public bool Return { get; set; } |
There was a problem hiding this comment.
It looks like these are always true?
|
|
||
| public bool ShouldTreatWarningAsError(string warningCode) | ||
| { | ||
| if (_taskLoggingContext == null || WarningsAsErrors == null) |
There was a problem hiding this comment.
nit:
| 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); |
benvillalobos
added
the
merge-when-branch-open
…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.
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
Testing
TaskReturnsTrueButLogsError_BuildShouldContinueWarningsAsErrors_ExpectBuildToStopWhenTaskLogsWarningAsErrorTestTranslationWithWarningsAsErrorsTaskHostis generated per 'bucket'Notes
Comment from Rainer in previous PR about this: #5957