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

[Rick Foos]

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/',
----------------
gkistanova wrote:
> 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.
> 
This keeps the old behavior. There are other special cases in the builders that might make a single project list from the caller.

    f = getLLVMSource(f,'llvm',['svn-llvm','svn-clang'])

    # Extra stuff that will be built/tested
    if checkout_clang_tools_extra:
        f = getLLVMSource(f,'llvm',['svn-clang-tools-extra'])
    if checkout_compiler_rt:
        f = getLLVMSource(f,'llvm',['svn-compiler-rt'])


================
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'}
+
----------------
gkistanova wrote:
> Maybe rename 'Visual' to 'VisualStudio' or 'VC'?
if key in cmakeGenerator:
'Visual' is a search key for strings like "Microsoft Visual Studio 12.0". Otherwise I'd be glad to change it.

I've moved cmakeProjectFile to a parameter. I think it's time to drop that code. It's a bit confusing to second guess the caller. The default will be 'build.ninja'.

================
Comment at: zorg/buildbot/builders/ClangBuilder.py:508
@@ -453,1 +507,3 @@
 
+            # Phase 2 build with built Clang.
+            cmakeGenerator2='Ninja',     
----------------
gkistanova wrote:
> 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.
> 
I'll try the ordered list. It's a balance between easy for the caller, or too confusing to use.

================
Comment at: zorg/buildbot/builders/ClangBuilder.py:539
@@ -481,3 +538,3 @@
     # Extra stuff that will be built/tested
     if checkout_clang_tools_extra:
         f.addStep(SVN(name='svn-clang-tools-extra',
----------------
rengolin wrote:
> getClangSource already checks out clang-tools-extra
Fixed.

================
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(
----------------
gkistanova wrote:
> 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.
> 
Fixed.

================
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)])
----------------
gkistanova wrote:
> 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.
> 
All the buildslaves define jobs, but only one defines loadaverage. To make it easier to change, I have to do something if the loadaverage property is not defined. Not sure I have the right answer in this code.

================
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',
----------------
gkistanova wrote:
> 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?
> 
Yes, the Web UI to force a build was what I was thinking of. The property would  only apply to a single build.

Fixed. I like cleanBuildRequested much better.

================
Comment at: zorg/buildbot/builders/ClangBuilder.py:659
@@ +658,3 @@
+                            warnOnFailure=True,
+                            doStepIf=doCleanIf
+                            ))
----------------
gkistanova wrote:
> 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?
> 
The default should be as you said.

Unless we soft code what step 2 does, yes it makes sense that the user can't override this.

http://reviews.llvm.org/D6866

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