Page MenuHomePhabricator

Move sanitizer bots's configs from an external repo to zorg
ClosedPublic

Authored by kcc on Sep 29 2014, 3:07 PM.

Details

Summary

Currently, we store the configs for sanitizer bots in a separate repository:
https://code.google.com/p/address-sanitizer/source/browse/#svn%2Ftrunk%2Fbuild%2Fscripts%2Fslave
We are planing to add more hardware to the bots, and before that we would like to
perform some cleanup.
First (this change) we want to move the config files from the external repo to zorg.
Next, we'd like to change zorg/buildbot/builders/SanitizerBuilder.py to use the configs
from zorg instead of the extarnal repo.
Then we'll add some new bots.

Diff Detail

Repository
rL LLVM

Event Timeline

kcc updated this revision to Diff 14191.Sep 29 2014, 3:07 PM
kcc retitled this revision from to Move sanitizer bots's configs from an external repo to zorg.
kcc updated this object.
kcc edited the test plan for this revision. (Show Details)
kcc added reviewers: gkistanova, samsonov, eugenis.
kcc added a subscriber: Unknown Object (MLST).

Hi Kostya, thanks for adding these. I haven't reviewed the code yet, but I
fully support this change!

Cheers,
Renato
Hi gkistanova, samsonov, eugenis,

Currently, we store the configs for sanitizer bots in a separate repository:
https://code.google.com/p/address-sanitizer/source/browse/#svn%2Ftrunk%2Fbuild%2Fscripts%2Fslave
We are planing to add more hardware to the bots, and before that we would
like to
perform some cleanup.
First (this change) we want to move the config files from the external repo
to zorg.
Next, we'd like to change zorg/buildbot/builders/SanitizerBuilder.py to use
the configs
from zorg instead of the extarnal repo.
Then we'll add some new bots.

http://reviews.llvm.org/D5531

Files:

zorg/buildbot/builders/sanitizers/
zorg/buildbot/builders/sanitizers/buildbot_bootstrap.sh
zorg/buildbot/builders/sanitizers/buildbot_chrome_asan.sh
zorg/buildbot/builders/sanitizers/buildbot_chrome_tsan.sh
zorg/buildbot/builders/sanitizers/buildbot_cmake.sh
zorg/buildbot/builders/sanitizers/buildbot_functions.sh
zorg/buildbot/builders/sanitizers/buildbot_perf_asan.sh
zorg/buildbot/builders/sanitizers/buildbot_selector.py
zorg/buildbot/builders/sanitizers/buildbot_standard.bat
zorg/buildbot/builders/sanitizers/buildbot_standard.sh
zorg/buildbot/builders/sanitizers/slaveutil.py
zorg/buildbot/builders/sanitizers/test_tsan.sh

llvm-commits mailing list
llvm-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

Kostya,

Stupid question, but did you have those scripts before Zorg had any builders? They seem completely disconnected with anything Zorg does. I was expecting a Builder class, like Buildbot docs describe...

cheers,
--renato

eugenis edited edge metadata.Oct 1 2014, 6:03 AM

No, this is just a flexible way of writing buildbot scripts. Is not it easier to write a straightforward shell (or python, or any other language) script than muck with ShellCommand and friends?

The idea, and part of implementation, was stolen from the Chromium buildbot.

rnk added a subscriber: rnk.Oct 2 2014, 3:32 PM

This seems like a reasonable place to put these scripts to me.

In D5531#7, @rengolin wrote:

Stupid question, but did you have those scripts before Zorg had any builders? They seem completely disconnected with anything Zorg does. I was expecting a Builder class, like Buildbot docs describe...

Yeah, see http://llvm.org/viewvc/llvm-project/zorg/trunk/zorg/buildbot/builders/SanitizerBuilder.py?view=markup . We would change that to point the svn checkout at zorg instead of some Google Code svn repo.

zorg/buildbot/builders/sanitizers/buildbot_chrome_asan.sh
1 ↗(On Diff #14191)

I doubt we need these *_chrome*.sh scripts.

zorg/buildbot/builders/sanitizers/slaveutil.py
1 ↗(On Diff #14191)

This file looks dead, even in the ASan repo. We probably don't need it.

rnk added a comment.Oct 2 2014, 3:41 PM

I should also mention that using these funky shell scripts in this way makes it possible for any committer to make changes the buildbot script and have it go live on the next build without waiting for a master restart.

kcc updated this revision to Diff 14356.Oct 2 2014, 3:42 PM
kcc edited edge metadata.

remove 3 files from the change list

zorg/buildbot/builders/sanitizers/buildbot_chrome_asan.sh
1 ↗(On Diff #14191)

These bots (currently private) are using Chromium trunk to test LLVM trunk.
I've removed them for now, but eventually we may still want them here -- Chromium is a great test for LLVM :)

zorg/buildbot/builders/sanitizers/slaveutil.py
1 ↗(On Diff #14191)

removed

In D5531#11, @rnk wrote:

I should also mention that using these funky shell scripts in this way makes it possible for any committer to make changes the buildbot script and have it go live on the next build without waiting for a master restart.

I wanted this before, but I was sceptical to its value. I don't have a strong opinion about separating the scripts to make bot changes easier, but I'd prefer less bash and more python.

If the need to have bash is so it can work better at the shell level, than make the bash part the minimum necessary. Right now, you're duplicating a lot of the functionality on both shell and batch versions, things that the Builder library already does it better.

For example, we're in the process of unifying the repository checkout (svn, git, etc) for the ClangCMakeBuilder and this could be replicated across all other bots (since they all do the same thing), but with your current implementation you won't benefit from it and will increase the maintenance cost.

Also, please use:

#!/usr/bin/env bash

instead of:

#!/bin/bash

to make sure it'll work *everywhere*.

Is it possible to teach the common checkout code to update these
scripts as well? It should be easier now that they are in the same
repository.

gkistanova edited edge metadata.Oct 3 2014, 1:24 PM

For a full picture I'm missing of how these scripts are going to be used with the buildbot. Devil is in the details.
But I could wait with this till the second set of the patches.

The major questions are

  1. What build properties and parameters will be supported?
  2. How properties will be propagated through the buildbot infrastructure?
  3. If more than one script is used with one build, then the build seems to be a bit heavy. If there is only one script for a build, then I'd prefer seeing build steps instead of just one step with a heavy log to get a more granular and precise failure point from the bot. Something similar to what we did for the dragon egg with the scripted builder, perhaps?
kcc added a comment.Oct 3 2014, 2:13 PM

These are the exact same scripts that are already used by the existing bots (e.g. lab.llvm.org:8011/builders/sanitizer-x86_64-linux),
but currently they are pulled from an external repo. see /zorg/buildbot/builders/SanitizerBuilder.py:

sanitizer_buildbot = "sanitizer_buildbot"
# Get sanitizer buildbot scripts.
f.addStep(SVN(name='svn-sanitizer-buildbot',
              mode='update',
              baseURL='http://address-sanitizer.googlecode.com/svn/',
              defaultBranch='trunk',
              workdir=sanitizer_buildbot,
              alwaysUseLatest=True))

sanitizer_script = os.path.join("..", sanitizer_buildbot, "build",
                                "scripts", "slave",
                                "buildbot_selector.py")

I am just moving them to zorg svn.
Once they are in zorg svn, we will modify SanitizerBuilder.py to not use the external repo any more.
The current structure: buildbot_selector.py selects which .sh script to invoke, one .sh script for the builder.
(I hope Alexey or Evgeniy can add some details to this)

I understand this.
The above questions are for when you will be changing the sanitizer builder.
I'd like to see those questions addressed then.

Looks good to me, assuming you will fix the scripts to reference to the shell on more generic way as Renato suggested.

samsonov edited edge metadata.Oct 3 2014, 3:03 PM

Galina,

I'm not sure I'm answering the correct question, but: buildbot configuration for each builder is determined by buildbot_selector.py - it defines which shell script (and with which additional configuration options) should be run on each builder. This file should be updated for every new builder. Note, however, that in order to switch builder to a different configuration (or enable different set of flags there), it's enough to just commit a change to buildbot_selector.py, master restart will not be necessary.

Oh, regarding your question (3). The log for sanitizer builders is granular enough (see http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/13223), thanks to AnnotatedFactory. We're running a single shell script, but it contains lines like

@@@BUILD_STEP check-asan@@@

which form separate cells in build log.

kcc updated this revision to Diff 14412.Oct 3 2014, 3:33 PM
kcc edited edge metadata.

Changed /usr/bin/bash to "!/usr/bin/env bash"
and udated the scripts to their current version from
code.google.com/p/address-sanitizer

PTAL

Thanks, everyone!
LGTM

kcc closed this revision.Oct 6 2014, 11:52 AM
kcc updated this revision to Diff 14467.

Closed by commit rL219136 (authored by @kcc).