Merge lp:~mabac/linaro-image-tools/bug-829220 into lp:linaro-image-tools/11.11

[Mattias Backman]

Hi,

This branch changes the format of the Snowball startfiles.cfg format a little bit.

L-m-c will now assume that the filename in the config file is an absolute path to the file on local file system. If that file does not exist, we fall back to assuming that the filename is relative to chroot/bin as is the case before this change.

This should make it backwards compatible for existing hwpacks but let anyone creating a Snowball hwpack specifiy absolute paths in case the files won't be installed to /boot on the target.

Also, if no interpretation of the path is found, we fall back to assuming that the filename is relative to /usr/lib/u-boot/u8500_snowball to support a hwpack where the uboot package does not link the binary in /boot. This should be removed asap or not merged if a fixed hwpack can be created.

Thanks,

Mattias

[Loïc Minier (lool)]

On Mon, Aug 22, 2011, Mattias Backman wrote:
> This branch changes the format of the Snowball startfiles.cfg format a
> little bit.
>
> L-m-c will now assume that the filname in the config file is an
> absolute path to the file on local file system. If that file does not
> exist, we fall back to assuming that the filename is relative to
> chroot/bin as is the case before this change.

 So the hwpack doesn't transport the files anymore? It seems to be the
 wrong thing to encourage

 Perhaps we could support looking up files in the hwpack tarball, or in
 a .deb in the hwpack tarball even if that isn't installed; e.g.
 startfiles.cfg would point at hwpack://xyz.bin or at
 hwpack+deb://xyz-files.deb/boot/xyz.bin?

--
Loïc Minier

[lp:~mabac/linaro-image-tools/bug-829220]

419. By Mattias Backman on 2011-08-22

Remove hacky extra check. Fix absolute path to be relative to the chroot and not system root.

[Mattias Backman (mabac)]

> So the hwpack doesn't transport the files anymore? It seems to be the
> wrong thing to encourage

Sorry, that was unclear. Yes, the hwpack still transports the files and they will still be installed on target. The change is only about allowing the startfiles.cfg to specifiy the files relative to the chroot and not be contrained to /boot on the target system.

Fow hwpacks v2, I'm thinking that we could pick the files from the deb and not install it, but I have yet to make sure that it would work.

[lp:~mabac/linaro-image-tools/bug-829220]

420. By Mattias Backman on 2011-08-22

Correct comment text.

[Loïc Minier (lool)]

On Mon, Aug 22, 2011, Mattias Backman wrote:
> Sorry, that was unclear. Yes, the hwpack still transports the files
> and they will still be installed on target. The change is only about
> allowing the startfiles.cfg to specifiy the files relative to the
> chroot and not be contrained to /boot on the target system.
>
> Fow hwpacks v2, I'm thinking that we could pick the files from the deb
> and not install it, but I have yet to make sure that it would work.

 Ok, we're in agreement them

 In terms of implementation, could we just check whether the filename is
 an absolute pathname in startfiles.cfg, and if it is use it relative to
 the chroot_dir, if not fallback to 'boot' + filename? That way,
 hwpack filenames would be required to have '/usr/lib/foo' or
 '/boot/foo' and we would fall back to '/boot/foo' if filename is 'foo'.

 I think it would be a matter of changing:
    filename = os.path.join(bin_dir, file_data[1])

 to:
    if file_data[1][0] == '/':
        filename = file_data[1]
    else:
        filename = os.path.join(bin_dir, file_data[1])

 and would force the hwpack to carry the filename that it ships rather
 than the linaro-image-tools code.

--
Loïc Minier

[lp:~mabac/linaro-image-tools/bug-829220]

421. By Mattias Backman on 2011-08-23

Detect absolute path in startfiles.cfg instead of checking that the file exists.

[Mattias Backman (mabac)]

> In terms of implementation, could we just check whether the filename is
> an absolute pathname in startfiles.cfg, and if it is use it relative to
> the chroot_dir, if not fallback to 'boot' + filename? That way,
> hwpack filenames would be required to have '/usr/lib/foo' or
> '/boot/foo' and we would fall back to '/boot/foo' if filename is 'foo'.

Sure, I have made that change now, please have a look.

[Loïc Minier (lool)]

 review approve

On Tue, Aug 23, 2011, Mattias Backman wrote:
> Sure, I have made that change now, please have a look.

 Looks good; thanks!

--
Loïc Minier

review: Approve

=== modified file 'linaro_image_tools/media_create/boards.py'
--- linaro_image_tools/media_create/boards.py	2011-08-19 13:59:34 +0000
+++ linaro_image_tools/media_create/boards.py	2011-08-23 08:36:26 +0000
@@ -759,7 +759,7 @@
         # Populate created raw partition with TOC and startup files.
         config_files_path = os.path.join(chroot_dir, 'boot')
         _, toc_filename = tempfile.mkstemp()
-        new_files = cls.get_file_info(config_files_path)
+        new_files = cls.get_file_info(chroot_dir)
         with open(toc_filename, 'wb') as toc:
             cls.create_toc(toc, new_files)
         cls.install_snowball_boot_loader(toc_filename, new_files,
@@ -820,19 +820,26 @@
             f.write(data)
 
     @classmethod
-    def get_file_info(cls, bin_dir):
+    def get_file_info(cls, chroot_dir):
         ''' Fills in the offsets of files that are located in
         non-absolute memory locations depending on their sizes.'
         Also fills in file sizes'''
         ofs = cls.TOC_SIZE
         files = []
+        bin_dir = os.path.join(chroot_dir, 'boot')
         with open(os.path.join(bin_dir, cls.SNOWBALL_STARTUP_FILES_CONFIG),
                   'r') as info_file:
             for line in info_file:
                 file_data = line.split()
                 if file_data[0][0] == '#':
                     continue
-                filename = os.path.join(bin_dir, file_data[1])
+                if file_data[1].startswith('/'):
+                    filename = os.path.join(chroot_dir,
+                                            file_data[1].lstrip('/'))
+                else:
+                    filename = os.path.join(bin_dir, file_data[1])
+                assert os.path.exists(filename), "File %s does not exist, " \
+                    "please check the startfiles config file." % file_data[1]
                 address = long(file_data[3], 16)
                 if address != 0:
                     ofs = address
 
=== modified file 'linaro_image_tools/media_create/tests/test_media_create.py'
--- linaro_image_tools/media_create/tests/test_media_create.py	2011-08-10 22:42:02 +0000
+++ linaro_image_tools/media_create/tests/test_media_create.py	2011-08-23 08:36:26 +0000
@@ -716,6 +716,45 @@
             i += 1
         return expected
 
+    def test_get_file_info_relative_path(self):
+        # Create a config file
+        cfg_file = os.path.join(self.temp_bootdir_path,
+                      boards.SnowballEmmcConfig.SNOWBALL_STARTUP_FILES_CONFIG)
+        uboot_file = 'u-boot.bin'
+        with open(cfg_file, 'w') as f:
+                f.write('%s %s %i %#x %s\n' % ('NORMAL', uboot_file, 0,
+                                               0xBA0000, '9'))
+        with open(os.path.join(self.temp_bootdir_path, uboot_file), 'w') as f:
+            file_info = boards.SnowballEmmcConfig.get_file_info(
+                self.tempdir)
+        self.assertEquals(file_info[0]['filename'],
+                          os.path.join(self.temp_bootdir_path, uboot_file))
+
+    def test_get_file_info_abs_path(self):
+        # Create a config file
+        cfg_file = os.path.join(self.temp_bootdir_path,
+                      boards.SnowballEmmcConfig.SNOWBALL_STARTUP_FILES_CONFIG)
+        uboot_dir = tempfile.mkdtemp(dir=self.tempdir)
+        uboot_file = os.path.join(uboot_dir, 'u-boot.bin')
+        uboot_relative_file = uboot_file.replace(self.tempdir, '')
+        with open(cfg_file, 'w') as f:
+                f.write('%s %s %i %#x %s\n' % ('NORMAL', uboot_relative_file, 0,
+                                               0xBA0000, '9'))
+        with open(uboot_file, 'w') as f:
+            file_info = boards.SnowballEmmcConfig.get_file_info(
+                self.tempdir)
+        self.assertEquals(file_info[0]['filename'], uboot_file)
+
+    def test_get_file_info_raises(self):
+        # Create a config file
+        cfg_file = os.path.join(self.temp_bootdir_path,
+                      boards.SnowballEmmcConfig.SNOWBALL_STARTUP_FILES_CONFIG)
+        with open(cfg_file, 'w') as f:
+                f.write('%s %s %i %#x %s\n' % ('NORMAL', 'u-boot.bin', 0,
+                                               0xBA0000, '9'))
+        self.assertRaises(AssertionError, boards.SnowballEmmcConfig.get_file_info,
+                          self.tempdir)
+
     def test_file_name_size(self):
         ''' Test using a to large toc file '''
         _, toc_filename = tempfile.mkstemp()
@@ -829,7 +868,7 @@
     def test_normal_case(self):
         expected = self.setupFiles()
         actual = boards.SnowballEmmcConfig.get_file_info(
-            self.temp_bootdir_path)
+            self.tempdir)
         self.assertEquals(expected, actual)