Merge lp:~salgado/linaro-image-tools/check-hwpack-format into lp:linaro-image-tools/11.11

[Guilherme Salgado]

Check that the hwpack's format is supported before trying to do anything with
it.

Fixes bug 638322

[James Westby (james-w)]

Hi,

Looks good to me. I just wonder what

for format in $SUPPORTED_FORMATS; do

requires SUPPORTED_FORMATS to be set to when there are multiple. Does
space-separated work, or does it need to be newline-separated?

Maybe want to put a hint about trying a newer linaro-hwpack-install
script in the error message?

Thanks,

James

review: Approve

[Guilherme Salgado (salgado)]

On Fri, 2010-10-08 at 02:16 +0000, James Westby wrote:
> Review: Approve
> Hi,
>
> Looks good to me. I just wonder what
>
> for format in $SUPPORTED_FORMATS; do
>
> requires SUPPORTED_FORMATS to be set to when there are multiple. Does
> space-separated work, or does it need to be newline-separated?

By default it will work with space/tab/newline separated values as
that's the default value for $IFS, but now I'm wondering if it's ok to
rely on that or if we should explicitly set it to whatever we want
(space, in this case) around the for loop.

>
> Maybe want to put a hint about trying a newer linaro-hwpack-install
> script in the error message?

That sounds like a good idea; I'll do it.

[lp:~salgado/linaro-image-tools/check-hwpack-format]

135. By Guilherme Salgado on 2010-10-13

Tell users to try a newer hwpack-install script when the format is not supported

136. By Guilherme Salgado on 2010-10-18

Ensure the format version of hwpacks don't contain white spaces as that is not necessary and could break linaro-hwpack-install

[Guilherme Salgado (salgado)]

James, r136 has that change we discussed on IRC to make sure white space is not used in the format version of hwpacks. Please let me know if that's ok and I'll (finally) merge this

[James Westby (james-w)]

The version you are checking there is the version, not that format version.

However, it is a good check to have anyway :-)

I think an AssertionError at build time if the self.FORMAT contains whitespace
would be good too.

Sorry for the delay in getting back to you on this.

Thanks,

James

review: Needs Fixing

[Guilherme Salgado (salgado)]

You mean something like

=== modified file 'hwpack/builder.py'
--- hwpack/builder.py 2010-09-14 18:09:01 +0000
+++ hwpack/builder.py 2010-11-05 17:20:13 +0000
@@ -31,6 +31,9 @@
             metadata = Metadata.from_config(
                 self.config, self.version, architecture)
             hwpack = HardwarePack(metadata)
+ if ' ' in hwpack.FORMAT:
+ raise AssertionError(
+ 'Format version cannot have white spaces')
             sources = self.config.sources
             hwpack.add_apt_sources(sources)
             fetcher = PackageFetcher(

?

There's something about it that feels unnatural to me. Maybe it's the
fact that we're asserting on a class variable, or maybe it's because the
HardwarePack was just created and the fact that we have to assert right
after that kind of means we don't have a good contract (as we don't
really know what we'll get back).

I wouldn't feel that way if we made FORMAT a private attribute (_FORMAT)
and created a property which asserts it has no space before returning.
But that sounds like overkill to me as the FORMAT is really a constant
that should not be changed unless you really know what you're doing.

Maybe we could have just a test asserting that HardwarePack.FORMAT has
no spaces?

[James Westby (james-w)]

> You mean something like
>
> === modified file 'hwpack/builder.py'
> --- hwpack/builder.py 2010-09-14 18:09:01 +0000
> +++ hwpack/builder.py 2010-11-05 17:20:13 +0000
> @@ -31,6 +31,9 @@
> metadata = Metadata.from_config(
> self.config, self.version, architecture)
> hwpack = HardwarePack(metadata)
> + if ' ' in hwpack.FORMAT:
> + raise AssertionError(
> + 'Format version cannot have white spaces')
> sources = self.config.sources
> hwpack.add_apt_sources(sources)
> fetcher = PackageFetcher(
>
> ?
>
> There's something about it that feels unnatural to me. Maybe it's the
> fact that we're asserting on a class variable, or maybe it's because the
> HardwarePack was just created and the fact that we have to assert right
> after that kind of means we don't have a good contract (as we don't
> really know what we'll get back).
>
> I wouldn't feel that way if we made FORMAT a private attribute (_FORMAT)
> and created a property which asserts it has no space before returning.
> But that sounds like overkill to me as the FORMAT is really a constant
> that should not be changed unless you really know what you're doing.

Right, my concern is the separation between when you change this, and the
actual affect that is has. You can change the FORMAT, and then happily build
hwpacks and not realise the mistake until you go and try and enable that format
in hwpack-install and try and install the hwpack.

Given that you are likely to do both together, and shouldn't be pushing out
hwpacks without testing, I don't think this is that big of a deal, so I'm
happy to leave out this change.

> Maybe we could have just a test asserting that HardwarePack.FORMAT has
> no spaces?

That would work too.

Thanks,

James

=== modified file 'hwpack/hardwarepack.py'
--- hwpack/hardwarepack.py	2010-09-15 19:24:29 +0000
+++ hwpack/hardwarepack.py	2010-10-18 12:34:41 +0000
@@ -15,7 +15,8 @@
 
     :ivar name: the name of the hardware pack.
     :type name: str
-    :ivar version: the version of the hardware pack.
+    :ivar version: the version of the hardware pack. It must not contain white
+        spaces.
     :type version: str
     :ivar origin: the origin of the hardware pack, or None if the origin
         is not known.
@@ -35,6 +36,10 @@
         See the instance variables for a description of the arguments.
         """
         self.name = name
+        if ' ' in version:
+            raise AssertionError(
+                'Hardware pack format version must not contain white '
+                'spaces: "%s"' % version)
         self.version = version
         self.origin = origin
         self.maintainer = maintainer
@@ -86,6 +91,7 @@
     :type FORMAT: str
     """
 
+    # The format version cannot contain white spaces. 
     FORMAT = "1.0"
     FORMAT_FILENAME = "FORMAT"
     METADATA_FILENAME = "metadata"
 
=== modified file 'hwpack/tests/test_hardwarepack.py'
--- hwpack/tests/test_hardwarepack.py	2010-09-15 19:24:29 +0000
+++ hwpack/tests/test_hardwarepack.py	2010-10-18 12:34:41 +0000
@@ -18,6 +18,10 @@
         metadata = Metadata("ahwpack", "3", "armel")
         self.assertEqual("3", metadata.version)
 
+    def test_version_with_whitespace(self):
+        self.assertRaises(
+            AssertionError, Metadata, "ahwpack", "3 (with extras)", "armel")
+
     def test_architecture(self):
         metadata = Metadata("ahwpack", "3", "armel")
         self.assertEqual("armel", metadata.architecture)
@@ -93,7 +97,7 @@
 
     def setUp(self):
         super(HardwarePackTests, self).setUp()
-        self.metadata = Metadata("ahwpack", 4, "armel")
+        self.metadata = Metadata("ahwpack", "4", "armel")
 
     def test_format_is_1_0(self):
         hwpack = HardwarePack(self.metadata)
 
=== modified file 'linaro-hwpack-install'
--- linaro-hwpack-install	2010-10-14 14:55:00 +0000
+++ linaro-hwpack-install	2010-10-18 12:34:41 +0000
@@ -28,6 +28,7 @@
 INSTALL_LATEST="no"
 SOURCES_LIST_FILE="${TEMP_DIR}/sources.list"
 APT_GET_OPTIONS="Dir::Etc::SourceList=${SOURCES_LIST_FILE}"
+SUPPORTED_FORMATS="1.0"  # A space-separated list of hwpack formats.
 
 die() {
   echo -e "$@"
@@ -75,6 +76,19 @@
 tar zxf "$HWPACK_TARBALL" -C "$HWPACK_DIR"
 echo "Done"
 
+# Check the format of the hwpack is supported.
+hwpack_format=$(cat ${HWPACK_DIR}/FORMAT)
+supported="false"
+for format in $SUPPORTED_FORMATS; do
+  if [ $hwpack_format == $format ]; then
+    supported="true"
+    break
+  fi
+done
+[ $supported == "true" ] || \
+  die "Unsupported hwpack format: $hwpack_format. "\
+      "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_ARCH" == `dpkg --print-architecture` ] || \