From 66318d9ae3cd2b8623990a85eb69b0ffa3d3c891 Mon Sep 17 00:00:00 2001 From: Jonathan Peppers Date: Thu, 9 Aug 2018 10:02:44 -0500 Subject: [PATCH] [Xamarin.Android.Build.Tasks] fix for parallel builds Context: https://github.com/xamarin/xamarin-android/issues/2043 When parallel builds occur, we can get `obj/Debug/lp` into a broken state: Resources/layout/abc_action_menu_layout.xml warning XA1001: AndroidResgen: Warning while updating Resource XML '/var/folders/nx/rzr6lcqj6j3_vp7vtp28v2cw0000gn/T/tmp6bafd8e4.tmp': Root element is missing. Then later, `` fails on the empty file: obj/Debug/lp/11/jl/res/layout/abc_action_menu_layout.xml(2,0): error APT0000: Error parsing XML: no element found It appears that `ConvertResourcesCases` still copies the file, even if `AndroidResource.UpdateXmlResource` fails. Subsequent builds will continue to fail, because an empty file has been copied with a newer timestamp. Once in this state, developers would have to `Rebuild` the project or delete `obj`. The fix here is to: - Return a `bool` indicating if `AndroidResource.UpdateXmlResource` succeeds - Don't write the final file if `AndroidResource.UpdateXmlResource` failed This change doesn't fully fix #2043, as I think VS for Mac should prevent builds from happening in parallel. I tried the change in the IDE once, and it worked. However, I feel like if I repeated the steps 10 times in a row, something would eventually go wrong. This change *does*, however, fix the issue for the user so a `Rebuild` is not required. Other changes: - The original temp file can use `File.Copy` since it is empty - We can use the `?` operator for invoking `logMessage` --- .../Tasks/ConvertResourcesCases.cs | 8 +++- .../Xamarin.Android.Build.Tests/BuildTest.cs | 44 +++++++++++++++++++ .../Utilities/AndroidResource.cs | 11 +++-- 3 files changed, 55 insertions(+), 8 deletions(-) diff --git a/src/Xamarin.Android.Build.Tasks/Tasks/ConvertResourcesCases.cs b/src/Xamarin.Android.Build.Tasks/Tasks/ConvertResourcesCases.cs index f41cafb8a08..c17520854f0 100644 --- a/src/Xamarin.Android.Build.Tasks/Tasks/ConvertResourcesCases.cs +++ b/src/Xamarin.Android.Build.Tasks/Tasks/ConvertResourcesCases.cs @@ -74,10 +74,10 @@ void FixupResources (ITaskItem item, Dictionary acwMap) } Log.LogDebugMessage (" Processing: {0} {1} > {2}", file, srcmodifiedDate, lastUpdate); var tmpdest = Path.GetTempFileName (); - MonoAndroidHelper.CopyIfChanged (file, tmpdest); + File.Copy (file, tmpdest, overwrite: true); MonoAndroidHelper.SetWriteable (tmpdest); try { - AndroidResource.UpdateXmlResource (resdir, tmpdest, acwMap, + bool success = AndroidResource.UpdateXmlResource (resdir, tmpdest, acwMap, ResourceDirectories.Where (s => s != item).Select(s => s.ItemSpec), (t, m) => { string targetfile = file; if (targetfile.StartsWith (resdir, StringComparison.InvariantCultureIgnoreCase)) { @@ -98,6 +98,10 @@ void FixupResources (ITaskItem item, Dictionary acwMap) break; } }); + if (!success) { + //If we failed to write the file, a warning is logged, we should skip to the next file + continue; + } // We strip away an eventual UTF-8 BOM from the XML file. // This is a requirement for the Android designer because the desktop Java renderer diff --git a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs index a8cd4d05122..0f1c6f2bcc4 100644 --- a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs +++ b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs @@ -55,6 +55,50 @@ public void BuildBasicApplicationReleaseFSharp () } } + [Test] + public void BuildInParallel () + { + var proj = new XamarinAndroidApplicationProject (); + proj.MainActivity = proj.DefaultMainActivity.Replace ("public class MainActivity : Activity", "public class MainActivity : Xamarin.Forms.Platform.Android.FormsAppCompatActivity"); + + var packages = proj.Packages; + packages.Add (KnownPackages.XamarinForms_3_0_0_561731); + packages.Add (KnownPackages.Android_Arch_Core_Common_26_1_0); + packages.Add (KnownPackages.Android_Arch_Lifecycle_Common_26_1_0); + packages.Add (KnownPackages.Android_Arch_Lifecycle_Runtime_26_1_0); + packages.Add (KnownPackages.AndroidSupportV4_27_0_2_1); + packages.Add (KnownPackages.SupportCompat_27_0_2_1); + packages.Add (KnownPackages.SupportCoreUI_27_0_2_1); + packages.Add (KnownPackages.SupportCoreUtils_27_0_2_1); + packages.Add (KnownPackages.SupportDesign_27_0_2_1); + packages.Add (KnownPackages.SupportFragment_27_0_2_1); + packages.Add (KnownPackages.SupportMediaCompat_27_0_2_1); + packages.Add (KnownPackages.SupportV7AppCompat_27_0_2_1); + packages.Add (KnownPackages.SupportV7CardView_27_0_2_1); + packages.Add (KnownPackages.SupportV7MediaRouter_27_0_2_1); + packages.Add (KnownPackages.SupportV7RecyclerView_27_0_2_1); + + using (var b = CreateApkBuilder (Path.Combine ("temp", TestName))) { + //We don't want these things stepping on each other + b.BuildLogFile = null; + b.Save (proj, saveProject: true); + proj.NuGetRestore (Path.Combine (Root, b.ProjectDirectory), b.PackagesDirectory); + + Parallel.For (0, 5, i => { + try { + //NOTE: things are going to break here + b.Build (proj); + } catch (Exception exc) { + TestContext.WriteLine ("Expected error in {0}: {1}", nameof (BuildInParallel), exc); + } + }); + + //The key here, is a build afterward should work + b.BuildLogFile = "after.log"; + Assert.IsTrue (b.Build (proj), "The build after a parallel failed build should succeed!"); + } + } + [Test] public void CheckKeystoreIsCreated () { diff --git a/src/Xamarin.Android.Build.Tasks/Utilities/AndroidResource.cs b/src/Xamarin.Android.Build.Tasks/Utilities/AndroidResource.cs index 082988ddcfe..3fbf06b93ba 100644 --- a/src/Xamarin.Android.Build.Tasks/Utilities/AndroidResource.cs +++ b/src/Xamarin.Android.Build.Tasks/Utilities/AndroidResource.cs @@ -11,7 +11,7 @@ namespace Monodroid { static class AndroidResource { - public static void UpdateXmlResource (string res, string filename, Dictionary acwMap, IEnumerable additionalDirectories = null, Action logMessage = null) + public static bool UpdateXmlResource (string res, string filename, Dictionary acwMap, IEnumerable additionalDirectories = null, Action logMessage = null) { // use a temporary file so we only update the real file if things actually changed string tmpfile = filename + ".bk"; @@ -24,14 +24,13 @@ public static void UpdateXmlResource (string res, string filename, Dictionary