Merge into trunk : multiple-boards-support : Code : Linaro Image Tools

Status:	Merged
Merged at revision:	537
Proposed branch:	lp://qastaging/~milo/linaro-image-tools/multiple-boards-support
Merge into:	lp://qastaging/linaro-image-tools/11.11
Diff against target:	3318 lines (+1824/-388) 15 files modified linaro-hwpack-install (+20/-5) linaro_image_tools/hwpack/builder.py (+35/-4) linaro_image_tools/hwpack/config.py (+490/-205) linaro_image_tools/hwpack/hardwarepack.py (+194/-25) linaro_image_tools/hwpack/hardwarepack_format.py (+9/-0) linaro_image_tools/hwpack/hwpack_convert.py (+15/-3) linaro_image_tools/hwpack/hwpack_fields.py (+3/-0) linaro_image_tools/hwpack/tests/__init__.py (+1/-0) linaro_image_tools/hwpack/tests/test_config.py (+10/-8) linaro_image_tools/hwpack/tests/test_config_v3.py (+738/-0) linaro_image_tools/hwpack/tests/test_hardwarepack.py (+177/-86) linaro_image_tools/hwpack/tests/test_hwpack_converter.py (+24/-0) linaro_image_tools/media_create/boards.py (+27/-27) linaro_image_tools/media_create/chroot_utils.py (+12/-2) linaro_image_tools/media_create/tests/test_media_create.py (+69/-23)
To merge this branch:	bzr merge lp://qastaging/~milo/linaro-image-tools/multiple-boards-support
Related bugs:	Link a bug report

Reviewer	Date Requested	Status
Данило Шеган (community)	2012-07-20	Needs Fixing on 2012-07-21
Stevan Radaković	2012-07-20	Pending
Linaro Infrastructure	2012-07-20	Pending
Review via email: mp+115991@code.qastaging.launchpad.net

Revision history for this message

Данило Шеган (danilo) wrote on 2012-07-21:

#

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

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

review: Needs Fixing

lp://qastaging/~milo/linaro-image-tools/multiple-boards-support updated on 2012-07-23

543. By Milo Casagrande on 2012-07-23: Removed unnecessary import.
544. By Milo Casagrande on 2012-07-23: Renamed method and args.

Revision history for this message

Paul Sokolovsky (pfalcon) wrote on 2012-07-23:

#

I can relate to Danilo's note that this merge request is huge. I had a look by the end of day on Friday, I thought I'd take quite a time to review it.

Some comments (not necessarily shows a problem, just something which caught my attention, partly may be due to fact that I'm reading diff vs flat code).

1. Inline python code in shell script linaro-hwpack-install looks a bit hacky IMHO. That's why I'm telling me "don't hack with shell for anything serious, start with Python right away". Nope, I understand that rewriting the whole script would add just more diff noise, but I hope the alternative of making this a separate aux script was considered. And I'd at least add comment for "keys = '$1'.split(' ')" line that $1 is being replaced by shell (and probably at the beginning of sub-script that doublequotes should not be used).

2. hwpack/config.py: I'd add comments for the purpose of translate_v2_to_v3/translate_v2_metadata. Also, IMHO initting them in one go with literal dictionary (vs multiple subscript assignments spread around) would make it clearer. (Again, may be artifact of me looking at the diff.)

3. obfuscated_e and friends - thumbs up for initing it a the beginning of the function, but you touched it in may lines already, so might have been possible to rename it to something, well, less obfuscated ;-).

4. 1459 + """Loop recursively thorugh a dictionary looking for the specified key

Typo ("thorugh")

:param serach_key: The key to search.

Ditto

5. hwpack/tests/test_hardwarepack.py: I'm not sure what's the logic applied there, but many of the changes appear to be semantically-null, just code reformatting, or put another way, spurious changes in the contexts of the functionality being implemented and already large patch size.

Revision history for this message

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

#

On 21 July 2012 07:55, Данило Шеган <email address hidden> wrote:

Thanks for the review. Enjoy your break :-)

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

The problem is that we need to bootstrap package installation. A few
lines down we actually add apt sources that would allow us to install
python-yaml, which then brings with it several more packages. We start
off from an ubuntu-minimal image (I think) and that doesn't seem to
have a full python install.

The following extra packages will be installed:
  libexpat1 libyaml-0-2 mime-support python python2.7
Suggested packages:
  python-doc python-tk python2.7-doc binutils
The following NEW packages will be installed:
  libexpat1 libyaml-0-2 mime-support python python-yaml python2.7
0 upgraded, 6 newly installed, 0 to remove and 99 not upgraded.
Need to get 2942 kB/3045 kB of archives.
After this operation, 9796 kB of additional disk space will be used.
WARNING: The following packages cannot be authenticated!
  libexpat1

We can't install these packages before we need python-yaml because we
inspect the metadata file for the architecture that the hardware pack
supports is the same as the architecture that dpkg is reporting.

The idea was to get enough YAML parsing done to pull out the values we
need in the shell script before re-implementing it in python
completely. It looks like it would have to be quite minimal python,
but that shouldn't be a problem (and probably make it a lot easier to
debug).

There is an alternative solution, which I hadn't thought of before -
we could pass parameters to linaro-hwpack-install from the main
linaro-media-create process (where we have a full python install and
all our config parsing magic) for the values we pull out of the
configuration files. Should be the least error prone and most reliable
way of getting those 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)

Good point.

Will respond to other points after I have tidied this up.

--
James Tunnicliffe

On 21 July 2012 07:55, Данило Шеган <danilo@canonical.com> wrote:

Thanks for the review. Enjoy your break :-)

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

The problem is that we need to bootstrap package installation. A few
lines down we actually add apt sources that would allow us to install
python-yaml, which then brings with it several more packages. We start
off from an ubuntu-minimal image (I think) and that doesn't seem to
have a full python install.

The following extra packages will be installed:
  libexpat1 libyaml-0-2 mime-support python python2.7
Suggested packages:
  python-doc python-tk python2.7-doc binutils
The following NEW packages will be installed:
  libexpat1 libyaml-0-2 mime-support python python-yaml python2.7
0 upgraded, 6 newly installed, 0 to remove and 99 not upgraded.
Need to get 2942 kB/3045 kB of archives.
After this operation, 9796 kB of additional disk space will be used.
WARNING: The following packages cannot be authenticated!
  libexpat1

We can't install these packages before we need python-yaml because we
inspect the metadata file for the architecture that the hardware pack
supports is the same as the architecture that dpkg is reporting.

The idea was to get enough YAML parsing done to pull out the values we
need in the shell script before re-implementing it in python
completely. It looks like it would have to be quite minimal python,
but that shouldn't be a problem (and probably make it a lot easier to
debug).

There is an alternative solution, which I hadn't thought of before -
we could pass parameters to linaro-hwpack-install from the main
linaro-media-create process (where we have a full python install and
all our config parsing magic) for the values we pull out of the
configuration files. Should be the least error prone and most reliable
way of getting those 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)

Good point.

Will respond to other points after I have tidied this up.

-- 
James Tunnicliffe

Revision history for this message

Milo Casagrande (milo) wrote on 2012-07-23:

#

Download full text (6.7 KiB)

Hello Danilo,

thanks for the review!

On Sat, Jul 21, 2012 at 8:55 AM, Данило Шеган <email address hidden> wrote:
>
> 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 :)

It is unnecessary, indeed.
Fixed and pushed.

>> 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"?

Yes, it is the bootloaders section. Renamed to clearly state that.

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

Hmmm... kind of. The YAML structure could be something like this too:

boards:
a_board:
  bootloaders: # this is specific for the board
   u_boot:
    file: a_file
bootloaders: # this is external, and is general for the whole hwpack config
uefi:
  pacakge: a
uboot:
  package: b

so we have multiple bootloaders section, each more specific than the others.

> Also,
> this method would probably be better named "find_bootloader_packages".

Done, and pushed.

>> === 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?

Basically, that method is called only during the construction of the
metadata file, starting from a Config object, during the build of the
hwpack archive. The args of that method should be specific to the v3
of the hwpack config.
If we are using a v3 hwpack config converted from a v2 config, we
might have a bootloaders section in the file (since it will not have a
boards section).

> Please add a docstring. Btw, are not the number of boards another new
> th...

Hello Danilo,

thanks for the review!

On Sat, Jul 21, 2012 at 8:55 AM, Данило Шеган <danilo@canonical.com> wrote:
>
> 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 :)

It is unnecessary, indeed.
Fixed and pushed.

>>  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"?

Yes, it is the bootloaders section. Renamed to clearly state that.

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

Hmmm... kind of. The YAML structure could be something like this too:

boards:
 a_board:
  bootloaders: # this is specific for the board
   u_boot:
    file: a_file
bootloaders: # this is external, and is general for the whole hwpack config
 uefi:
  pacakge: a
 uboot:
  package: b

so we have multiple bootloaders section, each more specific than the others.

> Also,
> this method would probably be better named "find_bootloader_packages".

Done, and pushed.

>> === 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?

Basically, that method is called only during the construction of the
metadata file, starting from a Config object, during the build of the
hwpack archive. The args of that method should be specific to the v3
of the hwpack config.
If we are using a v3 hwpack config converted from a v2 config, we
might have a bootloaders section in the file (since it will not have a
boards section).

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

Docstring added, for the boards part that it was missing, I created
another branch. Will merge it later after this one has been reviewed.

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

The previous loop should not be affected by this problem, since what
we need from that, is just a list of the packages that need to be
downloaded. In this one we need to change values.

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

Since we are changing basically two predefined fields at the moment
(spl and u_boot), my fear is that someone places the fields outside of
the bootloaders section (mostly the spl one), and when we check and
loop:
- the value is set,
- but the key is not in our dictionary of the bootloaders, where it should be

We do not really have a "checker" or validator for a hwpackv3 YAML based file.

Another problem I see with this one, is that the Config class does not
really have a clear separation between v2 and v3 style (and the same
happens with metadata; metadata that could be easily included in the
Config, since it is a representation of that), and we are carrying
information of both version in just one class. In v2, those values are
just normal values in a "plain" file, but in v3 they are part of the
bootloaders section (nested).

What happens here is that when creating the hwpack archive, and
writing the metadata file, those values are re-set, directly inside
the metadata, with a new "translated" value.

So, we start from a hwpack config file with something like this:
spl_file = usr/lib/u-boot/omap4_panda/MLO

And we end up in the metadata file with this:
spl_file = spl/MLO
(and in the old metadata this was converted into 'spl=spl/MLO',
translating also the key name, not just the value)

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

Actually, we should guard.
I reworked the section in a separate branch, and instead of looping at
that point, we loop directly in the builder, convert the values there,
and re-set the metadata variable.

Will address the other points in a separate mail.

-- 
Milo Casagrande
Infrastructure Engineer
Linaro.org <www.linaro.org> │ Open source software for ARM SoCs

lp://qastaging/~milo/linaro-image-tools/multiple-boards-support updated on 2012-07-23

545. By Milo Casagrande on 2012-07-23: Fixed method name, typos, removed unnecessary comments.

Revision history for this message

Milo Casagrande (milo) wrote on 2012-07-23:

#

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

Bug opened, it is 1027903.

lp://qastaging/~milo/linaro-image-tools/multiple-boards-support updated on 2012-07-23

546. By Milo Casagrande on 2012-07-23: Renamed method.
547. By Milo Casagrande on 2012-07-23: Fixed method name.
548. By Milo Casagrande on 2012-07-23: Fixed _get_v3_option.

Revision history for this message

Milo Casagrande (milo) wrote on 2012-07-23:

#

> > === modified file 'linaro_image_tools/hwpack/config.py'
>
> > + 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.

Fixed.

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

Fixed that too.

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

This should now be fixed. Also, worth noting, we should never get a ConfigParser type of error, since there we are on a v3 hwpack, so it should be a YAML parser.

lp://qastaging/~milo/linaro-image-tools/multiple-boards-support updated on 2012-07-23

549. By Milo Casagrande on 2012-07-23: Merged from James.
550. By Milo Casagrande on 2012-07-23: Merged from James.

Revision history for this message

Milo Casagrande (milo) wrote on 2012-07-23:

#

With the latest merge with James fixes, basically all of the previous concerns have been addressed.
The only thing missing, but that does not change functionalities, is the getattr part, that we didn't really get what should be done.

Since there is nothing big, we will do the merge.

Linaro Image Tools

Merge lp://qastaging/~milo/linaro-image-tools/multiple-boards-support into lp://qastaging/linaro-image-tools/11.11

Commit message

Description of the change

Preview Diff

Subscribers