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.

Sign up for GitHub

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

Jump to bottom

[AndroidToolTask] Log tool output as error #229

Merged
merged 6 commits into from
Apr 19, 2024
Merged

[AndroidToolTask] Log tool output as error #229

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e713c33
[AndroidToolTask] Log tool output as error
pjcollins Apr 4, 2024
6589d97
Apply feedback
pjcollins Apr 8, 2024
5a59b1b
Formatting
pjcollins Apr 8, 2024
3febc0b
Nullable fix
pjcollins Apr 10, 2024
7388582
Nullable cleanup
pjcollins Apr 10, 2024
3fb1fcd
Apply feedback
pjcollins Apr 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion src/Microsoft.Android.Build.BaseTasks/AndroidToolTask.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

using System;
using System.IO;
using System.Text;
using Microsoft.Build.Framework;
using Microsoft.Build.Utilities;

namespace Microsoft.Android.Build.Tasks
Expand All @@ -12,21 +14,35 @@ public abstract class AndroidToolTask : ToolTask

protected string WorkingDirectory { get; private set; }

StringBuilder toolOutput;
pjcollins marked this conversation as resolved.
Show resolved Hide resolved

public AndroidToolTask ()
{
WorkingDirectory = Directory.GetCurrentDirectory();
toolOutput = new StringBuilder ();
}

public override bool Execute ()
{
try {
return RunTask ();
bool taskResult = RunTask ();
if (!taskResult && !string.IsNullOrEmpty (toolOutput?.ToString ())) {
Log.LogError (toolOutput.ToString ().Trim ());
Copy link
Member

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?

https://github.com/xamarin/xamarin-android-tools/blob/37d79c9dcdf738a181084b0b5890877128d75f1e/src/Microsoft.Android.Build.BaseTasks/UnhandledExceptionLogger.cs#L24

Copy link
Member Author

@pjcollins pjcollins Apr 8, 2024 •
edited
Loading

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

Copy link
Member

@jonathanpeppers jonathanpeppers Apr 8, 2024 •
edited
Loading

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?

Copy link
Member Author

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?

Copy link
Member

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.

}
toolOutput.Clear ();
return taskResult;
} catch (Exception ex) {
Log.LogUnhandledException (TaskPrefix, ex);
return false;
}
}

protected override void LogEventsFromTextOutput(string singleLine, MessageImportance messageImportance)
{
base.LogEventsFromTextOutput (singleLine, messageImportance);
toolOutput.AppendLine (singleLine);
}

// Most ToolTask's do not override Execute and
// just expect the base to be called
public virtual bool RunTask () => base.Execute ();
Expand Down
58 changes: 50 additions & 8 deletions tests/Microsoft.Android.Build.BaseTasks-Tests/AndroidToolTaskTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,24 @@
using System.IO;
using System.Collections.Generic;
using System.Threading.Tasks;
using Microsoft.Android.Build.BaseTasks.Tests.Utilities;
using Microsoft.Android.Build.Tasks;
using NUnit.Framework;
using Microsoft.Build.Framework;
using Xamarin.Build;
using NUnit.Framework.Internal;
using System.Linq;

namespace Microsoft.Android.Build.BaseTasks.Tests
{
[TestFixture]
public class AndroidToolTaskTests
{
public class MyAndroidTask : AndroidTask {
List<BuildErrorEventArgs> errors;
List<BuildWarningEventArgs> warnings;
List<BuildMessageEventArgs> messages;
MockBuildEngine engine;

public class MyAndroidTask : AndroidTask
{
public override string TaskPrefix {get;} = "MAT";
public string Key { get; set; }
public string Value { get; set; }
Expand All @@ -25,7 +31,8 @@ public override bool RunTask ()
}
}

public class MyOtherAndroidTask : AndroidTask {
public class MyOtherAndroidTask : AndroidTask
{
public override string TaskPrefix {get;} = "MOAT";
public string Key { get; set; }
public bool ProjectSpecific { get; set; } = false;
Expand All @@ -40,15 +47,31 @@ public override bool RunTask ()
}
}

public class DotnetToolOutputTestTask : AndroidToolTask
{
public override string TaskPrefix {get;} = "DTOT";
protected override string ToolName => "dotnet";
protected override string GenerateFullPathToTool () => ToolExe;
public string CommandLineArgs { get; set; } = "--info";
protected override string GenerateCommandLineCommands () => CommandLineArgs;
}

[SetUp]
public void TestSetup()
{
errors = new List<BuildErrorEventArgs> ();
warnings = new List<BuildWarningEventArgs> ();
messages = new List<BuildMessageEventArgs> ();
engine = new MockBuildEngine (TestContext.Out, errors, warnings, messages);
}

[Test]
[TestCase (true, true, true)]
[TestCase (false, false, true)]
[TestCase (true, false, false)]
[TestCase (false, true, false)]
public void TestRegisterTaskObjectCanRetrieveCorrectItem (bool projectSpecificA, bool projectSpecificB, bool expectedResult)
{
var engine = new MockBuildEngine (TestContext.Out) {
};
var task = new MyAndroidTask () {
BuildEngine = engine,
Key = "Foo",
Expand All @@ -72,8 +95,6 @@ public void TestRegisterTaskObjectCanRetrieveCorrectItem (bool projectSpecificA,
[TestCase (false, true, false)]
public void TestRegisterTaskObjectFailsWhenDirectoryChanges (bool projectSpecificA, bool projectSpecificB, bool expectedResult)
{
var engine = new MockBuildEngine (TestContext.Out) {
};
MyAndroidTask task;
var currentDir = Directory.GetCurrentDirectory ();
Directory.SetCurrentDirectory (Path.Combine (currentDir, ".."));
Expand All @@ -96,5 +117,26 @@ public void TestRegisterTaskObjectFailsWhenDirectoryChanges (bool projectSpecifi
task2.Execute ();
Assert.AreEqual (expectedResult, string.Compare (task2.Value, task.Value, ignoreCase: true) == 0);
}

[Test]
[TestCase ("invalidcommand", false, "You intended to execute a .NET program, but dotnet-invalidcommand does not exist.")]
[TestCase ("--info", true, "")]
public void FailedAndroidToolTaskShouldLogOutputAsError (string args, bool expectedResult, string expectedErrorText)
{
var task = new DotnetToolOutputTestTask () {
BuildEngine = engine,
CommandLineArgs = args,
};
var taskSucceeded = task.Execute ();
Assert.AreEqual (expectedResult, taskSucceeded, "Task execution did not return expected value.");

if (taskSucceeded) {
Assert.IsEmpty (errors, "Successful task should not have any errors.");
} else {
Assert.IsNotEmpty (errors, "Task expected to fail should have errors.");
Assert.IsTrue (errors.Any (e => e.Message.Contains (expectedErrorText)),
"Task expected to fail should contain expected error text.");
}
}
}
}
1 change: 1 addition & 0 deletions tests/Microsoft.Android.Build.BaseTasks-Tests/Microsoft.Android.Build.BaseTasks-Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
<IsPackable>false</IsPackable>
<OutputPath>$(TestOutputFullPath)</OutputPath>
<AppendTargetFrameworkToOutputPath>false</AppendTargetFrameworkToOutputPath>
<RollForward>Major</RollForward>
</PropertyGroup>

<Import Project="..\..\src\Microsoft.Android.Build.BaseTasks\MSBuildReferences.projitems" />
Expand Down