[Xamarin.Android.Build.Tasks] New APT0000 msbuild errors are being reported from aapt output parsing #1047

dellis1972 · 2017-11-22T17:02:46Z

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems?id=528572

The current RegEx was capturing output which was not and error
or warning. It turns out that aapt is not very consistent with
its error messaging. Sometimes they use error, sometimes then
use Error or ERROR. Or in the worse case, they don't flag it
as anything.

This commit adds a few more tests to AndroidRegExTests as well
as alters the logic in Aapt a bit as well. This logic will
only log as an error if we have an error level or we have
a file and line. This seems to be fairly consistent when
looking at the aapt source code.

We also added a few extra Known errors which the regex might not
pick. This is because they do not contain a level or a file
and line number.

jonathanpeppers · 2017-11-27T14:09:51Z

src/Xamarin.Android.Build.Tasks/Tasks/Aapt.cs

 				if (!string.IsNullOrEmpty (match.Groups["line"]?.Value))
 					line = int.Parse (match.Groups["line"].Value) + 1;
-				var level = match.Groups["level"].Value;
+				var level = match.Groups["level"].Value.ToLower ();


Should it be ToLowerInvariant?

jonathanpeppers · 2017-11-27T14:12:40Z

src/Xamarin.Android.Build.Tasks/Tasks/AndroidToolTask.cs

 ( # optional warning|error:
 \s*
- (?<level>(warning|error)[^:]*)\s*
+ (?<level>(warning|error|Error|ERROR)[^:]*)\s*


Maybe we could just use RegexOptions.IgnoreCase, and it would also handle if they decide to use Warning or WARNING?

jonpryor · 2017-11-27T16:25:59Z

src/Xamarin.Android.Build.Tasks/Tasks/Aapt.cs


-				LogError ("APT0000", message, file, line);
-				return;
+				if (level.Contains ("error") || (line != 0 && !string.IsNullOrEmpty (file))) {


Once concern with this change is that previously, any message which wasn't a warning or fakeLogOpen was treated as an error.

Now, only messages which contain error are errors.

This sounds reasonable, but will there be any unexpected fallout? Will we "miss" any errors with this change?

Additionally, it looks like we won't log message at all if it's not explicitly a warning or an error. Is that desirable? (Presumably we should always log, no?)

As stated google is not consistent with its labelling of errors/warnings. I went through the aapt code and added any additional ones to https://github.com/xamarin/xamarin-android/pull/1047/files#diff-97e1ab3c8027acfecf85b8bb5a88dd9aR409

Should we always log the message? Or are there messages we should skip entirely?

It looks like the new version will only log messages which contain warning, fakeLogOpen, error, or contain a filename. Is that desirable?

We fall through to https://github.com/dellis1972/xamarin-android/blob/79e432f32f08f4b2c867e07cdd225a5de2c9449a/src/Xamarin.Android.Build.Tasks/Tasks/Aapt.cs#L405 if the regex does not match, or its not one of the KnownInvalidErrorMessages

That said, I'm sure we had an old bug where if we do log the error/warning using LogMessage as well as LogError or LogWarning we ended up with the same error appearing in the error list twice (sometimes). I think because msbuild does some parsing too.

Looking at the aapt source google are not consistent with labelling.. but :) they are consistent with writing ALL errors/warnings to stderr.. so I will change this PR to log anything we get from stdout as a message. and only process the errors/warnings from stderr.

https://android.googlesource.com/platform/frameworks/base/+/c761d8b0086f9e92eb1e4b3a5683ae6afb84036a/tools/aapt/Resource.cpp#1025

https://android.googlesource.com/platform/frameworks/base/+/c761d8b0086f9e92eb1e4b3a5683ae6afb84036a/tools/aapt/Resource.cpp#218

https://android.googlesource.com/platform/frameworks/base/+/c761d8b0086f9e92eb1e4b3a5683ae6afb84036a/tools/aapt/Resource.cpp#231

https://android.googlesource.com/platform/frameworks/base/+/c761d8b0086f9e92eb1e4b3a5683ae6afb84036a/tools/aapt/Resource.cpp#197

jonpryor · 2017-11-27T16:27:01Z

src/Xamarin.Android.Build.Tasks/Tasks/Aapt.cs

+			// Handle additional errors that doesn't match the regex
+			foreach (var knownInvalidError in KnownInvalidErrorMessages) {
+				if (singleLine.Trim ().StartsWith (knownInvalidError, StringComparison.OrdinalIgnoreCase)) {
+					LogError ("APT0000", string.Format ("{0} \"{1}\".", knownInvalidError, singleLine.Substring (singleLine.LastIndexOfAny (new char [] { '\\', '/' }) + 1)), ToolName);


Why do we only provide the "rightmost" directory name, and not the entire path? Is it too long and noisy?

no idea. its been that way for a long time from what I can see.

dellis1972 · 2017-11-27T17:45:46Z

build

jonathanpeppers · 2017-11-27T19:27:33Z

src/Xamarin.Android.Build.Tasks/Tasks/AndroidToolTask.cs

 (?<message>.*)
 $
-", RegexOptions.Compiled | RegexOptions.ExplicitCapture | RegexOptions.IgnorePatternWhitespace);
+", RegexOptions.Compiled | RegexOptions.ExplicitCapture | RegexOptions.IgnorePatternWhitespace, RegexOptions.IgnoreCase);


@dellis1972 I think this needs | instead of ,

…ported from aapt output parsing Fixes https://devdiv.visualstudio.com/DevDiv/_workitems?id=528572 The current RegEx was capturing output which was not and error or warning. It turns out that aapt is not very consistent with its error messaging. Sometimes they use `error`, sometimes then use `Error` or `ERROR`. Or in the worse case, they don't flag it as anything. This commit adds a few more tests to AndroidRegExTests as well as alters the logic in `Aapt` a bit as well. This logic will only log as an error if we have an error `level` or we have a `file` and `line`. This seems to be fairly consistent when looking at the `aapt` source code. We also added a few extra `Known` errors which the regex might not pick. This is because they do not contain a `level` or a `file` and `line` number.

dnfclas added the cla-already-signed label Nov 22, 2017

dellis1972 requested review from grendello and jonathanpeppers November 23, 2017 09:55

jonathanpeppers requested changes Nov 27, 2017

View reviewed changes

jonpryor reviewed Nov 27, 2017

View reviewed changes

dellis1972 closed this Nov 27, 2017

dellis1972 reopened this Nov 27, 2017

dnfclas added the cla-already-signed label Nov 27, 2017

jonathanpeppers reviewed Nov 27, 2017

View reviewed changes

dellis1972 added 5 commits November 30, 2017 11:49

Updated based on feedback.

78623f9

Fixed compilation Error

a805a94

ff

406e5d0

ff

bb8bb53

dellis1972 force-pushed the Bug528572 branch from 3518475 to bb8bb53 Compare November 30, 2017 12:07

jonathanpeppers approved these changes Nov 30, 2017

View reviewed changes

jonpryor merged commit 2135856 into dotnet:master Nov 30, 2017

github-actions bot locked and limited conversation to collaborators Feb 2, 2024

[Xamarin.Android.Build.Tasks] New APT0000 msbuild errors are being reported from aapt output parsing #1047

[Xamarin.Android.Build.Tasks] New APT0000 msbuild errors are being reported from aapt output parsing #1047

Uh oh!

Conversation

dellis1972 commented Nov 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dellis1972 commented Nov 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dellis1972 Nov 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dellis1972 Nov 27, 2017 •

edited

Loading