-
Notifications
You must be signed in to change notification settings - Fork 120
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
base: master
Are you sure you want to change the base?
Fix applaunch workload #1261
Conversation
@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.
d9c2ccd
to
3f5707e
Compare
There was a problem hiding this 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' |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
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.