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.

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

Build status-go for i386 only for simulator #15595

Closed
jakubgs opened this issue Apr 6, 2023 · 15 comments · Fixed by #17040
Closed

Build status-go for i386 only for simulator #15595

jakubgs opened this issue Apr 6, 2023 · 15 comments · Fixed by #17040
Assignees
Labels

Comments

@jakubgs
Copy link
Member

jakubgs commented Apr 6, 2023

There is a time saving to be made in building status-go only for i386, since currently we always build for 3 architectures:

android = callPackage ./build.nix {
platform = "android";
platformVersion = "23";
targets = [ "android/arm" "android/arm64" "android/386" ];
outputFileName = "status-go-${source.shortRev}.aar";

But building only for for i386 for simulator runs we can save over two minutes:

Config Build Time
[ "android/arm" "android/arm64" "android/386" ] 3 min 51 sec
[ "android/arm64" ] 1 min 27 sec
[ "android/arm" ] 1 min 28 sec
[ "android/386" ] 1 min 27 sec

The normal way developers use a simulator is via make run-android, so we'd need to modify the shell for that.

@yakimant
Copy link
Member

Trying to run-android on local macOS with Apple Silicon showed the following compatibility:

  • x86_64 emulator doesn't start, arm64-v8a is needed
  • ANDROID_ABI_INCLUDE=x86 can't run
  • status-go: targets=["android/386"] can't run

So if we do make run-android on arm - it makes sense to build both mobile app and status-go for arm.

Probably it's the same if we run on Linux (x86): x86 emulator/app abi/status-go targets.

@siddarthkay, please confirm.

@yakimant
Copy link
Member

I suggest to make status-go target mapped by ANDROID_ABI_INCLUDE of status-mobile app.

Here is the suggested mapping:

ANDROID_ABI_INCLUDE status-go target
armeabi-v7a android/arm
arm64-v8a android/arm64
x86 android/386
x86_64 android/386

@siddarthkay, what do you think?

@siddarthkay
Copy link
Contributor

please confirm.

  • yes Android Studio on mac allows us to create an x86_64 it however crashes on startup and only arm64 android emulators work on macOS.
  • not sure about not including x86 from CPU Architecture target in ANDROID_ABI_INCLUDE
  • not really sure about status-go: targets=["android/386"] either

@yakimant
Copy link
Member

So if my understading is correct, based on research and hints from @jakubgs, here is my proposal:

  • Make status-go targets as a parameter (targets.status-go.mobile.android Nix function)
  • Switch from config to env (status-im.android.abi-include to ANDROID_ABI_INCLUDE) for targets.mobile.android
    • Fix build-android.sh to support the change
    • Fix status-jenkins-lib/vars/android.groovy to support the change
    • Alternatives
      • add config to nix shells
      • add ANDROID_ABI_INCLUDE_OVERRIDE instead
  • Add mapping logic for targets.mobile.android:
    • Choose appropriate status-go targets based on ANDROID_ABI_INCLUDE
    • Set build/shell env vatiable to ANDROID_ABI_INCLUDE
  • Set ANDROID_ABI_INCLUDE in shell.sh based on architecture: x86 or arm64-v8a

@siddarthkay
Copy link
Contributor

by specifying ANDROID_ABI_INCLUDE in build.gradle we use those targets to build a universal apk which allows us to share apk with testing team and it works for them irrespective of the phones they have ( either x86 or arm )
@jakubgs correct me if i am wrong over here....

@jakubgs
Copy link
Member Author

jakubgs commented Jun 20, 2023

I think we need to get rid of nix/config.nix first in a separate PR, by using builins.getEnv, and moving the setting of those config values closer to where they are being used. And expand documentation in nix/DETAILS.md to include information about that.

Once that is done we will have a clearer picture of how to apply these settings, but the suggested nix/mobile/android/default.nix certainly looks like one of the candidates. At least for Android builds and shell.

@siddarthkay yes, that's correct, but we also make a separate build for E2E tests just for that reason with only x86:

  if (utils.isE2EBuild()) {
    androidAbiInclude = 'x86' /* e2e builds are used with simulators */

https://github.com/status-im/status-jenkins-lib/blob/4a933b88e2c9fc4cd6191797a89cfa4ef4fdb2cd/vars/android.groovy#L11-L12

@yakimant
Copy link
Member

@churik, can you please comment, how (if) you run e2e tests locally.

yakimant added a commit that referenced this issue Jun 29, 2023
To help with #15595 changes, refactoring is required.
In this PR we switch from config to env vars.
Doing some cleanup meanwhile.
yakimant added a commit that referenced this issue Jun 29, 2023
To help with #15595 changes, refactoring is required.
In this PR we switch from config to env vars.
Doing some cleanup meanwhile.
yakimant added a commit that referenced this issue Jun 30, 2023
To help with #15595 changes, refactoring is required.
In this PR we switch from config to env vars.
Doing some cleanup meanwhile.
yakimant added a commit that referenced this issue Jun 30, 2023
To help with #15595 changes, refactoring is required.
In this PR we switch from config to env vars.
Doing some cleanup meanwhile.
yakimant added a commit that referenced this issue Jun 30, 2023
To help with #15595 changes, refactoring is required.
In this PR we switch from config to env vars.
Doing some cleanup meanwhile.
yakimant added a commit that referenced this issue Jun 30, 2023
To help with #15595 changes, refactoring is required.
In this PR we switch from config to env vars.
Doing some cleanup meanwhile.
yakimant added a commit that referenced this issue Aug 7, 2023
To help with #15595 changes, refactoring is required.
In this PR we switch from config to env vars.
Doing some cleanup meanwhile.
yakimant added a commit that referenced this issue Aug 7, 2023
To help with #15595 changes, refactoring is required.
In this PR we switch from config to env vars.
Doing some cleanup meanwhile.
yakimant added a commit that referenced this issue Aug 7, 2023
To help with #15595 changes, refactoring is required.
In this PR we switch from config to env vars.
Doing some cleanup meanwhile.
yakimant added a commit that referenced this issue Aug 7, 2023
To help with #15595 changes, refactoring is required.
In this PR we switch from config to env vars.
Doing some cleanup meanwhile.
yakimant added a commit that referenced this issue Aug 7, 2023
To help with #15595 changes, refactoring is required.
In this PR we switch from config to env vars.
Doing some cleanup meanwhile.
mmilad75 pushed a commit that referenced this issue Aug 7, 2023
To help with #15595 changes, refactoring is required.
In this PR we switch from config to env vars.
Doing some cleanup meanwhile.
@yakimant
Copy link
Member

By default, -target=android builds shared libraries for all supported instruction sets (arm, arm64, 386, amd64). A subset of instruction sets can be selected by specifying target type with the architecture name. E.g., -target=android/arm,android/386.

By default, -target ios will generate an XCFramework for both ios and iossimulator. Multiple Apple targets can be specified, creating a "fat" XCFramework with each slice. To generate a fat XCFramework that supports iOS, macOS, and macCatalyst for all supportec architectures (amd64 and arm64), specify -target ios,macos,maccatalyst. A subset of instruction sets can be selectged by specifying the platform with an architecture name. E.g. -target=ios/arm64,maccatalyst/arm64.

https://pkg.go.dev/golang.org/x/mobile/cmd/gomobile

@yakimant
Copy link
Member

yakimant added a commit that referenced this issue Aug 22, 2023
Fixes partially #15595
In order to build less targets, when not needed we introduce this
mapping logic.
If only specific ABI is required - status-go will have the same
arhitecuture.
yakimant added a commit that referenced this issue Aug 22, 2023
Fixes partially #15595
In order to build less targets, when not needed we introduce this
mapping logic.
If only specific ABI is required - status-go will have the same
arhitecuture.
yakimant added a commit that referenced this issue Aug 22, 2023
Fixes partially #15595
In order to build less targets, when not needed we introduce this
mapping logic.
If only specific ABI is required - status-go will have the same
arhitecuture.
@yakimant
Copy link
Member

yakimant commented Aug 22, 2023

make shell TARGET=android and make run-android were not affected by #17040.
They will build for all architectures, unless specified by env variable ANDROID_ABI_INCLUDE.

We can leave as is or implement a logic, which will specify ANDROID_ABI_INCLUDE depending on:

  • host arch: x86_64 or arm64
  • target arhitecure: android emulator (x86_64 or arm64) or real device (arm or arm64)

Smth like that:

Host Arch Android Emulator Android Device
x86_64 x86 (maybe x86_64) armeabi-v7a and/or arm64-v8a
arm64 arm64-v8a armeabi-v7a and/or arm64-v8a

So
Option 1: Implement logic of choosing ABI based on adb

❯ adb devices -l
List of devices attached
emulator-5554          device product:sdk_gphone64_arm64 model:sdk_gphone64_arm64 device:emulator64_arm64 transport_id:1

Option 2: separate make targets for emulator and devices
Have separte make targets, eg run-android-emulator, run-android-device (first target could automatically start emulator, which doesn't happend now)

Option 3: ask developers to set env variable

  • ANDROID_ABI_INCLUDE="arm64-v8a" make run-android for real device and emulator on Apple Silicon
  • ANDROID_ABI_INCLUDE="x86" make run-android on intel based emulators
  • Actually we can set arm64 for Apple Silicon always, emulator or device

Commands to test:

  • make run-android
  • make shell TARGET=android

@jakubgs
Copy link
Member Author

jakubgs commented Aug 23, 2023

Option number 1 seems the best.

yevh-berdnyk pushed a commit that referenced this issue Aug 23, 2023
Fixes partially #15595
In order to build less targets, when not needed we introduce this
mapping logic.
If only specific ABI is required - status-go will have the same
arhitecuture.
@yakimant
Copy link
Member

yakimant commented Aug 23, 2023

macOS, Apple Silicon, arm64 emulator running:

❯ uname -m
arm64

❯ uname -a
Darwin yakimant-macbook-air.local 22.5.0 Darwin Kernel Version 22.5.0: Thu Jun  8 22:29:26 PDT 2023; root:xnu-8796.121.3~8/RELEASE_ARM64_T8112 arm64

❯ adb devices
List of devices attached
emulator-5554   device

❯ adb devices -l
List of devices attached
emulator-5554          device product:sdk_gphone64_arm64 model:sdk_gphone64_arm64 device:emulator64_arm64 transport_id:1

Real android device:

❯ adb devices
* daemon not running; starting now at tcp:5037
* daemon started successfully
List of devices attached
23251FDF6005BS    device

❯ adb devices -l
List of devices attached
23251FDF6005BS         device usb:18087936X product:oriole model:Pixel_6 device:oriole transport_id:1

Note: adb garbage sometimes

Case of several devices connected:

❯ adb devices
List of devices attached
23251FDF6005BS    device
emulator-5554    device

❯ adb devices -l
List of devices attached
23251FDF6005BS         device usb:18087936X product:oriole model:Pixel_6 device:oriole transport_id:1
emulator-5554          device product:sdk_gphone64_arm64 model:sdk_gphone64_arm64 device:emu64a transport_id:2

Note: run-android will install on both, so we can check arch of both or just ignore for our logic

@yakimant
Copy link
Member

Linux and 2 different arch emulators:

❯ uname -m
x86_64

❯ uname -a
Linux siddarth-ROG-Strix-GA35DX-G35DX 5.19.0-46-generic #47~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Wed Jun 21 15:35:31 UTC 2 x86_64 x86_64 x86_64 GNU/Linux

❯ adb devices
List of devices attached
emulator-5554    device

❯ adb devices -l
List of devices attached
emulator-5554          device product:sdk_phone_x86 model:Android_SDK_built_for_x86 device:generic_x86 transport_id:1

❯ adb devices
List of devices attached
emulator-5554    device

❯ adb devices -l
List of devices attached
emulator-5554          device product:sdk_gphone64_x86_64 model:sdk_gphone64_x86_64 device:emu64x transport_id:2

@yakimant yakimant reopened this Aug 24, 2023
andresceballosm pushed a commit to andresceballosm/status-mobile that referenced this issue Aug 29, 2023
To help with status-im#15595 changes, refactoring is required.
In this PR we switch from config to env vars.
Doing some cleanup meanwhile.
andresceballosm pushed a commit to andresceballosm/status-mobile that referenced this issue Aug 29, 2023
Fixes partially status-im#15595
In order to build less targets, when not needed we introduce this
mapping logic.
If only specific ABI is required - status-go will have the same
arhitecuture.
@yakimant
Copy link
Member

yakimant commented Dec 25, 2023

Also iOS improved here:

Some room for optimisation is still left there, see comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants