[PATCH] Add support for running lldb-test on android targets

Siva Chandra sivachandra at google.com
Fri May 1 15:17:58 PDT 2015


LGTM with nits.

I have not gone through the correctness of the various commands involved as I do not understand them yet. Chaoren, can you do that?


================
Comment at: zorg/buildbot/builders/LLDBBuilder.py:257
@@ +256,3 @@
+    if env is None:
+        env={}
+    extraTestFlag=''
----------------
Nit: space before and after '='. Except when specifying default function args, there should be a space before and after '='. Space like that are missing at a number of places in this change. Same should be done with '+=' as well.

================
Comment at: zorg/buildbot/builders/LLDBBuilder.py:347
@@ -315,1 +346,3 @@
+    terminatecmd = []
+    cleandircmd=''
     # rsync
----------------
Nit: Python variables "leak" out of scope. Hence, you do not need lines 344-347.

================
Comment at: zorg/buildbot/builders/LLDBBuilder.py:404
@@ -328,1 +403,3 @@
+                               haltOnFailure=True,
+                               workdir='%s' % llvm_builddir))
     # launch lldb-server
----------------
Seems like there should be an 'else' case which just returns 'f'. Not that it matters in the current state of affairs, but it seems like a hole in the logic. For, if the platform is neither linux nor android, you will still go ahead and add other steps. I am not sure if this is OK.

================
Comment at: zorg/buildbot/builders/LLDBBuilder.py:561
@@ +560,3 @@
+    if target_platform is 'linux':
+        f = getLLDBLinuxCMakeStep(f,
+                                  build_compiler,
----------------
Nit: seems like you could just say return getLLDB....

================
Comment at: zorg/buildbot/builders/LLDBBuilder.py:567
@@ +566,3 @@
+    elif target_platform is 'android':
+        f = getLLDBAndroidCMakeStep(f,
+                                    build_compiler,
----------------
Same here.

http://reviews.llvm.org/D9354

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the llvm-commits mailing list