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