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

Thanks. I've changed that and added a class function that gives the new arguments. I've made the parent function raise an error since I believe it's an error to call this function from other than the two BoardConfigs for which it makes sense. (SnowballEmmcConfig and AndroidSnowballEmmcConfig.)

> 
> > === 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. :)
> 

Going for all the extra points I can get. ;) Fixed.

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

That's been removed now.

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

Yup they are gone now.

> 
> >          # 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)
> >