Auto merge of #19720 - aneeshusa:support-env-vars-in-buildbot-steps, r=jdm

Update buildbot_steps lint to handle env variables

https://github.com/servo/saltfs/pull/687 added support
for specifying environment variables in `buildbot_steps.yml`.
Update the servo-tidy buildbot_steps.yml linter to reflect this.

Use the voluptuous Python library (BSD 3-clause license) for validation
in lieu of a much larger hand-written implementation.

Extracted out of #17171. Helps with servo/saltfs#770.

<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because they change the tests

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19720)
<!-- Reviewable:end -->

.travis.yml

@@ -20,11 +20,11 @@ matrix:
         - export LLVM_CONFIG=/usr/lib/llvm-3.9/bin/llvm-config
         - export CC=gcc-5 CXX=g++-5
       script:
-         - RUSTFLAGS='-D warnings' ./mach build -d --verbose
+         - ./mach build -d --verbose
-         - RUSTFLAGS='-D warnings' ./mach test-unit
+         - ./mach test-unit
          - ./mach clean
-         - RUSTFLAGS='-D warnings' ./mach build-geckolib
+         - ./mach build-geckolib
-         - RUSTFLAGS='-D warnings' ./mach test-stylo
+         - ./mach test-stylo
          - bash etc/ci/lockfile_changed.sh
       cache:
         directories:
@@ -34,7 +34,9 @@ matrix:
       before_cache:
         - ./mach clean-nightlies --keep 2 --force
         - ./mach clean-cargo-cache --keep 2 --force
-      env: CCACHE=/usr/bin/ccache
+      env:
+        CCACHE=/usr/bin/ccache
+        RUSTFLAGS=-Dwarnings
       addons:
         apt:
           sources:

appveyor.yml

@@ -3,6 +3,7 @@ version: 1.0.{build}
 environment:
   CCACHE_DIR: "%APPVEYOR_BUILD_FOLDER%\\.ccache"
   RUST_BACKTRACE: 1
+  RUSTFLAGS: -Dwarnings
   # The appveyor image we use has a pretty huge set of things installed... we make the
   # initial PATH something sane so we know what to expect
   PATH: "C:\\windows\\system32;\
@@ -48,7 +49,6 @@ cache:
 #  - ps: $blockRdp = $true; iex ((new-object net.webclient).DownloadString('https://raw.githubusercontent.com/appveyor/ci/master/scripts/enable-rdp.ps1'))
 
 build_script:
-  - set RUSTFLAGS=-D warnings
   - mach build -d -v
   - mach test-unit
 

etc/ci/buildbot_steps.yml

@@ -1,3 +1,6 @@
+env:
+  RUSTFLAGS: -Dwarnings
+
 mac-rel-wpt1:
   - ./mach clean-nightlies --keep 3 --force
   - env PKG_CONFIG_PATH=/usr/local/opt/zlib/lib/pkgconfig ./mach build --release
@@ -32,11 +35,11 @@ mac-rel-wpt4:
 
 mac-dev-unit:
   - ./mach clean-nightlies --keep 3 --force
-  - env PKG_CONFIG_PATH=/usr/local/opt/zlib/lib/pkgconfig RUSTFLAGS=-Dwarnings ./mach build --dev
+  - env PKG_CONFIG_PATH=/usr/local/opt/zlib/lib/pkgconfig ./mach build --dev
-  - env PKG_CONFIG_PATH=/usr/local/opt/zlib/lib/pkgconfig RUSTFLAGS=-Dwarnings ./mach test-unit
+  - env PKG_CONFIG_PATH=/usr/local/opt/zlib/lib/pkgconfig ./mach test-unit
-  - env ./mach package --dev
+  - ./mach package --dev
-  - env PKG_CONFIG_PATH=/usr/local/opt/zlib/lib/pkgconfig RUSTFLAGS=-Dwarnings ./mach build-cef
+  - env PKG_CONFIG_PATH=/usr/local/opt/zlib/lib/pkgconfig ./mach build-cef
-  - env RUSTFLAGS=-Dwarnings ./mach build-geckolib
+  - ./mach build-geckolib
   - bash ./etc/ci/lockfile_changed.sh
   - bash ./etc/ci/manifest_changed.sh
 
@@ -80,13 +83,13 @@ linux-dev:
   - ./mach clean-nightlies --keep 3 --force
   - ./mach test-tidy --no-progress --all
   - ./mach test-tidy --no-progress --self-test
-  - env CC=gcc-5 CXX=g++-5 RUSTFLAGS=-Dwarnings ./mach build --dev
+  - env CC=gcc-5 CXX=g++-5 ./mach build --dev
-  - env CC=gcc-5 CXX=g++-5 RUSTFLAGS=-Dwarnings ./mach test-unit
+  - env CC=gcc-5 CXX=g++-5 ./mach test-unit
-  - env ./mach package --dev
+  - ./mach package --dev
-  - env CC=gcc-5 CXX=g++-5 RUSTFLAGS=-Dwarnings ./mach build-cef
+  - env CC=gcc-5 CXX=g++-5 ./mach build-cef
-  - env CC=gcc-5 CXX=g++-5 RUSTFLAGS=-Dwarnings ./mach build --dev --no-default-features --features default-except-unstable
+  - env CC=gcc-5 CXX=g++-5 ./mach build --dev --no-default-features --features default-except-unstable
-  - env RUSTFLAGS=-Dwarnings ./mach build-geckolib
+  - ./mach build-geckolib
-  - env RUSTFLAGS=-Dwarnings ./mach test-stylo
+  - ./mach test-stylo
   - bash ./etc/ci/lockfile_changed.sh
   - bash ./etc/ci/manifest_changed.sh
   - bash ./etc/ci/check_no_panic.sh
@@ -120,7 +123,7 @@ linux-nightly:
 
 android:
   - ./mach clean-nightlies --keep 3 --force
-  - env ANDROID_SDK=/home/servo/android/sdk/r25.2.3 RUSTFLAGS=-Dwarnings ./mach build --android --dev
+  - env ANDROID_SDK=/home/servo/android/sdk/r25.2.3 ./mach build --android --dev
   - env ANDROID_SDK=/home/servo/android/sdk/r25.2.3 ./mach package --android --dev
   - bash ./etc/ci/lockfile_changed.sh
   - bash ./etc/ci/manifest_changed.sh
@@ -134,19 +137,18 @@ android-nightly:
 
 arm32:
   - ./mach clean-nightlies --keep 3 --force
-  - env RUSTFLAGS=-Dwarnings ./mach build --rel --target=arm-unknown-linux-gnueabihf
+  - ./mach build --rel --target=arm-unknown-linux-gnueabihf
   - bash ./etc/ci/lockfile_changed.sh
   - bash ./etc/ci/manifest_changed.sh
 
 arm64:
   - ./mach clean-nightlies --keep 3 --force
-  - env RUSTFLAGS=-Dwarnings ./mach build --rel --target=aarch64-unknown-linux-gnu
+  - ./mach build --rel --target=aarch64-unknown-linux-gnu
   - bash ./etc/ci/lockfile_changed.sh
   - bash ./etc/ci/manifest_changed.sh
 
 windows-msvc-dev:
   - mach.bat clean-nightlies --keep 3 --force
-  - set RUSTFLAGS=-D warnings
   - mach.bat build --dev
   - mach.bat test-unit
   - mach.bat package --dev

python/requirements.txt

@@ -15,6 +15,7 @@ pep8 == 1.5.7
 pyflakes == 0.8.1
 
 # For buildbot checking
+voluptuous == 0.10.5
 PyYAML == 3.12
 
 # For test-webidl

python/tidy/servo_tidy/tidy.py

@@ -17,8 +17,10 @@ import re
 import StringIO
 import subprocess
 import sys
+
 import colorama
 import toml
+import voluptuous
 import yaml
 
 from licenseck import MPL, APACHE, COPYRIGHT, licenses_toml, licenses_dep_toml
@@ -775,15 +777,24 @@ def duplicate_key_yaml_constructor(loader, node, deep=False):
 
 
 def lint_buildbot_steps_yaml(mapping):
-    # Check for well-formedness of contents
+    from voluptuous import Any, Extra, Required, Schema
-    # A well-formed buildbot_steps.yml should be a map to list of strings
-    for k in mapping.keys():
-        if not isinstance(mapping[k], list):
-            raise ValueError("Key '{}' maps to type '{}', but list expected".format(k, type(mapping[k]).__name__))
 
-        # check if value is a list of strings
+    # Note: dictionary keys are optional by default in voluptuous
-        for item in itertools.ifilter(lambda i: not isinstance(i, str), mapping[k]):
+    env = Schema({Extra: str})
-            raise ValueError("List mapped to '{}' contains non-string element".format(k))
+    commands = Schema([str])
+    schema = Schema({
+        'env': env,
+        Extra: Any(
+            commands,
+            {
+                'env': env,
+                Required('commands'): commands,
+            },
+        ),
+    })
+
+    # Signals errors via exception throwing
+    schema(mapping)
 
 
 class SafeYamlLoader(yaml.SafeLoader):
@@ -811,8 +822,8 @@ def check_yaml(file_name, contents):
         yield (line, e)
     except KeyError as e:
         yield (None, "Duplicated Key ({})".format(e.message))
-    except ValueError as e:
+    except voluptuous.MultipleInvalid as e:
-        yield (None, e.message)
+        yield (None, str(e))
 
 
 def check_for_possible_duplicate_json_keys(key_value_pairs):

python/tidy/servo_tidy_tests/test_tidy.py

@@ -210,12 +210,12 @@ class CheckTidiness(unittest.TestCase):
 
     def test_non_list_mapped_buildbot_steps(self):
         errors = tidy.collect_errors_for_files(iterFile('non_list_mapping_buildbot_steps.yml'), [tidy.check_yaml], [], print_text=False)
-        self.assertEqual("Key 'non-list-key' maps to type 'str', but list expected", errors.next()[2])
+        self.assertEqual("expected a list for dictionary value @ data['non-list-key']", errors.next()[2])
         self.assertNoMoreErrors(errors)
 
     def test_non_string_list_mapping_buildbot_steps(self):
         errors = tidy.collect_errors_for_files(iterFile('non_string_list_buildbot_steps.yml'), [tidy.check_yaml], [], print_text=False)
-        self.assertEqual("List mapped to 'mapping_key' contains non-string element", errors.next()[2])
+        self.assertEqual("expected str @ data['mapping_key'][0]", errors.next()[2])
         self.assertNoMoreErrors(errors)
 
     def test_lock(self):