Hey Milo,

Thanks for putting the extra effort to make this happen.  This includes
James as well :)

Overall, the changes are great.

My main gripe is that I am seeing a huge branch like this.  I still
can't see why this was not done in several branches: one to parse the
new hwpack config, another to produce the new hwpacks and the third one
to port l-m-c to use them.

Also, a BP workitems are still out of date I believe.

> === modified file 'linaro-hwpack-convert'
> --- linaro-hwpack-convert	2012-07-19 15:21:06 +0000
> +++ linaro-hwpack-convert	2012-07-20 14:12:31 +0000
> @@ -31,6 +31,7 @@
>      HwpackConverterException,
>      check_and_validate_args,
>      )
> +from linaro_image_tools.hwpack.builder import ConfigFileMissing
>  from linaro_image_tools.__version__ import __version__

This change looks unnecessary (judging by the diff itself).  If it's
not, something was badly broken before :)
 
> === modified file 'linaro-hwpack-install'
...
> @@ -111,6 +111,68 @@
>  tar zxf "$HWPACK_TARBALL" -C "$HWPACK_DIR"
>  echo "Done"
>  
> +function query_v3_metadata {
> +  python -c "import re
> +with open('${HWPACK_DIR}/metadata') as configv3:
> +    config = {}  # Will store decoded YAML in here
> +    root = config   # Current insert point for adding data
> +    root_at_indent = {}
> +    indent = 0
> +    for line in configv3.readlines():
> +        key_value = re.search('^(\s*)(\S.+):\s+(.+)\s*$', line)
> +        key_match = re.search('^(\s*)(\S.+):\s*$', line)
> +        list_item = re.search('^(\s*)-\s*(.+)\s*$', line)
> +
> +        if key_value:
> +            new_indent = len(key_value.group(1))
> +        elif key_match:
> +            new_indent = len(key_match.group(1))
> +        elif list_item:
> +            new_indent = len(list_item.group(1))
> +
> +        if new_indent < indent:  #Indent decreases: go back up config structure
> +            root = root_at_indent[new_indent]
> +        elif new_indent > indent:  # Indent increases: reset root (insert point)
> +            root_at_indent[indent] = root
> +            root = root[key]
> +        indent = new_indent
> +
> +        if key_value:  # key: value
> +            key = key_value.group(2)
> +            root[key] = key_value.group(3)
> +        elif key_match:  # key:
> +            key = key_match.group(2)
> +            root[key] = {}
> +        elif list_item:  # - value
> +            # If the list has extra indentation then root == {}
> +            # If the list doesn't have extra indentation, root = {key: {}}
> +            # We need to create a list in that empty dictionary. Work out
> +            # where it is, assign it to insert_point.
> +            insert_point = None
> +            if root == {}:
> +                insert_point = root
> +            keys = root.keys()
> +            if len(keys) == 1:
> +                insert_point = root[keys[0]]
> +
> +            if insert_point == {}:
> +                insert_point[''] = []
> +
> +            insert_point[''].append(list_item.group(2))
> +
> +    keys = '$1'.split(' ')
> +    for key in keys:
> +        if isinstance(config, list):
> +            key = int(key)
> +        config = config[key]
> +        if(isinstance(config, dict) and
> +           '' in config and
> +           isinstance(config[''], list)):
> +            config = config['']
> +    print config
> +  "
> +}

Since we have a requirement for Python, I don't think it's too much to
add a depedency on python-yaml as well: it's not a big deal, and should
allow us to simplify the code above by a significant margin.

Especially since we are using only a few top-level config values.

> @@ -125,7 +187,17 @@
>        "Try using a newer version of $(basename $0)."
>  
>  # Check the architecture of the hwpack matches that of the host system.
> -HWPACK_ARCH=`grep ARCHITECTURE "${HWPACK_DIR}/metadata" | cut -d "=" -f2`
> +HWPACK_VERSION=`grep VERSION "${HWPACK_DIR}/metadata" | cut -d "=" -f2`
> +if [ "$HWPACK_VERSION" = "" ]; then
> +  HWPACK_VERSION=$(query_v3_metadata 'version')
> +fi
> +

Any reason not to use $hwpack_format above, just like it's done below?
(I'd understand it if we used HWPACK_VERSION below as well, but like
this, it doesn't make much sense to me)

> +if [ "$hwpack_format" = "3.0" ]; then
> +  HWPACK_ARCH=$(query_v3_metadata 'architecture')
> +else
> +  HWPACK_ARCH=`grep ARCHITECTURE "${HWPACK_DIR}/metadata" | cut -d "=" -f2`
> +fi
> +
>  [ "$HWPACK_ARCH" == `dpkg --print-architecture` ] || \
>    die "Hardware pack architecture ($HWPACK_ARCH) does not match the host's architecture"
>  
> @@ -209,8 +281,12 @@
>  # For "older" hwpacks that don't have a dependency package, we just
>  # manually install the contents of the hwpack.
>  
> -HWPACK_NAME=`grep NAME "${HWPACK_DIR}/metadata" | cut -d "=" -f2`
> -HWPACK_VERSION=`grep VERSION "${HWPACK_DIR}/metadata" | cut -d "=" -f2`
> +if [ "hwpack_format" = "3.0" ]; then
> +  HWPACK_NAME=$(query_v3_metadata 'name')
> +else
> +  HWPACK_NAME=`grep NAME "${HWPACK_DIR}/metadata" | cut -d "=" -f2`
> +fi
> +
>  dependency_package="hwpack-${HWPACK_NAME}"
>  if grep -q "^${dependency_package}=${HWPACK_VERSION}\$" "${HWPACK_DIR}"/manifest; then
>    DEP_PACKAGE_PRESENT="yes"
> 
> === modified file 'linaro_image_tools/hwpack/builder.py'
> --- linaro_image_tools/hwpack/builder.py	2012-06-13 14:53:32 +0000
> +++ linaro_image_tools/hwpack/builder.py	2012-07-20 14:12:31 +0000
> @@ -36,6 +36,13 @@
>      PackageFetcher,
>      )
>  
> +from linaro_image_tools.hwpack.hwpack_fields import (
> +    PACKAGE_FIELD,
> +    SPL_PACKAGE_FIELD,
> +)
> +
> +PACKAGE_FIELDS = [PACKAGE_FIELD, SPL_PACKAGE_FIELD]
> +
>  
>  logger = logging.getLogger(__name__)
>  
> @@ -110,6 +117,22 @@
>              package.filepath, wanted_file)
>          return hwpack.add_file(target_path, tempfile_name)
>  
> +    def loop_bootloaders(self, dictionary):
> +        """Loop through the bootloaders dictionary searching for packages
> +        that should be installed, based on known keywords.
> +        
> +        :param dictionary: The bootloaders dictionary to loop through.

Is the "dictionary" actually a bootloaders config section?  If it is,
what do you think of naming the method argument "bootloaders_config"?

> +        :return A list of packages, without duplicates."""
> +        boot_packages = []
> +        for key, value in dictionary.iteritems():
> +            if isinstance(value, dict):
> +                boot_packages.extend(self.loop_bootloaders(value))

Wow, do we actually support recursive nesting of bootloaders?  Also,
this method would probably be better named "find_bootloader_packages".

...
> === modified file 'linaro_image_tools/hwpack/config.py'
...
> -    def __init__(self, fp):
> +    translate_v2_to_v3[UBOOT_DD_KEY] = DD_FIELD
> +
> +    def __init__(self, fp, bootloader=None, board=None):
>          """Create a Config.
>  
>          :param fp: a file-like object containing the configuration.
>          """

This constructor seems to do too much: it'd be nice if we split it out
into a separate method or two (to allow easier testing, among other
things), but it's not a big deal.

> +    def _yes_no(self, value):
> +        """Convert value, treated as boolean, to string "yes" or "no"."""

I'd prefer all the methods to have a name that reflects what they do:
"bool_to_string" might be better.

> +
> +    def _addr(self, value):
> +        """Convert value to 8 character hex string"""

No reason to shorten this (it only saves a few characters).
"hex_address" sounds better to me.

> +    def _get_v3_option(self, keys):
> +        """Find value in config dictionary based on supplied list (keys)."""
> +        result = self.parser
> +        for key in keys:
> +            key = self._v2_key_to_v3(key)
> +            try:
> +                result = result.get(key)
> +                if result == None:  # False is a valid boolean value...

Please do not use inline comments.  Also, I am not sure I understand the
comment :)

> +                    return None
> +            except ConfigParser.NoOptionError:
> +                return None

I doubt we ever get here.  result.get(key) should, by default, return
None if the key doesn't exist.  If you want to make this more explicit,
you can instead pass the default value in:

  result = result.get(key, None)


> +        return result
> +
> +    def get_option(self, name):
> +        """Return the value of an attribute by name.
> +
> +        Used when you can't use a property.
> +        """
> +        return attrgetter(name)(self)

Is it not simpler to do 'getattr(self, name)'?  FWIW, that might replace
the need for this method altogether :)

> +
> +    def _get_option(self, key, join_list_with=False, convert_to=None):
> +        """Return value for the given key. Precedence to board specific values.

This method is too long (harder to test, easier to break), but let's
leave it as is for now.

Also, we should not be accepting multiple types for join_list_with (i.e.
bools and strings).  No reason not to have join_list_with as None and
use "is (not) None" in boolean checks.

> @@ -624,36 +850,40 @@
>          if len(serial_tty) < 4 or serial_tty[:3] != 'tty':
>              raise HwpackConfigError("Invalid serial tty: %s" % serial_tty)
>  
> -    def _validate_addr(self, addr):
> -        return re.match(r"^0x[a-fA-F0-9]{8}$", addr)
> +    def _validate_addr(self, key):
> +        """Validate the address for the given key.
> +        Assumptions:
> +            1. key name is of the form name_addr
> +            2. property name matches key name
> +
> +        Currently these assumptions are met and it seems reasonable to place
> +        these restrictions on future code.
> +        """
> +        name = re.sub("_addr", "", key)
> +
> +        try:
> +            addr = attrgetter(key)(self)

And then we use attrgetter directly instead of the get_option :)  I
still believe getattr(self, key) would be slightly faster.

Nice clean-up, btw, though these things should have gone in a separate
branch before this one.


> === modified file 'linaro_image_tools/hwpack/hardwarepack.py'
> @@ -127,6 +166,10 @@
>          self.samsung_bl2_len = samsung_bl2_len
>  
>      @classmethod
> +    def add_v3_config(self, bootloaders):
> +        self.bootloaders = bootloaders

This is entirely unclear.  What's going on here?

Please add a docstring.  Btw, are not the number of boards another new
thing here?

> +    def loop_through_dictionary(self, dictionary, search_key, new_value):
> +        """Loop recursively thorugh a dictionary looking for the specified key
> +        substituting its value.

A typo: "thorugh".

I dislike the name here as well.  What this does is set a config value
in a recursive dict structure.  How about we name it along those lines?

Also, I wonder how this works when we do have multiple bootloaders
defined, eg.

  bootloaders:
    uefi1:
      boot_package: blah
    uefi2:
      boot_package: foo

and we try to set the value of "boot_package"?

(similar probably holds for the loop_bootloader method all the way at
the top of the diff)

> +
> +        :param dictionary: The dictionary to loop through.
> +        :param serach_key: The key to search.

"search_key".

> +        :param new_value: The new value for the key.
> +        """
> +        # XXX Probably better check what should happen if the key we are
> +        # looking for is not there, but the value is set. Should we set it
> +        # anyway? If yes, where?

I am not sure how can a value be set if the key is not there.

> +        for key, value in dictionary.iteritems():
> +            if key == search_key:
> +                dictionary[key] = new_value
> +                break
> +            elif isinstance(value, dict):
> +                self.loop_through_dictionary(value, search_key, new_value)

Judging by the use of it, perhaps we don't have to guard against the
problems I mention above, but then it is most important to get at least
some documentation about this in the docstring.

> === modified file 'linaro_image_tools/hwpack/hardwarepack_format.py'
> --- linaro_image_tools/hwpack/hardwarepack_format.py	2012-06-13 14:26:02 +0000
> +++ linaro_image_tools/hwpack/hardwarepack_format.py	2012-07-20 14:12:31 +0000
> @@ -57,3 +57,12 @@
>          self.is_supported = True
>          self.is_deprecated = False
>          self.has_v2_fields = True
> +
> +
> +class HardwarePackFormatV3(HardwarePackFormat):
> +    def __init__(self):
> +        super(HardwarePackFormatV3, self).__init__()
> +        self.format_as_string = "3.0"
> +        self.is_supported = True
> +        self.is_deprecated = False
> +        self.has_v2_fields = True

Let's set is_deprecated for v2 formats, though let's just file a bug to
do that next cycle after v3 stabilises a bit.  At that same time, we
should get rid of v1 format support since that's been deprecated for
quite a while now.

 review needs-fixing

Since I am not around on Monday, please just get someone to ack the
changes and then get this landed and l-i-t released.

Cheers,
Danilo