search

ARM-software / devlib

Library for interaction with and instrumentation of remote devices.
Apache License 2.0
45 stars 77 forks source link

tools/android: Port remaining features from LISA/install_base.sh #686

Closed metin-arm closed 3 weeks ago

metin-arm commented 2 months ago

tools/android/install_base.sh script has been forked from LISA in order to install Android tools [1] in devlib. There are duplicated functionalities between LISA and devlib versions of install_base.sh script.

Thus, port remaining bits from LISA to devlib. Also make some improvements (e.g., remove skins directory, address shellcheck findings, clean comments, remove duplicates from package list, etc) while we are here.

Another PR [2] will remove duplicated parts from LISA.

[1] https://github.com/ARM-software/devlib/commit/598c0c1d3cba [2] https://gitlab.arm.com/tooling/lisa/-/merge_requests/2279

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

douglas-raillard-arm commented 1 month ago

@marcbonnici The rational was that this code was originally in LISA where we wanted to have up to date android SDK tools. Now that devlib has a use for them as well (for the CI), we would end up with duplicated code which we don't want. With that PR, we can put the part used by devlib here and just call it from LISA side.

marcbonnici commented 1 month ago

Makes sense thanks. From the devlib perspective I'm happy to merge this, however from a quick look at the corresponding LISA PR it seems that review is currently ongoing so wanted to check if the outcome of that review may result in subsequent modifications to this PR?

metin-arm commented 1 month ago

I think I addressed the comments in both PRs, but @douglas-raillard-arm can confirm this.

douglas-raillard-arm commented 1 month ago

Small stuff on LISA side, I think the devlib side can be merged, then we update the vendored devlib version in LISA from which we will take the script and then merge the one in LISA.

@marcbonnici @metin-arm The name install_base.sh is inherited from the dawn of the LISA-verse, it's totally bikeshedding but if you feel like you can come up with a better name, now is the chance to change it :)

metin-arm commented 1 month ago

@marcbonnici @metin-arm The name install_base.sh is inherited from the dawn of the LISA-verse, it's totally bikeshedding but if you feel like you can come up with a better name, now is the chance to change it :)

Considering the script is already under tools/android directory, would install_sdk.sh be meaningful? Or what about deploy_utils.sh since the script also creates AVD, etc?

douglas-raillard-arm commented 1 month ago

would install_sdk.sh be meaningful

It's only about the SDK now but there is a good chance it grows at some point with things completely unrelated to the SDK.

Or what about deploy_utils.sh since the script also creates AVD, etc?

I'd keep "install" or "setup" as part of the name, as we are not deploying anything to some prod environment, we really are just setting up the host. Maybe something like "setup_host.sh" would be fitting.

metin-arm commented 1 month ago

I'd keep "install" or "setup" as part of the name, as we are not deploying anything to some prod environment, we really are just setting up the host. Maybe something like "setup_host.sh" would be fitting.

Will rename it to setup_host.sh. Thanks.

douglas-raillard-arm commented 4 weeks ago

@marcbonnici LGTM, ready to merge on LISA side once this one is merged