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

[Rick Foos]

Thanks for looking at it!


REPOSITORY
  rL LLVM

================
Comment at: zorg/buildbot/builders/ClangBuilder.py:513
@@ +512,3 @@
+'build_cmd': ['ninja'], Build source code command.
+build_jobs: '-j%(jobs)s -l%(loadaverage)', job control if supported.
+'build_dir': 'stage1.build',
----------------
gkistanova wrote:
> It looks like you have changed your mind and have a separate flag for the loadaverage. Could you update the description to address this, please?
> 
Not exactly. My intent was that jobs and loadaverage are not arguments, but rather completely specified by the caller. 

The default is specifying them as properties defined by the buildslave executing the command.

The second reason for xxx_jobs is the MSBuild case where only jobs can be specified in /maxcpucount:%(jobs)s.

Since most of the buildslaves have jobs define, perhaps the best default is '-j$(jobs)s -l%(jobs)'.



================
Comment at: zorg/buildbot/builders/ClangBuilder.py:540
@@ +539,3 @@
+            ],
+            update=[] # Update each stage [{},...]
+            ):
----------------
gkistanova wrote:
> You do not really need an empty array here, do you? None would do.
> 
yes, the comment says what I want None is better.

================
Comment at: zorg/buildbot/builders/ClangBuilder.py:611
@@ +610,3 @@
+        if 'usePreviousStageResults' in stage:
+            built_clang_dir = stage.get('usePreviousStageResults','stage1.install')
+            # CMake escapes these variables. Windows \ is fatal.
----------------
gkistanova wrote:
> 'stage1.install' is not always a previous stage.
> We either
> 
> 1. require the previous stage to do the install and use that as the path (in this case usePreviousStageResults is just a flag), or
> 
> 2. always require a valid path in usePreviousStageResults value without providing any default value.
> 
> I'm in favor of # 1, but either would do.
> 
Allowing the caller to define stages introduced extra information that two consecutive stages needed to be aware of.

If I 
1) make the default install step True;
2) create an iterator for stages;
3) set the default install stage to stage<i>.install (as it is now)

The usePreviousStageResults with no value would know to look for stage<i-1>.install.

This would support #1, and would be much like the current behavior w/o requiring communication between stages.

A value in usePreviousStageResults would be a filepath to an install directory, an option supporting #2.

It does too many installs, but it avoids having stages look ahead in the stage loop.

Sound good? 

================
Comment at: zorg/buildbot/builders/ClangBuilder.py:636
@@ +635,3 @@
+        if 'build_loadaverage' in stage:
+            build_cmd.append(WithProperties(stage['build_loadaverage']))
+        f.addStep(WarningCountingShellCommand(name=stageName+': build',
----------------
gkistanova wrote:
> This looks inconsistent with the usage of -l option in build_jobs, etc. as described in the doc strings.
> Did you change your mind in favor of having a separate 'build_loadaverage' parameter and forgot to change the description above?
> 
Oops, yes this is a bug. Thanks!

================
Comment at: zorg/buildbot/builders/ClangBuilder.py:660
@@ +659,3 @@
+                        flunkOnFailure=ignoreTestFail,
+                        env=env))
+
----------------
gkistanova wrote:
> Setting all 3 flags (haltOnFailure, warnOnFailure, and flunkOnFailure) to the same value doesn't look right.
> 
By default, all are true.
If I need to ignore failures, halt and flunk must be false.

Doing that I had a builder take the warn on failure, and instead of green I had yellow jobs in the summary.

Yes, it seems like overkill. 

maybe I can say only flunk on failure, and not specify the others and the build step will do the right thing. I'll try that and see.

================
Comment at: zorg/buildbot/builders/ClangBuilder.py:663
@@ +662,3 @@
+        ############# STAGE 2
+        if 'usePreviousStageResults' or 'install' in stage:
+            install_cmd = stage.get('install_cmd',['ninja', 'install'])
----------------
gkistanova wrote:
> It doesn't seem the usePreviousStageResults for this stage should require installation step.
> I can see if you would check the usePreviousStageResults of the next stage for this purpose, though.
> 
Yes, a bug left over from the previous code that declared install the first step of stage 2.

http://reviews.llvm.org/D6866

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