Improve adb_shell and move to 'su -c' #387

ptosi · 2019-05-14T11:33:57Z

Slight rework of adb_shell to improve readability and stability but, most importantly, to address #386.

Please note that this requires modern Py3 and should be merged post-WA3.1 (preferred) or could be modified to be Py2.7-compatible.

ptosi · 2019-05-14T11:34:45Z

devlib/utils/android.py

 import zipfile

-from pipes import quote
+from shlex import quote


pipes.quote is hidden and shouldn't be used; shlex.quote was added in Py3.3 for this purpose.

Note: we're currently still supporting Python 2.7; please add an import guard with fallback to importing from pipes.

FWIW they are indeed the same and was exposed in Python 3.3, although it has been present for a long time. It has been documented and deprecated at the same time in 2.7, so it's half-internal (that "private but documented" status does not make much sense ...): https://docs.python.org/2/library/pipes.html#pipes.quote

Indeed, under the hood of CPython, pipes.quote is shlex.quote but it's messy even for them. Using shlex.quote is incontestably the cleanest way (as that's what the Standard says) to get the quote function (in Py3.3+) but I agree with you that using pipes.quote works (until it doesn't?).

I guess I was just lazy and did not introduce an except block for something that appeared to work in Python 3 as well, using shlex when available is definitely less confusing :)

ptosi · 2019-05-14T11:36:05Z

devlib/utils/android.py

+    if device is not None:
+        parts += ['-s', device]
+    parts += ['shell',
+              command if not as_root else f'su -c {quote(command)}']


f-strings are introduced in Py3.6.

We're still aiming to be compatible with Python 2.7.

devlib/utils/android.py

ptosi · 2019-05-15T13:40:02Z

From #386 (comment):

I can't think of a reason other than that for the pipe, and would be happy to make the switching to using '-c' . Though it would be have to be thoroughly tested with WA/LISA before merging.

How should this testing be done?

setrofim · 2019-05-15T14:12:16Z

We can handle the testing for WA. For LISA, I believe there is a way to get it to point to an alternative branch/repo for devlib -- so just switch it over and make sure that everything still works. If it's been used for a while and nothing breaks, then we're probably good. Obviously, if LISA had it's own test suite, make sure that passes too.

valschneider · 2019-05-20T08:49:43Z

Regarding LISA: I'll pick that up and feed it through our device tests. Will update you folks here.

@douglas-raillard-arm FYI.

douglas-raillard-arm · 2019-05-21T12:56:09Z

devlib/utils/android.py

+    if device is not None:
+        parts += ['-s', device]
+    parts += ['shell',
+              command if not as_root else f'echo {quote(command)} | su']


formatted literal strings (f prefix) has been introduced in Python 3.6. AFAIK, devlib is still supporting (at least) Python 3.5. This version is needed to support Ubuntu 16.04 xenial, which is still used in quite a few places (AFAIK that's the most recent distro you can get on Travis).

I was unable to find documentation stating which (sub-)version of Python 3 devlib was supporting (I might have missed a document or webpage?) so I assumed Py3.6.

You're right that devlib is still supporting Py3.5 but, now that the projects (incl. WA) are slowly moving away from Python 2, it is necessary to define a "minimum" version of Python 3 to be supported. I though Py3.6 made sense as it's fairly common, relatively stable (2.5 years old; bug-fix support already ended at the end of last year) and provides some valuable new features.

The f-strings have been removed from this PR but it is still necessary to define which version of Python 3 is/will be used by the projects; @setrofim & @marcbonnici ?

So for this release we will continue to be targeting older version of 3 as well as 2.7. However as @pietos01 mentioned for both WA and devlib, python 2.7 is now deprecated and support will be dropped in the next release. At the point we are enforcing a switch to python 3 we were also thinking of setting Python 3.6 as our new required version as this appears to have a large adoption and introduces a number of worthwhile changes.
I realise this has not been communicated properly so we will need to formalise this information and get feedback on our plans. @douglas-raillard-arm Do you foresee any issues with this?

No worries, it eventually reaches involved people :)

So, in the current LISA landscape:

Travis' latest GNU/Linux environment is Ubuntu 16,04 xenial which ships with 3.5. Getting higher versions may be possible via some PPA. Travis can also install multiple version of python if we tell it it's a python project, but we can't use that since it also comes with its own (non-optional) virtualenv which conflicts with the one from LISA. (we could configure LISA to cooperate, but we want to check end-to-end install as well in a way that matches what users will do).

Some LAVA containers/Jenkins environment we use is using Ubuntu Xenial as well. That may be updated, but I would need to have a chat with the infra guys before committing on that. @ionela-voinescu any comment on that ?

Some people in our team may be using Ubuntu Xenial, I assume this can be extended to other teams, since it will be supported until 2021/2024.

All in all, we might be able to require Python >=3.6 but that means asking people to use PPA. Do you have some kind of informal list of things that are desirable in 3.6 and not in 3.5 ? (formatted literals are definitely desirable :-) but may not be big enough to warrant dropping native xenial support :-( ).

Ok thank you very much for the information, that's definitely something we'll need to take into account then.
We don't have such a list yet so we will need to evaluate this. In order to collate this information I've opened the issue #389 to discuss this further.

douglas-raillard-arm · 2019-05-21T12:58:57Z

devlib/utils/android.py

 import zipfile

-from pipes import quote
+from shlex import quote


FWIW they are indeed the same and was exposed in Python 3.3, although it has been present for a long time. It has been documented and deprecated at the same time in 2.7, so it's half-internal (that "private but documented" status does not make much sense ...): https://docs.python.org/2/library/pipes.html#pipes.quote

marcbonnici · 2019-07-09T10:08:11Z

@valschneider Just wondering if you'd had a chance to run this through your tests?

valschneider · 2019-07-09T10:09:44Z

I think this got somewhat lost in the mayhem caused by our Travis issues, let me check with @douglas-raillard-arm.

marcbonnici · 2019-07-17T16:45:49Z

@pietos01 Would you be able to squash the relevant commits and update the commit messages to reflect it is no longer Python3 only?
We'd like to merge this shortly once @valschneider's tests are done.

Move from pipes.quote (private) to shlex.quote (Py3.3+ standard). Make tests of inputs against None (their default value) instead of based on their truthiness. Improve logging through quoted commands (runnable as-is, less confusing). Make the command-building process straightforward for readability and maintainability.

Move from the current implementation (piping the command to su) which has unexpected behaviours to the '-c' su flag (which then becomes required).

ptosi commented May 14, 2019

View reviewed changes

This was referenced May 15, 2019

Add generic perf trace instrument #388

Closed

Add generic perf Instrument ARM-software/workload-automation#979

Closed

douglas-raillard-arm reviewed May 21, 2019

View reviewed changes

ptosi force-pushed the fix/adb_shell branch from 0a7c1f4 to 8f568b8 Compare May 21, 2019 15:03

marcbonnici mentioned this pull request May 22, 2019

[Discussion] Minimum Python 3 version required for next release #389

Closed

pietos01-arm added 2 commits July 18, 2019 14:50

utils.adb_shell: Move from 'echo CMD | su' to '-c'

89c40fb

Move from the current implementation (piping the command to su) which has unexpected behaviours to the '-c' su flag (which then becomes required).

ptosi force-pushed the fix/adb_shell branch from de21f26 to 89c40fb Compare July 18, 2019 13:51

marcbonnici approved these changes Jul 19, 2019

View reviewed changes

setrofim approved these changes Jul 19, 2019

View reviewed changes

marcbonnici merged commit 6b414cc into ARM-software:master Jul 19, 2019

v0lker mentioned this pull request Jul 25, 2019

pr #387 ('su -c') seems to break command execution for me #405

Closed

Improve adb_shell and move to 'su -c' #387

Improve adb_shell and move to 'su -c' #387

Uh oh!

Conversation

ptosi commented May 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ptosi commented May 15, 2019

Uh oh!

setrofim commented May 15, 2019

Uh oh!

valschneider commented May 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcbonnici commented Jul 9, 2019

Uh oh!

valschneider commented Jul 9, 2019

Uh oh!

marcbonnici commented Jul 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

valschneider commented May 20, 2019 •

edited

Loading