Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
marcbonnici merged 6 commits into from
Jun 26, 2024
Merged

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

marcbonnici merged 6 commits into from
Jun 26, 2024

Conversation

@metin-arm
Copy link
Contributor

@metin-arm metin-arm commented May 17, 2024 •
edited
Loading

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] 598c0c1d3cba
[2] https://gitlab.arm.com/tooling/lisa/-/merge_requests/2279

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

@metin-arm metin-arm force-pushed the unify-install-base branch 2 times, most recently from 5bd8d80 to 2e1823c Compare May 17, 2024 11:50
@metin-arm metin-arm force-pushed the unify-install-base branch 2 times, most recently from 9bafb8e to 9237ef6 Compare May 31, 2024 15:29
marcbonnici
marcbonnici reviewed Jun 12, 2024
View reviewed changes
Copy link
Collaborator

@marcbonnici marcbonnici left a comment

Choose a reason for hiding this comment

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

From a high level this makes sense to me @douglas-raillard-arm would you be able to provide your thoughts from a LISA perspective?

@douglas-raillard-arm
Copy link
Collaborator

douglas-raillard-arm commented Jun 13, 2024

@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
Copy link
Collaborator

marcbonnici commented Jun 13, 2024

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
Copy link
Contributor Author

metin-arm commented Jun 13, 2024

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

@douglas-raillard-arm
Copy link
Collaborator

douglas-raillard-arm commented Jun 19, 2024

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
Copy link
Contributor Author

metin-arm commented Jun 19, 2024

@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?

@metin-arm metin-arm force-pushed the unify-install-base branch from 9237ef6 to 4029936 Compare June 19, 2024 10:41
@douglas-raillard-arm
Copy link
Collaborator

douglas-raillard-arm commented Jun 19, 2024

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
Copy link
Contributor Author

metin-arm commented Jun 19, 2024

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
Copy link
Collaborator

douglas-raillard-arm commented Jun 26, 2024

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

metin-arm added 6 commits June 26, 2024 11:18
@metin-arm
tools/android: Make cleanups in install_base.sh
12a7327 
Just a house-keeping patch to do some trivial improvements:
- Move global variables to the beginning of the script
- Eliminate redundant echo commands
- Tidy up the system package list
- Drop superfluous ';'

Signed-off-by: Metin Kaya <metin.kaya@arm.com>
@metin-arm
tools/android: Clone install_android_platform_tools() from LISA
19c4018 
Make sure devlib/install_base.sh has complete Android SDK support. This
will be the first step of removing duplicate Android SDK installation
functions from LISA/install_base.sh.

Signed-off-by: Metin Kaya <metin.kaya@arm.com>
@metin-arm
tools/android: Remove emulator skins
af11734 
Apparently skins are just nice to have. Also devlib uses emulated
devices in command line (no GUI), so skins are unnecessary. Removing
skins will also reduce the disparity in install_base.sh scripts of LISA
and devlib.

Signed-off-by: Metin Kaya <metin.kaya@arm.com>
@metin-arm
tools/android: Make install_base.sh modular for LISA integration
989e6aa 
Both devlib and LISA utilizes install_base.sh script, but they install
different packages and support different input arguments. Also support
custom ANDROID_HOME environment variable in order to let LISA (or just
let users install Android SDK/tools wherever they want) choose install
location.

Signed-off-by: Metin Kaya <metin.kaya@arm.com>
@metin-arm
tools/setup_host.sh: Rename install_base.sh to setup_host.sh
07c8103 
install_base.sh is left-over from LISA/install_base.sh. Scope of the
script in question is different (and potentially can diverge more) than
its root in LISA. Hence, give it a more descriptive (hopefully) name.

Signed-off-by: Metin Kaya <metin.kaya@arm.com>
@metin-arm
tools/docker: Fixup test config file name
5424b37 
Apparently commit

492d42d ("target: tests: Address review comments on PR#667")

erroneously renamed target_configs.yaml to target_configs.yml.
Rename it to test_config.yml.

Also address 2 Docker warnings related to environment variables while we
are here.

Signed-off-by: Metin Kaya <metin.kaya@arm.com>
@metin-arm metin-arm force-pushed the unify-install-base branch from d01a094 to 5424b37 Compare June 26, 2024 13:28
@marcbonnici marcbonnici merged commit de84a08 into ARM-software:master Jun 26, 2024
@metin-arm metin-arm deleted the unify-install-base branch June 27, 2024 07:43
github-actions bot pushed a commit to ARM-software/lisa that referenced this pull request Jun 27, 2024
@metin-arm
install_base.sh: Offload Android SDK/tools installation to devlib
8bbce4c 
FIX

devlib will have necessary support [1] for installing Android tools to
meet LISA requirements. Thus, remove duplicated functionality from
install_base.sh script.

[1] ARM-software/devlib#686

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

Reviewers

@marcbonnici marcbonnici marcbonnici approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@metin-arm @douglas-raillard-arm @marcbonnici