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

jonathanpeppers · 2017-12-22T16:34:07Z

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 · 2017-12-22T16:39:10Z

@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 · 2017-12-22T16:40:07Z

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

jonpryor · 2017-12-22T17:19:44Z

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 · 2017-12-22T17:26:04Z

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 · 2017-12-22T17:33:38Z

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 · 2017-12-22T17:39:16Z

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 · 2017-12-26T16:44:59Z

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/AndroidUpdateResourcesTest.cs

 			}
 		}

+		[Test]


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?

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

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.

Fixes: dotnet#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 dotnet#1134. I added another test case that verifies a message with a blank `level` comes through the build output as a `warning`, not giving an `APT0000` error.

dellis1972 · 2018-01-02T12:45:34Z

@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 · 2018-01-02T12:46:18Z

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

jonathanpeppers · 2018-01-02T13:08:30Z

@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 · 2018-01-04T17:09:21Z

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

Context: #5331 Changes: xamarin/monodroid@0eef889...27736a7 * xamarin/monodroid@27736a7ff: [tools/msbuild] Fix App Bundle deployment (#1142) * xamarin/monodroid@f38dbc7ab: [tools/msbuild] use @(_AndroidMSBuildAllProjects) (#1140) * xamarin/monodroid@cfa21d57b: [tools/msbuild] Fix some issues with FastDev (#1139) * xamarin/monodroid@3ba648386: [tools/msbuild] Add missing parameter to BuildAppBundle. (#1138) * xamarin/monodroid@5bb29971b: [tools/msbuild] Add Multi User Support for Debugging. (#1135) * xamarin/monodroid@156abf47c: [tools/msbuild] default $(_AndroidAllowDeltaInstall) to false (#1137) * xamarin/monodroid@fbc961218: [build] Pass EnableCompression to BuildApk (#1094) Add support for running multi-user unit tests on device. It makes use of various adb commands to create test users and remove them. Create a guest user with name `{name}`: adb shell pm create-user --guest {name} Remove a guest user with the specified user id `{userId}`: adb shell pm remove-user --guest {userId} Note that user creation takes a "name", while deletion takes an id. To obtain the mapping, use: $ adb shell pm list users Users: UserInfo{0:Owner:c13} running UserInfo{10:guest1:414} The userId is the number *between* the `{` and the `:` and user name. The userId for `Owner` is 0, while the userId for `guest1` is 10. We can then use a `Regex` to pull out the required `userId` for a specific user. To switch a device to use user `{userId}`: adb shell am switch-user {userId} Emulators seem to create the main user with the name "Owner", so that should be the default name we use for the main user. In the unit tests if we cannot find a user named "Owner" when looking for "Owner" we will default to a `userId` of `0`. User ID 0 always means the default user. This is to handle the case where we are testing locally on devices where the main user is not called "Owner".

jonathanpeppers requested review from dellis1972 and jonpryor December 22, 2017 16:34

jonpryor reviewed Dec 26, 2017

View reviewed changes

jonathanpeppers force-pushed the fix-aapt-1134 branch from bbba4e4 to 55a2366 Compare December 30, 2017 04:14

dellis1972 closed this Jan 4, 2018

jonathanpeppers deleted the fix-aapt-1134 branch June 12, 2019 13:15

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

               			}
               		}
+              		[Test]

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

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

Uh oh!

Conversation

jonathanpeppers commented Dec 22, 2017

Uh oh!

jonathanpeppers commented Dec 22, 2017

Uh oh!

jonathanpeppers commented Dec 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonpryor commented Dec 22, 2017

Uh oh!

dellis1972 commented Dec 22, 2017 via email

Uh oh!

jonathanpeppers commented Dec 22, 2017

Uh oh!

dellis1972 commented Dec 22, 2017 via email

Uh oh!

jonpryor Dec 26, 2017

Choose a reason for hiding this comment

Uh oh!

dellis1972 Jan 2, 2018

Choose a reason for hiding this comment

Uh oh!

dellis1972 Jan 2, 2018

Choose a reason for hiding this comment

Uh oh!

dellis1972 commented Jan 2, 2018

Uh oh!

dellis1972 commented Jan 2, 2018

Uh oh!

jonathanpeppers commented Jan 2, 2018

Uh oh!

dellis1972 commented Jan 4, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jonathanpeppers commented Dec 22, 2017 •

edited

Loading