Use yaml for buildbot steps #325

aneeshusa · 2016-04-18T08:25:02Z

A bunch of changes here including various removals and rearrangements; I'd recommend checking this commit by commit (including commit messages).

This a) simplifies the Buildbot master configuration and b) uses YAML to configure build steps instead of Python code. This allows us to PR the YAML file over to the main servo repo for #316; I'd like to see this landed before working on the rest of #316.

cc @edunham @shinglyu

This change is

jdm · 2016-04-18T13:41:11Z

Having not looked at any of the rest of the implmentation, that's a really nice YAML configuration file!

aneeshusa · 2016-04-18T15:45:10Z

A note I left myself yesterday:

We don't need the GITHUB_STATUS_TOKEN pillar value anymore, so when this lands, we need to do the following:

Revoke the token (if it's still valid) (unless it's getting used for homu...)
Remove the value from the production pillar

aneeshusa · 2016-04-18T16:22:27Z

Also, to check that this works properly you can rename master.cfg to master.py, then in a Python2 repl import master and peruse the master.c['builders'] object - I found this helpful while testing.

larsbergstrom · 2016-04-18T20:33:48Z

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 2 of 2 files at r4, 2 of 2 files at r5, 3 of 3 files at r6, 1 of 1 files at r7, 4 of 4 files at r8, 2 of 2 files at r9, 3 of 3 files at r10, 2 of 2 files at r11.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

a discussion (no related file):
Can you please remove the pkill commit? It hasn't done so in a while, but it catches when servo processes spin out of control occasionally, so I'm loathe to remove it until we do something like reboot or image staging per-PR.

a discussion (no related file):
I think we need to keep the "duplicate" tests for manifest/lock file changes. They need to be run multiple times, because cargo can update different Cargo.lock files differently on different platforms and release vs. debug.

Comments from Reviewable

larsbergstrom · 2016-04-18T20:34:34Z

This is an awesome PR! @edunham and I just spent some time reviewing it line by line and were in awe of how much cleaner everything will be after this. Nice work!

edunham · 2016-04-18T20:38:22Z

These changes are beautiful, and were uniquely easy to review because of how well you structured your commits. Now our only changes are lines to add back into steps.yml, as Lars has mentioned:

We do still need to pkill -x servo at the start or end of every build, or the platform-specific equivalent, because sometimes unusual test failures leave servo processes lying around.
We'd prefer to keep running the "duplicate" tests for now, to catch subtle differences between platforms

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 2 of 2 files at r4, 2 of 2 files at r5, 3 of 3 files at r6, 1 of 1 files at r7, 4 of 4 files at r8, 2 of 2 files at r9, 3 of 3 files at r10, 2 of 2 files at r11.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

Comments from Reviewable

aneeshusa · 2016-04-18T21:45:56Z

Thanks, I'm a stickler for using git commits + messages effectively :)

Quick question about keeping the duplicates - the only two tests that should be duplicated are lockfile_changed.sh and manifest_changed.sh, not test-tidy or check_no_unwrap.sh, correct? Also, lockfile_changed.sh just runs git diff, so why would its results be platform specific?

larsbergstrom · 2016-04-18T21:54:26Z

Yes, just lockfile/manifest_changed.

The Cargo.lock file contains a list of all of the crates and versions used across all platforms. But, in a given platform-specific build, only a subset of those might get touched. Further, different platforms (cef, stylo, servo, and gonk) even have their own different Cargo.lock files that are only touched on those builds! That check tends to catch errors on builds that are not performed by the original author (e.g., MacOS package updates for people who only have linux).

Manishearth · 2016-04-18T21:59:07Z

This seems to get rid of the logfiles stuff we had? Perhaps we should add an extra logfiles key to the YAML?

aneeshusa · 2016-04-18T22:11:35Z

@Manishearth the DynamicServoFactory.make_step method uses a little magic - sorry, heuristics 😉 - to automatically populate the logfiles parameter when necessary by looking for --log-something arguments to ./mach. The logfiles parameter is easy to miss - see #310 for an example - but our usage of mozlog means this catches every important logfile so far.

In the future, we can make make_step smarter to handle explicit configuration of logfiles and such in the YAML file fairly easily, but I'd prefer to avoid that until we really need it.

aneeshusa · 2016-04-18T22:13:07Z

@larsbergstrom should we be running the lockfile and manifest checks on all the other builders as well, e.g. arm32 and arm64 and android?

Manishearth · 2016-04-18T22:21:11Z

Yes, because each platform might pull in platform-specific packages which won't have been updated in the committer's lockfile.

aneeshusa · 2016-04-18T23:19:29Z

What's the equivalent of pkill for Windows?

aneeshusa · 2016-04-18T23:48:56Z

Added back pkill (note, it's first removed then added back in a later commit), and updated the steps.yml file.

I also added a few TODO comments to do both of those things for Windows as well.

larsbergstrom · 2016-04-18T23:56:09Z

@aneeshusa taskkill, https://technet.microsoft.com/en-us/library/bb491009.aspx

I think we want taskkill /im servo.exe, but will check it out tomorrow when I'm in front of my Windows VM.

larsbergstrom · 2016-04-18T23:56:34Z

But yes, feel free to skip it on Windows for now!

aneeshusa · 2016-04-19T01:12:47Z

Fixed the build error.

edunham · 2016-04-19T02:22:28Z

Reviewable just made me go through every commit again, but showed everything unchanged from before you rebased as being empty diffs. Still looks good, though :)

Reviewed 9 of 9 files at r12, 1 of 1 files at r13, 1 of 1 files at r14, 2 of 2 files at r15, 2 of 2 files at r16, 1 of 1 files at r17, 3 of 3 files at r18, 1 of 1 files at r19, 4 of 4 files at r20, 2 of 2 files at r21, 2 of 2 files at r22, 8 of 8 files at r23, 1 of 1 files at r24, 3 of 3 files at r25, 1 of 1 files at r26, 4 of 4 files at r27, 2 of 2 files at r28, 2 of 2 files at r29, 1 of 1 files at r30.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable

shinglyu · 2016-04-19T07:51:26Z

Do we need to quote the commands in the yaml file? We might have some steps that has special characters that may be parsed incorrectly?

Manishearth · 2016-04-19T08:18:55Z

Yaml is pretty good that way, it uses newlines so you rarely need to quote

bors-servo · 2016-04-20T13:05:34Z

☔ The latest upstream changes (presumably #310) made this pull request unmergeable. Please resolve the merge conflicts.


        Remove gonk

In b7fc797, we disabled blocking on gonk; since we're not using the results, removing the builder will use less resources.


        Temporarily remove pkill steps

We should be running pkill (or the platform-specific equivalent) at the start and end of each build, but this is really a CI detail, not something that is integral to the list of steps, so remove it for now.


        Remove gzip steps

The steps gzipping build logs did not have logfiles listed, so they were not available for download from the web interface. The only way to get the logs is via ssh/scp, which works just as well for non-gzipped logs. This will cut down on build times.


        Create separate file for environments

The existing mechanism for chaining environment dictionaries is a hack that Guido has noted as abuse of the '**' mechanism: https://mail.python.org/pipermail/python-dev/2010-April/099459.html Create a subclass that allows 'adding' in a single expression, as copy() + update() takes two lines. Use str.join for listing PATHs to keep lines short. This also exposed an error in the Windows PATH, with a missing semicolon between r'C:\msys64\usr\bin' and r'C:\Windows\system32'. Move the environments into a separate file to keep them all together. Rename the various environment variables for clarity and consistency. The environments.py file now passes flake8; this should be added to our test suite.


        Create separate file for steps

In order to move the build steps to the main servo repository, they must be converted from python code that requires the Buildbot library to be installed (to manually build steps) into more declarative steps. This commit starts this conversion by using heuristics to convert a list of shell commands (akin to those in a .travis.yml or .appveyor.yml file) into the appropriate Buildbot Steps. The doc factory and windows factory still use hardcoded steps; see the comments in the code for more details. The android nightly factory now uses a relative instead of absolute path for the .apk file to make the Jinja templating unnecessary.


        Re-add pkill step

This automatically runs pkill for each build. It is cleaner to put this in the main configuration instead of the lists of steps because this is a CI-related cleanup task, not a necessary step to run the build + test suite. Also, this ensures that the pkill step cannot be forgotten when adding new builders. Note: this doesn't work on Windows yet.


        Remove GITHUB_STATUS_TOKEN

This was used a long time ago for bors, but Buildbot now reports status indirectly via Homu.


        Clean up master.cfg to pass flake8

Also rename LINUX_RESERVED_SLAVES to LINUX_SLAVES, since all of our builders are now on reserved instances.


        Use a YAML file to drive dynamic builders

As a stepping-stone to having builder steps live in the main Servo repo, switch to a simple data format to express builder configuration (lists of strings accessed via yaml.safe_load) instead of needing to run or import arbitrary untrusted Python code. Currently, the YAML file is still part of the master configuration, but it is now ready to be moved to the main Servo repo.


        Tweak testing steps

Use linux-dev to run all lints because it is the fastest builder; these are platform-independent. Ensure that the lockfile_changed.sh and manifest_changed.sh scripts are run for every builder, since these are platform-dependent. Note: these should also be added for the Windows builder. This is also meant as an example of how much easier a YAML file makes it to edit our build scripts; note the lack of needing to know about `logfiles` or other parameters which have been forgotten in the past.


        Add test for buildbot configuration passing flake8

This test requires the flake8 python module; to ensure it is installed, add a requirements.txt file. Travis-CI will automatically install the dependencies specified in that file.


        Add flake8 lint test for other python files

Currently, the only other python files are the test suite.

aneeshusa · 2016-04-20T16:32:28Z

Rebased again; sorry to make you sit through another Reviewable session!

larsbergstrom · 2016-04-20T17:24:28Z

@edunham I think this is probably good to go, right? At least, reviewable is just telling me it's a rebase with no edits, and I think everything is signed off...

edunham · 2016-04-20T17:34:12Z

@larsbergstrom I agree, let's land it now and then test carefully.

@bors-servo r+

bors-servo · 2016-04-20T17:34:13Z

📌 Commit acf76a1 has been approved by edunham

bors-servo · 2016-04-20T17:34:14Z

⌛ Testing commit acf76a1 with merge 59eb816...


        Auto merge of #325 - aneeshusa:use-yaml-for-buildbot-steps, r=edunham

@edunham

Use yaml for buildbot steps A bunch of changes here including various removals and rearrangements; I'd recommend checking this commit by commit (including commit messages). This a) simplifies the Buildbot master configuration and b) uses YAML to configure build steps instead of Python code. This allows us to PR the YAML file over to the main servo repo for #316; I'd like to see this landed before working on the rest of #316. cc @edunham @shinglyu  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/325)

bors-servo · 2016-04-20T17:47:15Z

☀️ Test successful - travis

aneeshusa mentioned this pull request Apr 18, 2016

Move buildbot logic to scripts in `/etc/ci` #316

Closed

4 of 4 tasks complete

aneeshusa force-pushed the aneeshusa:use-yaml-for-buildbot-steps branch from 6ea740c to abfd027 Apr 18, 2016

aneeshusa mentioned this pull request Apr 18, 2016

Fix typo in Windows PATH #326

Merged

aneeshusa force-pushed the aneeshusa:use-yaml-for-buildbot-steps branch from abfd027 to eb3fad2 Apr 18, 2016

aneeshusa force-pushed the aneeshusa:use-yaml-for-buildbot-steps branch from eb3fad2 to d720a97 Apr 19, 2016

aneeshusa force-pushed the aneeshusa:use-yaml-for-buildbot-steps branch from d720a97 to 195e548 Apr 19, 2016

aneeshusa added 3 commits Apr 16, 2016

Remove gonk

caa1d41

In b7fc797, we disabled blocking on gonk; since we're not using the results, removing the builder will use less resources.

Temporarily remove pkill steps

36b16ff

We should be running pkill (or the platform-specific equivalent) at the start and end of each build, but this is really a CI detail, not something that is integral to the list of steps, so remove it for now.

Remove gzip steps

ceff384

The steps gzipping build logs did not have logfiles listed, so they were not available for download from the web interface. The only way to get the logs is via ssh/scp, which works just as well for non-gzipped logs. This will cut down on build times.

aneeshusa added 9 commits Apr 16, 2016

Remove GITHUB_STATUS_TOKEN

90937e6

This was used a long time ago for bors, but Buildbot now reports status indirectly via Homu.

Clean up master.cfg to pass flake8

056ed34

Also rename LINUX_RESERVED_SLAVES to LINUX_SLAVES, since all of our builders are now on reserved instances.

Add test for buildbot configuration passing flake8

bab18c3

This test requires the flake8 python module; to ensure it is installed, add a requirements.txt file. Travis-CI will automatically install the dependencies specified in that file.

Add flake8 lint test for other python files

Loading status checks…

acf76a1

Currently, the only other python files are the test suite.

aneeshusa force-pushed the aneeshusa:use-yaml-for-buildbot-steps branch from 195e548 to acf76a1 Apr 20, 2016

This was referenced Apr 21, 2016

Improve properties.mako.rs file structure, take 2 servo/servo#10774

Merged

Investigations for #10760 servo/servo#10798

Closed

jdm mentioned this pull request Apr 22, 2016

Make it easy to try particular tests or directories in isolation servo/servo#10811

Open

aneeshusa mentioned this pull request Apr 27, 2016

Use steps.Test instead of steps.Compile for './mach test-unit' on Windows #343

Closed

shinglyu mentioned this pull request Apr 27, 2016

vagrant up failed #344

Closed

aneeshusa mentioned this pull request May 1, 2016

Run pkill on the Windows builders #351

Closed

servo / saltfs

Use yaml for buildbot steps #325

Use yaml for buildbot steps #325

aneeshusa commented Apr 18, 2016

jdm commented Apr 18, 2016

aneeshusa commented Apr 18, 2016 •

edited by edunham

aneeshusa commented Apr 18, 2016

larsbergstrom commented Apr 18, 2016

larsbergstrom commented Apr 18, 2016

edunham commented Apr 18, 2016 •

edited

aneeshusa commented Apr 18, 2016

larsbergstrom commented Apr 18, 2016

Manishearth commented Apr 18, 2016

aneeshusa commented Apr 18, 2016

aneeshusa commented Apr 18, 2016

Manishearth commented Apr 18, 2016

aneeshusa commented Apr 18, 2016

aneeshusa commented Apr 18, 2016

larsbergstrom commented Apr 18, 2016

larsbergstrom commented Apr 18, 2016

aneeshusa commented Apr 19, 2016

edunham commented Apr 19, 2016

shinglyu commented Apr 19, 2016

Manishearth commented Apr 19, 2016

bors-servo commented Apr 20, 2016

aneeshusa commented Apr 20, 2016

larsbergstrom commented Apr 20, 2016

edunham commented Apr 20, 2016

bors-servo commented Apr 20, 2016

bors-servo commented Apr 20, 2016

bors-servo commented Apr 20, 2016

servo / saltfs

Join GitHub today

Use yaml for buildbot steps #325

Use yaml for buildbot steps #325

Conversation

aneeshusa commented Apr 18, 2016

jdm commented Apr 18, 2016

aneeshusa commented Apr 18, 2016 • edited by edunham

aneeshusa commented Apr 18, 2016

larsbergstrom commented Apr 18, 2016

larsbergstrom commented Apr 18, 2016

edunham commented Apr 18, 2016 • edited

aneeshusa commented Apr 18, 2016

larsbergstrom commented Apr 18, 2016

Manishearth commented Apr 18, 2016

aneeshusa commented Apr 18, 2016

aneeshusa commented Apr 18, 2016

Manishearth commented Apr 18, 2016

aneeshusa commented Apr 18, 2016

aneeshusa commented Apr 18, 2016

larsbergstrom commented Apr 18, 2016

larsbergstrom commented Apr 18, 2016

aneeshusa commented Apr 19, 2016

edunham commented Apr 19, 2016

shinglyu commented Apr 19, 2016

Manishearth commented Apr 19, 2016

bors-servo commented Apr 20, 2016

aneeshusa commented Apr 20, 2016

larsbergstrom commented Apr 20, 2016

edunham commented Apr 20, 2016

bors-servo commented Apr 20, 2016

bors-servo commented Apr 20, 2016

bors-servo commented Apr 20, 2016

aneeshusa commented Apr 18, 2016 •

edited by edunham

edunham commented Apr 18, 2016 •

edited