Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[One .NET] use API 31 "ref" pack with 32 "runtime" pack #6647

Merged
merged 1 commit into from
Jan 19, 2022
Merged

[One .NET] use API 31 "ref" pack with 32 "runtime" pack #6647

merged 1 commit into from
Jan 19, 2022

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Jan 14, 2022 •
edited
Loading

After 6eb11f1 went in, I realized an issue when using net6.0-android31:

  1. You get API 31 by default. We bring in these packs from NuGet.org
    with the version 31.0.101-preview.11.117:

    Microsoft.Android.Ref.31
    Microsoft.Android.Runtime.31.[rid]

  2. So now you can't get the latest changes for libmonodroid.so, for
    example? You'd have to opt-in via net6.0-android32.

Thinking about it more, we should only need to use the API 31 "ref" or
"targeting" pack. The "runtime" pack can just use the latest from the
workload.

To fix this:

  1. Create new $(_AndroidTargetingPackId) and
    $(_AndroidTargetingPackVersion) properties to use independently
    of the runtime pack version.

  2. Remove Microsoft.Android.Runtime.31.[rid] packs from the workload.

  3. Remove the android-32 workload, as it should no longer be needed.
    The API 31 "ref" pack is fairly small and can go in the android
    workload.

Now our android workload is:

"Microsoft.Android.Sdk",
"Microsoft.Android.Ref.31",
"Microsoft.Android.Ref.32",
"Microsoft.Android.Runtime.32.android-arm",
"Microsoft.Android.Runtime.32.android-arm64",
"Microsoft.Android.Runtime.32.android-x86",
"Microsoft.Android.Runtime.32.android-x64",
"Microsoft.Android.Templates"

After these changes, I get this assembly at build time:

dotnet\packs\Microsoft.Android.Ref.31\31.0.101-preview.11.117\ref\net6.0\Mono.Android.dll

And this assembly at runtime:

dotnet\packs\Microsoft.Android.Runtime.32.android-arm64\31.0.200-preview.13.21\runtimes\android-arm64\lib\net6.0\Mono.Android.dll

@jonathanpeppers
[One .NET] use API 31 "ref" pack with 32 "runtime" pack
4d8138f 
After 6eb11f1 went in, I realized an issue when using `net6.0-android31`:

1. You get API 31 by default. We bring in these packs from NuGet.org
   with the version 31.0.101-preview.11.117:

    Microsoft.Android.Ref.31
    Microsoft.Android.Runtime.32.[rid]

2. So now you can't get the latest changes for `libmonodroid.so`, for
   example? You'd have to opt-in via `net6.0-android32`.

Thinking about it more, we should only need to use the API 31 "ref" or
"targeting" pack. The "runtime" pack can just use the latest from the
workload.

To fix this:

1. Create new `$(_AndroidTargetingPackId)` and
   `$(_AndroidTargetingPackVersion)` properties to use independently
   of the runtime pack version.

2. Remove `Microsoft.Android.Runtime.31.[rid]` packs from the workload.

3. Remove the `android-32` workload, as it should no longer be needed.
   The API 31 "ref" pack is fairly small and can go in the `android`
   workload.

Now our `android` workload is:

    "Microsoft.Android.Sdk",
    "Microsoft.Android.Ref.31",
    "Microsoft.Android.Ref.32",
    "Microsoft.Android.Runtime.32.android-arm",
    "Microsoft.Android.Runtime.32.android-arm64",
    "Microsoft.Android.Runtime.32.android-x86",
    "Microsoft.Android.Runtime.32.android-x64",
    "Microsoft.Android.Templates"

After these changes, I get this assembly at build time:

    dotnet\packs\Microsoft.Android.Ref.31\31.0.101-preview.11.117\ref\net6.0\Mono.Android.dll

And this assembly at runtime:

    dotnet\packs\Microsoft.Android.Runtime.32.android-arm64\31.0.200-preview.13.21\runtimes\android-arm64\lib\net6.0\Mono.Android.dll
@jonathanpeppers jonathanpeppers marked this pull request as ready for review January 14, 2022 20:54
pjcollins
pjcollins reviewed Jan 18, 2022
View reviewed changes
build-tools/create-packs/Microsoft.Android.Sdk.proj
</PropertyGroup>
<PropertyGroup>
<_AndroidTargetingPackId Condition=" '%24(_AndroidTargetingPackId)' == '' ">$(AndroidLatestStableApiLevel)</_AndroidTargetingPackId>
<_AndroidTargetingPackVersion Condition=" '%24(_AndroidTargetingPackVersion)' == '' ">**FromWorkload**</_AndroidTargetingPackVersion>
<_AndroidRuntimePackId Condition=" '%24(_AndroidRuntimePackId)' == '' ">$(AndroidLatestStableApiLevel)</_AndroidRuntimePackId>
<_AndroidRuntimePackVersion Condition=" '%24(_AndroidRuntimePackVersion)' == '' ">**FromWorkload**</_AndroidRuntimePackVersion>
Copy link
Member

@pjcollins pjcollins Jan 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need to use these _AndroidRuntimePackId _AndroidRuntimePackVersion based conditions if we aren't setting them anywhere anymore? Though I guess it doesn't hurt to keep them if we would want an easy way to override these for whatever reason.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left $(_AndroidRuntimePackId) and $(_AndroidRuntimePackVersion) as private properties. They might be useful to be able to put them in a project file for debugging something down the road.

pjcollins
pjcollins reviewed Jan 18, 2022
View reviewed changes
build-tools/create-packs/Microsoft.Android.Sdk.proj
TargetingPackName="Microsoft.Android.Ref.%24(_AndroidRuntimePackId)"
TargetingPackVersion="%24(_AndroidRuntimePackVersion)"
TargetingPackName="Microsoft.Android.Ref.%24(_AndroidTargetingPackId)"
TargetingPackVersion="%24(_AndroidTargetingPackVersion)"
RuntimePackNamePatterns="Microsoft.Android.Runtime.%24(_AndroidRuntimePackId).**RID**"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm somewhat concerned about the approach to compile and run against different API levels. This reminds me of a very old issue we ran into when updating the shared runtime to always include the latest Mono.Android.dll - https://github.com/xamarin/androidtools/commit/0b7bcd77f1519d2928d9c8e619610ee2afe44786.

Perhaps that particular point of failure is no longer an issue in API 31+, as I think default interface methods in Java may have helped with this case? I can't think of any other scenarios that might break with this approach, but maybe @jonpryor has some additional thoughts here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Historical context: xamarin/androidtools@0b7bcd7 fixes Xamarin Bugzilla 22074, in which a System.TypeLoadException was thrown from a Debug-configuration app when the shared runtime was used, and the app was built with a $(TargetFrameworkVersion) "sufficiently lower than" the $(TargetFrameworkVersion) included in the shared runtime, all because of android.database.Cursor, my favorite type ever. (Google kept adding methods to Cursor, even before Java interface default methods were supported, and it was a perfectly acceptable change…to Java, but not C#. See also API Design: Interfaces versus Abstract Classes.)

xamarin/androidtools@0b7bcd7 (2014-Aug) predates FixAbstractMethodsStep in xamarin/monodroid@73579e210e (2015-Nov); see also f96fcf9, 0c4c033, etc. If FixAbstractMethodStep had existed, we could have instead "fixed up" the to-be-fast-deployed assemblies against the latest API level, which would also have addressed the scenario in xamarin/androidtools@0b7bcd7 while continuing to include the latest Mono.Android.dll in the shared runtime package. xamarin/androidtools@0b7bcd7 also predates C#8 default interface members.

Today, with .NET 6, this shouldn't be a concern: we can now rely on C#8 default interface members, thus preserving API and ABI compatibility from lower API levels through higher API levels -- which means we could possibly consider dropping FixAbstractMethodsStep in .NET 6? -- and preserving API and ABI compatibility going forward is now one of our tenants.

@jonpryor
Copy link
Member 

Commit 6eb11f15 added support for API-32, while keeping the .NET 6
default `$(TargetFramework)` value as `net6.0-android31.0`:

> However, we don't want to change the default API level for .NET 6
> projects; the default will remain `net6.0-android31.0` (API-31),

This appears to have had some unforeseen complications: we would use
the API-31 `Mono.Android.dll`, with the API-32 `libmonodroid.so`/etc.
runtime libraries.  This in turn appears to be responsible for some
crashes we've seen on CI ever since commit c227042b when running the
`Mono.Android.NET_Tests` unit tests under .NET 6 with the interpreter
enabled, because `libxamarin-app.so` and `libmonodroid.so` have ABI
dependencies:

	DOTNET  : JNI_OnLoad: JNI_OnLoad in pal_jni.c
	libc    : Fatal signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x2 in tid 3666 (droid.NET_Tests), pid 3666 (droid.NET_Tests)
	crash_dump64: performing dump of process 3666 (target tid = 3666)
	DEBUG   : *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***
	DEBUG   : Build fingerprint: 'Android/sdk_phone_x86_64/generic_x86_64:10/QPP6.190730.005.B1/5775370:userdebug/test-keys'
	DEBUG   : Revision: '0'
	DEBUG   : ABI: 'x86_64'
	DEBUG   : Timestamp: 2022-01-18 16:53:04+0000
	DEBUG   : pid: 3666, tid: 3666, name: droid.NET_Tests  >>> Mono.Android.NET_Tests <<<
	DEBUG   : uid: 10105
	DEBUG   : signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x2
	DEBUG   : Cause: null pointer dereference
	DEBUG   :     rax 0000000000002b36  rbx 000078c8afb3f860  rcx 000078c98b6561f0  rdx 0000000000000000
	DEBUG   :     r8  0000000000000002  r9  0000000000000080  r10 000078c98b296080  r11 000078c987d35178
	DEBUG   :     r12 00007fffe46ae99c  r13 000078c8afb89ea0  r14 000078c8afb3f990  r15 000078c98b6746c0
	DEBUG   :     rdi 000078c8afb3f860  rsi 0000000000000002
	DEBUG   :     rbp 0000000000000001  rsp 00007fffe46ae448  rip 000078c8afb1b31c
	main    : type=1400 audit(0.0:40): avc: granted { read } for name="u:object_r:net_dns_prop:s0" dev="tmpfs" ino=6642 scontext=u:r:untrusted_app_25:s0:c512,c768 tcontext=u:object_r:net_dns_prop:s0 tclass=file app=Mono.Android.NET_Tests
	DEBUG   : 
	DEBUG   : backtrace:
	DEBUG   :       #00 pc 000000000002c31c  /data/app/Mono.Android.NET_Tests-fbdZV696v1UeW3jUzJg9yg==/lib/x86_64/libmonodroid.so (xamarin::android::Util::monodroid_store_package_name(char const*)+12) (BuildId: 91fe7d9c6b30356fcfb8337b8541d0132df4f44a)
	DEBUG   :       #01 pc 0000000000025bbc  /data/app/Mono.Android.NET_Tests-fbdZV696v1UeW3jUzJg9yg==/lib/x86_64/libmonodroid.so (xamarin::android::internal::MonodroidRuntime::Java_mono_android_Runtime_initInternal(_JNIEnv*, _jclass*, _jstring*, _jobjectArray*, _jstring*, _jobjectArray*, _jobject*, _jobjectArray*, int, unsigned char, unsigned char)+652) (BuildId: 91fe7d9c6b30356fcfb8337b8541d0132df4f44a)
	DEBUG   :       #02 pc 00000000000273fb  /data/app/Mono.Android.NET_Tests-fbdZV696v1UeW3jUzJg9yg==/lib/x86_64/libmonodroid.so (Java_mono_android_Runtime_initInternal+75) (BuildId: 91fe7d9c6b30356fcfb8337b8541d0132df4f44a)
	DEBUG   :       #03 pc 0000000000174641  /apex/com.android.runtime/lib64/libart.so (art_quick_generic_jni_trampoline+209) (BuildId: 8bb3225e7c408f2ca23abac3db0417f2)
	…

Thinking about it more, we should only need to use the API-31 "ref"
or "targeting" pack.  The "runtime" pack can just use the latest from
the workload.

To fix this:

 1. Create new `$(_AndroidTargetingPackId)` and
    `$(_AndroidTargetingPackVersion)` properties to use independently
    of the runtime pack version.

 2. Remove `Microsoft.Android.Runtime.31.[rid]` packs from the workload.

 3. Remove the `android-32` workload, as it should no longer be needed.
    The API 31 "ref" pack is fairly small and can go in the `android`
    workload.

Now our `android` workload is:

  * `Microsoft.Android.Sdk`
  * `Microsoft.Android.Ref.31`
  * `Microsoft.Android.Ref.32`
  * `Microsoft.Android.Runtime.32.android-arm`
  * `Microsoft.Android.Runtime.32.android-arm64`
  * `Microsoft.Android.Runtime.32.android-x86`
  * `Microsoft.Android.Runtime.32.android-x64`
  * `Microsoft.Android.Templates`

After these changes, I get this assembly at build time:

	dotnet\packs\Microsoft.Android.Ref.31\31.0.101-preview.11.117\ref\net6.0\Mono.Android.dll

And this assembly at runtime:

	dotnet\packs\Microsoft.Android.Runtime.32.android-arm64\31.0.200-preview.13.21\runtimes\android-arm64\lib\net6.0\Mono.Android.dll

Additionally, CI is fully green; in particular, the
**APKs .NET - macOS** step is green, which hasn't been true on
xamarin-android/main since commit c227042b.

@jonpryor jonpryor merged commit f93d3d8 into dotnet:main Jan 19, 2022
@jonathanpeppers jonathanpeppers deleted the android-31-ref-pack-only branch January 20, 2022 03:18
@github-actions github-actions bot locked and limited conversation to collaborators Jan 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pjcollins pjcollins pjcollins approved these changes

@dellis1972 dellis1972 Awaiting requested review from dellis1972 dellis1972 is a code owner

@jonpryor jonpryor Awaiting requested review from jonpryor jonpryor is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jonathanpeppers @jonpryor @pjcollins