[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