Code review comment for lp://qastaging/~milo/linaro-image-tools/multiple-boards-support

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