-
Notifications
You must be signed in to change notification settings - Fork 27
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
[AndroidToolTask] Log tool output as error #229
Conversation
Context: #208 Updates AndroidToolTask to capture all standard output from the tool and log it as an error if the task fails.
Testing this further with dotnet/android#8861 |
return RunTask (); | ||
bool taskResult = RunTask (); | ||
if (!taskResult && !string.IsNullOrEmpty (toolOutput?.ToString ())) { | ||
Log.LogError (toolOutput.ToString ().Trim ()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this one should have an error code, can we use $"XA{TaskPrefix}0000"
, maybe we can add new helper method in this class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I've attempted to address this with the latest changes that use LogCodedError instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was reading what @jonpryor wrote on #208:
Update AndroidToolTask so that it provides output logging infrastructure, a'la dotnet/android@b002dc38, such that all tool output is always captured (as well as logged via LogDebugMessage()), and when an error occurs, all captured output is part of the error message.
Do we somehow need to "smush" the extra error information into an existing error message? 🤦 This PR would show two errors, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current approach should indeed show two errors, though I think both are useful. Here is a sample of complete build output from the test case that was added:
"OutputTest.proj" (Test target) (1:2) ->
(Test target) ->
OutputTest.proj(10,9): error MSB6006: "dotnet" exited with code 1.
OutputTest.proj(10,9): error XADTOT0000: Could not execute because the specified command or file was not found.
OutputTest.proj(10,9): error XADTOT0000: Possible reasons for this include:
OutputTest.proj(10,9): error XADTOT0000: * You misspelled a built-in dotnet command.
OutputTest.proj(10,9): error XADTOT0000: * You intended to execute a .NET program, but dotnet-invalidcommand does not exist.
OutputTest.proj(10,9): error XADTOT0000: * You intended to run a global tool, but a dotnet-prefixed executable with this name could not be found on the PATH.
0 Warning(s)
2 Error(s)
The first error shows the tool name and exit code, and the second error is our new custom error with all of the output from the tool. Do we want to try to suppress/replace the MSB6006 error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you have here is 100% better, let's just have @jonpryor weigh in. He should be back very soon.
I think this is ready, let me know if there is anything else to consider before merging |
Doh, I was too late for an updated commit message: `AndroidToolTask.Execute()` logs full tool output on error
Context: https://github.com/xamarin/xamarin-android-tools/issues/208
Context: https://github.com/xamarin/xamarin-android/commit/b002dc38196a0bedb18c82d228056e1ba108459c
Very often the *context* to understand a tool error isn't printed to
stderr, but is instead printed earlier to stdout. For example,
consider this annotated `r8` output snippet from
xamarin/xamarin-android@b002dc38 (`[stderr]` and `[stdout]` are
annotations):
% $HOME/android-toolchain/jdk-11/bin/java -Xmx1G -classpath …/dotnet/packs/Microsoft.Android.Sdk.Darwin/33.0.0-preview.4.20/tools/r8.jar com.android.tools.r8.D8 --debug --min-api 23 --output obj/Debug/net7.0-android/android/bin/ …
[stdout] Error in obj/Debug/net7.0-android/lp/59/jl/classes.jar:android/support/v4/app/INotificationSideChannel$Stub.class:
[stdout] Type android.support.v4.app.INotificationSideChannel$Stub is defined multiple times: obj/Debug/net7.0-android/lp/59/jl/classes.jar:android/support/v4/app/INotificationSideChannel$Stub.class, obj/Debug/net7.0-android/lp/4/jl/bin/classes.jar:android/support/v4/app/INotificationSideChannel$Stub.class
[stdout] Compilation failed
[stdout] Exception in thread "main" java.lang.RuntimeException: com.android.tools.r8.CompilationFailedException: Compilation failed to complete, origin: obj/Debug/net7.0-android/lp/59/jl/classes.jar:android/support/v4/app/INotificationSideChannel$Stub.class
[stdout] at com.android.tools.r8.internal.Bj.a(R8_3.3.28_2aaf796388b4e9f6bed752d926eca110512a53a3f09a8d755196089c1cfdf799:98)
[stdout] at com.android.tools.r8.D8.main(R8_3.3.28_2aaf796388b4e9f6bed752d926eca110512a53a3f09a8d755196089c1cfdf799:4)
[stdout] Caused by: com.android.tools.r8.CompilationFailedException: Compilation failed to complete, origin: obj/Debug/net7.0-android/lp/59/jl/classes.jar:android/support/v4/app/INotificationSideChannel$Stub.class
[stdout] at Version.fakeStackEntry(Version_3.3.28.java:0)
[stdout] at com.android.tools.r8.internal.Bj.a(R8_3.3.28_2aaf796388b4e9f6bed752d926eca110512a53a3f09a8d755196089c1cfdf799:75)
[stdout] …
[stdout] Caused by: com.android.tools.r8.internal.f: Type android.support.v4.app.INotificationSideChannel$Stub is defined multiple times: obj/Debug/net7.0-android/lp/59/jl/classes.jar:android/support/v4/app/INotificationSideChannel$Stub.class, obj/Debug/net7.0-android/lp/4/jl/bin/classes.jar:android/support/v4/app/INotificationSideChannel$Stub.class
[stdout] at com.android.tools.r8.internal.DT.a(R8_3.3.28_2aaf796388b4e9f6bed752d926eca110512a53a3f09a8d755196089c1cfdf799:14)
[stdout] …
[stderr] java.lang.RuntimeException: com.android.tools.r8.CompilationFailedException: Compilation failed to complete, origin: obj/Debug/net7.0-android/lp/59/jl/classes.jar : android/support/v4/app/INotificationSideChannel$Stub.class
When originally processed, the *only* output included in the error
message was the the only line written to stderr:
error : java.lang.RuntimeException: com.android.tools.r8.CompilationFailedException:
Compilation failed to complete, origin: obj/Debug/net7.0-android/lp/59/jl/classes.jar :
android/support/v4/app/INotificationSideChannel$Stub.class
The fix in xamarin/xamarin-android@b002dc38 was to update
`<JavaToolTask/>` to capture *all* output from `java …`, and when an
error occurred, dump *all* of it:
Update `AndroidToolTask` to do the same thing, "lowering" the logic
into a common base class of `<JavaToolTask/>`: capture all output
(stdout & stderr) from the tool, and when an error occurs, log *all*
of the captured output into an `XAccc0000` error message.
`ccc` is the `AndroidToolTask.TaskPrefix` property. |
Oh sorry I jumped the gun on this, we can maybe include this in the commit message on the xamarin-android side |
Changes: dotnet/android-tools@c8a5b5b...1ea4e35 * dotnet/android-tools@1ea4e35: [AndroidToolTask] Log tool output as error (dotnet/android-tools#229) Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Context: #208
Updates AndroidToolTask to capture all standard output from the tool and log it as an error if the task fails.