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

tools/android: Add support for creating Android virtual devices #668

Merged
merged 1 commit into from
Mar 20, 2024

Conversation

metin-arm
Copy link
Contributor

@metin-arm metin-arm commented Jan 25, 2024

tools/android: Add support for creating Android virtual devices
Introduce tools/android/install_base.sh [1] script to install
Android command line tools including necessary platforms and
system-images for Linux and create Android Virtual Devices (AVD) for
Pixel 6 with Android v12 & v14 as well as an Android virtual desktop
device (v13) for ChromeOS tests.

[1] Forked from https://github.com/ARM-software/lisa/blob/main/install_base.sh

Signed-off-by: Metin Kaya metin.kaya@arm.com

@metin-arm metin-arm force-pushed the staging2 branch 2 times, most recently from 96b61ca to b7c55dd Compare January 26, 2024 16:31
yes | call_android_sdkmanager --verbose --channel=0 --install "platforms;android-31"
yes | call_android_sdkmanager --verbose --channel=0 --install "platforms;android-34"
yes | call_android_sdkmanager --verbose --channel=0 --install "system-images;android-31;google_apis;${HOST_ARCH}"
yes | call_android_sdkmanager --verbose --channel=0 --install "system-images;android-34;google_apis;${HOST_ARCH}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use HOST_ARCH here, but I believe Android SDK supports only arm64-v8a Arm variant. Maybe I should use arm64-v8a if the host is not an Intel machine.

Introduce ``tools/android/install_base.sh`` [1] script to install
Android command line tools including necessary platforms and
system-images for Linux and create Android Virtual Devices (AVD) for
Pixel 6 with Android v12 & v14 as well as an Android virtual *desktop*
device (v13) for ChromeOS tests.

[1] Forked from https://github.com/ARM-software/lisa/blob/main/install_base.sh

Signed-off-by: Metin Kaya <metin.kaya@arm.com>
@marcbonnici marcbonnici merged commit 598c0c1 into ARM-software:master Mar 20, 2024
@metin-arm metin-arm deleted the staging2 branch March 22, 2024 09:13
@@ -0,0 +1,314 @@
#!/usr/bin/env bash
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is essentially a copy/paste of the one from LISA. That creates a maintenance problem as this needs to evolve along how the android SDK manager is distributed, and that thing does evolve. On top of that I can see that even from day 0 of that PR, you made improvement to the script such as building a URL from a version number, without making the equivalent PR in LISA to keep them in sync:

    local url="https://dl.google.com/android/repository/commandlinetools-linux-${CMDLINE_VERSION}_latest.zip" 

So that stuff needs to be deduplicated with what we have in LISA. I think it would be reasonable to call the devlib script from lisa's install_base.sh and dedup it this way unless they really need to share some internal state.

However, once this happens, we need to be able to be reactive when things change and break. @marcbonnici would you be happy to grant me merge permission in the devlib repo ? I'd only use it for minor stuff that require short reaction time like fixing a broken SDK install procedure (e.g. a broken URL), or typos and such. Normal PRs would keep following the current review process.

Copy link
Contributor

Choose a reason for hiding this comment

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

EDIT: there is a whole range of helper functions that we definitely don't want to duplicate in here, such as test_os_release(). Maybe we should just source the devlib's script from LISA, and ensure that what we source "does nothing", i.e. just defines functions that we then call. devlib can have it's own entry point to call the functions, and the shared script would be a "library script".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

without making the equivalent PR in LISA to keep them in sync

Because I knew we would eliminate the one in LISA at some point.

#678 addresses all your comments except this one. Will work on removing the duplicate later (for a separate ticket).

# Forked from https://github.com/ARM-software/lisa/blob/main/install_base.sh
#

# shellcheck disable=SC2317
Copy link
Contributor

Choose a reason for hiding this comment

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

If that's the only violation found by shellcheck I'm glad :) (although I don't know how deep shellcheck actually looks)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that was the only shellcheck violation at least when I ran it for the last time.

fi
}

function set_host_arch
Copy link
Contributor

Choose a reason for hiding this comment

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

generic name for something not generic, this should be renamed with a more specific name.

Copy link
Contributor Author

@metin-arm metin-arm Mar 28, 2024

Choose a reason for hiding this comment

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

Will do.

function set_host_arch
{
# Google ABI type for Arm platforms
HOST_ARCH="arm64-v8a"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should call that HOST_ARCH. I'd expect at the very least such variable to match the devlib definition of abis, which would use arm64 in this case (see devlib.utils.misc.ABI_MAP). Maybe ANDROID_SDK_HOST_ARCH or something like that would be more appropriate.

local machine
machine=$(uname -m)
if [[ "${machine}" == "x86"* ]]; then
HOST_ARCH=${machine}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need any global mutable state here, with the problem that would ensue (someone not calling the function). The function can just print "%s" "$arch" the arch value and code that needs it can call the function as $(get_android_sdk_host_arch), like in basically any language.

metin-arm added a commit to metin-arm/devlib that referenced this pull request Mar 28, 2024
PR#668: ARM-software#668

- Fix mixed tab-space white-spacing issues
- s/CMDLINE_VERSION/ANDROID_CMDLINE_VERSION/ to be more precise
- s/set_host_arch/get_android_sdk_host_arch/ because the global variable
  for Android host architecture is removed now

Signed-off-by: Metin Kaya <metin.kaya@arm.com>
metin-arm added a commit to metin-arm/devlib that referenced this pull request Mar 28, 2024
PR#668: ARM-software#668

- Fix mixed tab-space white-spacing issues
- s/CMDLINE_VERSION/ANDROID_CMDLINE_VERSION/ to be more precise
- s/set_host_arch/get_android_sdk_host_arch/ because the global variable
  for Android host architecture is removed now

Signed-off-by: Metin Kaya <metin.kaya@arm.com>
metin-arm added a commit to metin-arm/devlib that referenced this pull request Apr 2, 2024
PR#668: ARM-software#668

- Fix mixed tab-space white-spacing issues
- s/CMDLINE_VERSION/ANDROID_CMDLINE_VERSION/ to be more precise
- s/set_host_arch/get_android_sdk_host_arch/ because the global variable
  for Android host architecture is removed now

Signed-off-by: Metin Kaya <metin.kaya@arm.com>
metin-arm added a commit to metin-arm/devlib that referenced this pull request Apr 2, 2024
PR#668: ARM-software#668

- Fix mixed tab-space white-spacing issues
- s/CMDLINE_VERSION/ANDROID_CMDLINE_VERSION/ to be more precise
- s/set_host_arch/get_android_sdk_host_arch/ because the global variable
  for Android host architecture is removed now

Signed-off-by: Metin Kaya <metin.kaya@arm.com>
metin-arm added a commit to metin-arm/devlib that referenced this pull request Apr 2, 2024
PR#668: ARM-software#668

- Fix mixed tab-space white-spacing issues
- s/CMDLINE_VERSION/ANDROID_CMDLINE_VERSION/ to be more precise
- s/set_host_arch/get_android_sdk_host_arch/ because the global variable
  for Android host architecture is removed now

Signed-off-by: Metin Kaya <metin.kaya@arm.com>
metin-arm added a commit to metin-arm/devlib that referenced this pull request Apr 2, 2024
PR#668: ARM-software#668

- Fix mixed tab-space white-spacing issues
- s/CMDLINE_VERSION/ANDROID_CMDLINE_VERSION/ to be more precise
- s/set_host_arch/get_android_sdk_host_arch/ because the global variable
  for Android host architecture is removed now

Signed-off-by: Metin Kaya <metin.kaya@arm.com>
metin-arm added a commit to metin-arm/devlib that referenced this pull request Apr 15, 2024
PR#668: ARM-software#668

- Fix mixed tab-space white-spacing issues
- s/CMDLINE_VERSION/ANDROID_CMDLINE_VERSION/ to be more precise
- s/set_host_arch/get_android_sdk_host_arch/ because the global variable
  for Android host architecture is removed now

Signed-off-by: Metin Kaya <metin.kaya@arm.com>
metin-arm added a commit to metin-arm/devlib that referenced this pull request Apr 15, 2024
PR#668: ARM-software#668

- Fix mixed tab-space white-spacing issues
- s/CMDLINE_VERSION/ANDROID_CMDLINE_VERSION/ to be more precise
- s/set_host_arch/get_android_sdk_host_arch/ because the global variable
  for Android host architecture is removed now

Signed-off-by: Metin Kaya <metin.kaya@arm.com>
metin-arm added a commit to metin-arm/devlib that referenced this pull request Apr 17, 2024
PR#668: ARM-software#668

- Fix mixed tab-space white-spacing issues
- s/CMDLINE_VERSION/ANDROID_CMDLINE_VERSION/ to be more precise
- s/set_host_arch/get_android_sdk_host_arch/ because the global variable
  for Android host architecture is removed now

Signed-off-by: Metin Kaya <metin.kaya@arm.com>
metin-arm added a commit to metin-arm/devlib that referenced this pull request May 16, 2024
PR#668: ARM-software#668

- Fix mixed tab-space white-spacing issues
- s/CMDLINE_VERSION/ANDROID_CMDLINE_VERSION/ to be more precise
- s/set_host_arch/get_android_sdk_host_arch/ because the global variable
  for Android host architecture is removed now

Signed-off-by: Metin Kaya <metin.kaya@arm.com>
metin-arm added a commit to metin-arm/devlib that referenced this pull request May 17, 2024
PR#668: ARM-software#668

- Fix mixed tab-space white-spacing issues
- s/CMDLINE_VERSION/ANDROID_CMDLINE_VERSION/ to be more precise
- s/set_host_arch/get_android_sdk_host_arch/ because the global variable
  for Android host architecture is removed now

Signed-off-by: Metin Kaya <metin.kaya@arm.com>
metin-arm added a commit to metin-arm/devlib that referenced this pull request May 17, 2024
PR#668: ARM-software#668

- Fix mixed tab-space white-spacing issues
- s/CMDLINE_VERSION/ANDROID_CMDLINE_VERSION/ to be more precise
- s/set_host_arch/get_android_sdk_host_arch/ because the global variable
  for Android host architecture is removed now

Signed-off-by: Metin Kaya <metin.kaya@arm.com>
marcbonnici pushed a commit that referenced this pull request May 29, 2024
PR#668: #668

- Fix mixed tab-space white-spacing issues
- s/CMDLINE_VERSION/ANDROID_CMDLINE_VERSION/ to be more precise
- s/set_host_arch/get_android_sdk_host_arch/ because the global variable
  for Android host architecture is removed now

Signed-off-by: Metin Kaya <metin.kaya@arm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants