[libc builder] Add custom step for AOR tests.

[PaulkaToast]

Summary

Add customer step to non-asan buildbot to run make check. These tests from AOR are meant to act as regression tests until AORs are merged into libc.

Tested locally.

[sivachandra]

zorg/buildbot/builders/annotated/libc-linux.py

30

A high level comment is, annotated_builder.AnnotatedBuilder is very strange as it stands today. It is essentially serving as a namespace as it is stateless. So, mixing and and matching functionality from there and from utils makes it even more strange. I would rather see us moving away from AnnotatedBuilder and building our own libc specific utility here.

32

Will the bot turn red if the command fails?

33

Nit: This script runs on the builder so we should prefer to use os.path.join(...).

[PaulkaToast]

Addressed comments. (:

[PaulkaToast]

zorg/buildbot/builders/annotated/libc-linux.py

32

Yes.

[sivachandra]

zorg/buildbot/builders/annotated/libc-linux.py

37

ISTM that you do not need a context manager. Name this function run_step. Then, you can add cmd and directory args to it and ...

42

... call util.report_run_cmd(cmd, cwd=directory) here.

43

Catch specific exception here instead of a catch all. That way, you can print more specific information. You could choose to catch a specific exception, and also a catch all like this:

try:
  util.report_run_cmd(cmd, cwd=directory)
except subprocess.CalledProcessError as e:
  ...
except Exception as e:
  ...
finally:
  ...

46

For consistency, use util.report through instead of print.

51

For now, we will enforce that cmd is always a list.

[PaulkaToast]

zorg/buildbot/builders/annotated/libc-linux.py

37

The point of using a context manager is the encapsulation of steps that might require multiple commands and actions. For example consider a step that requires copying a file and then running a command. It could be written as:

with step('my multi process step'):
  copy(src, dst)
  run_command(['make', 'test'], directory=dst)

These two actions are logically bound together and shouldn't be split up into multiple different buildbot steps.

This is also what the upstream AnnotatedCommand based tools do: https://source.chromium.org/chromium/chromium/src/+/master:native_client/buildbot/buildbot_standard.py;l=236?originalUrl=https:%2F%2Fcs.chromium.org%2F

Ideally we'd want to build up something like the buildbot_lib.py that they have and maybe even switch annotated_builder.AnnotatedBuilder so that it uses it but that's a later concern.

42

see above.

[sivachandra]

zorg/buildbot/builders/annotated/libc-linux.py

37

That is why I said we will enforce cmd to be a list for now. When we have a situation like this, which we will definitely have very soon, we can extend it to be either a list, or a list of lists, or a function.

[PaulkaToast]

zorg/buildbot/builders/annotated/libc-linux.py

37

You can run multiple commands within a step like this with the current implementation:

with step('Example_Name', halt_on_fail=False):
    run_command(['cmd_run_in', 'dir1'], directory=dir1)
    run_command(['cmd_run_in', 'dir2'], directory=dir2)

As opposed to a list of lists which might look like...

step('Example_Name', [['cmd_run_in', 'dir1'], ['cd', 'dir2'], ['cmd_run_in', 'dir2']], directory=dir1, halt_on_fail=False)

I think this approach is far more readable and extensible, and there is prior art behind it from the people who came up with the AnnotatedCommand concept.

I think the best approach for a clean API is to use small, reusable single actions (e.g copy, run_command ), with a context manager.
This is similar to how it's done in chromium and how buildbot master itself.

[sivachandra]

zorg/buildbot/builders/annotated/libc-linux.py

37

While I do not agree, I also want to make progress. So, I am going to approve this and we can make follow up changes as required.

zorg/buildbot/builders/annotated/libc-linux.py

 #!/usr/bin/python
 
 import os
+import subprocess
 import sys
+import traceback
+import util
+from contextlib import contextmanager
 import annotated_builder
 
 
 def main(argv):
     ap = annotated_builder.get_argument_parser()
     ap.add_argument('--asan', action='store_true', default=False)
     args = ap.parse_args(argv[1:])
 
     extra_cmake_args = ['-DCMAKE_BUILD_TYPE=Debug']
     if args.asan:
         extra_cmake_args.append('-DLLVM_USE_SANITIZER=Address')
 
     projects = ['llvm', 'libc']
     check_targets = ['check-libc']
 
     builder = annotated_builder.AnnotatedBuilder()
     builder.run_steps(projects=projects,
                       check_targets=check_targets,
                       extra_cmake_args=extra_cmake_args)
 
+    # AOR tests step
+    if not args.asan:
+        with step('AOR Tests'):
+            aor_dir = os.path.join('..', 'llvm-project', 'libc', 'AOR_v20.02')
+            run_command(['make', 'check'], directory=aor_dir)
+
+
+@contextmanager
+def step(step_name, halt_on_fail=True):
+    util.report('@@@BUILD_STEP {}@@@'.format(step_name))
+    if halt_on_fail:
+        util.report('@@@HALT_ON_FAILURE@@@')
+    try:
+        yield
+    except Exception as e:
+        if isinstance(e, subprocess.CalledProcessError):
+            util.report(
+                '{} exited with return code {}.'.format(e.cmd, e.returncode)
+            )
+        util.report('The build step threw an exception...')
+        traceback.print_exc()
+
+        util.report('@@@STEP_FAILURE@@@')
+    finally:
+        sys.stdout.flush()
+
+def run_command(cmd, directory='.'):
+    util.report_run_cmd(cmd, cwd=directory)
+
 
 if __name__ == '__main__':
     sys.path.append(os.path.dirname(__file__))
     sys.exit(main(sys.argv))