[Xamarin.Android.Build.Tasks] Add ConvertCustomView Task #2129

dellis1972 · 2018-08-31T16:41:47Z

Currently ConvertResourcesCases is called twice on ALL
resources referenced by the project. This is terribly inefficient.
However it is required. The first pass is done before a Compile,
this fixes up the casing for things like drawables etc ready to
be processed by aapt. The second is done after GenerateJavaStubs
which happens AFTER the Compile step. This is to replace any
custom view references with the correct {md5}.View style references.
This is because we replace the normal readable namespace with an md5 hash.

The problem was that ConvertResourcesCases is doing a TON of
work it doesn't really need to do the second time around. For example
checking if it needs to lower case names of items. The second pass
really only needs to worry about custom views. In addition to that
it was also re-scanning ALL the files again.

This commit introduces a new Task ConvertCustomView. The sole
purpose of this task is to fix up the layout files which contain
custom views. It does nothing else. To make this even quicker we
modify ConvertResourcesCases to emit a mapping file (class-map.txt). This file
contains items like

MonoDroid.Example.MyLayout;/fullpath/to/file.xml
android.support.v7.widget.ActionBarOverlayLayout;/fullpath/to/some/other/file.xml

This allows us to know were the files are which contain ANY layout.

With this information in conjuction with the acw_map.txt file will
allow us to do a targeted update. So we go through the acw_map.txt
values and fix up those files where we have entires in the class-map.txt.

This reduces the amount of time spent processing files quite a bit.
For a Blank Xamarin Forms app from a clean build.

2639 ms  ConvertResourcesCases                      1 calls
   3 ms  ConvertCustomView                          1 calls

Normally the ConvertResourcesCases would be called twice and would
take a total of 5-6 seconds.

Fix up Paths in Error Message
Write a decent Commit Message
General Clean up.

dellis1972 · 2018-09-03T19:39:45Z

@jonathanpeppers we need a VSTS build on this. I'm not sure if the tempdst change will break stuff on windows. I will try to test locally tomorrow

jonathanpeppers · 2018-09-04T02:06:37Z

@dellis1972 I started one.

I had to update $(XABundleDownloadPrefix) to 'https://xamjenkinsartifact.azureedge.net/mono-jenkins/xamarin-android-1154/xamarin-android/bin/Debug', but it seemed like the first try failed building master. We'll see what happens.

jonathanpeppers · 2018-09-04T13:06:10Z

One test is failing on Windows:

Xamarin.Android.Build.Tests.ConvertResourcesCasesTests.CheckClassIsNotReplacedWithMd5 / Debug

System.ArgumentNullException : Value cannot be null.
Parameter name: source

at System.Linq.Enumerable.FirstOrDefault[TSource](IEnumerable`1 source, Func`2 predicate)
   at Xamarin.Android.Tasks.ConvertCustomView.<>c__DisplayClass16_1.<Execute>b__0(TraceLevel t, String m) in E:\A\_work\14\s\src\Xamarin.Android.Build.Tasks\Tasks\ConvertCustomView.cs:line 50
   at Xamarin.Android.Tasks.ConvertCustomView.TryFixCustomView(XElement elem, Dictionary`2 acwMap, Action`2 logMessage) in E:\A\_work\14\s\src\Xamarin.Android.Build.Tasks\Tasks\ConvertCustomView.cs:line 147
   at Xamarin.Android.Tasks.ConvertCustomView.Execute() in E:\A\_work\14\s\src\Xamarin.Android.Build.Tasks\Tasks\ConvertCustomView.cs:line 48
   at Xamarin.Android.Build.Tests.ConvertResourcesCasesTests.CheckClassIsNotReplacedWithMd5() in E:\A\_work\14\s\src\Xamarin.Android.Build.Tasks\Tests\Xamarin.Android.Build.Tests\ConvertResourcesCasesTests.cs:line 91

dellis1972 · 2018-09-04T13:17:53Z

Ah yes, I need to update the unit tests to provide the new values (or check for nulls)

…

On Tue, 4 Sep 2018 at 14:06, Jonathan Peppers ***@***.***> wrote: One test is failing on Windows: Xamarin.Android.Build.Tests.ConvertResourcesCasesTests.CheckClassIsNotReplacedWithMd5 / Debug System.ArgumentNullException : Value cannot be null. Parameter name: source at System.Linq.Enumerable.FirstOrDefault[TSource](IEnumerable`1 source, Func`2 predicate) at Xamarin.Android.Tasks.ConvertCustomView.<>c__DisplayClass16_1.<Execute>b__0(TraceLevel t, String m) in E:\A\_work\14\s\src\Xamarin.Android.Build.Tasks\Tasks\ConvertCustomView.cs:line 50 at Xamarin.Android.Tasks.ConvertCustomView.TryFixCustomView(XElement elem, Dictionary`2 acwMap, Action`2 logMessage) in E:\A\_work\14\s\src\Xamarin.Android.Build.Tasks\Tasks\ConvertCustomView.cs:line 147 at Xamarin.Android.Tasks.ConvertCustomView.Execute() in E:\A\_work\14\s\src\Xamarin.Android.Build.Tasks\Tasks\ConvertCustomView.cs:line 48 at Xamarin.Android.Build.Tests.ConvertResourcesCasesTests.CheckClassIsNotReplacedWithMd5() in E:\A\_work\14\s\src\Xamarin.Android.Build.Tasks\Tests\Xamarin.Android.Build.Tests\ConvertResourcesCasesTests.cs:line 91 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2129 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAxeeY0FsJkbGwVORitPERXhqLy3v2WEks5uXnrJgaJpZM4WVh5S> .

jonathanpeppers · 2018-09-04T13:18:33Z

src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.cs

+					map.Add (items [0], new HashSet<string> ());
+				if (!map[items [0]].Contains (items [1]))
+					map [items [0]].Add (items [1]);
+			}


If there are a lot of items in this dictionary, we should use some local variable so indexers aren't called as much.

Would help perf a little to be something like:

foreach (var s in File.ReadLines (mapFile)) { var split = s.Split (';'); var key = split [0]; var value = split [1]; if (!map.TryGetValue (key, out HashSet<string> items)) { map [key] = items = new HashSet<string> (); } if (!items.Contains (value)) { items.Add (value); } }

jonpryor · 2018-09-04T15:08:38Z

src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.cs

+				var value = items [1];
+				if (!map.ContainsKey (key))
+					map.Add (key, new HashSet<string> ());
+				if (!map[key].Contains (value))


You don't need .Contains() here, as it's a HashSet<T>. Just always call map [key].Add(value).

jonpryor · 2018-09-04T15:10:35Z

src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.cs

+				var items = s.Split (';');
+				var key = items [0];
+				var value = items [1];
+				if (!map.ContainsKey (key))


This should instead do:

HashSet<string> set; if (!map.TryGetValue (key, out set)) { map.Add (key, set = new HashSet<string>(); } set.Add (value);

That is some nice code :)

jonpryor · 2018-09-04T15:18:38Z

src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.cs

+			if (!File.Exists (mapFile))
+				return map;
+			foreach (var s in File.ReadLines (mapFile)) {
+				var items = s.Split (';');


Don't use string.Split(char[]); use string.Split(char[], int):

var items = s.Split (new[]{';'}, 2);

This way if the filesystem path contains a ; it'll behave sanely.

dellis1972 · 2018-09-04T15:23:38Z

need to change the name from class-map to customview-map and the associated Load/Save methods.

jonpryor · 2018-09-04T15:25:22Z

src/Xamarin.Android.Build.Tasks/Tasks/ConvertResourcesCases.cs

@@ -104,7 +107,14 @@ void FixupResources (ITaskItem item, Dictionary<string, string> acwMap)
 									Log.LogDebugMessage (m);
 									break;
 							}
-						});
+					}, (e, filename) => {


Would it be possible to use named parameters here? I don't know what this callback is supposed to do.

}, customViewMapCallback: (e, filename) => { ...

jonpryor · 2018-09-04T19:18:15Z

src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.cs

+			var temp = Path.GetTempFileName ();
+			try {
+				using (var m = new StreamWriter (temp)) {
+					foreach (var i in map) {


This should contain a map.OrderBy (k => k.Key) or something. Enumeration order of Dictionaries is undefined, so it's possible that two "identical" Dictionary<string, HashSet,string>> values will result in different orders in m, which means the CopyIfChanged() (below) becomes "useless."

jonpryor · 2018-09-04T19:18:55Z

src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.cs

+			try {
+				using (var m = new StreamWriter (temp)) {
+					foreach (var i in map) {
+						foreach (var v in i.Value)


Similarly, enumeration order of a HashSet<string> is undefined, so this should be i.Value.OrderBy (f => f) so that we can produce consistent files.

Fixed dotnet#2100 Currently `ConvertResourcesCases` is called twice on ALL resources referenced by the project. This is terribly inefficient. However it is required. The first pass is done before a `Compile`, this fixes up the casing for things like drawables etc ready to be processed by aapt. The second is done after `GenerateJavaStubs` which happens AFTER the `Compile` step. This is to replace any custom view references with the correct `{md5}.View` style references. This is because we replace the normal readable namespace with an md5 hash. The problem was that `ConvertResourcesCases` is doing a TON of work it doesn't really need to do the second time around. For example checking if it needs to lower case names of items. The second pass really only needs to worry about custom views. In addition to that it was also re-scanning ALL the files again. This commit introduces a new Task `ConvertCustomView`. The sole purpose of this task is to fix up the layout files which contain custom views. It does nothing else. To make this even quicker we modify `ConvertResourcesCases` to emit a mapping file (class-map.txt). This file contains items like MonoDroid.Example.MyLayout;/fullpath/to/file.xml android.support.v7.widget.ActionBarOverlayLayout;/fullpath/to/some/other/file.xml This allows us to know were the files are which contain ANY layout. With this information in conjuction with the `acw_map.txt` file will allow us to do a targeted update. So we go through the `acw_map.txt` values and fix up those files where we have entires in the `class-map.txt`. This reduces the amount of time spent processing files quite a bit. For a Blank Xamarin Forms app from a clean build. 2639 ms ConvertResourcesCases 1 calls 3 ms ConvertCustomView 1 calls Normally the `ConvertResourcesCases` would be called twice and would take a total of 5-6 seconds.

src/Xamarin.Android.Build.Tasks/Utilities/AndroidResource.cs

src/Xamarin.Android.Build.Tasks/Tasks/ConvertCustomView.cs

StephaneDelcroix · 2018-09-06T09:12:00Z

src/Xamarin.Android.Build.Tasks/Tasks/CopyAndConvertResources.cs

@@ -85,11 +98,32 @@ public override bool Execute ()
 				var dstmodifiedDate = File.Exists (destfilename) ? File.GetLastWriteTimeUtc (destfilename) : DateTime.MinValue;
 				var tmpdest = Path.GetTempFileName ();


I don't see this file being used before it gets deleted

Context: dotnet#2088 Context: dotnet#2129 Context: dotnet#2199 In 4deec52, I fixed the #deletebinobj problem we discovered. However... I introduced a regression to incremental builds, I noticed that the `_CompileJava` target is now running on a second build with no changes. Third build? oddly it gets skipped... It seems to be due to our use of flag files: 1. `_UpdateAndroidResgen` updates `R.cs.flag` and uses the file as an output 2. `_GenerateJavaDesignerForComponent` now uses `R.cs.flag` as an input 3. `_GenerateJavaStubs` *also* updates the timestamp on `R.cs.flag`. This was added in 970da9e, as a workaround for our two instances of `ConvertResourcesCases`. 4. `_GenerateJavaDesignerForComponent` will now run again on the next build. Since 1886e6f eliminated the second call to `ConvertResourcesCases`, we don't need to update `R.cs.flag` in no. 3 any longer. Removing the call to `<Touch />` `R.cs.flag` in `_GenerateJavaStubs` fixed the issue, and I added some assertions in relevant tests to check that the `_CompileJava` and `_GenerateJavaDesignerForComponent` targets aren't running on incremental builds.

Context: #2088 Context: #2129 Context: #2199 In 4deec52, I fixed the #deletebinobj problem we discovered. However... I introduced a regression to incremental builds, I noticed that the `_CompileJava` target is now running on a second build with no changes. Third build? oddly it gets skipped... It seems to be due to our use of flag files: 1. `_UpdateAndroidResgen` updates `R.cs.flag` and uses the file as an output 2. `_GenerateJavaDesignerForComponent` now uses `R.cs.flag` as an input 3. `_GenerateJavaStubs` *also* updates the timestamp on `R.cs.flag`. This was added in 970da9e, as a workaround for our two instances of `ConvertResourcesCases`. 4. `_GenerateJavaDesignerForComponent` will now run again on the next build. Since 1886e6f eliminated the second call to `ConvertResourcesCases`, we don't need to update `R.cs.flag` in no. 3 any longer. Removing the call to `<Touch />` `R.cs.flag` in `_GenerateJavaStubs` fixed the issue, and I added some assertions in relevant tests to check that the `_CompileJava` and `_GenerateJavaDesignerForComponent` targets aren't running on incremental builds.

dellis1972 added the do-not-merge PR should not be merged. label Aug 31, 2018

dellis1972 changed the title ~~[Xamarin.Android.Build.Tasks] Add ConvertCustomView Task [WIP~~ [Xamarin.Android.Build.Tasks] Add ConvertCustomView Task [WIP] Aug 31, 2018

dellis1972 force-pushed the Issue2100 branch 4 times, most recently from fcd102e to 10d1f6c Compare September 3, 2018 19:34

dellis1972 requested a review from jonathanpeppers September 3, 2018 19:39

dellis1972 removed the do-not-merge PR should not be merged. label Sep 3, 2018

dellis1972 changed the title ~~[Xamarin.Android.Build.Tasks] Add ConvertCustomView Task [WIP]~~ [Xamarin.Android.Build.Tasks] Add ConvertCustomView Task Sep 3, 2018

jonathanpeppers reviewed Sep 4, 2018

View reviewed changes

dellis1972 force-pushed the Issue2100 branch 3 times, most recently from ec65bdc to fca3b91 Compare September 4, 2018 14:29

jonpryor reviewed Sep 4, 2018

View reviewed changes

dellis1972 force-pushed the Issue2100 branch from fca3b91 to 909b727 Compare September 4, 2018 15:58

jonathanpeppers approved these changes Sep 4, 2018

View reviewed changes

jonpryor reviewed Sep 4, 2018

View reviewed changes

dellis1972 force-pushed the Issue2100 branch 2 times, most recently from db4cdf7 to ae22049 Compare September 5, 2018 08:33

dellis1972 force-pushed the Issue2100 branch from ae22049 to f8a31a5 Compare September 5, 2018 10:21

StephaneDelcroix reviewed Sep 5, 2018

View reviewed changes

src/Xamarin.Android.Build.Tasks/Utilities/AndroidResource.cs Show resolved Hide resolved

jonpryor requested changes Sep 5, 2018

View reviewed changes

src/Xamarin.Android.Build.Tasks/Tasks/ConvertCustomView.cs Outdated Show resolved Hide resolved

jonpryor reviewed Sep 5, 2018

View reviewed changes

src/Xamarin.Android.Build.Tasks/Tasks/ConvertCustomView.cs Outdated Show resolved Hide resolved

dellis1972 added 2 commits September 5, 2018 16:18

Updated to unify the logging a bit

c9489ee

Fixed up Formatting

59fc89b

jonpryor merged commit 1886e6f into dotnet:master Sep 5, 2018

StephaneDelcroix reviewed Sep 6, 2018

View reviewed changes

jonathanpeppers mentioned this pull request Sep 21, 2018

[Xamarin.Android.Build.Tasks] fix _CompileJava running too often #2207

Merged

dellis1972 mentioned this pull request Oct 7, 2020

[Xamarin.Android.Build.Tasks] remove acw-map.txt usage in <ConvertResourcesCases/> #5184

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Xamarin.Android.Build.Tasks] Add ConvertCustomView Task #2129

[Xamarin.Android.Build.Tasks] Add ConvertCustomView Task #2129

dellis1972 commented Aug 31, 2018 •

edited

Loading

dellis1972 commented Sep 3, 2018

jonathanpeppers commented Sep 4, 2018

jonathanpeppers commented Sep 4, 2018

dellis1972 commented Sep 4, 2018 via email

jonathanpeppers Sep 4, 2018

jonpryor Sep 4, 2018

jonpryor Sep 4, 2018

dellis1972 Sep 4, 2018

jonpryor Sep 4, 2018 •

edited

Loading

dellis1972 commented Sep 4, 2018

jonpryor Sep 4, 2018 •

edited

Loading

jonpryor Sep 4, 2018

jonpryor Sep 4, 2018

StephaneDelcroix Sep 6, 2018

		@@ -85,11 +98,32 @@ public override bool Execute ()
		var dstmodifiedDate = File.Exists (destfilename) ? File.GetLastWriteTimeUtc (destfilename) : DateTime.MinValue;
		var tmpdest = Path.GetTempFileName ();

[Xamarin.Android.Build.Tasks] Add ConvertCustomView Task #2129

[Xamarin.Android.Build.Tasks] Add ConvertCustomView Task #2129

Conversation

dellis1972 commented Aug 31, 2018 • edited Loading

dellis1972 commented Sep 3, 2018

jonathanpeppers commented Sep 4, 2018

jonathanpeppers commented Sep 4, 2018

dellis1972 commented Sep 4, 2018 via email

jonathanpeppers Sep 4, 2018

Choose a reason for hiding this comment

jonpryor Sep 4, 2018

Choose a reason for hiding this comment

jonpryor Sep 4, 2018

Choose a reason for hiding this comment

dellis1972 Sep 4, 2018

Choose a reason for hiding this comment

jonpryor Sep 4, 2018 • edited Loading

Choose a reason for hiding this comment

dellis1972 commented Sep 4, 2018

jonpryor Sep 4, 2018 • edited Loading

Choose a reason for hiding this comment

jonpryor Sep 4, 2018

Choose a reason for hiding this comment

jonpryor Sep 4, 2018

Choose a reason for hiding this comment

StephaneDelcroix Sep 6, 2018

Choose a reason for hiding this comment

dellis1972 commented Aug 31, 2018 •

edited

Loading

jonpryor Sep 4, 2018 •

edited

Loading

jonpryor Sep 4, 2018 •

edited

Loading