[PATCH] [zorg] Support Windows/Linux Clang CMake builds with unified builder.

[Galina]

Thanks, Rick!

I like how the ordered array of stages is coming together.
Skipping optional parameters (those with the default values) would help to keep the definition short and clear and still flexible.

Please also see my comments in-line.

Thanks

Galina


REPOSITORY
  rL LLVM

================
Comment at: zorg/buildbot/builders/ClangBuilder.py:68
@@ +67,3 @@
+
+SCMs = {'SVN':scmSvn,'GIT':scmGit}
+
----------------
May I suggest using scmSvn and scmGit functions references directly in the arrays in llvmSourceTree?
You do not need another mapping between a string and a function, really.

And maybe better names? How about SVN and Git (no need to import SVN and Git from buildbot.steps.source in this case, just use fully qualified names in the local SVN and Git implementation)? :)

The source definition would look like

    'svn-llvm': [SVN,"%s",'http://llvm.org/svn/llvm-project/llvm/','trunk','update'],


================
Comment at: zorg/buildbot/builders/ClangBuilder.py:523
@@ +522,3 @@
+            'useTwoStage': True,
+            'test': 'ignoreFail',
+            },
----------------
How about using just one parameter 'usePreviousStageResults' (or some better name) instead of useTwoStage and useBuiltClang?

Besides, since we support multiple stages, useTwoStage sounds a bit misleading.

This is a cosmetic issue, not a show stopper.


================
Comment at: zorg/buildbot/builders/ClangBuilder.py:540
@@ +539,3 @@
+            },
+            ],
+            update=[] # Update each stage [{},...]
----------------
Maybe reduce the list of parameters for those that match exactly the defaults you are applying when you use the dictionary below? Like skipping 'extra_cmake_args' and such.

This is cosmetic, though.

================
Comment at: zorg/buildbot/builders/ClangBuilder.py:541
@@ +540,3 @@
+            ],
+            update=[] # Update each stage [{},...]
+            ):
----------------
I feel like I'm missing a use case for the update parameter. Could you elaborate, please?


================
Comment at: zorg/buildbot/builders/ClangBuilder.py:563
@@ +562,3 @@
+            f = getLLVMSource(f,stage_source,stage['source'])
+            haveSource = True
+
----------------
You do not really use haveSource, do you?
What was the original idea?

================
Comment at: zorg/buildbot/builders/ClangBuilder.py:652
@@ +651,3 @@
+
+def XorY(x,y):
+    return x or y
----------------
It looks like this function is not used.

http://reviews.llvm.org/D6866

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/