Code review comment for lp://staging/~mabac/linaro-image-tools/snowball-android-startfiles-hack

Hi Mattias,

I think it's not ideal to add two snowball-specific arguments to populate_raw_partition(), but I think it should be easy to get rid of them... see my suggestion below.

> === modified file 'linaro-android-media-create'
> --- linaro-android-media-create 2012-01-19 06:31:50 +0000
> +++ linaro-android-media-create 2012-02-16 15:27:17 +0000
> @@ -139,7 +139,8 @@
> board_config, media, args.image_size, args.boot_label,
> args.should_create_partitions, args.should_align_boot_part)
>
> - board_config.populate_raw_partition(args.device, BOOT_DIR)
> + board_config.populate_raw_partition(args.device, BOOT_DIR,
> + os.path.join(BOOT_DIR, 'boot'))
> populate_partition(BOOT_DIR + "/boot", BOOT_DISK, boot_partition)
> board_config.populate_boot_script(boot_partition, BOOT_DISK, args.consoles)
> with partition_mounted(boot_partition, BOOT_DISK):
>
> === modified file 'linaro_image_tools/media_create/android_boards.py'
> --- linaro_image_tools/media_create/android_boards.py 2012-02-16 15:27:17 +0000
> +++ linaro_image_tools/media_create/android_boards.py 2012-02-16 15:27:17 +0000
> @@ -161,8 +161,10 @@
> _userdata_len, sdcard_start)
>
> @classmethod
> - def populate_raw_partition(cls, media, boot_dir):
> - super(AndroidBoardConfig, cls).populate_raw_partition(media, boot_dir)
> + def populate_raw_partition(cls, media, boot_dir, config_files_dir,
> + delete_startupfiles=False):
> + super(AndroidBoardConfig, cls).populate_raw_partition(
> + media, boot_dir, config_files_dir, delete_startupfiles)
>
> @classmethod
> def install_boot_loader(cls, boot_partition, boot_device_or_file):
> @@ -231,6 +233,24 @@
> return '%s,%s,0xDA\n%s' % (
> loader_start, loader_len, command)
>
> + @classmethod
> + def populate_raw_partition(cls, media, boot_dir, config_files_dir,
> + delete_startupfiles=False):
> + # To avoid adding a Snowball specific command line option, we assume
> + # that the user already has unpacked the startfiles to ./startupfiles
> + local_config_dir = os.path.join('.', 'startupfiles')
> + assert os.path.exists(local_config_dir), "You need to unpack the "
> + "Snowball startupfiles to the directory 'startupfiles' in your current "
> + "working directory. See igloocommunity.org for more information."

This doesn't work as a multi-line string, you need to wrap it with
parentheses. And for bonus points you can indent them one extra level. :)

> + # We copy the u-boot files from the unpacked boot.tar.bz2
> + # and put it with the startfiles.
> + boot_files = ['u-boot.bin']
> + for boot_file in boot_files:
> + cmd_runner.run(['cp', os.path.join(boot_dir, 'boot', boot_file),

Ain't os.path.join(boot_dir, 'boot') the same as the new argument
(config_files_dir)? Maybe not for all boards?

> + local_config_dir], as_root=True).wait()
> + super(AndroidSnowballEmmcConfig, cls).populate_raw_partition(
> + media, boot_dir, local_config_dir, False)
> +
>
> class AndroidMx53LoCoConfig(AndroidBoardConfig, Mx53LoCoConfig):
> extra_boot_args_options = (
>
> === modified file 'linaro_image_tools/media_create/boards.py'
> --- linaro_image_tools/media_create/boards.py 2012-02-16 15:27:17 +0000
> +++ linaro_image_tools/media_create/boards.py 2012-02-16 15:27:17 +0000
> @@ -711,7 +711,8 @@
>
> if (cls.snowball_startup_files_config is not None and
> cls.board != 'snowball_sd'):
> - cls.populate_raw_partition(boot_device_or_file, chroot_dir)
> + cls.populate_raw_partition(boot_device_or_file, chroot_dir,
> + os.path.join(chroot_dir, 'boot'), True)
>
> if cls.env_dd:
> # Do we need to zero out the env before flashing it?
> @@ -816,7 +817,8 @@
> "No kernel found matching %s." % (cls.vmlinuz))
>
> @classmethod
> - def populate_raw_partition(cls, media, boot_dir):
> + def populate_raw_partition(cls, media, boot_dir, chroot_dir,

Hmm, in android-boards.py the new argument is called config_files_dir but here
it's called chroot_dir. That's confusing.

Also, I wonder if it really does make sense to have that as a required
argument even though it's only needed for one specific board config?

> + delete_startupfiles=False):

Same here; delete_startupfiles doesn't make sense for anything other than
snowball. I think we could easily get rid of it by having a, say,
should_delete_startupfiles @classproperty which returns True if
cls.snowball_startup_files_config is not None and cls.board != 'snowball_sd',
just like in the conditional above.

I wonder if it'd be possible to do the same for the other new argument?

> # Override in subclass if needed
> pass
>
> @@ -1070,26 +1072,30 @@
> make_uImage(cls.load_addr, k_img_data, boot_dir)
> boot_script_path = os.path.join(boot_dir, cls.boot_script)
> make_boot_script(boot_env, boot_script_path)
> - cls.populate_raw_partition(boot_device_or_file, chroot_dir)
> + cls.populate_raw_partition(boot_device_or_file, chroot_dir,
> + os.path.join(chroot_dir, 'boot'), True)
>
> @classmethod
> - def populate_raw_partition(cls, boot_device_or_file, chroot_dir):
> + def populate_raw_partition(cls, boot_device_or_file, chroot_dir,
> + config_files_dir, delete_startupfiles=False):
> # 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(chroot_dir)
> + new_files = cls.get_file_info(chroot_dir, config_files_dir)
> with open(toc_filename, 'wb') as toc:
> cls.create_toc(toc, new_files)
> cls.install_snowball_boot_loader(toc_filename, new_files,
> - boot_device_or_file,
> - cls.SNOWBALL_LOADER_START_S)
> + boot_device_or_file,
> + cls.SNOWBALL_LOADER_START_S,
> + delete_startupfiles)
> cls.delete_file(toc_filename)
> - cls.delete_file(os.path.join(config_files_path,
> - cls.snowball_startup_files_config))
> + if delete_startupfiles:
> + cls.delete_file(os.path.join(config_files_dir,
> + cls.snowball_startup_files_config))
>
> @classmethod
> def install_snowball_boot_loader(cls, toc_file_name, files,
> - boot_device_or_file, start_sector):
> + boot_device_or_file, start_sector,
> + delete_startupfiles=False):
> ''' Copies TOC and boot files into the boot partition.
> A sector size of 1 is used for some files, as they do not
> necessarily start on an even address. '''
> @@ -1107,7 +1113,8 @@
> else:
> seek_sectors = start_sector + file['offset'] / SECTOR_SIZE
> _dd(filename, boot_device_or_file, seek=seek_sectors)
> - cls.delete_file(filename)
> + if delete_startupfiles:
> + cls.delete_file(filename)
>
> @classmethod
> def delete_file(cls, file_path):
> @@ -1138,14 +1145,14 @@
> f.write(data)
>
> @classmethod
> - def get_file_info(cls, chroot_dir):
> + def get_file_info(cls, chroot_dir, config_files_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),
> + with open(os.path.join(config_files_dir,
> + cls.snowball_startup_files_config),
> 'r') as info_file:
> for line in info_file:
> file_data = line.split()
> @@ -1158,7 +1165,7 @@
> filename = os.path.join(chroot_dir,
> file_data[1].lstrip('/'))
> else:
> - filename = os.path.join(bin_dir, file_data[1])
> + filename = os.path.join(config_files_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)
> @@ -1398,7 +1405,8 @@
> return uboot_file
>
> @classmethod
> - def populate_raw_partition(cls, boot_device_or_file, chroot_dir):
> + def populate_raw_partition(cls, boot_device_or_file, chroot_dir,
> + config_files_dir, delete_startupfiles=False):
> # Zero the env so that the boot_script will get loaded
> _dd("/dev/zero", boot_device_or_file, count=cls.SAMSUNG_V310_ENV_LEN,
> seek=cls.SAMSUNG_V310_ENV_START)
>