[Zorg][PowerPC] Move RHEL bot back to cmake clang factory and add cmake option for test-suite

[Conanap]

Summary

This patch moves the RHEL buildbot back to clang factory with the cmake option implemented.
The patch was tested locally with a test buildmaster server. The patch also makes sure lld is enabled.

[jsji]

What is the reason of moving back to clang builder? Looks like we need to add the cmake test-suite for ClangBuilder here as well. Any particular reason that we can not add it as an optional step in UnifiedTreeBuilder?

[Conanap]

In D108783#2968000, @jsji wrote:

What is the reason of moving back to clang builder? Looks like we need to add the cmake test-suite for ClangBuilder here as well. Any particular reason that we can not add it as an optional step in UnifiedTreeBuilder?

The Unified Tree builder did not have a test suite step, so it was easier to use cmake clang builder which already have a test suite step for us to take advantage of.

[jsji]

The Unified Tree builder did not have a test suite step, so it was easier to use cmake clang builder which already have a test suite step for us to take advantage of.

OK ... Thanks.
But since we have to add the cmake steps anyhow, the only difference is about source checkout, which should be easy in Unified Tree as well?
Anyway, let us see what is Galina's comments about this first.

[Conanap]

In D108783#2968036, @jsji wrote:

The Unified Tree builder did not have a test suite step, so it was easier to use cmake clang builder which already have a test suite step for us to take advantage of.

OK ... Thanks.
But since we have to add the cmake steps anyhow, the only difference is about source checkout, which should be easy in Unified Tree as well?
Anyway, let us see what is Galina's comments about this first.

I did initially look into adding this step into unified tree builder, although ran into a bit more problems with the variables and stuff. I'm sure I can figure it out if that is what we much prefer, although personally I don't see a big benefit unified tree builder may have over cmake clang builder.

[jsji]

I'm sure I can figure it out if that is what we much prefer, although personally I don't see a big benefit unified tree builder may have over cmake clang builder.

Thanks! My understanding is that UnifiedTreeBuilder is the builder that the community want to promote , especially for all future new builders.
If we extend the UnifiedTreeBuilder, it may be easier for others to add test-suites into their builders.

[amyk]

Could you also please update the patch with full context?

buildbot/osuosl/master/config/builders.py

642

Unrelated change?

[Conanap]

Changed implementation to UnifiedTreeBuilder. I kept the clang factory builder implementation too in case that could be useful to others, but can be removed if we think it's not necessary.

[jsji]

Thanks for the great job. Some more feedback regarding UnifiedTreeBuilder extension. Thanks!

zorg/buildbot/builders/UnifiedTreeBuilder.py

248 ↗

(On Diff #369785)

obj_dir -> maybe compiler_dir is better? and then we don't need compiler_path assignment anymore.

249 ↗

(On Diff #369785)

targets, checks, install_dir , stage_name are not used at all in testsuite step. Can we remove them.

260 ↗

(On Diff #369785)

test_suite_dir -> test_suite_src_dir?

271 ↗

(On Diff #369785)

What is the reason that we choose Makefiles instead of Ninja?

UnifiedTree default to use Ninja to build clang, so I think it would be great if we can align with it (so that we don't need to require both ninja and make in buildbot machines.

273 ↗

(On Diff #369785)

Do we need the full path defined above as lit = utils.Interploate.. instead of using the one in path?

276 ↗

(On Diff #369785)

It would be great if we can make the -k and -j20 as parameters instead of hardcoding here, so that users can configure for their buildbots.

If we use Ninja, I don't think we need the -j20 anymore.

280 ↗

(On Diff #369785)

Should we use LitTestCommand and pass through env and **kwargs ?

283 ↗

(On Diff #369785)

Maybe add a test-suite-wordir = utils.Interpolate(...) above and use them here in all steps?

377 ↗

(On Diff #369785)

nit: ident is off for f?

[jsji]

zorg/buildbot/builders/UnifiedTreeBuilder.py

267 ↗

(On Diff #369785)

Should we use CmakeCommand instead of ShellCommand here?

275 ↗

(On Diff #369785)

If we use Ninja, we should be able to use NinjaCommand instead of ShellCommand here too.

[Conanap]

Improved implementation

[Conanap]

Thanks for the suggestions @jsji , I think this should address all of your comments.

zorg/buildbot/builders/UnifiedTreeBuilder.py

280 ↗

(On Diff #369785)

For this comment, I found that it seems to work without env and kwargs, so I'm wondering if it's preferred for us to not pass it if we don't need it or if we should pass it anyways.

[jsji]

zorg/buildbot/builders/UnifiedTreeBuilder.py

280 ↗

(On Diff #369785)

should pass it anyways so that others may try to pass additional env/args to control the tests in their buildbots.

[NeHuang]

zorg/buildbot/builders/ClangBuilder.py

472 ↗

(On Diff #370488)

nit: comment indentation

549 ↗

(On Diff #370488)

nit: indentation. same issue from line 549 - 584

[jsji]

LGTM with some nits. Thanks for working on this!

buildbot/osuosl/master/config/builders.py

671

Just to double confirm that you have tested 96 in our buildbot? Wanna to make sure we don't set it too large .

zorg/buildbot/builders/UnifiedTreeBuilder.py

246 ↗

(On Diff #370488)

llvm_srcdir is not used , can we remove it.

257 ↗

(On Diff #370488)

Add

test_suite_workdir='test/sandbox/test-suite',

267 ↗

(On Diff #370488)

workdir= `test_suite_workdir`.

276 ↗

(On Diff #370488)

workdir= `test_suite_workdir`.

281 ↗

(On Diff #370488)

workdir= `test_suite_workdir`.

282 ↗

(On Diff #370488)

-j20?
Should this flag be derived from the LLVM_LIT_ARGS that is passed down in extra_configure_args from clang-ppc64le-rhel builders.

But yeah, this is small enhancement, can be done in a follow up patch. Thanks.

[Conanap]

buildbot/osuosl/master/config/builders.py

671

yup! tested on our buildbot machine

zorg/buildbot/builders/UnifiedTreeBuilder.py

282 ↗

(On Diff #370488)

I agree with this, although I'm not actually quite sure how to do this. I'll look into it a bit more; if I can't figure it out today I'll leave it for a future patch.

Would also appreciate any input and concerns you might have @gkistanova. Thank you!

[Conanap]

Addressed Jinsong's comments

[amyk]

zorg/buildbot/builders/UnifiedTreeBuilder.py

273 ↗

(On Diff #371172)

Do you mean:

[Conanap]

Added a way for lit arguments to be parsed

[Conanap]

Updated descriptions

[Conanap]

zorg/buildbot/builders/UnifiedTreeBuilder.py

342 ↗

(On Diff #371333)

@jsji This is what I came up with for parsing the lit arguments. Tested works, but I'm not sure if this is optimal. I could not find a built-in way to derive these arguments.

[jsji]

zorg/buildbot/builders/UnifiedTreeBuilder.py

342 ↗

(On Diff #371333)

Looks OK for me. Thanks.

[gkistanova]

Hello everyone.

Sorry for being late to the discussion.

Unless I'm missing something, we are here after 2 stages: (1) build the compiler and tools, and then (2) build and run LLVM test-suite by using just built compiler and tools.

It doesn't seem extending either UnifiedTreeBuilder or ClangBuilder by adding extra parameters and if-else logic looks good, with all due respect to the work already done in this review.

How about introducing a new TestSuiteBuilder instead, which would use UnifiedTreeBuilder to do the stage 1 (configure,build+unit tests), and then add missing test-suite checkout, configuration, build and run step? Should be a nice clean build factory. Something similar to how this is done in zorg/buildbot/builders/ABITestsuitBuilder.py should do.

What do you think?

[jsji]

How about introducing a new TestSuiteBuilder instead, which would use UnifiedTreeBuilder to do the stage 1 (configure,build+unit tests), and then add missing test-suite checkout, configuration, build and run step? Should be a nice clean build factory. Something similar to how this is done in zorg/buildbot/builders/ABITestsuitBuilder.py should do.

What do you think?

test-suite test should be quite common and can be similar to lit test, that is why we extending ClangBuilder/UnifiedTreeBuilder to do it.

The TestsuiteBuilder proposal treats test-suite as *out of tree* repo/test instead, but yeah, that is not a big deal , OK to me.

Thanks for the feedback.

[gkistanova]

Are we talking about the same test-suite?
https://github.com/llvm/llvm-test-suite is out of tree.

[jsji]

In D108783#2992790, @gkistanova wrote:

Are we talking about the same test-suite?
https://github.com/llvm/llvm-test-suite is out of tree.

Yes, you are right. It is out of tree from llvm-project.

[Conanap]

I'm happy to reimplement this as a separate builder; would you like me to revert the commit or is it alright for me to have the other builder implemented and revert the changes in that commit as well?

[gkistanova]

Hi Albion,

I think you can keep the UnifiedTreeBuilder changes till you are ready with the new build factory and can switch the builder, then revert.
But please revert the ClangBuilder changes before some will start using that.

Another option is reverting the commit, and temporarily staging your patch for UnifiedTreeBuilder.

Sorry again for being late with the feedback and thanks for your patience.

[Conanap]

I'll go ahead and revert the commit so that no one goes and use these options. Thanks!

[Conanap]

new patch is here: https://reviews.llvm.org/D109628

[Conanap]

Removed .gitignore changes, fixed import naming, removed jobs option.

buildbot/osuosl/master/config/builders.py

 from zorg.buildbot.builders import ABITestsuitBuilder
 from zorg.buildbot.builders import ClangLTOBuilder
 from zorg.buildbot.builders import UnifiedTreeBuilder
 from zorg.buildbot.builders import AOSPBuilder
 from zorg.buildbot.builders import AnnotatedBuilder
 from zorg.buildbot.builders import LLDPerformanceTestsuite
 from zorg.buildbot.builders import FuchsiaBuilder
 from zorg.buildbot.builders import XToolchainBuilder
+from zorg.buildbot.builders import TestSuiteBuilder
 
 from zorg.buildbot.builders import HtmlDocsBuilder
 from zorg.buildbot.builders import DoxygenDocsBuilder
 
 reload(HtmlDocsBuilder)
 reload(DoxygenDocsBuilder)
 
 
'factory' : ClangBuilder.getClangCMakeBuildFactory(
                     stage1_config='Release',
                     stage2_config='Release',
                     extra_cmake_args=['-DLLVM_ENABLE_ASSERTIONS=ON', '-DBUILD_SHARED_LIBS=ON'])},
 
     {'name' : "clang-ppc64be-linux",
     'tags'  : ["clang", "ppc"],
     'workernames' : ["ppc64be-clang-test"],
     'builddir': "clang-ppc64be",
     'factory' : ClangBuilder.getClangCMakeBuildFactory(
                     clean=False,
                     checkout_lld=False,
                     useTwoStage=False,
                     stage1_config='Release',
                     extra_cmake_args=["-DLLVM_ENABLE_ASSERTIONS=ON"])},
 
     {'name' : "clang-ppc64le-linux",
     'tags'  : ["clang", "ppc", "ppc64le"],
     'workernames' : ["ppc64le-clang-test"],
     'builddir': "clang-ppc64le",
     'factory' : ClangBuilder.getClangCMakeBuildFactory(
                     clean=False,
                     checkout_lld=False,
                     useTwoStage=False,
                     stage1_config='Release',
                     extra_cmake_args=[
                         "-DLLVM_ENABLE_ASSERTIONS=ON",
                         "-DPYTHON_EXECUTABLE:FILEPATH=/usr/local/bin/python3"])},
 
     {'name' : "clang-ppc64le-rhel",
     'tags'  : ["clang", "ppc", "ppc64le"],
     'workernames' : ["ppc64le-clang-rhel-test"],
     'builddir': "clang-ppc64le-rhel",
-    'factory' : UnifiedTreeBuilder.getCmakeWithNinjaBuildFactory(
+    'factory' : TestSuiteBuilder.getTestSuiteBuildFactory(
                     depends_on_projects=["llvm", "clang", "clang-tools-extra", "lld", "compiler-rt"],
                     checks=["check"],
                     enable_runtimes="auto",
                     extra_configure_args=[
                         "-DLLVM_ENABLE_ASSERTIONS=On", "-DCMAKE_C_COMPILER=clang",
                         "-DCMAKE_CXX_COMPILER=clang++",
                         "-DCLANG_DEFAULT_LINKER=lld",
                         "-DCMAKE_C_COMPILER_EXTERNAL_TOOLCHAIN:PATH=/opt/rh/devtoolset-7/root/usr",
                         "-DCMAKE_CXX_COMPILER_EXTERNAL_TOOLCHAIN:PATH=/opt/rh/devtoolset-7/root/usr",
                         "-DLLVM_BINUTILS_INCDIR=/usr/include", "-DBUILD_SHARED_LIBS=ON", "-DLLVM_ENABLE_WERROR=ON",
                         "-DCMAKE_BUILD_TYPE=Release",
                         "-DLLVM_LIT_ARGS=-vj 20"])},
 

zorg/buildbot/builders/TestSuiteBuilder.py

+from zorg.buildbot.builders.UnifiedTreeBuilder import getCmakeWithNinjaBuildFactory
+
+from buildbot.plugins import steps, util
+
+from zorg.buildbot.commands.CmakeCommand import CmakeCommand
+from zorg.buildbot.commands.NinjaCommand import NinjaCommand
+from zorg.buildbot.commands.LitTestCommand import LitTestCommand
+
+# This builder is uses UnifiedTreeBuilders and adds running
+# llvm-test-suite with cmake and ninja step.
+
+def addTestSuiteStep(
+            f,
+            compiler_dir = None,
+            env = None,
+            lit_args = [],
+            **kwargs):
+
+    cc = util.Interpolate('-DCMAKE_C_COMPILER=' + '%(prop:builddir)s/'+compiler_dir+'/bin/clang')
+    cxx = util.Interpolate('-DCMAKE_CXX_COMPILER=' + '%(prop:builddir)s/'+compiler_dir+'/bin/clang++')
+    lit = util.Interpolate('%(prop:builddir)s/' + compiler_dir + '/bin/llvm-lit')
+    test_suite_src_dir = util.Interpolate('%(prop:builddir)s/' + 'test/test-suite')
+    test_suite_workdir='test/sandbox/test-suite'
+    cmake_lit_arg = util.Interpolate('-DTEST_SUITE_LIT:FILEPATH=' + '%(prop:builddir)s/' + compiler_dir + '/bin/llvm-lit')
+    # used for cmake building test-suite step
+    options = [cc, cxx, cmake_lit_arg]
+
+    f.addGetSourcecodeForProject(
+        project='test-suite',
+        src_dir=test_suite_src_dir,
+        alwaysUseLatest=True)
+
+    f.addStep(CmakeCommand(name='cmake Test Suite',
+                           haltOnFailure=True,
+                           description='Running cmake on Test Suite dir',
+                           workdir=test_suite_workdir,
+                           options=options,
+                           path=test_suite_src_dir,
+                           generator='Ninja'))
+
+    f.addStep(NinjaCommand(name='ninja Test Suite',
+                           description='Running Ninja on Test Suite dir',
+                           haltOnFailure=True,
+                           workdir=test_suite_workdir))
+
+    f.addStep(LitTestCommand(name='Run Test Suite with lit',
+                             haltOnFailure=True,
+                             description='Running test suite tests',
+                             workdir=test_suite_workdir,
+                             command=[lit] + lit_args + ['.'],
+                             env=env,
+                             **kwargs))
+
+    return f
+
+def getTestSuiteBuildFactory(
+           depends_on_projects = None,
+           enable_runtimes = None,
+           targets = None,
+           llvm_srcdir = None,
+           obj_dir = None,
+           checks = None,
+           install_dir = None,
+           clean = False,
+           extra_configure_args = None,
+           env = None,
+           **kwargs):
+
+    # handle the -DCMAKE args for lit
+    lit_args = list()
+    if any("DLLVM_LIT_ARGS" in arg for arg in extra_configure_args):
+        arg = [arg for arg in extra_configure_args
+                if "DLLVM_LIT_ARGS" in arg][0]
+        lit_args = arg.split("=")[1]
+        lit_args = lit_args.split(" ")
+
+    f = getCmakeWithNinjaBuildFactory(
+            depends_on_projects = depends_on_projects,
+            enable_runtimes = enable_runtimes,
+            targets = targets,
+            llvm_srcdir = llvm_srcdir,
+            obj_dir = obj_dir,
+            checks = checks,
+            install_dir = install_dir,
+            clean = clean,
+            extra_configure_args = extra_configure_args,
+            env = env,
+            **kwargs)
+
+
+    addTestSuiteStep(f,
+           compiler_dir=f.obj_dir,
+           env=env,
+           lit_args=lit_args,
+           **kwargs)
+
+    return f