Linaro Image Tools

Merge lp://staging/~mabac/linaro-image-tools/snowball-android-startfiles-hack into lp://staging/linaro-image-tools/11.11

snowball-android-startfiles-hack
Merge into trunk

Proposed by Mattias Backman on 2012-02-16

Status:	Merged
Merged at revision:	493
Proposed branch:	lp://staging/~mabac/linaro-image-tools/snowball-android-startfiles-hack
Merge into:	lp://staging/linaro-image-tools/11.11
Prerequisite:	lp://staging/~mabac/linaro-image-tools/fix-raw-partition-arguments
Diff against target:	507 lines (+321/-22) 3 files modified linaro_image_tools/media_create/android_boards.py (+22/-0) linaro_image_tools/media_create/boards.py (+27/-12) linaro_image_tools/media_create/tests/test_media_create.py (+272/-10)
To merge this branch:	bzr merge lp://staging/~mabac/linaro-image-tools/snowball-android-startfiles-hack
Related bugs:	Link a bug report

Reviewer	Date Requested	Status
Guilherme Salgado (community)	2012-02-16	Needs Fixing on 2012-02-17
Mathieu Poirier	2012-02-16	Pending
Review via email: mp+93430@code.staging.launchpad.net

Description of the change

Hi,

This branch adds Android support for the Snowball EMMC boot mode. We do this by assuming that the user has downloaded a special tarball with the startfiles and unpacked them to ./startupfiles. We could add this as a command line option but for now decided to not clutter the cli with an option that is specific to one board.

We also get the u-boot.bin from the boot partition, where it is unpacked from the boot tarball.

mpoirier: should I copy the u-boot-env file too? It's not in the overlay boot file I have now.

This change should not affect any other board than the Snowball and only when using l-a-m-c to create an image to flash to emmc.

Thanks,

Mattias

Revision history for this message

Mattias Backman (mabac) wrote on 2012-02-16:

I've also added a delete_startupfiles flag so we can set wether to delete the files after dd:ing them. We want to delete them when we use l-m-c since the files have been copied to /boot on the target by the deb package. We don't want to leave them there.

In the l-a-m-c case the user has unpacked the files locally and does not want to have them deleted every time he runs l-a-m-c.

Revision history for this message

Mattias Backman (mabac) wrote on 2012-02-16:

> mpoirier: should I copy the u-boot-env file too? It's not in the overlay boot file I have now.

That is not needed so it will not be referenced in the startfiles.cfg provided by the boot overlay tarball.

Revision history for this message

Mattias Backman (mabac) wrote on 2012-02-17:

On Thu, Feb 16, 2012 at 10:39 PM, Mathieu Poirier <email address hidden> wrote:
> Good day,
>
> I tested your latest modifications and everything work perfectly.

Revision history for this message

Guilherme Salgado (salgado) wrote on 2012-02-17:

Download full text (9.4 KiB)

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

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

review: Needs Fixing

lp://staging/~mabac/linaro-image-tools/snowball-android-startfiles-hack updated on 2012-02-20

498. By Mattias Backman on 2012-02-17: Start fixing review comments.
499. By Mattias Backman on 2012-02-20: Remove the new arguments to populate_raw_partition and get them from a class function instead. Add tests for populate_raw_partition.
500. By Mattias Backman on 2012-02-20: Revert the call from linaro-android-media-create.

Revision history for this message

Mattias Backman (mabac) wrote on 2012-02-20:

Download full text (10.1 KiB)

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

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

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

lp://staging/~mabac/linaro-image-tools/snowball-android-startfiles-hack updated on 2012-02-20

501. By Mattias Backman on 2012-02-20: Undo line break and remove new arguments from another call.

Revision history for this message

Guilherme Salgado (salgado) wrote on 2012-02-20:

Wow, nice, lots of new tests! This looks quite good; the only comment I have is about snowball_config(chroot_dir), which has no docstring and return a two-tuple where one value is a hard-coded boolean so there's no way to know what it means. I'd prefer if the hard-coded value were in a class attribute and snowball_config(chroot_dir) returned just the path to the config dir, but having just a docstring on snowball_config() would be ok as well.

Revision history for this message

Mattias Backman (mabac) wrote on 2012-02-21:

On Mon, Feb 20, 2012 at 7:51 PM, Guilherme Salgado
<email address hidden> wrote:
> Wow, nice, lots of new tests! This looks quite good; the only comment I have is about snowball_config(chroot_dir), which has no docstring and return a two-tuple where one value is a hard-coded boolean so there's no way to know what it means. I'd prefer if the hard-coded value were in a class attribute and snowball_config(chroot_dir) returned just the path to the config dir, but having just a docstring on snowball_config() would be ok as well.

I did the classproperty fix before merging.

Also mpoirier re-acked the change via email.

> --
> https://code.launchpad.net/~mabac/linaro-image-tools/snowball-android-startfiles-hack/+merge/93430
> You are the owner of lp:~mabac/linaro-image-tools/snowball-android-startfiles-hack.