[PATCH] D17351: [zorg] Addition of extraLitArgs variable to ClangAndLLDBuilder.py

[Mike Edwards via llvm-commits]

sqlbyme closed this revision.

================
Comment at: zorg/buildbot/builders/ClangAndLLDBuilder.py:111
@@ -119,1 +110,3 @@
+        lit_args += extraLitArgs
+    lit_args_str = "-DLLVM_LIT_ARGS=\"%s\"" % (" ".join(lit_args))
 
----------------
gkistanova wrote:
> Maybe just let user define -DLLVM_LIT_ARGS with extraCmakeOptions param instead? With a meaningful default, let's say "-v".
> This way it would be clear of how a particular builder is configured.
> 
> With the current approach, you may end up having multiple conflicting -DLLVM_LIT_ARGS, one coming from extraCmakeOptions, and the other one coming from extraLitArgs.
> 
> The other option is to assert on having  -DLLVM_LIT_ARGS in extraCmakeOptions. This is up to you which option to choose.
> 
As "-DLLVM_LIT_ARGS=\"-v\"" was previously defined and may still be relied upon I'll add an assertion.

================
Comment at: zorg/buildbot/builders/ClangAndLLDBuilder.py:104
@@ -102,2 +103,3 @@
     if extraCompilerOptions:
+        assert extraCompilerOptions not in ["-DLLVM_LIT_ARGS="], "%s is not allowed to be defined here.  PLease use extraLitArgs to pass Lit arguments instead."
         options += extraCompilerOptions
----------------
gkistanova wrote:
> Did you mean checking extraCmakeOptions instead? extraCompilerOptions are C/C++ flags...
> 
> However, this assert would not work as you think it is.
> You want something similar to this:
> 
> ```
>   assert any(a.startswith('-DLLVM_LIT_ARGS=') for a in extraCmakeOptions, "Please use extraLitArgs for LIT arguments instead of defining them in extraCmakeOptions."
> ```
> 
Yep.  I'll fix this and re-submit.  Thanks.


http://reviews.llvm.org/D17351