[Xamarin.Android.Build.Tasks] Aapt MSBuild Task treats no default translation warnings as MSBuild errors

[dellis1972]

Fixes https://bugzilla.xamarin.com/show_bug.cgi?id=60445

The regex is picking up the warning in

warning: string 'app_name1' has no default translation.

but the "warning" part is being picked up by the very first
capture group so the current code does not detect its a warning.
So in addition to checking the capture group, we should look at
the entire line and check for "warning"

[dellis1972]

dont merge I need to create a test.

[jonpryor]

My concern with this approach is that if you happen to have a resource named "warning" which actually has an error, we won't properly report it as an error. Possibly bad example, based on the current commented examples:

res/drawable/warning-deadbeef.jpg: Invalid file name: must contain only [a-z0-9_.]

I believe that the above example would elicit a warning, not an error, even though it should be an error.

Part of me wonders if the "proper" fix is to update AndroidToolTask.AndroidErrorRegex so that file requires a directory separator char:

(?<file>.+[\/].+)?

Is that sane/plausible/possible?

A not-very-well-tested regex:

var r2 = new Regex(@"^(?<file>.+[\/].+)?([:(](?<line>\d+)[)])?(:+\s*)?((error)\s*(?<level>\w*(?=:)):?)?(?<message>.*)", RegexOptions.Compiled | RegexOptions.ExplicitCapture);
var m2 = r2.Match ("warning: string 'app_name1' has no default translation.");
// m2.Groups ["file"].Value == '"
// m2.Groups ["line"].Value == ""
// m2.Groups ["level"].Value == ""
// m2.Groups ["message"].Value == "warning: string 'app_name1' has no default translation."

This raises a different problem: it's a regex! Comments are not tests. :-)

I think, regardless, that we should make AndroidToolTask.AndroidErrorRegex public, and then write a set of tests, ensuring that it produces warnings or errors -- as appropriate -- based on a set of "historical messages."

Turning back to the regex, it seems to have "bad captures" to me. I'd expect warning to be captured in (?<level>), but it isn't. Nor is error, for that matter; error is a "random" string which may or may not be present. This seems quite odd.

[jonpryor]

Playing around a bit, I have:

using System;
using System.Text.RegularExpressions;

class App {
  public static void Main (string[] args)
  {
    var r = new Regex (
@"
^
( # start optional path followed by `:`
 (?<path>
  (?<file>.+[\\/][^:]+)
  (
   ([:](?<line>\d+))
   |
   (\((?<line>\d+)\))
  )?
 )
 \s*
 :
)?
( # optional warning|error:
 \s*
 (?<level>(warning|error)[^:]*)\s*
 :
)?
\s*
(?<message>.*)
$
",
      RegexOptions.Compiled | RegexOptions.ExplicitCapture | RegexOptions.IgnorePatternWhitespace
    );
    foreach (var line in args) {
      Console.WriteLine (line);
      var m = r.Match (line);
      if (!m.Success) {
        Console.WriteLine ("\t# no match");
        continue;
      }
      Console.WriteLine ($"\t   path: '{m.Groups ["path"].Value}'");
      Console.WriteLine ($"\t   file: '{m.Groups ["file"].Value}'");
      Console.WriteLine ($"\t   line: '{m.Groups ["line"].Value}'");
      Console.WriteLine ($"\t  level: '{m.Groups ["level"].Value}'");
      Console.WriteLine ($"\tmessage: '{m.Groups ["message"].Value}'");
    }
  }
}

Samples:

$ mono e.exe \
  'warning: string 'app_name1' has no default translation.' \
  'res\layout\main.axml: error: No resource identifier found for attribute "id2" in package "android" (TaskId:22)' \
  "Resources/values/theme.xml(2): error APT0000: Error retrieving parent for item: No resource found that matches the given name '@android:style/Theme.AppCompat'." \
  "Resources/values/theme.xml:2: error APT0000: Error retrieving parent for item: No resource found that matches the given name '@android:style/Theme.AppCompat'." \
  "res/drawable/foo-bar.jpg: Invalid file name: must contain only [a-z0-9_.]"
warning: string app_name1 has no default translation.
	   path: ''
	   file: ''
	   line: ''
	  level: 'warning'
	message: 'string app_name1 has no default translation.'
res\layout\main.axml: error: No resource identifier found for attribute "id2" in package "android" (TaskId:22)
	   path: 'res\layout\main.axml'
	   file: 'res\layout\main.axml'
	   line: ''
	  level: 'error'
	message: 'No resource identifier found for attribute "id2" in package "android" (TaskId:22)'
Resources/values/theme.xml(2): error APT0000: Error retrieving parent for item: No resource found that matches the given name '@android:style/Theme.AppCompat'.
	   path: 'Resources/values/theme.xml(2)'
	   file: 'Resources/values/theme.xml(2)'
	   line: ''
	  level: 'error APT0000'
	message: 'Error retrieving parent for item: No resource found that matches the given name '@android:style/Theme.AppCompat'.'
Resources/values/theme.xml:2: error APT0000: Error retrieving parent for item: No resource found that matches the given name '@android:style/Theme.AppCompat'.
	   path: 'Resources/values/theme.xml:2'
	   file: 'Resources/values/theme.xml'
	   line: '2'
	  level: 'error APT0000'
	message: 'Error retrieving parent for item: No resource found that matches the given name '@android:style/Theme.AppCompat'.'
res/drawable/foo-bar.jpg: Invalid file name: must contain only [a-z0-9_.]
	   path: 'res/drawable/foo-bar.jpg'
	   file: 'res/drawable/foo-bar.jpg'
	   line: ''
	  level: ''
	message: 'Invalid file name: must contain only [a-z0-9_.]'

This isn't perfect -- note that Resources/values/theme.xml(2) doesn't properly extract the line number -- but it might be a start.

[jonpryor]

Love the change, one more request; as requested above:

I think, regardless, that we should make AndroidToolTask.AndroidErrorRegex public, and then write a set of tests, ensuring that it produces warnings or errors -- as appropriate -- based on a set of "historical messages."

This is so that if we need to update the regex again, we'll have a set of tests to ensure we don't break current expectations.