Re: [Lldb-commits] [PATCH] D16830: Move some android platform functions to lldbplatformutil

[Pavel Labath via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

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



Comment at: 
packages/Python/lldbsuite/test/functionalities/inferior-assert/TestInferiorAssert.py:63
@@ -62,3 +62,3 @@
 def check_stop_reason(self):
-match_result = matchAndroid(api_levels=list(range(1, 16+1)))(self)
-if match_result is not None:
+match_result = 
lldbplatformutil.match_android_device(self.getArchitecture(), 
api_levels=list(range(1, 16+1)))
+if match_result == lldbplatformutil.AndroidMatchResult.Matched:

valid_api_levels=...


Comment at: packages/Python/lldbsuite/test/lldbplatformutil.py:38
@@ +37,3 @@
+full_cmd = ["adb"] + device_id_args + cmd
+p = subprocess.Popen(full_cmd, stdout=PIPE, stderr=PIPE)
+stdout, stderr = p.communicate()

subprocess.PIPE


http://reviews.llvm.org/D16830



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D16830: Move some android platform functions to lldbplatformutil

[Zachary Turner via lldb-commits]

zturner added a comment.

r259724


Repository:
  rL LLVM

http://reviews.llvm.org/D16830



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D16830: Move some android platform functions to lldbplatformutil

[Siva Chandra via lldb-commits]

This change broke Android testsuite run:
http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-android/builds/5147

On Wed, Feb 3, 2016 at 11:20 AM, Zachary Turner via lldb-commits
 wrote:
> zturner added a comment.
>
> In http://reviews.llvm.org/D16830#343232, @zturner wrote:
>
>> In http://reviews.llvm.org/D16830#342842, @tberghammer wrote:
>>
>> > 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.
>
>
> Repository:
>   rL LLVM
>
> http://reviews.llvm.org/D16830
>
>
>
> ___
> lldb-commits mailing list
> lldb-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D16830: Move some android platform functions to lldbplatformutil

[Zachary Turner via lldb-commits]

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.

On Wed, Feb 3, 2016 at 2:51 PM Siva Chandra  wrote:

> This change broke Android testsuite run:
>
> http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-android/builds/5147
>
> On Wed, Feb 3, 2016 at 11:20 AM, Zachary Turner via lldb-commits
>  wrote:
> > zturner added a comment.
> >
> > In http://reviews.llvm.org/D16830#343232, @zturner wrote:
> >
> >> In http://reviews.llvm.org/D16830#342842, @tberghammer wrote:
> >>
> >> > 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.
> >
> >
> > Repository:
> >   rL LLVM
> >
> > http://reviews.llvm.org/D16830
> >
> >
> >
> > ___
> > lldb-commits mailing list
> > lldb-commits@lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D16830: Move some android platform functions to lldbplatformutil

[Tamas Berghammer via lldb-commits]

tberghammer accepted this revision.
tberghammer added a comment.

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



Comment at: packages/Python/lldbsuite/test/lldbplatformutil.py:73-81
@@ +72,11 @@
+
+def match_android_device(device_arch, valid_archs=None, valid_api_levels=None):
+if not _target_is_android():
+return AndroidMatchResult.InvalidTarget
+if valid_archs is not None and device_arch not in valid_archs:
+return AndroidMatchResult.InvalidArch
+if valid_api_levels is not None and android_device_api() not in 
valid_api_levels:
+return AndroidMatchResult.InvalidApiLevel
+
+return AndroidMatchResult.Matched
+

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.


Comment at: packages/Python/lldbsuite/test/lldbtest.py:683-685
@@ -741,2 +682,5 @@
 """
-return expectedFailure(matchAndroid(api_levels, archs), bugnumber)
+def skip_for_android(self):
+result = lldbplatformutil.match_android_device(self.getArchitecture(), 
archs, api_levels)
+return "skipping for android" if result == 
lldbplatformutil.AndroidMatchResult.Matched else None
+return expectedFailure(skip_for_android, bugnumber)

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


http://reviews.llvm.org/D16830



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D16830: Move some android platform functions to lldbplatformutil

[Zachary Turner via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL259680: Move some android platform functions to 
lldbplatformutil. (authored by zturner).

Changed prior to commit:
  http://reviews.llvm.org/D16830?vs=46721=46806#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D16830

Files:
  
lldb/trunk/packages/Python/lldbsuite/test/functionalities/inferior-assert/TestInferiorAssert.py
  lldb/trunk/packages/Python/lldbsuite/test/lldbplatformutil.py
  lldb/trunk/packages/Python/lldbsuite/test/lldbtest.py

Index: lldb/trunk/packages/Python/lldbsuite/test/lldbplatformutil.py
===
--- lldb/trunk/packages/Python/lldbsuite/test/lldbplatformutil.py
+++ lldb/trunk/packages/Python/lldbsuite/test/lldbplatformutil.py
@@ -4,12 +4,16 @@
 from __future__ import absolute_import
 
 # System modules
+import re
+import subprocess
 
 # Third-party modules
+from six.moves.urllib import parse as urlparse
 
 # LLDB modules
-
-import re
+from . import configuration
+import use_lldb_suite
+import lldb
 
 def check_first_register_readable(test_case):
 arch = test_case.getArchitecture()
@@ -25,3 +29,59 @@
 else:
 # TODO: Add check for other architectures
 test_case.fail("Unsupported architecture for test case (arch: %s)" % test_case.getArchitecture())
+
+def _run_adb_command(cmd, device_id):
+device_id_args = []
+if device_id:
+device_id_args = ["-s", device_id]
+full_cmd = ["adb"] + device_id_args + cmd
+p = subprocess.Popen(full_cmd, stdout=PIPE, stderr=PIPE)
+stdout, stderr = p.communicate()
+return p.returncode, stdout, stderr
+
+def _target_is_android():
+if not hasattr(_target_is_android, 'result'):
+triple = lldb.DBG.GetSelectedPlatform().GetTriple()
+match = re.match(".*-.*-.*-android", triple)
+_target_is_android.result = match is not None
+return _target_is_android.result
+
+def android_device_api():
+if not hasattr(android_device_api, 'result'):
+assert configuration.lldb_platform_url is not None
+device_id = None
+parsed_url = urlparse.urlparse(configuration.lldb_platform_url)
+host_name = parsed_url.netloc.split(":")[0]
+if host_name != 'localhost':
+device_id = host_name
+if device_id.startswith('[') and device_id.endswith(']'):
+device_id = device_id[1:-1]
+retcode, stdout, stderr = _run_adb_command(
+["shell", "getprop", "ro.build.version.sdk"], device_id)
+if retcode == 0:
+android_device_api.result = int(stdout)
+else:
+raise LookupError(
+">>> Unable to determine the API level of the Android device.\n"
+">>> stdout:\n%s\n"
+">>> stderr:\n%s\n" % (stdout, stderr))
+return android_device_api.result
+
+def match_android_device(device_arch, valid_archs=None, valid_api_levels=None):
+if not _target_is_android():
+return False
+if valid_archs is not None and device_arch not in valid_archs:
+return False
+if valid_api_levels is not None and android_device_api() not in valid_api_levels:
+return False
+
+return True
+
+def finalize_build_dictionary(dictionary):
+if _target_is_android():
+if dictionary is None:
+dictionary = {}
+dictionary["OS"] = "Android"
+if android_device_api() >= 16:
+dictionary["PIE"] = 1
+return dictionary
Index: lldb/trunk/packages/Python/lldbsuite/test/lldbtest.py
===
--- lldb/trunk/packages/Python/lldbsuite/test/lldbtest.py
+++ lldb/trunk/packages/Python/lldbsuite/test/lldbtest.py
@@ -54,13 +54,13 @@
 import unittest2
 from six import add_metaclass
 from six import StringIO as SixStringIO
-from six.moves.urllib import parse as urlparse
 import six
 
 # LLDB modules
 import use_lldb_suite
 import lldb
 from . import configuration
+from . import lldbplatformutil
 from . import lldbtest_config
 from . import lldbutil
 from . import test_categories
@@ -448,51 +448,6 @@
 else:
 return True
 
-def run_adb_command(cmd, device_id):
-device_id_args = []
-if device_id:
-device_id_args = ["-s", device_id]
-full_cmd = ["adb"] + device_id_args + cmd
-p = Popen(full_cmd, stdout=PIPE, stderr=PIPE)
-stdout, stderr = p.communicate()
-return p.returncode, stdout, stderr
-
-def append_android_envs(dictionary):
-if dictionary is None:
-dictionary = {}
-dictionary["OS"] = "Android"
-if android_device_api() >= 16:
-dictionary["PIE"] = 1
-return dictionary
-
-def target_is_android():
-if not hasattr(target_is_android, 'result'):
-triple = lldb.DBG.GetSelectedPlatform().GetTriple()
-match = re.match(".*-.*-.*-android", triple)
-target_is_android.result = match is not None
-return 

Re: [Lldb-commits] [PATCH] D16830: Move some android platform functions to lldbplatformutil

[Zachary Turner via lldb-commits]

zturner added a comment.

In http://reviews.llvm.org/D16830#343232, @zturner wrote:

> In http://reviews.llvm.org/D16830#342842, @tberghammer wrote:
>
> > 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.


Repository:
  rL LLVM

http://reviews.llvm.org/D16830



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D16830: Move some android platform functions to lldbplatformutil

[Zachary Turner via lldb-commits]

zturner marked 2 inline comments as done.
zturner added a comment.

In http://reviews.llvm.org/D16830#342842, @tberghammer wrote:

> 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


http://reviews.llvm.org/D16830



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits