Page MenuHomePhabricator

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

Authored by rengolin on Jan 7 2015, 8:38 AM.

Details

Summary

This adds the basic ability to build Clang with cmake on both Windows (MSVC), and Linux. The default options work as the current getClangCMakeBuilder providing Linux builds.

CMake generators can be selected. Ninja, MSVC, and Unix Makefiles generators have been tested.

Windows and Linux builds have also been tested. Example builders for Hexagon show options for both Windows and Linux.

This might be an interim step. By adding a dictionary of llvm projects and SVN/Git parameters for fetching an LLVM based source tree, and general this becomes a generic LLVM CMake builder with the user passing a list of projects desired in the build.

Diff Detail

Repository
rL LLVM

Event Timeline

rfoos updated this revision to Diff 17860.Jan 7 2015, 8:38 AM
rfoos retitled this revision from to Support Windows/Linux Clang CMake builds with unified builder..
rfoos updated this object.
rfoos edited the test plan for this revision. (Show Details)
rfoos added a project: Restricted Project.
rfoos set the repository for this revision to rL LLVM.
rfoos added a subscriber: Unknown Object (MLST).
rfoos retitled this revision from Support Windows/Linux Clang CMake builds with unified builder. to [zorg] Support Windows/Linux Clang CMake builds with unified builder..Jan 7 2015, 9:22 AM
rfoos planned changes to this revision.Jan 8 2015, 8:20 AM
rfoos added inline comments.
zorg/buildbot/builders/ClangBuilder.py
28

This should be a unified getLLVMSource using a dictionary for SVN arguments/tree locations. The use passing a list of projects for update.

59

This particular one is used by an lldb experimental builder and should be removed.

508

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).

563

This section needs to support the jobs property.

rengolin edited edge metadata.Jan 9 2015, 9:18 AM

Overall, looks good. I think we're getting there. I'll wait until you finish your refactoring based on your comments and look at my comments.

To actually put this in production, I'd be a lot more confident if we could make it run on a staging environment before changing it. So, whenever you're happy with it, I'll apply the patch on my local buildmaster and set it to run with my boards. Other interested parties can do the same, and when everyone is happy, we push and apply.

cheers,
--renato

zorg/buildbot/builders/ClangBuilder.py
508

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. :)

539

getClangSource already checks out clang-tools-extra

595

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.

rfoos added a comment.Jan 9 2015, 10:13 AM

I completely agree with several other testers.

I have a Windows/Linux staging environment set up on my servers, but
this will catch all of the things I might miss.

--rick

gkistanova edited edge metadata.Jan 9 2015, 11:52 AM

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

zorg/buildbot/builders/ClangBuilder.py
41

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.

490

Maybe rename 'Visual' to 'VisualStudio' or 'VC'?

508

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.

551

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.

562

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?

565

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.

595

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?

659

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?

rfoos added inline comments.Jan 15 2015, 3:11 PM
zorg/buildbot/builders/ClangBuilder.py
41

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'])
490

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'.

508

I'll try the ordered list. It's a balance between easy for the caller, or too confusing to use.

539

Fixed.

551

Fixed.

565

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.

595

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.

659

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.

rfoos added inline comments.Jan 19 2015, 12:12 PM
zorg/buildbot/builders/ClangBuilder.py
508

Please Review:
Here's the list I'm implementing per comments.
Since it is a list of dictionaries, the caller will have to specify everything.
Selections, like useTwoStage, or testStage1 are specified by leaving or adding to the build list.

The key names in the dictionary describe actions for one or more cmake build, install, check steps.

def getCMakeBuildFactory(
        build = [
        {'name': "stage 1",
        'source': ['svn-llvm','svn-clang'],
        'source_dir': 'llvm',
        'config': "Release",
        'slave_envCmd': None,
        'cmakeGenerator': 'Ninja',
        'cmakeProjectfile': 'build.ninja',
        'extra_cmake_args': [],
        'build_cmd': ['ninja'],
        'build_dir': 'stage1',
        'install_cmd': ['ninja', 'install'],
        'install_dir': None,
        'check_cmd': ['ninja', 'check-all'],
        'check_dir': None,
        },
        {'name': "stage 2",
        'config': "Release",
        'slave_envCmd': None,
        'cmakeGenerator': 'Ninja',
        'cmakeProjectfile': 'build.ninja',
        'extra_cmake_args': [],
        'build_cmd': ['ninja'],
        'build_dir': None,
        'install_cmd': ['ninja', 'install'],
        'install_dir': None,
        'check_cmd': ['ninja', 'check-all'],
        'check_dir': None,
        },
        ]
        ):
return
rfoos updated this revision to Diff 18519.Jan 21 2015, 8:34 AM
rfoos edited edge metadata.

SVN/Git checkout using list of projects [svn-llvm, svn-clang]

Build stages driven by caller defined list of dictionaries
[{stage1},{stage2},...]

Allow user to update individual values in stage dictionaries update=[{sources=[svn-llvn,svn-clang,svn-compiler-rt]}].

The caller does not need to specify the entire set of stage dictionaries to change a few things for thier needs.

Default stages set to do a self hosted linux clang build.

Moved code into new function name. Tested on Windows/Linux, MSVC, Makefiles, Ninja.

Thanks, Rick!

I like how the ordered array of stages is coming together.
Skipping optional parameters (those with the default values) would help to keep the definition short and clear and still flexible.

Please also see my comments in-line.

Thanks

Galina

zorg/buildbot/builders/ClangBuilder.py
68

May I suggest using scmSvn and scmGit functions references directly in the arrays in llvmSourceTree?
You do not need another mapping between a string and a function, really.

And maybe better names? How about SVN and Git (no need to import SVN and Git from buildbot.steps.source in this case, just use fully qualified names in the local SVN and Git implementation)? :)

The source definition would look like

'svn-llvm': [SVN,"%s",'http://llvm.org/svn/llvm-project/llvm/','trunk','update'],
523

How about using just one parameter 'usePreviousStageResults' (or some better name) instead of useTwoStage and useBuiltClang?

Besides, since we support multiple stages, useTwoStage sounds a bit misleading.

This is a cosmetic issue, not a show stopper.

540

Maybe reduce the list of parameters for those that match exactly the defaults you are applying when you use the dictionary below? Like skipping 'extra_cmake_args' and such.

This is cosmetic, though.

541

I feel like I'm missing a use case for the update parameter. Could you elaborate, please?

563

You do not really use haveSource, do you?
What was the original idea?

652

It looks like this function is not used.

rfoos added inline comments.Jan 21 2015, 4:23 PM
zorg/buildbot/builders/ClangBuilder.py
68

Excellent idea. I need to get my head around it a little better, but I'll do it.

523

Using one previous stage results is much better.

Eliminating defaults from the argument list is a great idea. I left the argument list as a working example as there are quite a few things that need to be entered/remembered.

541

Rather than tackling re-defining the entire list, I found MSVC followed by a clang built compiler easier to do with an update to the original list.

Probably will need something to document all the things you can do with this.

ClangBuilder.getCMakeBuildFactory(
                update=[
                {
                'cmakeGenerator': "Visual Studio 12",
                'cmakeProjectfile': "ALL_BUILD.vcxproj",
                # MSBuild command with user arguments.
                'build_cmd': ["MSBuild","/p:Configuration=RelWithDebInfo",
                "/maxcpucount", "ALL_BUILD.vcxproj"],
                'install_cmd': ["MSBuild","/p:Configuration=RelWithDebInfo",
                "/maxcpucount", "INSTALL.vcxproj"],
                'check_cmd': ["MSBuild","/p:Configuration=RelWithDebInfo",
                "/maxcpucount", "check-clang.vcxproj"],
                'check_dir': 'stage1/tools/clang/test',
                'slave_envCmd': r""""%VS120COMNTOOLS%\..\..\VC\vcvarsall.bat" %PROCESSOR_ARCHITECTURE% & set""",
                'extra_cmake_args': [
                 '-DLLVM_BUILD_RUNTIME:BOOL=OFF',
                 '-DLIBCLANG_BUILD_STATIC=ON',
                 '-DLLVM_ENABLE_PIC:BOOL=ON',
                 '-DLLVM_MINIMAL_INSTALL:BOOL=ON',
                 '-DLLVM_ENABLE_ASSERTIONS:BOOL=ON',
                 '-DLLVM_ENABLE_STATS:BOOL=ON',
                 '-DLLVM_LIT_ARGS:STRING=--verbose --no-progress-bar --param build_config=Win32',
                 '-DCMAKE_BUILD_TYPE=RelWithDebInfo',
                 '-DLLVM_APPEND_VC_REV:BOOL=ON',
                 "-DLLVM_TARGETS_TO_BUILD='Hexagon'"
                "-DTARGET_TRIPLE:STRING=hexagon-unknown-elf",
                "-DLLVM_DEFAULT_TARGET_TRIPLE:STRING=hexagon-unknown-elf",
                "-DLLVM_TARGET_ARCH:STRING=hexagon-unknown-elf"
                 ]})

I haven't figured out how to delete a key with update. I may need to go back around and check for both keys and values so that everything can be updated.

Using defaults more intelligently will this by reducing the number of entries.

563

If you don't specify source and try to build something, what should I do and how should I do it.

I haven't figured that out yet. Bombing out of a checkconfig would be the best way, but I don't know of a way to do it and pass back why something failed yet.

652

Yes, I used it in the routine below (which will be replaced by this routine). I'll remove it.

gkistanova added inline comments.Jan 22 2015, 1:44 PM
zorg/buildbot/builders/ClangBuilder.py
68

Something like this:

def SVN(f, llvmsrcdir, project=''):
    (workdir, baseURL, branch, mode) = llvmSourceTree[project]
    f = f.addStep(
        buildbot.steps.source.SVN(
            name=project,
            mode=mode,
            baseURL=baseURL,
            defaultBranch=branch,
            workdir=workdir % llvmsrcdir))
    return f

...

llvmSourceTree = {
    'svn-llvm': [SVN,"%s",'http://llvm.org/svn/llvm-project/llvm/','trunk','update'],
    ...

And since we use the conflicting names, the import of buildbot.steps.source shouldn't make them available by their short names.

541

I still don't think I see clearly how update will be used with the stages array. I mean, let's say you have 2 stages, stage 1 and stage 2 as described by the build array.

Are you thinking of generating a stage 2 definition by applying an update item for the stage 2 to the build item for the stage 1?

Or you want to combine a final definition for the stage 2 by applying the stage 2 build item with stage 2 update item?

If this the second, why not making the stage 2 build definition correctly in the first place? One can provide a generator instead of an array and describe steps one by one using some more complex logic.

Anyway, it i not like you have to convince me to get this patch committed. I guess I'll see it later from the real usage. Just wanted to make sure it still makes sense after the transition to the stages description array.

563

Frankly, I would let it be as described in the stages array, without trying to sanitize. I.e. if there is no source code specified at all, the first clean build would fail, providing enough information for troubleshooting.

If we want to support a case when every stage could have its own source, then we should work out the default case, when the next stage uses the source from the previous one unless otherwise specified explicitly. The most usual setup when only the first stage specify the source and all sequential stages share it would be supported naturally.

The one important thing here is to make sure every time the source gets checked out, it would checkout the same revision a particular build is for. Meaning that only buildbot SVN/Git/whatever steps are used and not some custom shell commands.

Anyway, if you feel strongly toward sanitizing, thrown exceptions will be exposed at the checkconfig.

rfoos planned changes to this revision.Jan 22 2015, 2:22 PM

I have the updates done, and now testing.

Properties needed some re-work, and job control is added.

zorg/buildbot/builders/ClangBuilder.py
68

Yes, and it works. Thanks.

def getLLVMSource(f,llvmsrcdir='.',project_list=[]):
  for project in project_list:
      if llvmSourceTree[project]:
          llvmSourceTree[project][0](f,llvmsrcdir,project)
return f
541

The default builder will do a self-hosted ninja build. With defaults removed it will look something like this.

def getCMakeBuildFactory(
        build=[
        {'name': "stage 1",
        'build_dir': 'stage1.build',
        'install_dir': 'stage1.install',
        'test': 'ignoreFail',
        },
        {'name': "stage 2",
        'useBuiltClang': True,
        'built_dir': 'stage1.install',
        'build_dir': 'stage2.build',
        'install_dir': 'stage2.install',
        'test': 'ignoreFail',
        },
        ],
        update=[] # Update each stage [{},...]
        ):

Once the caller touches build, all of this goes away and all of the build stages need to be specified.

Update allows a few variables to be modified without breaking the self-hosted build. It's a convenience that could be removed once we specify a bunch of builders.

Update is completely optional.

ATM I think it's useful, but I could change my mind when everyone tries to use this.

563

I tried several forms of assert. I give up.

I like your way better.

rfoos updated this revision to Diff 18685.Jan 23 2015, 11:23 AM

Incorporated all comments.
llvmGetSource direct calls from source tree dictionary.
Added user environment variables.
Fixed ignore fail
Added install for use Previous built compiler.
Windows escaping fixes to built compiler path.

gkistanova added inline comments.Jan 26 2015, 2:50 PM
zorg/buildbot/builders/ClangBuilder.py
513

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?

540

You do not really need an empty array here, do you? None would do.

611

'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
  1. always require a valid path in usePreviousStageResults value without providing any default value.

I'm in favor of # 1, but either would do.

636

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?

660

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

663

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.

rfoos planned changes to this revision.Jan 26 2015, 5:13 PM

Thanks for looking at it!

zorg/buildbot/builders/ClangBuilder.py
513

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)'.

540

yes, the comment says what I want None is better.

611

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?

636

Oops, yes this is a bug. Thanks!

660

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.

663

Yes, a bug left over from the previous code that declared install the first step of stage 2.

Hello Rick,
Just checking how things are going with this patch.
I might have a day or two to make a pass on this, if this is fine with you and you are not about to publish a new version of the change, of course.

rfoos added a comment.Mar 3 2015, 11:05 AM

Yes, I've been tied up with several things.

If I can't get something up later this afternoon, please go for it, this patch is really needed.

rengolin commandeered this revision.Jul 28 2015, 4:18 AM
rengolin edited reviewers, added: rfoos; removed: rengolin.

This revision is not several months stale and will have to be rebased and heavily modified.

I'll close this one and we can start a new one when either of us have time to look at this.

rengolin abandoned this revision.Jul 28 2015, 4:18 AM