First steps for generating nuget packages for ARM64/Android. by cydhaselton · Pull Request #10286 · dotnet/coreclr

cydhaselton · 2017-03-19T01:05:09Z

Adds RIDS and related conditionals/variables to the appropriate files as part of Enabling Android/ARM64 for .NET Core (issue here).

Code appears to have been refactored since the original PR to which I was referred, cc-ing @janvorli and @qmfrederik for review in case of missed items

cydhaselton · 2017-03-20T13:54:00Z

Also tagging @gkhanna79 for review because of #6056 and #6000

gkhanna79 · 2017-03-21T00:04:24Z

 <Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <PropertyGroup>
-    <SupportedPackageOSGroups>Linux;OSX</SupportedPackageOSGroups>
+    <SupportedPackageOSGroups>Linux;Android;OSX</SupportedPackageOSGroups>


Is there a reason you are enabling this here?

CC @jkotas

Because Linux and OSX were enabled here. If it's not applicable I can remove; I'm basically working from PRs (see dotnet/core-setup#1651 Porting to New Platform References) and extrapolating based on refactored code so some of the edits may not be needed. Feel free to let me know and I'll make changes

I think you can skip this file from the change.

gkhanna79 · 2017-03-21T00:10:18Z

    <RuntimeOS Condition="'$(RuntimeOS)' == ''">$(OSRid)</RuntimeOS>

-    <SupportedPackageOSGroups Condition="'$(SupportedPackageOSGroups)' == ''">Windows_NT;OSX;Linux</SupportedPackageOSGroups>
+    <SupportedPackageOSGroups Condition="'$(SupportedPackageOSGroups)' == ''">Windows_NT;OSX;Android;Linux</SupportedPackageOSGroups>


What is the __DistroRid for Android build?

~~android.21-arm64 and/or android.21-arm, unless there's a difference between RID as registered in CoreFX and __DistroRid?~~

Did some additional code parsing; will update this file with __DistroRid generation

Was this updated?

@gkhanna79 ~~Not yet; still working out where __DistroRid originates and/or is generated and how to do so without uname or $ROOTFS_DIR/etc/os-release~~

Updated

cydhaselton · 2017-03-26T10:26:51Z

@gkhanna79 The test hack I added to build.sh generates __DistroRid for Android build and, with following commits, allows package build for Android.

I've added a TODO for figuring out a reliable way to generate __DistroRid for Android. Since I generated the commit changes from the `runtime.OS.filename.props files and guesswork, a once-over to make sure I haven't missed anything would be appreciated

cydhaselton · 2017-03-26T14:07:29Z

cc-ing @qmfrederik @mellinoe and @janvorli for additional visibility into package generation

cydhaselton · 2017-03-26T18:24:00Z

Also copying @schellap for input

gkhanna79 · 2017-03-29T16:26:40Z

-                export __DistroRid=""
+                #Test hack to generate __DistroRid for Android
+                #TODO Find reliable way to generate __DistroRid for Android.
+                export __DistroRid="android.21-arm64"


Just to be clear - we cannot commit with this change. Have you considered specifying this at command (e.g. __DistroRid=... ./build.sh) and then adding a check in this function if __DistroRid is already defined or not?

@gkhanna79: I did not, but that is a good idea.

IMHO, you need to save ANDROID_PLATFORM variable as a file when you build rootfs and then you can use it in building coreclr. :)

@jyoungyun This may be better; so far I've tried every iteration of detecting __DistroRid in this function and all have failed except for ! [[ -n "$__DistroRid" ]], which is the same as hardcoding that value.

Would help if correct variable was specified at cmdline

~~@gkhanna79 See recent commit~~

FYI, Walking back all changes to build.sh and going route @jyoungyun recommended. Too difficult getting logic check in initTargetDistroRid() to work without causing CI errors

gkhanna79 · 2017-03-29T16:27:27Z

    <SupportedPackageOSGroups>Linux;OSX</SupportedPackageOSGroups>
  </PropertyGroup>
-</Project>
+</Project>


Undo change to this file.

@gkhanna79 Done

gkhanna79 · 2017-03-29T16:29:14Z

+  </PropertyGroup>
+  <ItemGroup>
+    <NativeBinary Include="$(BinDir)libcoreclr.so" />
+    <NativeBinary Condition="'$(_PlatformDoesNotSupportNiFiles)' != 'true'" Include="$(BinDir)libcoreclrtraceptprovider.so" />


This check is incorrect. _PlatformDoesNotSupportNiFiles should be used for platforms where we cannot generate NI images right now. They just happen to be same as where this binary does not build.

If you are able to build this for Android, please fix this check accordingly so that it can be included for that platform.

FYI - @hqueue @hseok-oh @jyoungyun

gkhanna79 · 2017-03-29T16:30:40Z

+    <OfficialBuildRID Include="android.21-arm64">
+      <Platform>arm64</Platform>
+    </OfficialBuildRID>
+    <OfficialBuildRID Include="android.21-arm">


Have you tested the build for Arm32 for Android? If so, your change for __DsitroRid should account for it as well.

I've only tested...and am only able to test...for arm64. I can add arm into this change but won't be able to test it.

In that case, please undo the Android for Arm change.

jyoungyun · 2017-03-31T04:35:25Z

      "description": "Specifies the OS to build Core_Root for",
      "valueType": "property",
-      "values": [ "debian.8-x64", "fedora.23-x64", "opensuse.42.1-x64", "osx.10.12-x64", "rhel.7-x64", "ubuntu.14.04-x64", "ubuntu.16.04-x64", "ubuntu.16.10-x64" ],
+      "values": [ "debian.8-x64", "fedora.23-x64", "opensuse.42.1-x64", "osx.10.10-x64", "rhel.7-x64", "ubuntu.14.04-x64", "ubuntu.16.04-x64", "ubuntu.16.10-x64", "android.21-arm64", "android.21-arm" ],


I think you need to undo osx version change.

That's odd…don't remember making that change. Fixed

cydhaselton · 2017-04-01T20:03:51Z

Tagging @qmfrederik to review/ok addition to build-android-rootfs.sh

cydhaselton · 2017-04-01T21:34:50Z

General question: I'm still getting up to speed with CI builds. Is there anything I need to do with the previous commits in this PR that have failed build checks if a later commit or commits are successful?

qmfrederik · 2017-04-01T22:50:04Z

@cydhaselton The change to build-android-rootfs is fine with me.

Regarding to the CI builds, only the last commit matters. If the previous commits are no longer relevant, you can squash them, and they will disappear from the list of commits in this pull request.

Would it be easier to treat Android as a Linux distribution? That would avoid the need to add a new OS group.

cydhaselton · 2017-04-01T23:04:49Z

@qmfrederik: Would it be easier to treat Android as a Linux distribution? That would avoid the need to add a new OS group.

I'm not sure; I'm still piecing together how nuget packages are generated. Since I don't know all of the places a) where it would be important to separate Android from Linux and b) where it wouldn't matter, I'm taking a "separate everything now, integrate with Linux after" approach.

I can comment my code to that effect if it would help.

cydhaselton · 2017-04-01T23:31:08Z

Another (probably obvious) question: is there a way for me manually to re-run a CI build for a specific commit?

qmfrederik · 2017-04-02T08:44:19Z

@cydhaselton You can force the CI build to re-run, but only for the last commit in your PR.

cydhaselton · 2017-04-02T15:02:54Z

@gkhanna79 @jyoungyun and others…any other changes/additions I should make to this PR before merge?

jyoungyun · 2017-04-03T02:25:28Z

                export __DistroRid="$ID.$VERSION_ID-$__BuildArch"
            fi
-        fi
+         fi


Remove redundant space :)

Good catch! Fixed.

gkhanna79 · 2017-04-04T05:46:55Z

-                export __DistroRid=""
+                if [ -e $ROOTFS_DIR/android_platform ]; then
+                    source $ROOTFS_DIR/android_platform
+                    export __DistroRid="$RID"


I presume that $RID is being set by the source statement earlier,right? What is the value it gets?

Nevermind - saw it further below.

gkhanna79 · 2017-04-04T05:47:14Z

      "description": "Specifies the OS to build Core_Root for",
      "valueType": "property",
-      "values": [ "debian.8-x64", "fedora.23-x64", "opensuse.42.1-x64", "osx.10.12-x64", "rhel.7-x64", "ubuntu.14.04-x64", "ubuntu.16.04-x64", "ubuntu.16.10-x64" ],
+      "values": [ "debian.8-x64", "fedora.23-x64", "opensuse.42.1-x64", "osx.10.12-x64", "rhel.7-x64", "ubuntu.14.04-x64", "ubuntu.16.04-x64", "ubuntu.16.10-x64", "android.21-arm64", "android.21-arm" ],


Were you not planning to remove the "arm" entry?

I did, but reverted when working on fixing initTargetDistroRid logic in build.sh and forgot to re-remove.
Fixed

gkhanna79 · 2017-04-04T05:49:30Z

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


Thinking a bit more about this, why do you need to add these android specific files? Isn't the Linux version sufficient?

I'm not sure; I'm still piecing together how nuget packages are generated. Since I don't know all of the places a) where it would be important to separate Android from Linux and b) where it wouldn't matter, I'm taking a "separate everything now, integrate with Linux after" approach.

I can remove the files and see if package build still works.

@gkhanna79: So the build is successful, including packages, once packageTargetOsGroup for Android is removed from dir.props and conditionals added to Microsoft.NETCore.Runtime.CoreCLR.pkgproj

So my questions are:

What elements distinguish one package group from another? Or: What factors determine the creation of a new OS group?

Is it possible that a separate package group would be needed for Android in the future?

Given that Android is sufficiently distinct, but still Linux based, I am fine if a new OS group for it is used while leveraging the existing Linux definitions of the components.

CC @chcosta

Unfortunately, unless I'm missing something, packageTargetOSGroup means adding Android-specific runtime-prefixed files (runtime.Android...). Are you saying just to leave those files in or make changes so that Android has it's own OSGroup/packageTargetOSGroup but uses the Linux component defs?

cc @gkhanna79

I think it's reasonable for Android to use the Linux _packageTargetOSGroup to import the runtime package imports

@chcosta and @gkhanna79: Still not sure how to achieve what you are suggesting; if I create a packageTargetOSGroup for Android without adding the runtime.Android files package build fails. Is there another OS that does this that I could reference?

gkhanna79 · 2017-04-04T05:50:19Z

    </When>
+    <When Condition="'$(_runtimeOSFamily)' == 'android'">
+      <PropertyGroup>
+        <PackageRID>android.21-$(ArchGroup)</PackageRID>


Should we have a check for arm64 since that is all we are supporting for now?

cydhaselton · 2017-04-07T14:30:13Z

@gkhanna79 @qmfrederik and others; re-posting this question in general discussion for greater visibility. Code discussion here:

So my questions are:

What elements distinguish one package group from another? Or: What factors determine the creation of a new OS group?
Is it possible that a separate package group would be needed for Android in the future?

cydhaselton · 2017-04-07T19:58:51Z

cc @hseok-oh and @ericstj for insight into OS Group package info

danmoseley · 2017-04-08T16:34:05Z

@dotnet-bot test OSX10.12 x64 Checked Build and Test (network)

cydhaselton · 2017-04-08T23:34:29Z

Also cc-ing @jkotas as per documentation; is there a person or persons that are responsible for the packaging subsystem that I should copy on this for feedback on the OS Group question?

jkotas · 2017-04-08T23:55:18Z

You have the right people copied on this already (@gkhanna79, @ericstj).

cydhaselton · 2017-04-09T10:23:53Z

Thanks, @jkotas. Would it be appropriate for me to update the CODE_OWNERS.TXT with that info?

Removed extra space in initTargetDistroRid() logic

removed Android-specific runtime files for packaging added conditional to have Android use Linux runtime files

… issue

cydhaselton · 2017-04-30T21:12:00Z

@gkhanna79 The above looks messy; did you mean "merge" instead of "rebase?

EDIT: Followed git hints instead of running git push -f post-rebase. Ran hard reset to rebase point, then ran git push -f

gkhanna79 · 2017-05-01T03:27:43Z

What is looking messy?

cydhaselton · 2017-05-01T13:47:17Z

What is looking messy?
@gkhanna79 Ignore…see edited message. Basically didn't do git push -f after rebase.

cydhaselton · 2017-05-02T16:41:07Z

@dotnet-bot test Windows_NT x64 Debug Build and Test

cydhaselton · 2017-05-02T18:17:03Z

@gkhanna79, @ericstj and others…is there anything else that needs to be done for this PR?

ericstj · 2017-05-02T18:22:53Z

      "description": "Specifies the OS to build Core_Root for",
      "valueType": "property",
-      "values": [ "debian.8-x64", "fedora.24-x64", "fedora.25-x64", "opensuse.42.1-x64", "osx.10.12-x64", "rhel.7-x64", "ubuntu.14.04-x64", "ubuntu.16.04-x64", "ubuntu.16.10-x64" ],
+      "values": [ "debian.8-x64", "fedora.23-x64", "fedora.25-x64", "opensuse.42.1-x64", "osx.10.12-x64", "rhel.7-x64", "ubuntu.14.04-x64", "ubuntu.16.04-x64", "ubuntu.16.10-x64", "android.21-arm64" ],


This change to fedora appears to be a bad merge.

Crap. Fixed

ericstj · 2017-05-02T18:28:01Z

+    <When Condition="'$(_runtimeOSFamily)' == 'android'">
+      <PropertyGroup>
+        <LibraryFileExtension>.so</LibraryFileExtension>
+<!--symbols included in .so, like Linux, but can be generated externally and if so, uses .debug ext-->


nit: indentation

Fixed accidental fedora version change Fixed indenting

ericstj · 2017-05-02T20:53:32Z

I've reviewed and commented on stuff that stuck out to me. I don't see anything else concerning. Most of these changes are around how coreclr decides to factor its package source which I don't have an opinion on so long as it produces the correct results. I'd like for @gkhanna79 or @chcosta to give explicit sign off on that.

cydhaselton · 2017-05-03T15:24:38Z

Thanks Eric.
@gkhanna79 and/or @chcosta let me know if there's anything you see that needs to be fixed.

cydhaselton · 2017-05-13T20:38:02Z

Polite re-ping to @gkhanna79 and @chcosta … is there anything I need to change in this PR before merge?

chcosta · 2017-05-15T17:31:19Z

+# TODO: replace with logic that generates RID based on $__ApiLevel and $__BuildArch
+echo "Generating platform file..."

+echo "RID=android.21-arm64" > $__ToolchainDir/sysroot/android_platform


Any reason not to address this TODO and use the ApiLevel / BuildArch here?

@chcosta The TODO was addressed in previous commit by adding logic to build-android-rootfs.sh; I accidentally left the comment. Will remove comment and echo commands.

chcosta · 2017-05-15T17:34:23Z

    </OfficialBuildRID>
  </ItemGroup>
+  <ItemGroup Condition="$(SupportedPackageOSGroups.Contains(';Android;'))">
+    <OfficialBuildRID Include="android.21-arm64">


Do you want to include the Portable rid?

@chcosta At this line? I don't think so…a quick check of master shows that the other supported OSs don;t include it at this point. Should it be?

@gkhanna79 Anything you see that needs changing?

OfficialBuildRID is only used for platform targets that the official .NET Core build is going to support and we should add it only when we enable building for it. Otherwise, this is not required.

@cydhaselton Can you please undo this change for now?

Out of town but will push change when I return.

@gkhanna79 Removed

chcosta · 2017-05-15T17:35:40Z

Two additional comments, and then LGTM

cydhaselton · 2017-06-06T20:23:33Z

@dotnet-bot test Tizen armel Cross Debug Build

cydhaselton · 2017-06-07T12:16:39Z

@dotnet-bot test Windows_NT x64 Debug Build and Test

cydhaselton · 2017-06-07T21:35:07Z

@gkhanna79 and @chcosta are there any other changes needed for this PR?

cydhaselton · 2017-06-09T10:06:44Z

Thanks all. Ok to keep this branch around until I have time to write up documentation?

dnfclas added the cla-already-signed label Mar 19, 2017

gkhanna79 reviewed Mar 21, 2017

View reviewed changes

gkhanna79 reviewed Mar 29, 2017

View reviewed changes

jyoungyun reviewed Mar 31, 2017

View reviewed changes

jyoungyun reviewed Apr 3, 2017

View reviewed changes

gkhanna79 reviewed Apr 4, 2017

View reviewed changes

cydhaselton added 3 commits April 30, 2017 17:59

Syntax cleanup for build.sh

f9fe11a

Removed extra space in initTargetDistroRid() logic

Recommended review changes

ae816f6

removed Android-specific runtime files for packaging added conditional to have Android use Linux runtime files

Updated topic branch from master, resolving SPC.ni.dll file nuget pkg…

25eef7d

… issue

cydhaselton force-pushed the android-nuget-pkg branch from c44c5db to 25eef7d Compare April 30, 2017 21:27

ericstj reviewed May 2, 2017

View reviewed changes

ericstj review changes

97b8848

Fixed accidental fedora version change Fixed indenting

chcosta reviewed May 15, 2017

View reviewed changes

cydhaselton added 2 commits May 21, 2017 19:11

Removed TODO comment; logic already in place, added in earlier commit

c5bcc93

Removed Android OfficialBuildRID

9026f67

gkhanna79 approved these changes Jun 8, 2017

View reviewed changes

gkhanna79 merged commit b4cbd9d into dotnet:master Jun 8, 2017

karelz modified the milestone: 2.1.0 Aug 28, 2017

cydhaselton mentioned this pull request Jan 22, 2020

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

Closed

8 tasks

cydhaselton mentioned this pull request Jan 31, 2020

Android nuget package build broken in runtime.Linux.Microsoft.NETCore.Runtime.CoreCLR.props dotnet/runtime#8620

Closed

Conversation

cydhaselton commented Mar 19, 2017

Uh oh!

cydhaselton commented Mar 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cydhaselton Mar 21, 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!

cydhaselton Mar 21, 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!

cydhaselton Mar 23, 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!

cydhaselton commented Mar 26, 2017

Uh oh!

cydhaselton commented Mar 26, 2017

Uh oh!

cydhaselton commented Mar 26, 2017

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 Apr 1, 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!

cydhaselton Apr 1, 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!

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!

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 Apr 1, 2017

Uh oh!

cydhaselton commented Apr 1, 2017

Uh oh!

qmfrederik commented Apr 1, 2017

Uh oh!

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

Uh oh!

Uh oh!

cydhaselton commented Apr 1, 2017

Uh oh!

qmfrederik commented Apr 2, 2017

Uh oh!

cydhaselton commented Apr 2, 2017

Uh oh!

cydhaselton Mar 21, 2017 •

edited

Loading

cydhaselton Mar 21, 2017 •

edited

Loading

cydhaselton Mar 23, 2017 •

edited

Loading

cydhaselton Apr 1, 2017 •

edited

Loading

cydhaselton Apr 1, 2017 •

edited

Loading

cydhaselton commented Apr 1, 2017 •

edited

Loading

cydhaselton Apr 17, 2017 •

edited

Loading