Add RIDs for Android by cydhaselton · Pull Request #16729 · dotnet/corefx

cydhaselton · 2017-03-05T18:57:52Z

This PR introduces new RIDs for Android in preparation for bringing up cross-compilation setup in core-setup. It covers next steps mentioned here, and used references from the .NET Core Guide

Tagging @qmfrederik for feedback on appropriate RID naming.

dnfclas · 2017-03-05T18:57:56Z

@cydhaselton,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla2.dotnetfoundation.org.

It will cover your contributions to all Microsoft-managed open source projects.
Thanks,
.NET Foundation Pull Request Bot

qmfrederik · 2017-03-05T19:29:12Z

Hi @cydhaselton

I only now see you've also opened a PR for this, we must have been working on this at the same time.

Quick question - I see you didn't inherit from Linux (although Android uses the Linux kernel) and created RIDs for MIPS Android & x64 Android, but not arm nor arm64.

So I guess in a way the PRs are complimentary. Any reason you left out arm and arm64?

As to API level 22 and 23, I don't think they are needed at this moment, since there are no CoreFX/CoreCLR builds specific to those API levels.

cydhaselton · 2017-03-05T20:14:15Z

Hey @qmfrederik

I was thinking that, while Android is based off of Linux, it has significant enough differences that making it inherit from Linux would be problematic. That said, I'm not familiar with the RID component; would making Android inherit from Linux break anything?

The android-x64 RID was supposed to be for arm64; I've pushed a commit to change this so it's clear.

dnfclas · 2017-03-05T20:25:07Z

@cydhaselton, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, .NET Foundation Pull Request Bot

qmfrederik · 2017-03-05T20:59:52Z

@cydhaselton Well, I think Android is similar to Alpine in that matter - both use the Linux kernel, but have a very different userspace (like the C library being used), breaking ABI compatibility with most other Linux-based OSes.

Because Alpine is marked as Linux, I think Android should also be marked as such. In the end, it'll be up for the .NET team to make a decision on that one.

mellinoe · 2017-03-06T19:44:22Z

+        "android-arm64": {
+	           "#import": [ "any" ]
+        },
+        "android-mips": {


We haven't done any work for MIPS, have we? It's probably not worth adding unless there's a real commitment to getting that working.

Correct, no work has been done for MIPS that I'm aware of. (Some) work has been done for ARM, so I'd suggest replacing MIPS with arm.

mellinoe · 2017-03-06T19:51:09Z

This is related to the discussion over in https://github.com/dotnet/coreclr/issues/9208. Right now, the "linux-x64" rid is being used for our glibc-based portable binaries.

Because Alpine is marked as Linux, I think Android should also be marked as such. In the end, it'll be up for the .NET team to make a decision on that one.

I agree; let's do that for now. Ultimately, we need to come to a broader conclusion regarding https://github.com/dotnet/coreclr/issues/9208, and what it means for distros like Alpine and Android.

Actually, it looks like we've decided that Alpine should be changed. In that case, let's leave this as-is for now. Perhaps @Petermarcu has some thoughts.

qmfrederik · 2017-03-06T20:01:43Z

+	           "#import": [ "android.21", "android-mips" ]
+        },
+
+        "android.22": {


Similarly, no work has been done for API level (versions) other than 21, so I'm not sure it makes sense to add RIDs for those versions new.

That also sounds reasonable to me. How different / incompatible are the different versions, though? If there's very minimal work to get them working after API level 21, then there's not much harm in adding them now. MIPS on the other hand sounds like a much more niche thing that we might not end up doing anytime soon. Disclaimer: I know little about Android API levels or Android hardware in general.

From an ABI point of view, the different versions are very backwards compatible. The package management system we've used to cross-compile for Android, Termux, only compiles for API level 21 at this moment - and you can take those binaries and run them up to API level 25, which is the most recent version available.

So I'm thinking we should either add API level 21 through 25, or only 21.

I don't know where MIPS comes from - I think the idea was to add arm.
At this time, CoreCLR supports x86, x64, arm and arm64 - there is no support for MIPS on any platform. Adding MIPS support would, amongst others, require porting assembly code to MIPS, I don't think anyone is working on it.

Net, I would suggest:

Replacing MIPS with arm

Adding only API level 21, or 21 through 25.

As for porting to MIPS, the asm code and PAL would be the easiest part. JIT would be the largest and most complex beast in that case.

@qmfrederik is right, the APIs are backwards compatible but there are some breaking changes when moving forward through the API levels. For example position-independent executables is enforced for dynamically-linked exes in API-21.

I vaguely recall a post in which MIPS was mentioned as a valid Android processor; if no one is actively working on it it can be removed.

To sum up:

Add API level 25, remove 21-23

replace MIPS with arm

I would keep API level 21 as the baseline - .NET Core will work on API level 21 so I would include it in the runtime.json file. You could add 22, 23, 24 and 25 as well.

Apart from remove MIPS, I would also add ARM. (so you'll have both arm and arm64).

I think the best way forward would be to update the PR, the discussion will be more tangible once I see the updated files.

Will do; just need to fix push after enabling two-factor

Done. Commit

cydhaselton · 2017-03-07T00:09:51Z

@mellinoe and/or @qmfrederik: Based on the referenced #9208, should the RID format be changed to something like linux-android.21-arm or linux-bionic.21-arm?

mellinoe · 2017-03-07T00:38:02Z

+
+        "android.21": {
+	           "#import": [ "android" ]
+	},


nit: spacing

mellinoe · 2017-03-07T00:39:47Z

should the RID format be changed to something like linux-android.21-arm or linux-bionic.21-arm?

We haven't really decided what things will look like for non-glibc Linuxes, although I think we will land on something similar to what you described. Having these inherit from "any" is okay for now.

mellinoe · 2017-03-07T00:40:30Z

@cydhaselton I also notice that the commit author doesn't match your GitHub account. Could you fix that?

(corrected authoring info)

qmfrederik · 2017-03-07T12:43:52Z

+	    "#import": [ "any" ]
+        },
+        "android-arm64": {
+	    "#import": [ "any" ]


I think the spacing is still off. You should indent the "#import" node, like on line 11. I think the previous feedback was about you using tabs instead of spaces.

Well this is embarrassing.
Removed tabs and re-pushed. If spacing still isn't correct I'll review using a different editor

cydhaselton

sigh

Missed one…fixing

cydhaselton · 2017-03-07T20:22:35Z

@qmfrederik or @mellinoe, can you provide a second set of eyes on (hopefully) last commit before merge?

mellinoe · 2017-03-07T20:25:52Z

Looks good to me. Excited to see the progress on this 😄

Add RIDs for Android Commit migrated from dotnet/corefx@125bc42

Add RIDs for Android

24f64f9

dnfclas added the cla-required label Mar 5, 2017

dnfclas added cla-signed and removed cla-required labels Mar 5, 2017

Changed android x64 RID to clearer arm64

f72592b

cydhaselton mentioned this pull request Mar 5, 2017

Add RIDs for Android #16731

Closed

karelz added the area-Infrastructure-libraries label Mar 6, 2017

karelz assigned mellinoe and janvorli Mar 6, 2017

karelz added the os-unsupported OS which is not officially supported label Mar 6, 2017

mellinoe reviewed Mar 6, 2017

View reviewed changes

qmfrederik reviewed Mar 6, 2017

View reviewed changes

mellinoe reviewed Mar 7, 2017

View reviewed changes

Replace Android MIPS RID with arm. Remove Android API levels 22-23

e2be12a

(corrected authoring info)

cydhaselton force-pushed the android-rids branch from e6039d7 to e2be12a Compare March 7, 2017 12:30

Fixed whitespace issue

f52368d

qmfrederik reviewed Mar 7, 2017

View reviewed changes

Fixing hidden tabs, replacing with correct spacing

01c20a1

cydhaselton commented Mar 7, 2017

View reviewed changes

Removed final hidden tab, replace with whitespace.

c7c4fcc

mellinoe merged commit 125bc42 into dotnet:master Mar 7, 2017

cydhaselton deleted the android-rids branch March 7, 2017 21:55

karelz modified the milestone: 2.0.0 Mar 10, 2017

Lemmingh mentioned this pull request Aug 13, 2019

How to build for android dotnet/docs#9889

Closed

cydhaselton mentioned this pull request Jan 22, 2020

Enabling Android ARM64 for .NET Core dotnet/runtime#2044

Closed

8 tasks

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#16729 from cydhaselton/android-rids

8b8c727

Add RIDs for Android Commit migrated from dotnet/corefx@125bc42

Conversation

cydhaselton commented Mar 5, 2017

Uh oh!

dnfclas commented Mar 5, 2017

Uh oh!

qmfrederik commented Mar 5, 2017

Uh oh!

cydhaselton commented Mar 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfclas commented Mar 5, 2017

Uh oh!

qmfrederik commented Mar 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mellinoe commented Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cydhaselton Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cydhaselton commented Mar 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mellinoe commented Mar 7, 2017

Uh oh!

mellinoe commented Mar 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cydhaselton left a comment

Choose a reason for hiding this comment

Uh oh!

cydhaselton commented Mar 7, 2017

Uh oh!

mellinoe commented Mar 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cydhaselton commented Mar 5, 2017 •

edited

Loading

qmfrederik commented Mar 5, 2017 •

edited

Loading

mellinoe commented Mar 6, 2017 •

edited

Loading

cydhaselton Mar 6, 2017 •

edited

Loading