Page MenuHomePhabricator

[libc builder] Add custom step for AOR tests.

Authored by PaulkaToast on Tue, Mar 17, 2:54 PM.



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.

Diff Detail

Event Timeline

PaulkaToast created this revision.Tue, Mar 17, 2:54 PM
PaulkaToast edited the summary of this revision. (Show Details)Tue, Mar 17, 2:58 PM
sivachandra added inline comments.Tue, Mar 17, 4:43 PM

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.


Will the bot turn red if the command fails?


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

PaulkaToast marked 4 inline comments as done.

Addressed comments. (:

PaulkaToast added inline comments.Wed, Mar 18, 12:46 PM


PaulkaToast updated this revision to Diff 251601.
Harbormaster completed remote builds in B49864: Diff 251601.
sivachandra added inline comments.Fri, Mar 20, 11:30 AM

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


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


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:

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

For consistency, use through instead of print.


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

PaulkaToast marked 7 inline comments as done.
PaulkaToast added inline comments.

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:;l=236?

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


see above.

sivachandra added inline comments.Fri, Mar 20, 1:28 PM

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 marked an inline comment as done.Fri, Mar 20, 4:46 PM
PaulkaToast added inline comments.

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 accepted this revision.Fri, Mar 20, 5:44 PM
sivachandra added inline comments.

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.

This revision is now accepted and ready to land.Fri, Mar 20, 5:44 PM
This revision was automatically updated to reflect the committed changes.