Hi Jeremy, This looks quite good as it's not too intrusive. I just have a few comments. On Wed, 2011-03-30 at 16:05 +0000, Jeremy Chang wrote: [...] > === added file 'linaro-android-media-create' > --- linaro-android-media-create 1970-01-01 00:00:00 +0000 > +++ linaro-android-media-create 2011-03-30 16:05:52 +0000 [...] > + > +# Registered as the first atexit handler as we want this to be the last > +# handler to execute. > +@atexit.register > +def cleanup_tempdir(): > + """Remove TEMP_DIR with all its contents. > + > + Before doing so, make sure BOOT_DISK and ROOT_DISK are not mounted. > + """ > + devnull = open('/dev/null', 'w') > + # ignore non-zero return codes > + for disk in BOOT_DISK, ROOT_DISK: Don't you want this cleanup function to umount DATA_DISK and SYSTEM_DISK as well? > + if disk is not None: > + try: > + cmd_runner.run(['umount', disk], > + stdout=devnull, stderr=devnull, as_root=True).wait() > + except cmd_runner.SubcommandNonZeroReturnValue: > + pass > + # Remove TMP_DIR as root because some files written there are > + # owned by root. > + if TMP_DIR is not None: > + cmd_runner.run(['rm', '-rf', TMP_DIR], as_root=True).wait() > + > + > +def ensure_required_commands(args): > + """Ensure we have the commands that we know are going to be used.""" > + required_commands = [ > + 'mkfs.vfat', 'sfdisk', 'mkimage', 'parted'] > + if not is_arm_host(): > + required_commands.append('qemu-arm-static') > + required_commands.append('qemu-img') > + if args.rootfs in ['ext2', 'ext3', 'ext4']: > + required_commands.append('mkfs.%s' % args.rootfs) > + else: > + required_commands.append('mkfs.btrfs') > + for command in required_commands: > + ensure_command(command) I don't think we should, at this point, be striving to share as much code as we can between this and the linaro-media-create script, but this would be trivial to share -- you can just move it to linaro_image_tools/media_create/utils.py and then import it here and in l-m-c. > + > + > +if __name__ == '__main__': > + parser = get_android_args_parser() > + args = parser.parse_args() > + > + # If --help was specified this won't execute. > + # Create temp dir and initialize rest of path vars. > + TMP_DIR = tempfile.mkdtemp() > + ROOT_DIR = os.path.join(TMP_DIR, 'root') > + SYSTEM_DIR = os.path.join(TMP_DIR, 'system') > + DATA_DIR = os.path.join(TMP_DIR, 'data') > + > + BOOT_DISK = os.path.join(TMP_DIR, 'boot-disc') > + ROOT_DISK = os.path.join(TMP_DIR, 'root-disc') > + SYSTEM_DISK = os.path.join(TMP_DIR, 'system-disc') > + CACHE_DISK = os.path.join(TMP_DIR, 'cache-disc') > + DATA_DISK = os.path.join(TMP_DIR, 'userdata-disc') > + SDCARD_DISK = os.path.join(TMP_DIR, 'sdcard-disc') > + > + board_config = board_configs[args.board] > + > + ensure_required_commands(args) > + > + media = Media(args.device) > + if media.is_block_device: > + if not confirm_device_selection_and_ensure_it_is_ready(args.device): > + sys.exit(1) > + elif not args.should_format_rootfs or not args.should_format_bootfs: > + print ("Do not use --no-boot or --no-part in conjunction with " > + "--image_file.") > + sys.exit(1) > + else: > + # All good, move on. > + pass > + > + > + cmd_runner.run(['mkdir', '-p', ROOT_DIR]).wait() > + cmd_runner.run(['mkdir', '-p', SYSTEM_DIR]).wait() > + cmd_runner.run(['mkdir', '-p', DATA_DIR]).wait() > + > + unpack_android_binary_tarball(args.root, ROOT_DIR) > + unpack_android_binary_tarball(args.system, SYSTEM_DIR) > + unpack_android_binary_tarball(args.userdata, DATA_DIR) > + > + # Create partitions > + boot_partition, root_partition, system_partition, cache_partition, \ > + data_partition, sdcard_partition = setup_android_partitions( \ > + board_config, media, args.image_size, args.boot_label, args.rfs_label, > + args.rootfs, args.should_create_partitions, args.should_format_bootfs, > + args.should_format_rootfs, args.should_align_boot_part) > + > + populate_partition(ROOT_DIR, ROOT_DISK, root_partition) > + populate_partition(SYSTEM_DIR + "/system", SYSTEM_DISK, system_partition) > + populate_partition(DATA_DIR + "/data", DATA_DISK, data_partition) > + print "Done creating Linaro Android image on %s" % args.device > > === modified file 'linaro_image_tools/media_create/boards.py' > --- linaro_image_tools/media_create/boards.py 2011-03-28 20:21:09 +0000 > +++ linaro_image_tools/media_create/boards.py 2011-03-30 16:05:52 +0000 > @@ -155,7 +155,7 @@ > serial_tty = None > > @classmethod > - def get_sfdisk_cmd(cls, should_align_boot_part=False): > + def get_sfdisk_cmd(cls, should_align_boot_part=False, image_type=None): > """Return the sfdisk command to partition the media. > > :param should_align_boot_part: Whether to align the boot partition too. > @@ -168,6 +168,15 @@ > else: > partition_type = '0x0E' > > + if image_type == "ANDROID": > + LOADER_MIN_SIZE_S = align_up(1 * 1024 * 1024, SECTOR_SIZE) / SECTOR_SIZE > + BOOT_MIN_SIZE_S = align_up(128 * 1024 * 1024, SECTOR_SIZE) / SECTOR_SIZE > + ROOT_MIN_SIZE_S = align_up(128 * 1024 * 1024, SECTOR_SIZE) / SECTOR_SIZE > + SYSTEM_MIN_SIZE_S = align_up(256 * 1024 * 1024, SECTOR_SIZE) / SECTOR_SIZE > + CACHE_MIN_SIZE_S = align_up(256 * 1024 * 1024, SECTOR_SIZE) / SECTOR_SIZE > + USERDATA_MIN_SIZE_S = align_up(512 * 1024 * 1024, SECTOR_SIZE) / SECTOR_SIZE > + SDCARD_MIN_SIZE_S = align_up(512 * 1024 * 1024, SECTOR_SIZE) / SECTOR_SIZE > + > # align on sector 63 for compatibility with broken versions of x-loader > # unless align_boot_part is set > boot_align = 63 > @@ -182,14 +191,32 @@ > # there should still be enough room > boot_len = boot_len - boot_len % 2 > boot_end = boot_start + boot_len - 1 > - # we ignore _root_end / _root_len and return a sfdisk command to > - # instruct the use of all remaining space; XXX if we had some root size > - # config, we could do something more sensible > - root_start, _root_end, _root_len = align_partition( > - boot_end + 1, ROOT_MIN_SIZE_S, PART_ALIGN_S, PART_ALIGN_S) > - > - return '%s,%s,%s,*\n%s,,,-' % ( > - boot_start, boot_len, partition_type, root_start) > + > + if image_type == "ANDROID": > + root_start, _root_end, _root_len = align_partition( > + boot_end + 1, ROOT_MIN_SIZE_S, PART_ALIGN_S, PART_ALIGN_S) > + system_start, _system_end, _system_len = align_partition( > + _root_end + 1, SYSTEM_MIN_SIZE_S, PART_ALIGN_S, PART_ALIGN_S) > + cache_start, _cache_end, _cache_len = align_partition( > + _system_end + 1, CACHE_MIN_SIZE_S, PART_ALIGN_S, PART_ALIGN_S) > + userdata_start, _userdata_end, _userdata_len = align_partition( > + _cache_end + 1, USERDATA_MIN_SIZE_S, PART_ALIGN_S, PART_ALIGN_S) > + sdcard_start, _sdcard_end, _sdcard_len = align_partition( > + _userdata_end + 1, SDCARD_MIN_SIZE_S, PART_ALIGN_S, PART_ALIGN_S) > + > + return '%s,%s,%s,*\n%s,%s,L\n%s,%s,L\n%s,-,E\n%s,%s,L\n%s,%s,L\n%s,,,-' % ( > + boot_start, boot_len, partition_type, root_start, _root_len, > + system_start, _system_len, cache_start, cache_start, _cache_len, > + userdata_start, _userdata_len, sdcard_start) > + else: > + # we ignore _root_end / _root_len and return a sfdisk command to > + # instruct the use of all remaining space; XXX if we had some root size > + # config, we could do something more sensible > + root_start, _root_end, _root_len = align_partition( > + boot_end + 1, ROOT_MIN_SIZE_S, PART_ALIGN_S, PART_ALIGN_S) > + > + return '%s,%s,%s,*\n%s,,,-' % ( > + boot_start, boot_len, partition_type, root_start) > I think it might make sense to have separate BoardConfig classes for android images; they would inherit from the existing ones and just override the appropriate bits. Then they could be stored in a separate registry (e.g. a android_board_configs dict similar to the existing board_configs one) that would be used by the new script to lookup the appropriate config class. I'm thinking that this might make sense because there's not much code being shared between the linaro and android implementations of get_sfdisk_cmd(). > @classproperty > def bootcmd(cls): > > === modified file 'linaro_image_tools/media_create/partitions.py' > --- linaro_image_tools/media_create/partitions.py 2011-03-24 10:10:38 +0000 > +++ linaro_image_tools/media_create/partitions.py 2011-03-30 16:05:52 +0000 > @@ -41,6 +41,63 @@ > UDISKS = "org.freedesktop.UDisks" > > > +def setup_android_partitions(board_config, media, image_size, bootfs_label, > + rootfs_label, rootfs_type, should_create_partitions, > + should_format_bootfs, should_format_rootfs, > + should_align_boot_part=False): > + cylinders = None > + create_partitions( > + board_config, media, HEADS, SECTORS, cylinders, > + should_align_boot_part=should_align_boot_part, image_type="ANDROID") > + > + if media.is_block_device: > + bootfs, rootfs, system, cache, data, sdcard = \ > + get_android_partitions_for_media (media, board_config) > + ensure_partition_is_not_mounted(bootfs) > + ensure_partition_is_not_mounted(rootfs) > + ensure_partition_is_not_mounted(system) > + ensure_partition_is_not_mounted(cache) > + ensure_partition_is_not_mounted(data) > + ensure_partition_is_not_mounted(sdcard) > + > + proc = cmd_runner.run( > + ['mkfs.vfat', '-F', str(board_config.fat_size), bootfs, '-n', > + bootfs_label], > + as_root=True) > + proc.wait() > + > + mkfs = 'mkfs.%s' % rootfs_type > + proc = cmd_runner.run( > + [mkfs, rootfs, '-L', rootfs_label], > + as_root=True) > + proc.wait() > + > + mkfs = 'mkfs.%s' % "ext4" > + proc = cmd_runner.run( > + [mkfs, system, '-L', "system"], > + as_root=True) > + proc.wait() > + > + mkfs = 'mkfs.%s' % "ext4" > + proc = cmd_runner.run( > + [mkfs, cache, '-L', "cache"], > + as_root=True) > + proc.wait() > + > + mkfs = 'mkfs.%s' % "ext4" > + proc = cmd_runner.run( > + [mkfs, data, '-L', "userdata"], > + as_root=True) > + proc.wait() > + > + proc = cmd_runner.run( > + ['mkfs.vfat', '-F', str(board_config.fat_size), sdcard, '-n', > + "sdcard"], > + as_root=True) > + proc.wait() > + > + return bootfs, rootfs, system, cache, data, sdcard ISTM that the new script doesn't really support --image_file, --no-bootfs or --no-rootfs. If that's correct it'd be nice to remove them from the list of args accepted. -- Guilherme Salgado