This is an archive of the discontinued LLVM Phabricator instance.

Move some android platform functions to lldbplatformutil
ClosedPublic

Authored by zturner on Feb 2 2016, 4:17 PM.

Details

Summary
Move some android platform functions to lldbplatformutil.

My eventual goal is to move all of the test decorators to their
own module such as `decorators.py`.  But some of the decorators
use existing functions in `lldbtest.py` and conceptually the
functions are probably more appropriately placed in lldbplatformutil.
Moreover, `lldbtest.py` is a huge file with a ton of random utility
functions scattered around, so this patch also works toward the
goal of reducing the footprint of this one module to a more
reasonable size.

So this patch starts by moveing some utility functions over to
`lldbplatformutil` with the eventual goal of moving decorators over
 to their own module as well.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner updated this revision to Diff 46721.Feb 2 2016, 4:17 PM
zturner retitled this revision from to Move some android platform functions to lldbplatformutil.
zturner updated this object.
zturner added reviewers: labath, tberghammer.
zturner added a subscriber: lldb-commits.
labath accepted this revision.Feb 3 2016, 2:51 AM
labath edited edge metadata.

Seems to be working after applying the fixes below.

I was considering whether this shouldn't be moved to an even more platform-specific file (say androidutil.py or something), but I'll leave that up to you...

packages/Python/lldbsuite/test/functionalities/inferior-assert/TestInferiorAssert.py
63 ↗(On Diff #46721)

valid_api_levels=...

packages/Python/lldbsuite/test/lldbplatformutil.py
38 ↗(On Diff #46721)

subprocess.PIPE

This revision is now accepted and ready to land.Feb 3 2016, 2:51 AM
tberghammer accepted this revision.Feb 3 2016, 4:15 AM
tberghammer edited edge metadata.

Looks reasonable and I agree with Pavel in the idea of moving the android related utility functions into their own file for better separation

packages/Python/lldbsuite/test/lldbplatformutil.py
73–81 ↗(On Diff #46721)

Can we just return True/False? I don't see any usecase where we are caring about the reason why the device isn't matched.

packages/Python/lldbsuite/test/lldbtest.py
683–685 ↗(On Diff #46721)

Can we move this function to a higher scope so we don't have to duplicate it in every android specific decorator?

zturner marked 2 inline comments as done.Feb 3 2016, 10:20 AM

Looks reasonable and I agree with Pavel in the idea of moving the android related utility functions into their own file for better separation

I thought about creating an android specific module but I decided taht there is so little code right now it might not make sense. And then we already had this lldbplatformutil file that had only 1 function in it, so nobody was even really using it. So rather than end up with 2 different modules with 3 functions each, just have just have 1 module with 6 functions. On the other hand, if they continue to grow, then splitting apart makes a lot of sense.

It's not a big deal to me either way, so if you guys feel strongly I can move it. Since you're in a different time zone though and might not get this for another 24 hours, maybe I'll commit like this and then if you

This revision was automatically updated to reflect the committed changes.

Looks reasonable and I agree with Pavel in the idea of moving the android related utility functions into their own file for better separation

I thought about creating an android specific module but I decided taht there is so little code right now it might not make sense. And then we already had this lldbplatformutil file that had only 1 function in it, so nobody was even really using it. So rather than end up with 2 different modules with 3 functions each, just have just have 1 module with 6 functions. On the other hand, if they continue to grow, then splitting apart makes a lot of sense.

It's not a big deal to me either way, so if you guys feel strongly I can move it. Since you're in a different time zone though and might not get this for another 24 hours, maybe I'll commit like this and then if you

(somehow my previous message got cut off).

if you want it changed let me know and I'll move it to an android-specific file tomorrow.

zturner added a subscriber: zturner.Feb 3 2016, 2:55 PM

I ahad 3 CLs in progress locally and I guess I messed up a merge. There's
supposed to be a subprocess.PIPE instead of just PIPE. I'll check in a fix
shortly.