This is an archive of the discontinued LLVM Phabricator instance.

Add support of multiple test compilers and architecture for LLDB cmake builder
ClosedPublic

Authored by chying on Mar 13 2015, 3:45 PM.

Diff Detail

Event Timeline

chying updated this revision to Diff 21968.Mar 13 2015, 3:45 PM
chying retitled this revision from to Add support of multiple test compilers and architecture for LLDB cmake builder.
chying updated this object.
chying edited the test plan for this revision. (Show Details)
chying added reviewers: ovyalov, chaoren, sivachandra.
chying added a subscriber: Unknown Object (MLST).Mar 13 2015, 3:47 PM
sivachandra requested changes to this revision.Mar 16 2015, 11:31 AM
sivachandra edited edge metadata.
sivachandra added inline comments.
buildbot/osuosl/master/config/builders.py
722

Can we have two archs on the same build slave??

zorg/buildbot/builders/LLDBBuilder.py
257

This should be initialized with the default system architecture.

259

You should probably check for presence of clang, gcc or cc in that order and default a test compiler to that.

314

You should probably have a step which cleans up the source tree and the build tree after each test step. Side effects from a previous test run could potentially affect a consequent test run.

This revision now requires changes to proceed.Mar 16 2015, 11:31 AM

+r vharron just so that he is aware that multiple compilers get tested on a single build slave. He probably already knows, but just in case!

chying added inline comments.Mar 16 2015, 12:36 PM
buildbot/osuosl/master/config/builders.py
722

Yes, it's one of the test branches to run i386 build on x86_64 machine.
The test result of i386 build running on x86_64 vs. i386 build running on 32 bit machine might be different, so next step would be to run i386 build on 32 bit machine.

zorg/buildbot/builders/LLDBBuilder.py
257

The thought was that if no test arch or compilers is specified, then lldb-test will be skipped.

259

Same as above. If no test_compilers were specified, lldb-test will be skipped

314

Since build/test is running in a separate build folder, and I checked that there's no files added or changed to source tree after running test.
But as you suggested, I will create separate output folder for each test so there's no possible interference.

sivachandra added inline comments.Mar 16 2015, 2:09 PM
buildbot/osuosl/master/config/builders.py
722

OK. Since getLLDBUbuntuCMakeBuildFactory is a generic utility, you should check whether the architectures listed are compatible in getLLDBUbuntuCMakeBuildFactory. For example, using x86_64 lldb to debug x86_64 binaries on i386 is fine. But, using i386 LLDB is probably not OK for debugging x86_64 binaries. You could for now keep something very simple like and stick to x86_64 and i386 like this:

if platform.machine() == 'x86_64':
    compatible_archs = ['x86_64', 'i386']
    assert(all([x in compatible_archs for x in test_archs]))
zorg/buildbot/builders/LLDBBuilder.py
247

Plural test_archs is probably more indicative of the fact that a sequence is expected.

257

In which case, this should be noted in a comment near the function header. Also, instead of making test_arch={}, the intent is more obvious if you have something like this after you add the compile step:

if not test_arch:
    return

Note: Even if you end up keeping code as is, initializing to an empty list [] is more meaningful than an empty dict {}.

259

Same as my comment with test_arch.

314

OK about the source tree. How about the build tree? Does running tests create any side effects in the build tree apart from lldb-test-traces?

chying updated this revision to Diff 22068.Mar 16 2015, 7:52 PM
chying edited edge metadata.
chying updated this object.

Clean svn source tree after running lldb-test
Put test trace logs to separate folder for each compiler/arch

buildbot/osuosl/master/config/builders.py
722

This code will run on buildbot master machine, so it will return the machine architecture of master host. And assert will also fail on master side if any unsupported arch is specified.
The only way to run code on slave side is by factory.addStep(...).
We need to create some script on the fly and invoke script on slave machine to achieve this.
I suggest to keep the code as is for now, and come back to this when we have more time.

zorg/buildbot/builders/LLDBBuilder.py
257

yes, i changed to empty list in the next revision.

314

I take back the statement that "there's no files added or changed to source tree after running test".
Actually there're *.pyc and some output files left in the source tree.
I did not found these cause they're ignored by default.
Added step to clean these files in the next revision.

And yes, lldb-test-traces is the only side effect happened in build tree.

sivachandra accepted this revision.Mar 17 2015, 3:52 AM
sivachandra edited edge metadata.

Please address comments before committing.

buildbot/osuosl/master/config/builders.py
722

OK. Sorry, I missed the fact that this runs on the master. If you do not have a simple solution for now, put a TODO about this just so that readers are aware of the problem.

zorg/buildbot/builders/LLDBBuilder.py
241

s/bulids/builds

242

s/sequnce/sequence
s/doset/dosep

243

Just so that it doesn't seem I missed, you should ideally put pydoc style comments. However, it does not seem to be the style in this file. Hence, I leave it up to you.

243

I think a better comment would be:

If test_archs or test_compilers are None or empty, test steps will not be added to the build factory.

259

Please modify lines 256 to 259 to this:

if (not test_archs) or (not test_compilers):
    return f

and move these two lines to after line 305.

This revision is now accepted and ready to land.Mar 17 2015, 3:52 AM
chying updated this revision to Diff 22122.Mar 17 2015, 1:48 PM
chying edited edge metadata.

Address several nits based on feedback.

Add comments for TODO task - check listed archs are compatible with host
Correct typos in comment message
Return function early if no test arch or compiler specified

This revision was automatically updated to reflect the committed changes.