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

[Galina]

Thanks for working on this, Rick!

I like the changes you made and how they are done.
Please also see my comments in-line.

Thanks

Galina


REPOSITORY
  rL LLVM

================
Comment at: zorg/buildbot/builders/ClangBuilder.py:41
@@ +40,3 @@
+                  workdir='%s/tools/clang' % llvm_srcdir))
+    f.addStep(SVN(name='svn-clang-tools-extra',
+                  mode=mode, baseURL = repourl + 'clang-tools-extra/',
----------------
There is checkout_clang_tools_extra getClangCMakeBuildFactory param which you are ignoring here. If you want to check out always, then you don't need the checkout_clang_tools param, and vice versa.

Though, if you up to introducing a list of components to check out, this issue will be automatically solved by that change.


================
Comment at: zorg/buildbot/builders/ClangBuilder.py:490
@@ +489,3 @@
+# Map Cmake Generator to Project file.
+cmakeGenToProjectfile={'Makefiles':'Makefile','Ninja':'build.ninja','Visual':'ALL_BUILD.vcxproj'}
+
----------------
Maybe rename 'Visual' to 'VisualStudio' or 'VC'?

================
Comment at: zorg/buildbot/builders/ClangBuilder.py:508
@@ -453,1 +507,3 @@
 
+            # Phase 2 build with built Clang.
+            cmakeGenerator2='Ninja',     
----------------
rengolin wrote:
> rfoos wrote:
> > This is original behavior but allows the caller to pass an entire step 2 definition.
> > 
> > The only exception is the stage1/stage2 framework for the built compiler(s).
> Maybe better as a dictionary? Like:
> 
>     build { stage : 1, { cmd = "ninja", install = cmd + " install", check = cmd + " check-all" }
>               stage : 2, { cmd = "ninja", install = cmd + " install", check = cmd + " check-all" } }
> 
> or whatever Python would do for something like that. :)
Ordered list would do better, I think.
    [ 
    { name="stage 1", source=[<list of (components and directories tuples)>], cmd = "ninja", install = cmd + " install", check = cmd + " check-all" },
    { name="stage 2", cmd = "ninja", install = cmd + " install", check = cmd + " check-all" },
    ...
    ]

As a next re-factoring pass we shall standardize the build parameters names to have the dictionary as much common for all the builders as practical.


================
Comment at: zorg/buildbot/builders/ClangBuilder.py:551
@@ +550,3 @@
+    # Set and return slave environment, if slave_envCmd defined.
+    if slave_envCmd is not None:
+        f.addStep(SetProperty(
----------------
No need in "is not None" check here.

    if slave_envCmd:

would give you exactly what you want. We do not want to run an empty command.


================
Comment at: zorg/buildbot/builders/ClangBuilder.py:562
@@ -495,3 +561,3 @@
     lit_args="'-v"
     if jobs is not None:
         jobs_cmd=["-j"+str(jobs)]
----------------
Do you want to have 0 as a valid value? If you do not, then checking it as

    if jobs:

would do.

I'm sure you have figured this by now that you are loosing the "job" property here. Could you reconcile the param with the property instead, please?

================
Comment at: zorg/buildbot/builders/ClangBuilder.py:565
@@ +564,3 @@
+        lit_args+=" -j"+str(jobs)
+    if loadaverage is not None:
+        jobs_cmd.extend(["-l"+str(loadaverage)])
----------------
The same is here. It would be great to have a way to use the "loadaverage" property similar to the "job" one. Not a show stopper at this point, though.


================
Comment at: zorg/buildbot/builders/ClangBuilder.py:595
@@ -510,16 +594,3 @@
     ############# CLEANING
-    if clean:
-        f.addStep(ShellCommand(name='clean stage 1',
-                               command=['rm','-rf',stage1_build],
-                               warnOnFailure=True,
-                               description='cleaning stage 1',
-                               descriptionDone='clean',
-                               workdir='.',
-                               env=env))
-    else:
-        f.addStep(SetProperty(name="check ninja files 1",
-                              workdir=stage1_build,
-                              command=["sh", "-c",
-                                       "test -e build.ninja && echo OK || echo Missing"],
-                              flunkOnFailure=False,
-                              property="exists_ninja_1"))
+    doCleanIf = lambda step: step.build.getProperty("clean") or clean
+    f.addStep(RemoveDirectory(name='clean stage 1',
----------------
rengolin wrote:
> I'm not sure why you set the property here, since this is normally a config option, not a run-time one. 
> 
> But this could be potentially good if we can detect failure on previous runs and automatically clean after every failure, to make sure it's not an infrastructure failure. Not for this iteration, though.
Renato: This way someone could define clean=True property in the builder Web UI and rebuild a particular incremental build cleanly. Very nice feature to have.

How about renaming "doCleanIf" to something more descriptive? Like "cleanBuildRequested" or similar?


================
Comment at: zorg/buildbot/builders/ClangBuilder.py:659
@@ +658,3 @@
+                            warnOnFailure=True,
+                            doStepIf=doCleanIf
+                            ))
----------------
I think we should always do a clean build on the stage 2. It does not make much sense to use intermediate files built by a different compiler (the compiler on the stage 1 has been changed, right?).

What do you think?

http://reviews.llvm.org/D6866

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