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

Proposed by Mattias Backman
Status: Merged
Merged at revision: 422
Proposed branch: lp:~mabac/linaro-image-tools/bug-829220
Merge into: lp:linaro-image-tools/11.11
Diff against target: 98 lines (+50/-4)
2 files modified
linaro_image_tools/media_create/boards.py (+10/-3)
linaro_image_tools/media_create/tests/test_media_create.py (+40/-1)
To merge this branch: bzr merge lp:~mabac/linaro-image-tools/bug-829220
Reviewer Review Type Date Requested Status
Loïc Minier (community) Approve
Review via email: mp+72405@code.launchpad.net

Description of the change

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

To post a comment you must log in.
Revision history for this message
Loïc Minier (lool) wrote :

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

419. By Mattias Backman

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

Revision history for this message
Mattias Backman (mabac) wrote :

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

420. By Mattias Backman

Correct comment text.

Revision history for this message
Loïc Minier (lool) wrote :

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

421. By Mattias Backman

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

Revision history for this message
Mattias Backman (mabac) wrote :

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

Revision history for this message
Loïc Minier (lool) wrote :

 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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'linaro_image_tools/media_create/boards.py'
2--- linaro_image_tools/media_create/boards.py 2011-08-19 13:59:34 +0000
3+++ linaro_image_tools/media_create/boards.py 2011-08-23 08:36:26 +0000
4@@ -759,7 +759,7 @@
5 # Populate created raw partition with TOC and startup files.
6 config_files_path = os.path.join(chroot_dir, 'boot')
7 _, toc_filename = tempfile.mkstemp()
8- new_files = cls.get_file_info(config_files_path)
9+ new_files = cls.get_file_info(chroot_dir)
10 with open(toc_filename, 'wb') as toc:
11 cls.create_toc(toc, new_files)
12 cls.install_snowball_boot_loader(toc_filename, new_files,
13@@ -820,19 +820,26 @@
14 f.write(data)
15
16 @classmethod
17- def get_file_info(cls, bin_dir):
18+ def get_file_info(cls, chroot_dir):
19 ''' Fills in the offsets of files that are located in
20 non-absolute memory locations depending on their sizes.'
21 Also fills in file sizes'''
22 ofs = cls.TOC_SIZE
23 files = []
24+ bin_dir = os.path.join(chroot_dir, 'boot')
25 with open(os.path.join(bin_dir, cls.SNOWBALL_STARTUP_FILES_CONFIG),
26 'r') as info_file:
27 for line in info_file:
28 file_data = line.split()
29 if file_data[0][0] == '#':
30 continue
31- filename = os.path.join(bin_dir, file_data[1])
32+ if file_data[1].startswith('/'):
33+ filename = os.path.join(chroot_dir,
34+ file_data[1].lstrip('/'))
35+ else:
36+ filename = os.path.join(bin_dir, file_data[1])
37+ assert os.path.exists(filename), "File %s does not exist, " \
38+ "please check the startfiles config file." % file_data[1]
39 address = long(file_data[3], 16)
40 if address != 0:
41 ofs = address
42
43=== modified file 'linaro_image_tools/media_create/tests/test_media_create.py'
44--- linaro_image_tools/media_create/tests/test_media_create.py 2011-08-10 22:42:02 +0000
45+++ linaro_image_tools/media_create/tests/test_media_create.py 2011-08-23 08:36:26 +0000
46@@ -716,6 +716,45 @@
47 i += 1
48 return expected
49
50+ def test_get_file_info_relative_path(self):
51+ # Create a config file
52+ cfg_file = os.path.join(self.temp_bootdir_path,
53+ boards.SnowballEmmcConfig.SNOWBALL_STARTUP_FILES_CONFIG)
54+ uboot_file = 'u-boot.bin'
55+ with open(cfg_file, 'w') as f:
56+ f.write('%s %s %i %#x %s\n' % ('NORMAL', uboot_file, 0,
57+ 0xBA0000, '9'))
58+ with open(os.path.join(self.temp_bootdir_path, uboot_file), 'w') as f:
59+ file_info = boards.SnowballEmmcConfig.get_file_info(
60+ self.tempdir)
61+ self.assertEquals(file_info[0]['filename'],
62+ os.path.join(self.temp_bootdir_path, uboot_file))
63+
64+ def test_get_file_info_abs_path(self):
65+ # Create a config file
66+ cfg_file = os.path.join(self.temp_bootdir_path,
67+ boards.SnowballEmmcConfig.SNOWBALL_STARTUP_FILES_CONFIG)
68+ uboot_dir = tempfile.mkdtemp(dir=self.tempdir)
69+ uboot_file = os.path.join(uboot_dir, 'u-boot.bin')
70+ uboot_relative_file = uboot_file.replace(self.tempdir, '')
71+ with open(cfg_file, 'w') as f:
72+ f.write('%s %s %i %#x %s\n' % ('NORMAL', uboot_relative_file, 0,
73+ 0xBA0000, '9'))
74+ with open(uboot_file, 'w') as f:
75+ file_info = boards.SnowballEmmcConfig.get_file_info(
76+ self.tempdir)
77+ self.assertEquals(file_info[0]['filename'], uboot_file)
78+
79+ def test_get_file_info_raises(self):
80+ # Create a config file
81+ cfg_file = os.path.join(self.temp_bootdir_path,
82+ boards.SnowballEmmcConfig.SNOWBALL_STARTUP_FILES_CONFIG)
83+ with open(cfg_file, 'w') as f:
84+ f.write('%s %s %i %#x %s\n' % ('NORMAL', 'u-boot.bin', 0,
85+ 0xBA0000, '9'))
86+ self.assertRaises(AssertionError, boards.SnowballEmmcConfig.get_file_info,
87+ self.tempdir)
88+
89 def test_file_name_size(self):
90 ''' Test using a to large toc file '''
91 _, toc_filename = tempfile.mkstemp()
92@@ -829,7 +868,7 @@
93 def test_normal_case(self):
94 expected = self.setupFiles()
95 actual = boards.SnowballEmmcConfig.get_file_info(
96- self.temp_bootdir_path)
97+ self.tempdir)
98 self.assertEquals(expected, actual)
99
100

Subscribers

People subscribed via source and target branches