Audit LogErrorFromException #2238
Labels
Area: xamarin-android Build
Issues building the xamarin-android repo *itself*.
Milestone
Comments
jonpryor
added a commit
to jonpryor/xamarin-android
that referenced
this issue
Sep 27, 2018
Context: dotnet#2238 Context: 8905dfa Context: 31a23ca For *months*, I would occasionally see an error message which would infuriate me, because it didn't make sense to my feeble mind: xamarin-android/build-tools/scripts/TestApks.targets(223,5): error : Root element is missing The actual line number isn't relevant and changes over time, but what *killed* me is that, *somehow*, there'd be (presumably) invalid XML ...in the middle of a `.targets` file? Does that make any sense at all? (No, that does not make sense, adding to the infuriation.) Turns Out™ that I was *misreading* the message: The *task* at e.g. `TestApks.targets` line 223 was reporting an exception with the message text "Root element is missing." However, *because* the task name wasn't mentioned in the error message, this very significant fact escaped my attention. This messaging structure is how `Log.LogErrorFromException()` works. As such, we should modify how we're using it, to reduce confusion. In the case of `<RenameTestCases/>`: 1. Add an additional `LogError("Unable to process...")` message, so that `Root element is missing` won't be "alone" without context. 2. Generate an output NUnit XML file which contains an error message. Hopefully, by generating such an XML file, the Jenkins Test Results page will show *an* error. (Not necessarily a helpful one, but a less-than-helpful error is better than none at all when *something* has gone wrong...)
dellis1972
pushed a commit
that referenced
this issue
Sep 28, 2018
…#2239) Context: #2238 Context: 8905dfa Context: 31a23ca For *months*, I would occasionally see an error message which would infuriate me, because it didn't make sense to my feeble mind: xamarin-android/build-tools/scripts/TestApks.targets(223,5): error : Root element is missing The actual line number isn't relevant and changes over time, but what *killed* me is that, *somehow*, there'd be (presumably) invalid XML ...in the middle of a `.targets` file? Does that make any sense at all? (No, that does not make sense, adding to the infuriation.) Turns Out™ that I was *misreading* the message: The *task* at e.g. `TestApks.targets` line 223 was reporting an exception with the message text "Root element is missing." However, *because* the task name wasn't mentioned in the error message, this very significant fact escaped my attention. This messaging structure is how `Log.LogErrorFromException()` works. As such, we should modify how we're using it, to reduce confusion. In the case of `<RenameTestCases/>`: 1. Add an additional `LogError("Unable to process...")` message, so that `Root element is missing` won't be "alone" without context. 2. Generate an output NUnit XML file which contains an error message. Hopefully, by generating such an XML file, the Jenkins Test Results page will show *an* error. (Not necessarily a helpful one, but a less-than-helpful error is better than none at all when *something* has gone wrong...)
jonathanpeppers
added
the
Area: xamarin-android Build
|
It looks like we've still got some occurrences of Updating milestones for further consideration. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
An occasional error that we'll see in the build logs is:
This message is borderline infuriating, because
TestApks.targets(249,5)is a<RenameTestCases/>task execution. How is there a missing root element?!Turns Out™ that this is MSBuild being helpful:
<RenameTestCases/>'sExecute()method has a catch block which only callsLog.LogErrorFromException():https://github.com/xamarin/xamarin-android/blob/30eea71177853a73ab07911d49be1c1707c300f0/build-tools/Xamarin.Android.Tools.BootstrapTasks/Xamarin.Android.Tools.BootstrapTasks/RenameTestCases.cs#L43-L45
Log.LogErrorFromException()helpfully provides the filename and line information for where the task was invoked -- henceTestApks.targets(249,5)-- but the output isn't clear that there's an intervening task here. For months (years?) I've been laboring under the assumption that thisRoot element is missingmessage is being generated from MSBuild.That is not the case. The error is reported from our task.
The Real™ problem is that the "UI" reporting of the error is inscrutable.
In short,
Log.LogErrorFromException()should be considered a code smell: if it's the only thing present, then when it logs an error, the way that the error is reported -- which includes the MSBuild.targetscallsite location, but no mention that this is being reported from the task that executed -- is extremely misleading and unhelpful.We should audit our codebase for all uses of
Log.LogErrorFromException()and ensure that when we do use it, we provide useful "error context" as well, so that when an error occurs, more than justException.Message("Root element is missing") and the.targetscall site is provided.The text was updated successfully, but these errors were encountered: