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

Fix applaunch workload #1261

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kbaladurin
Copy link

@kbaladurin kbaladurin commented May 10, 2024

This patch fixes following issues:

  • Starting from Android 11 it is not possible to use Runtime.exec to execute commands that require shell permissions like am start.

  • Engineering Android versions has su that doesn't support -c argument.

@kbaladurin
Copy link
Author

@marcbonnici please take a look!

This patch fixes following issues:

* Starting from Android 11 it is not possible to use Runtime.exec to
execute commands that require shell permissions like `am start`.

* Engineering Android versions has su that doesn't support -c argument.
Copy link
Contributor

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

Few thoughts, otherwise confirmed to work on my nexus5.

# Android doesn't allow to writable dex files starting 14 version and
# default location (/sdcard/devlib-target) doesn't support readonly files
# so we use /data/local/tmp as asset directory for this workload.
self.asset_directory = '/data/local/tmp'
Copy link
Contributor

Choose a reason for hiding this comment

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

We tend to try and avoid hardcoding paths to support different configurations if possible.
Currently we have the default self.executables_directory set to /data/local/tmp/bin. I'm wondering if these directories would share the same requirements which we could also make use of here rather than setting this parameter in two locations?

If not perhaps this be a workload Parameter with a default so a user can understand the requirements for the directory and optionally change this for their system if required?

[1] https://github.com/ARM-software/devlib/blob/7276097d4e12ff2b3cfa1bb0ba40cee24ae3372b/devlib/target.py#L2532

Copy link
Author

@kbaladurin kbaladurin May 21, 2024

Choose a reason for hiding this comment

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

The main requirement for assert_directory for applaunch workload is a ability to create readonly files there and looks like executables_directory in general doesn't require it. But adding a new parameter for applauch workload with a default value is a good idea, thank you!

try:
self.target.execute('su root id')
except TargetError:
raise WorkloadError(
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing import for WorkloadError.

# su [WHO [COMMAND...]]
# - Targets with rooted user Android version have su that supports passing
# command via -c argument.
def su_has_command_option(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking about this a bit more, do other workloads that require root work on these engineering versions if they are relying on the su -c format?

In devlib we try to detect [1] which su command (su -c or 'echo {} | su) [2] we should use for a device so I'm wondering if it would be beneficial to move this check to the common code and then we can query it from here?

[1] https://github.com/ARM-software/devlib/blob/master/devlib/utils/android.py#L466
[2] https://github.com/ARM-software/devlib/blob/master/devlib/utils/android.py#L258

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I think moving this check to devlib is a good idea, thank you! I will create a PR for it.

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.

2 participants