[Xamarin.Android.Build.Tasks] <Aapt /> task showing warnings as errors

[jonathanpeppers]

Fixes: #1134
Context: 2135856

Dean’s changes in 2135856 were more accurate at picking out warnings
& errors, but if the level value from the Regex was blank, it was
counting the message as an error. The level value is matching against
the words warning or error, ignoring case.

I think the fix here is to count the message as a warning if the
level is blank.

I added a Regex test case of what was on #1134. I added another test case
that verifies a message with a blank level comes through the build output
as a warning.

[jonathanpeppers]

@jonpryor I am a little puzzled on how we can get this to work perfectly...

Take the following message from aapt, which is a warning:
max res 10, skipping values-sw720dp-land-v13

This one is an error:
res\layout\main.axml: error: No resource identifier found for attribute "id2" in package "android"

However, this one comes out as a warning with our logic:
Invalid file name: must contain only [a-z0-9_.]

But it is actually an error! aapt will fail, Resource.designer.cs isn't generated.

Perhaps this fix is good enough for now? or wait and let @dellis1972 take a look after the break?

[jonathanpeppers]

Another thought is to make the word invalid an error with our Regex:
invalid resource directory name: bar-55

[jonpryor]

I am a little puzzled on how we can get this to work perfectly...

I think part of the issue is that aapt is crazy. There appears to be no consistency between what is a warning and what is an error.

Question: is there some consistency that we're ignoring?

LogEventsFromTextOutput() is invoked for both stdout and stderr. Perhaps what we should instead do is, if the level group is not set, see if this message came from stdout (MessageImportance.Normal) or from stderr (MessageImportance.High).

If the message is from stdout and there is no level group, it's a warning.

If the message is from stderr and there is no level group, it's an error.

Would this fix the problem?

[dellis1972]

Warnings are written to stderr so that won't work. Check the aapt source code and see 😊

…

Sent from my Windows 10 phone From: Jonathan Pryor Sent: 22 December 2017 17:19 To: xamarin/xamarin-android Cc: Dean Ellis; Mention Subject: Re: [xamarin/xamarin-android] [Xamarin.Android.Build.Tasks] <Aapt />task showing warnings as errors (#1139) I am a little puzzled on how we can get this to work perfectly... I think part of the issue is that aapt is crazy. There appears to be no consistency between what is a warning and what is an error. Question: is there some consistency that we're ignoring? LogEventsFromTextOutput() is invoked for both stdout and stderr. Perhaps what we should instead do is, if the level group is not set, see if this message came from stdout (MessageImportance.Normal) or from stderr (MessageImportance.High). If the message is from stdout and there is no level group, it's a warning. If the message is from stderr and there is no level group, it's an error. Would this fix the problem? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[jonathanpeppers]

I think what dean is saying, the code only looks at stderr for these warnings/errors--stdout just logs to LogMessage

• stderr gets max res 10, skipping values-sw720dp-land-v13, which is a warning
• stderr also gets Invalid file name: must contain only [a-z0-9_.], which is an error

[dellis1972]

I'm also saying that aapt itself writes warnings to the wrong place... So we are in a right pickle.

…

Sent from my Windows 10 phone From: Jonathan Peppers Sent: 22 December 2017 17:33 To: xamarin/xamarin-android Cc: Dean Ellis; Mention Subject: Re: [xamarin/xamarin-android] [Xamarin.Android.Build.Tasks] <Aapt />task showing warnings as errors (#1139) I think what dean is saying, the code only looks at stderr for these warnings/errors--stdout just logs to LogMessage • stderr gets max res 10, skipping values-sw720dp-land-v13, which is a warning • stderr also gets Invalid file name: must contain only [a-z0-9_.], which is an error — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[jonpryor]

This test should be redundant with the addition to AndroidRegExTests.cs. The only things this will do is slow down the tests (MSBuild-based tests are slow), and trigger a warning if/when aapt changes the error message it generates.

@dellis1972: agreed?

[dellis1972]

We should extend the AndroidRegExTests.cs test cases rather than adding new tests which do a full build I think.

[dellis1972]

actually. I take that back.. the AndroidRegExTests.cs only tests the regex.. it doesn't test the logic of what happens after we get the regex.. So we need this test.

[dellis1972]

@jonathanpeppers how does this change effect the Resources/values/theme.xml:55: No start tag found error? It has no level.. but I guess is has a file so that will hit 2135856#diff-97e1ab3c8027acfecf85b8bb5a88dd9aR391.

[dellis1972]

then again.. it might log the Warning and exit before logging the error

[jonathanpeppers]

@dean I was going to see how hard it would be to get a quick excel sheet of all the messages out of aapt’s source code.

I think it might be possible to add a few more words that count as an error.

I might not be in today, however...

[dellis1972]

Superseded by #1153 . We are reworking the output processing completely.