New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Xamarin.Android.Build.Tasks] Add support for new InstantRun #609

Merged
merged 2 commits into from Sep 25, 2018

Conversation

Projects
None yet
6 participants
@dellis1972
Copy link
Contributor

dellis1972 commented May 26, 2017

This commit does a couple of things.

  1. It reworks the monodroid-glue to copy any .so file it finds
    in the .override directory over to the local app directory.
    This should allow us to fast deploy native libraries. Which means
    we no longer need to re-build the apk if a native library changes.

  2. Removes and replaces the StubApplication with a new MultiDexLoader
    ContentProvider. This provider is responsible for doing some the things
    the previous StubApplication used to do. But because it is not
    derived from an Application it means we should be able to support
    custom Application types for our debug builds. The MultiDexLoader will
    run before EVERYTHING else. This is so we can use dex files from the
    override directory.

  3. Move the Resource Patching code from StubApplication over to a
    dedicated MonkeyPatcher class so it is all in one place.

  4. Introduce a ResourceLoader ContentProvider which will deal with
    patching the resource in a FastDev environment. This is to support Custom
    Application classes
    .
    The problem we had with this is the resource patching requires that the application
    be created. But because the code was in the MultiDexLoader or StubApplication
    this could not happen since it would require the app to exist. So the solution is to move
    the patching to a dedicated ContentProvider which is loaded AFTER the user app has
    been created.

@dellis1972 dellis1972 requested a review from jonpryor May 26, 2017

@jonpryor

This comment has been minimized.

Copy link
Contributor

jonpryor commented May 26, 2017

This is failing .apk execution on the emulator:

java.lang.RuntimeException: Unable to get provider mono.android.MultiDexLoader: java.lang.ClassNotFoundException: Didn't find class "mono.android.MultiDexLoader" on path: DexPathList[[zip file "/data/app/Xamarin.Android.Locale_Tests-1/base.apk", zip file "/data/app/Xamarin.Android.Locale_Tests-1/base.apk"],nativeLibraryDirectories=[/data/app/Xamarin.Android.Locale_Tests-1/lib/x86, /data/app/Xamarin.Android.Locale_Tests-1/lib/x86, /vendor/lib, /system/lib]]
	at android.app.ActivityThread.installProvider(ActivityThread.java:4967)
	at android.app.ActivityThread.installContentProviders(ActivityThread.java:4559)
	at android.app.ActivityThread.handleBindApplication(ActivityThread.java:4499)
	at android.app.ActivityThread.access$1500(ActivityThread.java:144)
	at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1339)
	at android.os.Handler.dispatchMessage(Handler.java:102)
	at android.os.Looper.loop(Looper.java:135)
	at android.app.ActivityThread.main(ActivityThread.java:5221)
	at java.lang.reflect.Method.invoke(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:372)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:899)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:694)
Caused by: java.lang.ClassNotFoundException: Didn't find class "mono.android.MultiDexLoader" on path: DexPathList[[zip file "/data/app/Xamarin.Android.Locale_Tests-1/base.apk", zip file "/data/app/Xamarin.Android.Locale_Tests-1/base.apk"],nativeLibraryDirectories=[/data/app/Xamarin.Android.Locale_Tests-1/lib/x86, /data/app/Xamarin.Android.Locale_Tests-1/lib/x86, /vendor/lib, /system/lib]]
	at dalvik.system.BaseDexClassLoader.findClass(BaseDexClassLoader.java:56)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:511)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:469)
	at android.app.ActivityThread.installProvider(ActivityThread.java:4952)
	... 11 more
	Suppressed: java.lang.ClassNotFoundException: mono.android.MultiDexLoader
		at java.lang.Class.classForName(Native Method)
		at java.lang.BootClassLoader.findClass(ClassLoader.java:781)
		at java.lang.BootClassLoader.loadClass(ClassLoader.java:841)
		at java.lang.ClassLoader.loadClass(ClassLoader.java:504)
		... 13 more
	Caused by: java.lang.NoClassDefFoundError: Class not found using the boot class loader; no stack available

@dellis1972 dellis1972 force-pushed the dellis1972:instantrun branch 2 times, most recently from 202adfc to d40bc5d May 26, 2017

char *to_libmonoso = path_combine (to, "libmonosgen-2.0.so");
unlink (to_libmonoso);
char *to_file = path_combine (to, file);
unlink (to_file);

This comment has been minimized.

@jonpryor

jonpryor May 27, 2017

Contributor

I realize that the original code didn't do this...but we should probably cleanup anyway. ;-)

This should probably be:

int r = unlink (to_file);
if (r < 0 && errno != ENOENT) {
    log_warn (LOG_DEFAULT, "Unable to delete file `%s`", to_file);
}

Though this raises entire questions about ensuring that memory is freed...

Which means I now get to do what I keep suggesting on ##csharp! "How do we sanely cleanup resources without using goto? By abusing do{}while!"

static void
copy_file_to_internal_location (const char *to_dir, const char *from_dir, const char *file)
{
	char *from_file = path_combine (from_dir, file);
	char *to_file   = NULL;
	
	do {
		if (!from_file || !file_exists (from_file))
			break;

		log_warn (LOG_DEFAULT, "Copying file `%s` from external location `%s` to internal location `%s`",
				file, from_dir, to_dir);
		
		to_file = path_combine (to_dir, file);
		if (!to_file)
			break;
		
		int r = unlink (to_file);
		if (r < 0 && errno != ENOENT) {
			log_warn (LOG_DEFAULT, "Unable to delete file `%s`: %s", to_file, strerror (errno));
			break;
		}
		
		if (file_copy (to_file, from_file) < 0) {
			log_warn (LOG_DEFAULT, "Copy failed from `%s` to `%s`: %s", from_file, to_file, strerror (errno));
			break;
		}
		
		set_user_executable (to_file);
	} while (0);
	free (from_file);
	free (to_file);
}

This comment has been minimized.

@dellis1972

dellis1972 Jul 2, 2018

Contributor

@jonpryor do you still want me to implement this while loop?

if ((dir = monodroid_opendir (dir_path)) == NULL)
continue;

while (readdir_r (dir, &b, &e) == 0 && e) {

This comment has been minimized.

@jonpryor

jonpryor May 27, 2017

Contributor

I suspect that this will break on the Windows build; if we add the full-mono-integration-build Labe and rebuildl, we'll find out. ;-)

Assuming this is the case, we'll need to add a monodroid_readdir() function to util.c and use it here.


public class MultiDexLoader extends ContentProvider {

boolean created;

This comment has been minimized.

@jonpryor

jonpryor May 27, 2017

Contributor

I don't see why we need this field. It doesn't appear to be used anywhere, other than onCreate().

@dellis1972

This comment has been minimized.

Copy link
Contributor

dellis1972 commented May 27, 2017

build

@dellis1972 dellis1972 force-pushed the dellis1972:instantrun branch from d40bc5d to b3eb06c May 27, 2017

@dellis1972

This comment has been minimized.

Copy link
Contributor

dellis1972 commented May 27, 2017

@jonpryor I updated this and added the full build label. not sure if that did anything though? How can you tell?

@jonpryor

This comment has been minimized.

Copy link
Contributor

jonpryor commented May 30, 2017

How can you tell?

Normal PR build will only build one Mono.Android.dll, and ~3 ABIs for mono (if no bundle.zip exists): armv7, x86, "host".

full-mono-integration-build does everything that a non-PR build does, so all API levels, all the ABIs, both Debug and Release.

As such:

  1. The full-mono-integration build takes longer.
  2. If Mono was bumped, AOT-related tests more reliably pass, because the AOT compilers will exist. (In theory these should pass anyway, but historically they don't.)
  3. If you download the oss*.zip build artifact, you'll see all the Mono.Android.dll assemblies. Ditto if you read the the log file.
@jonpryor

This comment has been minimized.

Copy link
Contributor

jonpryor commented May 30, 2017

I think this PR should also have a new test, "somewhere/somehow", which sets $(AndroidEnableMultiDex)=True.

I'm less certain on how this should look; perhaps we could make e.g. src/Mono.Android/Test/*.csproj into a shared project+.csproj "combo", and add a new tests/Runtime-MultiDex directory which references the shared project?

Or perhaps just a new "Hello, World"-esque test would be sufficient to test MultiDex?

Either way, this new test should also contain an Android.App.Application subclass, to ensure that this works. See also https://bugzilla.xamarin.com/show_bug.cgi?id=55050.

char *to_libmonoso = path_combine (to, "libmonosgen-2.0.so");
unlink (to_libmonoso);
if (dir_path == NULL || !directory_exists (dir_path)) {
if (dir_path != NULL)

This comment has been minimized.

@jonpryor

jonpryor May 30, 2017

Contributor

No need to check for null. free(3) will accept the NULL pointer and ignore it:

The free() function.... If ptr is NULL, no operation is performed.

@dellis1972 dellis1972 force-pushed the dellis1972:instantrun branch 2 times, most recently from 5ac892a to 619b55c Jun 7, 2017

@dellis1972

This comment has been minimized.

Copy link
Contributor

dellis1972 commented Jun 7, 2017

build

@dellis1972 dellis1972 force-pushed the dellis1972:instantrun branch 3 times, most recently from 0a8566b to 5fb5ba2 Jun 8, 2017

@dellis1972 dellis1972 force-pushed the dellis1972:instantrun branch 2 times, most recently from a2ad397 to d91761a Jun 15, 2017

@dellis1972 dellis1972 force-pushed the dellis1972:instantrun branch from d91761a to 009a135 Jul 26, 2017

@dellis1972 dellis1972 force-pushed the dellis1972:instantrun branch from 009a135 to afc9bb6 Aug 4, 2017

@dellis1972 dellis1972 force-pushed the dellis1972:instantrun branch from afc9bb6 to 83c325b Sep 22, 2017

@dellis1972 dellis1972 force-pushed the dellis1972:instantrun branch from 83c325b to f418e73 Oct 31, 2017

@dellis1972 dellis1972 force-pushed the dellis1972:instantrun branch from 1442f8b to fcb2462 Sep 12, 2018

@dellis1972

This comment has been minimized.

Copy link
Contributor

dellis1972 commented Sep 13, 2018

build

@dellis1972 dellis1972 force-pushed the dellis1972:instantrun branch 2 times, most recently from 833152b to c0d01a5 Sep 14, 2018

@@ -0,0 +1,17 @@
<?xml version="1.0" encoding="utf-8"?>

This comment has been minimized.

@jonpryor

jonpryor Sep 17, 2018

Contributor

Why do we have this file? It doesn't appear to be used from anywhere, and it's confusing being alongside Main.axml.

@@ -0,0 +1,49 @@
<?xml version="1.0" encoding="utf-8"?>

This comment has been minimized.

@jonpryor

jonpryor Sep 17, 2018

Contributor

Why do we have this file? It doesn't appear to be used from anywhere, and it's confusing being alongside NameFixups.axml.

@jonpryor

This comment has been minimized.

Copy link
Contributor

jonpryor commented Sep 17, 2018

@dellis1972

This comment has been minimized.

Copy link
Contributor

dellis1972 commented Sep 17, 2018

@dellis1972 dellis1972 force-pushed the dellis1972:instantrun branch 3 times, most recently from 395d5ee to 4460c76 Sep 18, 2018

// Older platforms
Map activities = (HashMap) collection;
c = activities.values();
} else if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.KITKAT &&

This comment has been minimized.

@jonpryor

jonpryor Sep 18, 2018

Contributor

This will probably fail when building on e.g. API-10, as Build.VERSION_CODES.KITKAT won't exist.

@dellis1972

This comment has been minimized.

Copy link
Contributor

dellis1972 commented Sep 19, 2018

build

@dellis1972 dellis1972 force-pushed the dellis1972:instantrun branch 2 times, most recently from 5c76f63 to 97b3deb Sep 20, 2018

@dellis1972 dellis1972 force-pushed the dellis1972:instantrun branch from 97b3deb to 1f4ead5 Sep 22, 2018

@dellis1972

This comment has been minimized.

Copy link
Contributor

dellis1972 commented Sep 24, 2018

build

[Xamarim.Android.Build.Tasks] Add support for new InstantRun
This commit does a couple of things.

1) It reworks the monodroid-glue to copy any .so file it finds
in the .__override__ directory over to the local app directory.
This should allow us to fast deploy native libraries. Which means
we no longer need to re-build the apk if a native library changes.

2) Replaces the StubApplication with a new MultiDexLoader
ContentProvider. This provider is reposible for doing all the things
the previous StubApplication used to do. But because it is not
derived from an Application it means we can now support custom
Application types for our debug builds.

@dellis1972 dellis1972 force-pushed the dellis1972:instantrun branch from 7df5d89 to 5b5f668 Sep 25, 2018

@jonpryor jonpryor merged commit 3f360c0 into xamarin:master Sep 25, 2018

1 of 3 checks passed

Ubuntu PR Build Build started for merge commit.
Details
macOS PR Build Build triggered for merge commit.
Details
license/cla All CLA requirements met.
Details

jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this pull request Sep 27, 2018

[Xamarin.Android.Build.Tasks] CopyResource should fail with an error …
…code

Context: xamarin#609

In a recent change, we removed `StubApplication.java`, but downstream
in `monodroid` we had MSBuild tasks that still expected the file to
exist.

We got an error such as:

    Xamarin.Android.Common.Debugging.targets(319,2): error MSB4018: The "CopyResource" task failed unexpectedly.
    Xamarin.Android.Common.Debugging.targets(319,2): error MSB4018: System.NullReferenceException: Object reference not set to an instance of an object
    Xamarin.Android.Common.Debugging.targets(319,2): error MSB4018: at Xamarin.Android.Tasks.CopyResource.Run (System.Reflection.Assembly assm, System.String ResourceName, System.String OutputPath, Microsoft.Build.Utilities.TaskLoggingHelper Log) [0x00058] in <5782345e71104ee895a29d54bde93c43>:0
    Xamarin.Android.Common.Debugging.targets(319,2): error MSB4018: at Xamarin.Android.Tasks.CopyResource.Execute () [0x00018] in <5782345e71104ee895a29d54bde93c43>:0
    Xamarin.Android.Common.Debugging.targets(319,2): error MSB4018: at Microsoft.Build.BackEnd.TaskExecutionHost.Microsoft.Build.BackEnd.ITaskExecutionHost.Execute () [0x00023] in <528faf194f4946628868b84241d6ad15>:0
    Xamarin.Android.Common.Debugging.targets(319,2): error MSB4018: at Microsoft.Build.BackEnd.TaskBuilder+<ExecuteInstantiatedTask>d__26.MoveNext () [0x001f6] in <528faf194f4946628868b84241d6ad15>:0

The `CopyResource` MSBuild task should fail more gracefully: log a
coded error, and mention the file name that was missing.

In addition to this change:
- Added `CopyResourceTests` to verify resources that should be
  working, and the error code if it fails. We should add to the list
  if there are other files we want to verify are working. This test is
  very fast (<1s for entire class), because it doesn't need to build
  an entire project.
- Documented the `XA0116` error code.

jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this pull request Sep 28, 2018

[Xamarin.Android.Build.Tasks] CopyResource should fail with an error …
…code

Context: xamarin#609

In a recent change, we removed `StubApplication.java`, but downstream
in `monodroid` we had MSBuild tasks that still expected the file to
exist.

We got an error such as:

    Xamarin.Android.Common.Debugging.targets(319,2): error MSB4018: The "CopyResource" task failed unexpectedly.
    Xamarin.Android.Common.Debugging.targets(319,2): error MSB4018: System.NullReferenceException: Object reference not set to an instance of an object
    Xamarin.Android.Common.Debugging.targets(319,2): error MSB4018: at Xamarin.Android.Tasks.CopyResource.Run (System.Reflection.Assembly assm, System.String ResourceName, System.String OutputPath, Microsoft.Build.Utilities.TaskLoggingHelper Log) [0x00058] in <5782345e71104ee895a29d54bde93c43>:0
    Xamarin.Android.Common.Debugging.targets(319,2): error MSB4018: at Xamarin.Android.Tasks.CopyResource.Execute () [0x00018] in <5782345e71104ee895a29d54bde93c43>:0
    Xamarin.Android.Common.Debugging.targets(319,2): error MSB4018: at Microsoft.Build.BackEnd.TaskExecutionHost.Microsoft.Build.BackEnd.ITaskExecutionHost.Execute () [0x00023] in <528faf194f4946628868b84241d6ad15>:0
    Xamarin.Android.Common.Debugging.targets(319,2): error MSB4018: at Microsoft.Build.BackEnd.TaskBuilder+<ExecuteInstantiatedTask>d__26.MoveNext () [0x001f6] in <528faf194f4946628868b84241d6ad15>:0

The `CopyResource` MSBuild task should fail more gracefully: log a
coded error, and mention the file name that was missing.

In addition to this change:
- Added `CopyResourceTests` to verify resources that should be
  working, and the error code if it fails. We should add to the list
  if there are other files we want to verify are working. This test is
  very fast (<1s for entire class), because it doesn't need to build
  an entire project.
- Documented the `XA0116` error code.

dellis1972 added a commit that referenced this pull request Sep 28, 2018

[Xamarin.Android.Build.Tasks] CopyResource should fail with an error …
…code (#2237)

Context: #609

In a recent change, we removed `StubApplication.java`, but downstream
in `monodroid` we had MSBuild tasks that still expected the file to
exist.

We got an error such as:

    Xamarin.Android.Common.Debugging.targets(319,2): error MSB4018: The "CopyResource" task failed unexpectedly.
    Xamarin.Android.Common.Debugging.targets(319,2): error MSB4018: System.NullReferenceException: Object reference not set to an instance of an object
    Xamarin.Android.Common.Debugging.targets(319,2): error MSB4018: at Xamarin.Android.Tasks.CopyResource.Run (System.Reflection.Assembly assm, System.String ResourceName, System.String OutputPath, Microsoft.Build.Utilities.TaskLoggingHelper Log) [0x00058] in <5782345e71104ee895a29d54bde93c43>:0
    Xamarin.Android.Common.Debugging.targets(319,2): error MSB4018: at Xamarin.Android.Tasks.CopyResource.Execute () [0x00018] in <5782345e71104ee895a29d54bde93c43>:0
    Xamarin.Android.Common.Debugging.targets(319,2): error MSB4018: at Microsoft.Build.BackEnd.TaskExecutionHost.Microsoft.Build.BackEnd.ITaskExecutionHost.Execute () [0x00023] in <528faf194f4946628868b84241d6ad15>:0
    Xamarin.Android.Common.Debugging.targets(319,2): error MSB4018: at Microsoft.Build.BackEnd.TaskBuilder+<ExecuteInstantiatedTask>d__26.MoveNext () [0x001f6] in <528faf194f4946628868b84241d6ad15>:0

The `CopyResource` MSBuild task should fail more gracefully: log a
coded error, and mention the file name that was missing.

In addition to this change:
- Added `CopyResourceTests` to verify resources that should be
  working, and the error code if it fails. We should add to the list
  if there are other files we want to verify are working. This test is
  very fast (<1s for entire class), because it doesn't need to build
  an entire project.
- Documented the `XA0116` error code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment