Page MenuHomePhabricator

[zorg] Add options allowing MSVC builds with Clang Cmake Build Factory.
AbandonedPublic

Authored by rfoos on Oct 2 2014, 2:35 PM.

Details

Summary

This version updates the clang cmake factory so that it can be used for both Linux and Windows MSVC builders.

It successfully builds and tests with Visual Studio 2013.

An original update to the MSVC builder was dropped after discussion with Renato for this method. The is better than the first patch, and this is close to a common cmake factory for other projects besides clang.

The only addition to allow this to expand to other projects is the ability to check out different source trees.

This also enables two new builders that use this script.

Diff Detail

Event Timeline

rfoos updated this revision to Diff 14347.Oct 2 2014, 2:35 PM
rfoos retitled this revision from to [zorg] Add options allowing MSVC builds with Clang Cmake Build Factory..
rfoos updated this object.
rfoos edited the test plan for this revision. (Show Details)
rfoos added reviewers: gkistanova, rengolin.
rfoos added a subscriber: Restricted Project.
rfoos edited subscribers, added: Unknown Object (MLST); removed: Restricted Project.
rengolin edited edge metadata.Oct 2 2014, 3:22 PM

Rick,

Can you do a diff -U999, so we can see the whole file? It makes reviewing a lot easier.

Thanks!
--renato

rfoos added a comment.Oct 2 2014, 3:36 PM

Yes, I'll attach them, and hopefully not spam everyone.

gkistanova edited edge metadata.Oct 3 2014, 2:10 PM

Thanks for working on this!

What is the use case for merging env on a slave?

zorg/buildbot/builders/ClangBuilder.py
537

Could you consider defining and using a dict instead? Seems more natural for Python...

548

May I suggest defining the "clean" arg as a property and then do a unified check if we need to clean.
The build property will overwrite the builder or a slave one.

At least this is what I'm going to do as a common approach for all builders.

Hi Rick,

Can you attach the U99 diffs to the phabricator review? Just go to the top, "Create diff", upload your new files, then chose the current review and it'll update the revision.

About testing, have you tested the Linux build with your changes? It'd be good to get a confirmation that it's not breaking or we'll have a hard time reverting and getting the existing bots back to green again.

I'd like to see this change merged, so that I can add the test-suite run and get a new bot with compiler-rt + test-suite, but only after we're sure that this is working well on both Windows and Linux, and we address all of Galina's concerns.

cheers,
--renato

Apologies, I've been out for a bit. It looks like the slave environment capture has been merged. I'll address the rest of the issues today.

I have a windows clang and 64 bit lldb builder I would like to enable.

zturner added inline comments.
buildbot/osuosl/master/config/builders.py
405–413

Any reason this can't use ninja? It will build faster than MSBuild while still using MSVC for the actual compilation and link.

rfoos added a comment.Dec 17 2014, 1:29 PM

I'm using MSBuild on MSVC Windows to be conservative.

My current windows buildslave is very slow, and ninja would not solve any problems.

The changes I'm proposing allow the same builder to be used multiple ways, ninja, make, MSBuild.

So MSBuild is a great proof of concept to the generic cmake builder.

rfoos added inline comments.Dec 17 2014, 4:29 PM
buildbot/osuosl/master/config/builders.py
405–413

I think the msvc builder and this change are obsolete.

The unified cmake builder should allow ninja, make, or MSBuild with the builder definition setting the parameters for each.

zorg/buildbot/builders/ClangBuilder.py
537

Yes, a dictionary would be more extensible.

cmakeGenToProjectfile={'Makefiles':'Makefile','Ninja':'build.ninja','Visual':'ALL_BUILD.vcxproj'}

cmakeProjectfile = 'Missing'
for key,val in cmakeGenToProjectfile.items():
   if key in cmakeGenerator:
       cmakeProjectfile = val
548

It looked like the Force Build form 'clean before building' checkbox was setting a property named force_build_clean.

If force_build_clean is a standard buildbot property, I thought we might want to incorporate it.

doStepIf=Property('clean')
rfoos added inline comments.Dec 18 2014, 4:39 PM
zorg/buildbot/builders/ClangBuilder.py
548

force_build_clean is NOT a standard property.

I found it on a different hexbot and it turned out to be a property set up in ForceScheduler,

Using clean as a property will be fine.

Below is how a checkbox can be made to set a clean property clean on the ForceScheduler form if you want.

c['schedulers'].append(ForceScheduler(
   ...
    # A completely customized property list.  The name of the
    # property is the name of the parameter
    properties=[
            BooleanParameter(name="force_build_clean",
                                  label="force a make clean",
                                  default=False),
    ]
...
)

Any progress in this? If not, can we close this review?

rfoos abandoned this revision.Mar 10 2015, 9:25 AM

Replaced by D6866