[zorg] ClangLTO3StageBuilder.py - Adding a cast string to list item and some asserts

[sqlbyme]

Summary

When this function was originally authored I forgot to cast the cmake_cache_file as a list item. This caused a bug to appear on the first deployment of this script which result in a bot failing to configure a build correctly. I have corrected the cast and added a couple of asserts to ensure any future users of this script will be warned if their builder definitions are not setup correctly.

[silvas]

zorg/buildbot/builders/ClangLTOBuilder3Stage.py

79

cmake_command += ['-C', cmake_cache_file]

82

assert isinstance(extra_cmake_options, list)

Also, the assert just needs to avoid things blowing up down the road. No need for a lengthy description. "extra_cmake_options must be a list" is probably fine (or no message, since isinstance(extra_cmake_options, list) says that already).

Same applies to the other assertions.

84

I don't think it's worth it to iterate the entire list (if we want to do that you should define a helper and use it consistently for all arguments that need checking; but I don't think it is worth it).

[sqlbyme]

I applied the comments silvas provided and also adjusted the formatting of the shell_command for the last build step.

[gkistanova]

zorg/buildbot/builders/ClangLTOBuilder3Stage.py

77

May I suggest not doing assertion here? Basically, you want to make sure that whatever comes from the arg is renderable to string on a buildslave. Someone might want to use WithProperties, for example. Unfortunately, there is no easy way to make sure the arg is renderable on a slave without getting the slave involved. So, this is one of the cases, when you trade potential troubleshooting / debugging for flexibility.

Anyway, if you feel strongly for having the assertion, you may want to handle Unicode strings as well, i.e. checking for basestring instead.

81

May I suggest not doing assertion here as well?
There are ways in Python to get a list as a result, which would not be a list per se, For example, lambdas.
Failing natively is a good choice here, I think.
https://docs.python.org/2/glossary.html#term-eafp

[sqlbyme]

zorg/buildbot/builders/ClangLTOBuilder3Stage.py

77

Ok, I wasn't certain if it was appropriate, thus my question. I'll remove the asserts and repost in a moment

[sqlbyme]

Removed unnecessary assertions.

[gkistanova]

LGTM.
Committed r 261608.

zorg/buildbot/builders/ClangLTOBuilder3Stage.py

     cmake_command = ["cmake"]
     
     if cmake_cache_file:
-        cmake_command += "-C %s" % (" ".join(cmake_cache_file))
+        cmake_command += ['-C', cmake_cache_file]
 
     if extra_cmake_options:
         cmake_command += extra_cmake_options

     )
 
     # Compare stage2 & stage3 clang
-    shell_command = ["diff", "-q", "tools/clang/stage2-bins/bin/clang-%(llvm_ver)s", "tools/clang/stage2-bins/tools/clang/stage3-bins/bin/clang-%(llvm_ver)s"]
+    shell_command = [
+        "diff",
+        "-q",
+        "tools/clang/stage2-bins/bin/clang-%(llvm_ver)s",
+        "tools/clang/stage2-bins/tools/clang/stage3-bins/bin/clang-%(llvm_ver)s"
+    ]
     f.addStep(
         ShellCommand(
             name="compare",