From 2ede847952653cba9a8ab83c7ecc840c8ba73d2b Mon Sep 17 00:00:00 2001 From: Dean Ellis Date: Fri, 3 Nov 2017 06:56:55 +0000 Subject: [PATCH 1/8] [Xamarin.Android.Build.Tasks] Aapt MSBuild Task treats no default translation warnings as MSBuild errors 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" --- src/Xamarin.Android.Build.Tasks/Tasks/Aapt.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Xamarin.Android.Build.Tasks/Tasks/Aapt.cs b/src/Xamarin.Android.Build.Tasks/Tasks/Aapt.cs index 3ff06581597..a0790c5e68d 100644 --- a/src/Xamarin.Android.Build.Tasks/Tasks/Aapt.cs +++ b/src/Xamarin.Android.Build.Tasks/Tasks/Aapt.cs @@ -365,7 +365,7 @@ protected void LogEventsFromTextOutput (string singleLine, MessageImportance mes if (!string.IsNullOrEmpty (match.Groups["line"]?.Value)) line = int.Parse (match.Groups["line"].Value) + 1; var error = match.Groups["message"].Value; - if (error.Contains ("warning")) { + if (error.Contains ("warning") || singleLine.Contains ("warning")) { LogWarning (singleLine); return; } From 4b355fdd72fb7d31ae59e6402b2755116060b2eb Mon Sep 17 00:00:00 2001 From: Dean Ellis Date: Fri, 3 Nov 2017 07:32:26 +0000 Subject: [PATCH 2/8] Added a unit test --- .../AndroidUpdateResourcesTest.cs | 17 +++++++++++++++++ .../Xamarin.ProjectTools/Common/BuildActions.cs | 1 + .../Xamarin.ProjectTools/Common/BuildItem.cs | 12 ++++++++++++ 3 files changed, 30 insertions(+) diff --git a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/AndroidUpdateResourcesTest.cs b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/AndroidUpdateResourcesTest.cs index bd2905b787d..b1832367880 100644 --- a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/AndroidUpdateResourcesTest.cs +++ b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/AndroidUpdateResourcesTest.cs @@ -1083,5 +1083,22 @@ public void BuildAppWithManagedResourceParserAndLibraries () } } } + + [Test] + public void CheckDefaultTranslationWarnings () + { + var path = Path.Combine ("temp", TestName); + var proj = new XamarinAndroidApplicationProject () { + IsRelease = true, + OtherBuildItems = { + new BuildItem.Folder ("Resources\\values-fr\\") { + }, + }, + }; + using (var builder = CreateApkBuilder (path, false, false)) { + Assert.IsTrue (builder.Build (proj), "Build should have succeeded."); + StringAssert.Contains ("has no default translation", builder.LastBuildOutput, "Build output should contain a warning about 'no default translation'"); + } + } } } diff --git a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Common/BuildActions.cs b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Common/BuildActions.cs index 668f90adf32..c23af2dfcb1 100644 --- a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Common/BuildActions.cs +++ b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Common/BuildActions.cs @@ -15,5 +15,6 @@ public static class BuildActions public const string Compile = "Compile"; public const string EmbeddedResource = "EmbeddedResource"; public const string Content = "Content"; + public const string Folder = "Folder"; } } diff --git a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Common/BuildItem.cs b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Common/BuildItem.cs index ba5ec269195..bef93df1404 100644 --- a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Common/BuildItem.cs +++ b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Common/BuildItem.cs @@ -34,6 +34,18 @@ public NoActionResource (Func include) } } + public class Folder : BuildItem + { + public Folder (string include) + : this (() => include) + { + } + public Folder (Func include) + : base (BuildActions.Folder, include) + { + } + } + public class Content : BuildItem { public Content (string include) From 448a60f622d4ff7c844530a9c5bdedd485974b12 Mon Sep 17 00:00:00 2001 From: Dean Ellis Date: Fri, 3 Nov 2017 23:21:58 +0000 Subject: [PATCH 3/8] Updated based on feedback --- src/Xamarin.Android.Build.Tasks/Tasks/Aapt.cs | 17 +++++++++---- .../Tasks/AndroidToolTask.cs | 24 ++++++++++++++++++- .../AndroidUpdateResourcesTest.cs | 7 ++++++ 3 files changed, 42 insertions(+), 6 deletions(-) diff --git a/src/Xamarin.Android.Build.Tasks/Tasks/Aapt.cs b/src/Xamarin.Android.Build.Tasks/Tasks/Aapt.cs index a0790c5e68d..62492ba32ed 100644 --- a/src/Xamarin.Android.Build.Tasks/Tasks/Aapt.cs +++ b/src/Xamarin.Android.Build.Tasks/Tasks/Aapt.cs @@ -360,12 +360,19 @@ protected void LogEventsFromTextOutput (string singleLine, MessageImportance mes var match = AndroidToolTask.AndroidErrorRegex.Match (singleLine.Trim ()); if (match.Success) { + Console.WriteLine ($"\t path: '{match.Groups ["path"].Value}'"); + Console.WriteLine ($"\t file: '{match.Groups ["file"].Value}'"); + Console.WriteLine ($"\t line: '{match.Groups ["line"].Value}'"); + Console.WriteLine ($"\t level: '{match.Groups ["level"].Value}'"); + Console.WriteLine ($"\tmessage: '{match.Groups ["message"].Value}'"); + var file = match.Groups["file"].Value; int line = 0; if (!string.IsNullOrEmpty (match.Groups["line"]?.Value)) line = int.Parse (match.Groups["line"].Value) + 1; - var error = match.Groups["message"].Value; - if (error.Contains ("warning") || singleLine.Contains ("warning")) { + var level = match.Groups["level"].Value; + var message = match.Groups ["message"].Value; + if (level.Contains ("warning")) { LogWarning (singleLine); return; } @@ -379,10 +386,10 @@ protected void LogEventsFromTextOutput (string singleLine, MessageImportance mes } // Strip any "Error:" text from aapt's output - if (error.StartsWith ("error: ", StringComparison.InvariantCultureIgnoreCase)) - error = error.Substring ("error: ".Length); + if (message.StartsWith ("error: ", StringComparison.InvariantCultureIgnoreCase)) + message = message.Substring ("error: ".Length); - LogError ("APT0000", error, file, line); + LogError ("APT0000", message, file, line); return; } diff --git a/src/Xamarin.Android.Build.Tasks/Tasks/AndroidToolTask.cs b/src/Xamarin.Android.Build.Tasks/Tasks/AndroidToolTask.cs index b53d4322f4d..71f213ecaa4 100644 --- a/src/Xamarin.Android.Build.Tasks/Tasks/AndroidToolTask.cs +++ b/src/Xamarin.Android.Build.Tasks/Tasks/AndroidToolTask.cs @@ -83,7 +83,29 @@ protected virtual Regex ErrorRegex { internal static Regex AndroidErrorRegex { get { if (androidErrorRegex == null) - androidErrorRegex = new Regex (@"^(?.+?)([:(](?\d+)[:)])?:+\s*((error)\s*(?\w*(?=:)):?)?(?.*)", RegexOptions.Compiled | RegexOptions.ExplicitCapture); + androidErrorRegex = new Regex (@" +^ +( # start optional path followed by `:` + (? + (?.+[\\/][^:\(]+) + ( + ([:](?\d+)) + | + (\((?\d+)\)) + )? + ) + \s* + : +)? +( # optional warning|error: + \s* + (?(warning|error)[^:]*)\s* + : +)? +\s* +(?.*) +$ +", RegexOptions.Compiled | RegexOptions.ExplicitCapture | RegexOptions.IgnorePatternWhitespace); return androidErrorRegex; } } diff --git a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/AndroidUpdateResourcesTest.cs b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/AndroidUpdateResourcesTest.cs index b1832367880..a0ea8e8bcc3 100644 --- a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/AndroidUpdateResourcesTest.cs +++ b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/AndroidUpdateResourcesTest.cs @@ -1095,6 +1095,13 @@ public void CheckDefaultTranslationWarnings () }, }, }; + + proj.AndroidResources.Add (new AndroidItem.AndroidResource ("Resources\\values-fr\\Strings.xml") { + TextContent = () => @" + + Test +", + }); using (var builder = CreateApkBuilder (path, false, false)) { Assert.IsTrue (builder.Build (proj), "Build should have succeeded."); StringAssert.Contains ("has no default translation", builder.LastBuildOutput, "Build output should contain a warning about 'no default translation'"); From 1338023e6496f267e077bb00cc24ced2ec384fc4 Mon Sep 17 00:00:00 2001 From: Dean Ellis Date: Tue, 7 Nov 2017 19:06:02 +0000 Subject: [PATCH 4/8] Removed Debugging Output --- src/Xamarin.Android.Build.Tasks/Tasks/Aapt.cs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/Xamarin.Android.Build.Tasks/Tasks/Aapt.cs b/src/Xamarin.Android.Build.Tasks/Tasks/Aapt.cs index 62492ba32ed..3d02828977f 100644 --- a/src/Xamarin.Android.Build.Tasks/Tasks/Aapt.cs +++ b/src/Xamarin.Android.Build.Tasks/Tasks/Aapt.cs @@ -360,12 +360,6 @@ protected void LogEventsFromTextOutput (string singleLine, MessageImportance mes var match = AndroidToolTask.AndroidErrorRegex.Match (singleLine.Trim ()); if (match.Success) { - Console.WriteLine ($"\t path: '{match.Groups ["path"].Value}'"); - Console.WriteLine ($"\t file: '{match.Groups ["file"].Value}'"); - Console.WriteLine ($"\t line: '{match.Groups ["line"].Value}'"); - Console.WriteLine ($"\t level: '{match.Groups ["level"].Value}'"); - Console.WriteLine ($"\tmessage: '{match.Groups ["message"].Value}'"); - var file = match.Groups["file"].Value; int line = 0; if (!string.IsNullOrEmpty (match.Groups["line"]?.Value)) From 52a55d9db7fe2a1ebf1acd222520fda22242c878 Mon Sep 17 00:00:00 2001 From: Dean Ellis Date: Wed, 8 Nov 2017 20:18:41 +0000 Subject: [PATCH 5/8] Make AndroidErrorRegex public --- src/Xamarin.Android.Build.Tasks/Tasks/AndroidToolTask.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Xamarin.Android.Build.Tasks/Tasks/AndroidToolTask.cs b/src/Xamarin.Android.Build.Tasks/Tasks/AndroidToolTask.cs index 71f213ecaa4..18f66b83916 100644 --- a/src/Xamarin.Android.Build.Tasks/Tasks/AndroidToolTask.cs +++ b/src/Xamarin.Android.Build.Tasks/Tasks/AndroidToolTask.cs @@ -80,7 +80,7 @@ protected virtual Regex ErrorRegex { // res/drawable/foo-bar.jpg: Invalid file name: must contain only [a-z0-9_.] // Look for them and convert them to MSBuild compatible errors. static Regex androidErrorRegex; - internal static Regex AndroidErrorRegex { + public static Regex AndroidErrorRegex { get { if (androidErrorRegex == null) androidErrorRegex = new Regex (@" From 0a4b04ca49b1c929f4efc646a74c62baabc1f0be Mon Sep 17 00:00:00 2001 From: Dean Ellis Date: Wed, 8 Nov 2017 20:23:07 +0000 Subject: [PATCH 6/8] Add Xamarin.Android.Build.Tasks to the Tests --- .../Xamarin.Android.Build.Tests.csproj | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Xamarin.Android.Build.Tests.csproj b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Xamarin.Android.Build.Tests.csproj index 11100590be9..76c178f456b 100644 --- a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Xamarin.Android.Build.Tests.csproj +++ b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Xamarin.Android.Build.Tests.csproj @@ -49,6 +49,10 @@ {E248B2CA-303B-4645-ADDC-9D4459D550FD} libZipSharp + + {3F1F2F50-AF1A-4A5A-BEDB-193372F068D7} + Xamarin.Android.Build.Tasks + From e6707950e409a303c61a0f506308f5474b23c3d6 Mon Sep 17 00:00:00 2001 From: Dean Ellis Date: Wed, 8 Nov 2017 20:52:41 +0000 Subject: [PATCH 7/8] Added AndroidRegExTests.cs --- .../Xamarin.Android.Build.Tests/AndroidRegExTests.cs | 10 ++++++++++ .../Xamarin.Android.Build.Tests.csproj | 11 +++++++---- 2 files changed, 17 insertions(+), 4 deletions(-) create mode 100644 src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/AndroidRegExTests.cs diff --git a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/AndroidRegExTests.cs b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/AndroidRegExTests.cs new file mode 100644 index 00000000000..8dba835c48f --- /dev/null +++ b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/AndroidRegExTests.cs @@ -0,0 +1,10 @@ +using System; +namespace Xamarin.Android.Build.Tests +{ + public class AndroidRegExTests + { + public AndroidRegExTests () + { + } + } +} diff --git a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Xamarin.Android.Build.Tests.csproj b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Xamarin.Android.Build.Tests.csproj index 76c178f456b..93fcd3d77a0 100644 --- a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Xamarin.Android.Build.Tests.csproj +++ b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Xamarin.Android.Build.Tests.csproj @@ -38,6 +38,12 @@ ..\..\..\..\packages\NUnit.3.7.1\lib\net45\nunit.framework.dll + + $(OutputPath)..\$(Configuration)\lib\xamarin.android\xbuild\Xamarin\Android\Xamarin.Android.Build.Tasks.dll + + + + @@ -49,10 +55,6 @@ {E248B2CA-303B-4645-ADDC-9D4459D550FD} libZipSharp - - {3F1F2F50-AF1A-4A5A-BEDB-193372F068D7} - Xamarin.Android.Build.Tasks - @@ -63,5 +65,6 @@ + From 88f5eb576fd1a8540777976ecced9aa17cbf921b Mon Sep 17 00:00:00 2001 From: Dean Ellis Date: Thu, 9 Nov 2017 14:36:15 +0000 Subject: [PATCH 8/8] Added Tests for AndroidRegEx --- .../AndroidRegExTests.cs | 70 ++++++++++++++++++- .../Xamarin.Android.Build.Tests.csproj | 8 ++- 2 files changed, 76 insertions(+), 2 deletions(-) diff --git a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/AndroidRegExTests.cs b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/AndroidRegExTests.cs index 8dba835c48f..ab9162330f0 100644 --- a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/AndroidRegExTests.cs +++ b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/AndroidRegExTests.cs @@ -1,10 +1,78 @@ using System; +using System.Collections; +using System.Collections.Generic; +using NUnit.Framework; + namespace Xamarin.Android.Build.Tests { public class AndroidRegExTests { - public AndroidRegExTests () + + + /* +"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_.]" + + */ + class AndroidRegExTestsCases : IEnumerable { + public IEnumerator GetEnumerator () + { + yield return new object [] { + /*message*/ "warning: string 'app_name1' has no default translation.", + /*expectedToMatch*/ true, + /*expectedFile*/ "", + /*expectedLine*/ "", + /*expectedLevel*/ "warning", + /*expectedMessage*/ "string 'app_name1' has no default translation." + }; + yield return new object [] { + /*message*/ "res\\layout\\main.axml: error: No resource identifier found for attribute \"id2\" in package \"android\" (TaskId:22)", + /*expectedToMatch*/ true, + /*expectedFile*/ "res\\layout\\main.axml", + /*expectedLine*/ "", + /*expectedLevel*/ "error", + /*expectedMessage*/ "No resource identifier found for attribute \"id2\" in package \"android\" (TaskId:22)" + }; + yield return new object [] { + /*message*/ "Resources/values/theme.xml(2): error APT0000: Error retrieving parent for item: No resource found that matches the given name '@android:style/Theme.AppCompat'.", + /*expectedToMatch*/ true, + /*expectedFile*/ "Resources/values/theme.xml", + /*expectedLine*/ "2", + /*expectedLevel*/ "error APT0000", + /*expectedMessage*/ "Error retrieving parent for item: No resource found that matches the given name '@android:style/Theme.AppCompat'." + }; + yield return new object [] { + /*message*/ "Resources/values/theme.xml:2: error APT0000: Error retrieving parent for item: No resource found that matches the given name '@android:style/Theme.AppCompat'.", + /*expectedToMatch*/ true, + /*expectedFile*/ "Resources/values/theme.xml", + /*expectedLine*/ "2", + /*expectedLevel*/ "error APT0000", + /*expectedMessage*/ "Error retrieving parent for item: No resource found that matches the given name '@android:style/Theme.AppCompat'." + }; + yield return new object [] { + /*message*/ "res/drawable/foo-bar.jpg: Invalid file name: must contain only [a-z0-9_.]", + /*expectedToMatch*/ true, + /*expectedFile*/ "res/drawable/foo-bar.jpg", + /*expectedLine*/ "", + /*expectedLevel*/ "", + /*expectedMessage*/ "Invalid file name: must contain only [a-z0-9_.]" + }; + + } + } + + [Test] + [TestCaseSource(typeof (AndroidRegExTestsCases))] + public void RegExTests(string message, bool expectedToMatch, string expectedFile, string expectedLine, string expectedLevel, string expextedMessage) { + var regex = Xamarin.Android.Tasks.AndroidToolTask.AndroidErrorRegex; + var result = regex.Match (message); + Assert.AreEqual (expectedToMatch,result.Success); + Assert.AreEqual (expectedFile, result.Groups["file"].Value); + Assert.AreEqual (expectedLine.ToString (), result.Groups ["line"].Value); + Assert.AreEqual (expectedLevel, result.Groups ["level"].Value); + Assert.AreEqual (expextedMessage, result.Groups ["message"].Value); } } } diff --git a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Xamarin.Android.Build.Tests.csproj b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Xamarin.Android.Build.Tests.csproj index 93fcd3d77a0..9a04c0c8228 100644 --- a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Xamarin.Android.Build.Tests.csproj +++ b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Xamarin.Android.Build.Tests.csproj @@ -38,9 +38,11 @@ ..\..\..\..\packages\NUnit.3.7.1\lib\net45\nunit.framework.dll - + @@ -55,6 +57,10 @@ {E248B2CA-303B-4645-ADDC-9D4459D550FD} libZipSharp + + {3F1F2F50-AF1A-4A5A-BEDB-193372F068D7} + Xamarin.Android.Build.Tasks +