[PATCH] D27766: [buildbot] Added CUDATestsuiteBuilder.py

[Galina via Phabricator via llvm-commits]

gkistanova added a comment.

It is almost there.

Few small things to address and the patch would be ready to land.



================
Comment at: zorg/buildbot/builders/CUDATestsuiteBuilder.py:2
+from buildbot.process.properties import WithProperties
+from buildbot.plugins import steps
+from zorg.buildbot.builders import ClangBuilder
----------------
For the version of the buildbot we use, it should be
`from buildbot.steps.slave import RemoveDirectory`

and it used just as `RemoveDirectory` below.


================
Comment at: zorg/buildbot/builders/CUDATestsuiteBuilder.py:18
+        cuda_jobs=1,  # number of simultaneous CUDA apps to run
+        env={},  # Environmental variables for all steps.
+        enable_thrust_tests=False,
----------------
Please do not use mutable default arguments.


================
Comment at: zorg/buildbot/builders/CUDATestsuiteBuilder.py:42
+
+    jobs_cmd = [] if jobs is None else ["-j" + str(jobs)]
+
----------------
You do not need this any more. Instead feel free to pass `jobs` straight to the `NinjaCommand` as is, and it will take care of the correct propagation of the jobs from a factory argument, or from a buildslave, a builder, or even a build parameters.


================
Comment at: zorg/buildbot/builders/CUDATestsuiteBuilder.py:84
+        steps.RemoveDirectory(name="Remove old test-suite build directory",
+                              dir=ts_build_dir))
+
----------------
Just `RemoveDirectory` here. Please see my comment about the buildbot version above.


================
Comment at: zorg/buildbot/builders/CUDATestsuiteBuilder.py:132
+
+    ninja_build_cmd = ['ninja'] + jobs_cmd
+
----------------
You do not need this any more, do you?


================
Comment at: zorg/buildbot/builders/CUDATestsuiteBuilder.py:156
+    # If we've enabled thrust tests, build them now.
+    # WARNING: This takes a lot of time to build.
+    if (enable_thrust_tests):
----------------
If it takes a long to build and there is no TTY output during that time, you may want to set a longer timeout for this step.


https://reviews.llvm.org/D27766