Merge into trunk : validate-build-conf : Code : Linaro Android Build Tools

Status:	Merged
Merged at revision:	460
Proposed branch:	lp://qastaging/~pfalcon/linaro-android-build-tools/validate-build-conf
Merge into:	lp://qastaging/linaro-android-build-tools
Diff against target:	309 lines (+237/-24) 4 files modified node/build (+26/-22) node/lava-submit (+1/-2) node/prepare_build_config.py (+114/-0) tests/test_prepare_build_config.py (+96/-0)
To merge this branch:	bzr merge lp://qastaging/~pfalcon/linaro-android-build-tools/validate-build-conf
Related bugs:	Link a bug report

Reviewer	Date Requested	Status
Georgy Redkozubov	2012-04-23	Approve on 2012-04-24
Paul Sokolovsky		Approve on 2012-04-24
James Tunnicliffe	2012-04-23	Pending
Review via email: mp+103118@code.qastaging.launchpad.net

Revision history for this message

Georgy Redkozubov (gesha) wrote on 2012-04-20: Posted in a previous version of this proposal

#

At least following 2 errors were found:

1) convert_config_to_shell() takes 1 argument in definition, 2 was given: config = convert_config_to_shell(config_text, get_slave_type())

   Looks like you have mixed up 2 functions, please move get_slave_type() form first to second:
    config = convert_config_to_shell(config_text, get_slave_type())
    try:
        validate_config(config)

2) global name 'cfg' is not defined
for l in cfg.split("\n"):
You should use config_text.split("\n") instead

review: Needs Fixing

Revision history for this message

Paul Sokolovsky (pfalcon) wrote on 2012-04-20: Posted in a previous version of this proposal

#

Thanks, that definitely shows that more unit tests should be added ;-)

Revision history for this message

James Tunnicliffe (dooferlad) wrote on 2012-04-23:

#

Download full text (11.0 KiB)

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

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 <paul.sokolovsky@linaro.org> 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

lp://qastaging/~pfalcon/linaro-android-build-tools/validate-build-conf updated on 2012-04-24

452. By Paul Sokolovsky on 2012-04-24: Import optparse
453. By Paul Sokolovsky on 2012-04-24: Typo fix
454. By Paul Sokolovsky on 2012-04-24: Make sure that /var/run/build-tools exists.
455. By Paul Sokolovsky on 2012-04-24: Handle short string properly.
456. By Paul Sokolovsky on 2012-04-24: Be sure to base64-decode initial config from jenkins.

Revision history for this message

Paul Sokolovsky (pfalcon) wrote on 2012-04-24:

#

Download full text (10.2 KiB)

On Mon, 23 Apr 2012 16:34:17 -0000
James Tunnicliffe <email address hidden> wrote:

> 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 already proposed to expand it to 132 chars, don't remember anyone
objecting.

I'll run code thru pep8.

> 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 :-)

Fixed.

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

Thanks, that code is more easy to grasp, no need to hide the primitive
impl. behind not immediately graspable regexps.

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

On Mon, 23 Apr 2012 16:34:17 -0000
James Tunnicliffe <launchpad@nanosheep.org> wrote:

> 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 already proposed to expand it to 132 chars, don't remember anyone
objecting.

I'll run code thru pep8.

>  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 <paul.sokolovsky@linaro.org>
> 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 :-)

Fixed.

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

Thanks, that code is more easy to grasp, no need to hide the primitive
impl. behind not immediately graspable regexps.

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

That's far too new syntax to rely on it IMHO, details of its semantics
unlikely well known by general public, so IMHO, makes sense to stay
away from it. File closure issue fixed, even though this is
short-running script, thus there's no resource leak, and python/OS are
guaranteed to close them for us, so IMHO it's ok to rely on that.

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

It will fail with exception and stacktrace, that's enough.

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

How it makes writing tests much easier, if it for starters requires
"turning into class and splitting into several functions"? That's
rather complicated, taking into account that it doesn't add much to
test semantics per se. Enter nosetests
(http://readthedocs.org/docs/nose/en/latest/) - tool which really makes
testing easier, to not require any extra syntactic noise at all ;-)
(but it can eat it too). That test is written with that too in mind, it
gives all the niceties test runner does.

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

Not so obvious.

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

Yeah, many more tests would be good, but taking into account that
linaro-android-build-tools had 0 tests before, that's a good start. We
need to have a BP for adding tests to it.

Thanks!

-- 
Best Regards,
Paul

Linaro.org | Open source software for ARM SoCs
Follow Linaro: http://www.facebook.com/pages/Linaro
http://twitter.com/#!/linaroorg - http://www.linaro.org/linaro-blog

lp://qastaging/~pfalcon/linaro-android-build-tools/validate-build-conf updated on 2012-04-24

457. By Paul Sokolovsky on 2012-04-24: Fix comment
458. By Paul Sokolovsky on 2012-04-24: Close file.

Revision history for this message

Paul Sokolovsky (pfalcon) wrote on 2012-04-24:

#

Both pep8 --ignore=E501 and pyflakes are ok with the code now (and only minor double-line issues were before).

review: Approve

lp://qastaging/~pfalcon/linaro-android-build-tools/validate-build-conf updated on 2012-04-24

459. By Paul Sokolovsky on 2012-04-24: PEP8 double-lining.

Revision history for this message

Paul Sokolovsky (pfalcon) wrote on 2012-04-24:

#

Test build using this branch: https://android-build.linaro.org/builds/~pfalcon/panda-ics-gcc47-tilt-tracking-blob/#build=7

lp://qastaging/~pfalcon/linaro-android-build-tools/validate-build-conf updated on 2012-04-24

460. By Paul Sokolovsky on 2012-04-24: Wrap comments a bit/fix typos.
461. By Paul Sokolovsky on 2012-04-24: Update restricted slave name
462. By Paul Sokolovsky on 2012-04-24: Remove newline from slave type file.
463. By Paul Sokolovsky on 2012-04-24: [merge] Merge in main branch

Revision history for this message

Georgy Redkozubov (gesha) wrote on 2012-04-24:

#

Looks good

review: Approve

Revision history for this message

Paul Sokolovsky (pfalcon) wrote on 2012-04-24:

#

Thanks. Verified for both normal and restricted build (https://android-build.linaro.org/builds/~linaro-android-restricted/test-buildconf-validate/#build=3), time to deploy.

Linaro Android Build Tools

Merge lp://qastaging/~pfalcon/linaro-android-build-tools/validate-build-conf into lp://qastaging/linaro-android-build-tools

Commit message

Description of the change

Preview Diff

Subscribers