[PATCH] D120653: ClangBuilder: Use list of checks instead of boolean. NFCI

[Galina via Phabricator via llvm-commits]

gkistanova requested changes to this revision.
gkistanova added a comment.
This revision now requires changes to proceed.

Thanks, Diana!

List of checks is a good idea.



================
Comment at: zorg/buildbot/builders/ClangBuilder.py:75
             clean=True,
-            test=True,
+            checks=['check-all'],
             cmake='cmake',
----------------
Please do not use mutable objects as defaults. Instead make it None and handle that in the body as the trigger to set a default value.

https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments


================
Comment at: zorg/buildbot/builders/ClangBuilder.py:136
             clean=True,
-            test=True,
+            checks=['check-all'],
             cmake='cmake',
----------------
Ditto.


================
Comment at: zorg/buildbot/builders/ClangBuilder.py:186
             clean=True,
-            test=True,
+            checks=['check-all'],
             cmake='cmake',
----------------
Ditto.

`_getClangCMakeBuildFactory` seems the right place to implement that “default check-all” logic.


================
Comment at: zorg/buildbot/builders/ClangBuilder.py:295
     ninja_install_cmd = ['ninja', 'install'] + jobs_cmd
-    ninja_check_cmd = ['ninja', 'check-all'] + jobs_cmd
+    ninja_check_cmd = ['ninja'] + checks + jobs_cmd
 
----------------
Just for a better UX you may want each check to be a separate build step, if multiple specified. Not a stopper for this patch.


Repository:
  rZORG LLVM Github Zorg

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120653/new/

https://reviews.llvm.org/D120653