Page MenuHomePhabricator

[zorg] Rev2: Fix get slave environment in LLDB Windows builder.
ClosedPublic

Authored by rfoos on Jan 23 2015, 8:29 AM.

Details

Summary

Fixes for lldb-x86-windows-msvc

Change vcvars_command to use '\\' to join paths.
Change vs_common default to r"""...""" string.

Fixes for lldb-x86-win7-msvc

Change ninja_cmd to unique build, install, and check commands.
Use a list append to attach job control property variable.
Add ignoreInstallFail
Add warnings and halt to ignore fail actions.

Diff Detail

Repository
rL LLVM

Event Timeline

rfoos updated this revision to Diff 18680.Jan 23 2015, 8:29 AM
rfoos retitled this revision from to [zorg] Rev2: Fix get slave environment in LLDB Windows builder..
rfoos updated this object.
rfoos edited the test plan for this revision. (Show Details)
rfoos added reviewers: gkistanova, zturner.
rfoos set the repository for this revision to rL LLVM.
rfoos added a project: Restricted Project.
rfoos added a subscriber: Unknown Object (MLST).
gkistanova added inline comments.Jan 26 2015, 2:15 PM
zorg/buildbot/builders/LLDBBuilder.py
42

os.path.join runs in the context of the buildmaster, which is MacOS for the LLVM buildbot.
Having the directory separator defined explicitly for Windows is the right fix for this case.

56

Maybe usage of ' vs. " would make it more readable?

139

Setting all 3 mutually exclusive flags (haltOnFailure, warnOnFailure, and flunkOnFailure) to the same value doesn't look right.

You want only one of these be true, right?

zturner edited edge metadata.Jan 30 2015, 8:53 AM

This looks good to me after Galina's suggestion about the 3 mutually exclusive values. Is this ready to go otherwise?

rfoos added a comment.Jan 30 2015, 5:43 PM

Yes, the three comments are good to implement.

The _jobs variables will be renamed jobcontrol, default for ninja, -j%(jobs)s.
The caller, knowing the buildslave properties, can add loadaverage, or the different syntax for msbuild (/maxcpucount:%(jobs)s).

in the original self hosted builder, the first step of stage2 was an install in stage1. I did this by specifying an install step, so the caller had to remember to stitch this together in both stages.

By adding an iterator on the stage list, it becomes possible to look in the next stage to see if you need to do the install step in the first stage. There are probably other use cases for lookahead, or look previous stages.

The before and after stage operations will be done by functions so the command itself can just return a flag in an if statement.

Then I think its ready.

rfoos planned changes to this revision.Feb 6 2015, 5:20 PM

Testing now.

zorg/buildbot/builders/LLDBBuilder.py
56

changing vs_common to vs.

In the common builder, this will become an entire command executed on the slave to set up whatever environment you want.

83

Since this is windows, no loadaverage, I'll leave jobs alone here.

139

Yes, dropping to flunkOnFailure is correct.

When I had haltOnFailure in as well, then warnOnFailure triggered, so then I added all three.

The right answer is flunk on failure only.

rfoos updated this revision to Diff 19524.Feb 6 2015, 5:50 PM
rfoos edited edge metadata.

vs_common -> vs
ignoreFail: remove halt and warn OnFailure.
default is no install or test steps.

This should detect compile problems, and be a platform for finishing the lit tests for windows.

gkistanova accepted this revision.Feb 9 2015, 10:37 AM
gkistanova edited edge metadata.

Looks good to me.

I'd prefer using NinjaCommand instead of building a shell command for ninja, but this is not critical for this patch.

This revision is now accepted and ready to land.Feb 9 2015, 10:37 AM
rfoos added a comment.Feb 9 2015, 11:24 AM

Yes, NinjaCommand looks interesting.

It looks like a wrapper for WarningCountingShell. If it could handle make and msbuild, it would cover all current cases.

I'm not quite sure how an override to env gets passed to NinjaCommand.

Thanks for looking at this! Time to make some builders green.

rfoos closed this revision.Mar 4 2015, 9:54 AM