Hi,

This is a good start and while I have a few comments below, they are
all for small changes that won't take long to make and should
hopefully make maintaining the code easier. I am completely behind
changing as much of the build process into Python so thanks for doing
this!

Could you run the python through the pep8 tool
(http://www.python.org/dev/peps/pep-0008/ for what it is checking).
You certainly have some lines that are too long and I think some
non-standard spacing between classes and functions. The other tool to
help reduce the number of style nags you will get is pyflakes (sudo
apt-get install pep8 pyflakes). While I think the 80 character line
length thing is a pain and being longer would be nice, it is the
standard we code to. I will happily buy you a beer and let you
complain about it at the next Connect if you want. Took me a few rants
to get over!

You could try using PyCharm as your Python editor. It provides real
time feedback on some pep8 and pyflakes errors as well as doing nice
code browsing, auto-completion, debugging and unit test running (among
many other features). We have a license.

Other comments inline.

On 23 April 2012 15:56, Paul Sokolovsky <email address hidden> wrote:

> https://code.launchpad.net/~pfalcon/linaro-android-build-tools/validate-build-conf/+merge/103118

> === added file 'node/prepare_build_config.py'
> --- node/prepare_build_config.py        1970-01-01 00:00:00 +0000
> +++ node/prepare_build_config.py        2012-04-23 14:55:21 +0000
> @@ -0,0 +1,103 @@
> +#!/usr/bin/env python
> +import sys
> +import os
> +import base64
> +import re
> +import pipes
> +
> +
> +SLAVE_TYPE_FILE = "/var/run/build-tools/slave-type"
> +SLAVE_TYPE_RESTRICTED = "Natty Release 64bit Instance Store - private builds"
> +# sf-safe build config is written to this file
> +BUILD_CONFIG_FILE = "/var/run/build-tools/build-config"
> +
> +
> +class BuildConfigMismatchException(Exception):
> +    pass
> +
> +def shell_unquote(s):
> +    # Primitive implementation, we should

Seem to be missing the end of that comment :-)

> +    if s[0] == '"' and s[-1] == '"':
> +        return s[1:-1]
> +    if s[0] == "'" and s[-1] == "'":
> +        return s[1:-1]
> +    return s

This may work better (will cope with whitespace at the beginning and
end of the line):

unquote = re.search("^\s*["'](.*?)["']\s*$", s)

if unquote:
    return unquote s.group(1)

You could do two searches instead of using the character class or a
reference so the second quote matches the first if you wanted.

> +
> +def get_slave_type():
> +    slave_type = ""
> +    try:
> +        slave_type = open(SLAVE_TYPE_FILE).read()
> +    except:
> +        pass
> +    return slave_type

This will close the file for you and avoids the need for the try/except block:

with open(SLAVE_TYPE_FILE) as slave_type_file:
    return slave_type_file.read()

return None

> +def validate_config(config, slave_type):
> +    """Validate various constraints on the config params and overall
> +    environment."""
> +    full_job_name = os.environ.get("JOB_NAME")

os.environ is a dictionary so this will return the value of "JOB_NAME"
or None. Is it worth checking for None? The next line will fail
without a useful error if None is returned. How about:

if "JOB_NAME not in os.environ:
    # error handling
    ...

owner, job_subname = os.environ["JOB_NAME"].split("_", 1)

> +    owner, job_subname = full_job_name.split("_", 1)
> +
> +    # Deduce parameter "categories" which we can directly match against each other
> +    if slave_type == SLAVE_TYPE_RESTRICTED:
> +        slave_type_cat = "restricted"
> +    else:
> +        slave_type_cat = "normal"
> +
> +    if owner in ["linaro-android-private", "linaro-android-restricted"]:
> +        owner_cat = "restricted"
> +    else:
> +        owner_cat = "normal"
> +
> +    if config.get("BUILD_TYPE", "build-android") in ["build-android-private", "build-android-restricted"]:
> +        build_type_cat = "restricted"
> +    else:
> +        build_type_cat = "normal"
> +
> +    # Now, process few most expected mismatches in adhox way, to provide better error messages
> +    if slave_type_cat == "restricted" and owner_cat != "restricted":
> +        raise BuildConfigMismatchException("Only jobs owned by ~linaro-android-restricted may run on this build slave type")
> +
> +    if owner_cat == "restricted" and build_type_cat != "restricted":
> +        raise BuildConfigMismatchException("Jobs owned by ~linaro-android-restricted must use BUILD_TYPE=build-android-restricted")
> +
> +    # Finally, generic mismatch detection
> +    if slave_type_cat != owner_cat or slave_type_cat != build_type_cat:
> +        raise BuildConfigMismatchException("Incompatible slave type, job owner and build type")
> +
> +
> +def convert_config_to_shell(config_text, out_filename):
> +    config = {}
> +    out = open(out_filename, "w")
> +
> +    for l in config_text.split("\n"):
> +        l = l.strip()
> +        if not l or l[0] == "#":
> +            continue
> +        if not re.match(r"[A-Za-z][A-Za-z0-9_]*=", l):
> +            print "Invalid build config syntax: " + l
> +            sys.exit(1)
> +        var, val = l.split("=", 1)
> +        val = shell_unquote(val)
> +        config[var] = val
> +        out.write("%s=%s\n" % (var, pipes.quote(val)))
> +    out.close()
> +    return config
> +
> +def main(config_in, is_base64):
> +    if is_base64:
> +        config_in = base64.b64decode(config_in)
> +    config = convert_config_to_shell(config_in, BUILD_CONFIG_FILE)
> +    try:
> +        validate_config(config, get_slave_type())
> +    except BuildConfigMismatchException, e:
> +        print str(e)
> +        sys.exit(1)
> +
> +if __name__ == "__main__":
> +    optparser = optparse.OptionParser(usage="%prog")
> +    optparser.add_option("--base64", action="store_true", help="Process only jobs matching regex pattern")
> +    options, args = optparser.parse_args(sys.argv[1:])
> +    if len(args) != 1:
> +        optparser.error("Wrong number of arguments")
> +    main(args[0], opts.base64)
> +
>
> === added directory 'tests'
> === added file 'tests/test_prepare_build_config.py'
> --- tests/test_prepare_build_config.py  1970-01-01 00:00:00 +0000
> +++ tests/test_prepare_build_config.py  2012-04-23 14:55:21 +0000
> @@ -0,0 +1,96 @@
> +import sys
> +import os
> +import base64
> +import tempfile
> +sys.path.append(os.path.dirname(__file__) + "/../node")
> +import prepare_build_config
> +

Any reason not to use http://docs.python.org/library/unittest.html? It
makes the tests much easier to write. The tests in
test_validate_config could then be turned into a class and split into
several functions (there really are several tests here) and unittest
would produce nice error messages when things went wrong.

> +def test_validate_config():
> +    os.environ["JOB_NAME"] = "user_job"
> +    config = {}
> +    prepare_build_config.validate_config(config, "")
> +
> +    os.environ["JOB_NAME"] = "linaro-android-private_job"
> +    config = {}
> +    try:
> +        prepare_build_config.validate_config(config, "")
> +        assert False, "Mismatch wasn't caught"
> +    except prepare_build_config.BuildConfigMismatchException:
> +        pass
> +    os.environ["JOB_NAME"] = "linaro-android-private_job"
> +    config = {"BUILD_TYPE": "build-android-restricted"}
> +    try:
> +        prepare_build_config.validate_config(config, "")
> +        assert False, "Mismatch wasn't caught"
> +    except prepare_build_config.BuildConfigMismatchException:
> +        pass

If you used unittest you could use
self.assertRaises(BuildConfigMismatchException) here.

> +    os.environ["JOB_NAME"] = "linaro-android-private_job"
> +    config = {"BUILD_TYPE": "build-android-restricted"}
> +    prepare_build_config.validate_config(config, prepare_build_config.SLAVE_TYPE_RESTRICTED)
> +
> +    os.environ["JOB_NAME"] = "linaro-android-private_job"
> +    config = {}
> +    try:
> +        prepare_build_config.validate_config(config, prepare_build_config.SLAVE_TYPE_RESTRICTED)
> +        assert False, "Mismatch wasn't caught"
> +    except prepare_build_config.BuildConfigMismatchException:
> +        pass
> +
> +    del os.environ["JOB_NAME"]

Test for JOB_NAME not set in environment (and any other env variables
used) would be good.

> +def test_convert_config_to_shell():
> +    config_base64 = """\
> +TUFOSUZFU1RfUkVQTz1naXQ6Ly9hbmRyb2lkLmdpdC5saW5hcm8ub3JnL3BsYXRmb3JtL21hbmlm
> +ZXN0LmdpdApNQU5JRkVTVF9CUkFOQ0g9bGluYXJvX2FuZHJvaWRfMi4zLjUKTUFOSUZFU1RfRklM
> +RU5BTUU9c3RhZ2luZy1vbWFwNDQ2MC54bWwKVEFSR0VUX1BST0RVQ1Q9cGFuZGFib2FyZApUQVJH
> +RVRfU0lNVUxBVE9SPWZhbHNlClRPT0xDSEFJTl9VUkw9aHR0cHM6Ly9hbmRyb2lkLWJ1aWxkLmxp
> +bmFyby5vcmcvamVua2lucy9qb2IvbGluYXJvLWFuZHJvaWRfdG9vbGNoYWluLTQuNi1ienIvbGFz
> +dFN1Y2Nlc3NmdWxCdWlsZC9hcnRpZmFjdC9idWlsZC9vdXQvYW5kcm9pZC10b29sY2hhaW4tZWFi
> +aS00LjYtZGFpbHktbGludXgteDg2LnRhci5iejIKTEFWQV9TVUJNSVQ9MQ==
> +"""
> +    config_dict = {
> +        'TARGET_SIMULATOR': 'false',
> +        'TARGET_PRODUCT': 'pandaboard',
> +        'TOOLCHAIN_URL': 'https://android-build.linaro.org/jenkins/job/linaro-android_toolchain-4.6-bzr/lastSuccessfulBuild/artifact/build/out/android-toolchain-eabi-4.6-daily-linux-x86.tar.bz2',
> +        'MANIFEST_BRANCH': 'linaro_android_2.3.5',
> +        'MANIFEST_FILENAME': 'staging-omap4460.xml', 'LAVA_SUBMIT': '1',
> +        'MANIFEST_REPO': 'git://android.git.linaro.org/platform/manifest.git'
> +    }
> +
> +    config_text = base64.b64decode(config_base64)
> +    out_fd, out_filename = tempfile.mkstemp()
> +    os.close(out_fd)
> +    dict = prepare_build_config.convert_config_to_shell(config_text, out_filename)
> +    assert dict == config_dict
> +
> +def test_main():
> +    config_base64 = """\
> +TUFOSUZFU1RfUkVQTz1naXQ6Ly9hbmRyb2lkLmdpdC5saW5hcm8ub3JnL3BsYXRmb3JtL21hbmlm
> +ZXN0LmdpdApNQU5JRkVTVF9CUkFOQ0g9bGluYXJvX2FuZHJvaWRfMi4zLjUKTUFOSUZFU1RfRklM
> +RU5BTUU9c3RhZ2luZy1vbWFwNDQ2MC54bWwKVEFSR0VUX1BST0RVQ1Q9cGFuZGFib2FyZApUQVJH
> +RVRfU0lNVUxBVE9SPWZhbHNlClRPT0xDSEFJTl9VUkw9aHR0cHM6Ly9hbmRyb2lkLWJ1aWxkLmxp
> +bmFyby5vcmcvamVua2lucy9qb2IvbGluYXJvLWFuZHJvaWRfdG9vbGNoYWluLTQuNi1ienIvbGFz
> +dFN1Y2Nlc3NmdWxCdWlsZC9hcnRpZmFjdC9idWlsZC9vdXQvYW5kcm9pZC10b29sY2hhaW4tZWFi
> +aS00LjYtZGFpbHktbGludXgteDg2LnRhci5iejIKTEFWQV9TVUJNSVQ9MQ==
> +"""
> +    config_dict = {
> +        'TARGET_SIMULATOR': 'false',
> +        'TARGET_PRODUCT': 'pandaboard',
> +        'TOOLCHAIN_URL': 'https://android-build.linaro.org/jenkins/job/linaro-android_toolchain-4.6-bzr/lastSuccessfulBuild/artifact/build/out/android-toolchain-eabi-4.6-daily-linux-x86.tar.bz2',
> +        'MANIFEST_BRANCH': 'linaro_android_2.3.5',
> +        'MANIFEST_FILENAME': 'staging-omap4460.xml', 'LAVA_SUBMIT': '1',
> +        'MANIFEST_REPO': 'git://android.git.linaro.org/platform/manifest.git'
> +    }
> +
> +    out_fd, out_filename = tempfile.mkstemp()
> +    prepare_build_config.BUILD_CONFIG_FILE = out_filename
> +    os.environ["JOB_NAME"] = "foo_bar"
> +    prepare_build_config.main(config_base64, True)
> +    dict = {}
> +    for l in open(out_filename):
> +        var, val = l.rstrip().split("=", 1)
> +        dict[var] = val
> +    assert dict == config_dict



-- 
James Tunnicliffe