Merge lp:~dooferlad/linaro-image-tools/add-directory-option into lp:linaro-image-tools/11.11

[James Tunnicliffe]

Slightly modified patch from https://bugs.launchpad.net/linaro-image-tools/+bug/962147

Updated:
--directory can now be specified at the same time as --image-file. If --directory and --image-file are combined, a file, whose name is specified by --image-file, will be created in --directory. If --image-file is not specified, a sensible default file name will be used and printed for the user to see.

--directory and --mmc are incompatable.

--directory and specifying a full path to a file with --image-file are incompatable.

Updated(2):
Fixed spelling of IncompatibleOptions.
Renamed --directory option to --output-directory. Set default file name for an image to sd.img.
Moved defaulting of args.device into ArgumentParser set up.
Moved additional option checks into their own function (additional_option_checks).

I am unable to move more argument processing into media_create/__init__.py get_args_parser() because ArgumentParser can't cope with the complexity of the logic. I have split it out into another function though, so it is clear what is happening.

[Loïc Minier (lool)]

On Mon, Apr 16, 2012, James Tunnicliffe wrote:
[...]
> + if args.directory is not None:
> + if not board_config.outputs_directory:
> + raise BoardAbilityNotMet("The board '%s' does not support the"
> + "--directory option." % args.board)

 I find the per board attribute kinda ugly and I'm not sure we want to
 suffer from supporting both a single file output or a directory with
 multiple files as output.

 How about we just default to always outputting to a directory, default
 to ./ as the target directory, and allow overriding that, but keep the
 --image-file as the default filename for the main image? We could
 document for each boardconfig what it outputs as to display it at the
 end of the build or in the help output.

 There are alternate approaches such as output a zip/tgz file with all
 the assets and naming that after --image-file, but that's probably a
 bit heavy. We could also abuse --image-file by outputting a warning
 and creating a directory there instead (e.g. --image-file foo.img would
 create ./foo.img/ and output the assets below that).

--
Loïc Minier

[James Tunnicliffe (dooferlad)]

On 16 April 2012 17:40, Loïc Minier <email address hidden> wrote:
> On Mon, Apr 16, 2012, James Tunnicliffe wrote:
> [...]
>> +    if args.directory is not None:
>> +        if not board_config.outputs_directory:
>> +            raise BoardAbilityNotMet("The board '%s' does not support the"
>> +                                     "--directory option." % args.board)
>
>  I find the per board attribute kinda ugly and I'm not sure we want to
>  suffer from supporting both a single file output or a directory with
>  multiple files as output.
>
>  How about we just default to always outputting to a directory, default
>  to ./ as the target directory, and allow overriding that, but keep the
>  --image-file as the default filename for the main image?  We could
>  document for each boardconfig what it outputs as to display it at the
>  end of the build or in the help output.

I like this approach since it gracefully reverts back to the old
behaviour. Will make some changes.

--
James Tunnicliffe

[James Tunnicliffe (dooferlad)]

Oops, forgot to delete the outputs_directory flag. Hang on...

[James Tunnicliffe (dooferlad)]

OK. Done.

[Loïc Minier (lool)]

On Tue, Apr 17, 2012, James Tunnicliffe wrote:
> + IncompatableOptions,

 IncompatibleOptions

> + parser.add_argument(
> + '--directory', dest='directory',
> + help='Directory where image and accessories should be written to.')

 --output-directory?

> + outputs_directory = False

 not too nice as property; perhaps outputs_multiple_artifacts = False?
 Or expected_artifacts = 1?

> +def prep_media_path(args, board_config):
> +
> + # If args.device isn't set, generate a sensible default file name and
> + # tell the user
> + if not args.device:
> + args.device = board_config.board + ".img"
> + print "Setting destination file name to", args.device

 I think it would be more stable to just call it mmc.img or sd.img.

> + if args.directory is not None:
> + # If args.device is a path to a device (/dev/) then this is an error
> + if "--mmc" in sys.argv:
> + raise IncompatableOptions("--directory option incompatable with "
> + "option --mmc")
> +
> + # If directory is used as well as having a full path (rather than just
> + # a file name or relative path) in args.device, this is an error.
> + if re.search(r"^/", args.device):
> + raise IncompatableOptions("--directory option incompatable with "

 IncompatibleOptions + "incompatible"

 We could actually support absolute --image-file with --directory and
 just output the image where the user wants it, but I don't really care
 strongly about the behavior in that particular use case.

 Generally ok with the design apart of the minor comments here, but
 I have one objection: I think the argument validation / mangling should
 happen when parsing the cmdline rather than in boards code.

 Could you just include prep_media_path logic as part of the cmdline
 args validation? This would also remove the need for a special
 exception down the road.

 Otherwise good, thanks!

--
Loïc Minier

[Ricardo Salveti (rsalveti)]

On Tue, Apr 17, 2012 at 3:37 PM, Loïc Minier <email address hidden> wrote:
>> +def prep_media_path(args, board_config):
>> +
>> +    # If args.device isn't set, generate a sensible default file name and
>> +    # tell the user
>> +    if not args.device:
>> +        args.device = board_config.board + ".img"
>> +        print "Setting destination file name to", args.device
>
>  I think it would be more stable to just call it mmc.img or sd.img.

+1, that would help scripting the output later on.

>> +    if args.directory is not None:
>> +        # If args.device is a path to a device (/dev/) then this is an error
>> +        if "--mmc" in sys.argv:
>> +            raise IncompatableOptions("--directory option incompatable with "
>> +                                      "option --mmc")
>> +
>> +        # If directory is used as well as having a full path (rather than just
>> +        # a file name or relative path) in args.device, this is an error.
>> +        if re.search(r"^/", args.device):
>> +            raise IncompatableOptions("--directory option incompatable with "
>
>  IncompatibleOptions + "incompatible"
>
>  We could actually support absolute --image-file with --directory and
>  just output the image where the user wants it, but I don't really care
>  strongly about the behavior in that particular use case.
>
>  Generally ok with the design apart of the minor comments here, but
>  I have one objection: I think the argument validation / mangling should
>  happen when parsing the cmdline rather than in boards code.

+1

>  Otherwise good, thanks!

+1

[lp:~dooferlad/linaro-image-tools/add-directory-option]

516. By James Tunnicliffe on 2012-04-18

remove .idea ignore

[Loïc Minier (lool)]

 review approve

 Seems almost good to go except minor points below:

On Wed, Apr 18, 2012, James Tunnicliffe wrote:
> + try:
> + additional_option_checks(args)

 I would just include the logic here rather than adding a separate
 function + Exception, but up to you

> === modified file 'linaro_image_tools/media_create/partitions.py'
> --- linaro_image_tools/media_create/partitions.py 2012-01-17 14:19:19 +0000
> +++ linaro_image_tools/media_create/partitions.py 2012-04-18 13:29:21 +0000
> @@ -590,4 +590,5 @@
>
> def __init__(self, path):
> self.path = path
> + self.directory = None
> self.is_block_device = path.startswith('/dev/')

 Hmm shouldn't you take directory in __init__?

> +class Args():
> + def __init__(self, directory, device, board):
> + self.directory = directory
> + self.device = device
> + self.board = board

 Don't think board is needed anymore, but you shouldn't you just use a
 Media() instance?

--
Loïc Minier

review: Approve

[lp:~dooferlad/linaro-image-tools/add-directory-option]

517. By James Tunnicliffe on 2012-04-18

Removed unused directory variable.

[James Tunnicliffe (dooferlad)]

On 18 April 2012 14:55, Loïc Minier <email address hidden> wrote:
> Review: Approve
>
>  review approve
>
>  Seems almost good to go except minor points below:
>
> On Wed, Apr 18, 2012, James Tunnicliffe wrote:
>> +    try:
>> +        additional_option_checks(args)
>
>  I would just include the logic here rather than adding a separate
>  function + Exception, but up to you

I just got to write tests for it this way.

>> === modified file 'linaro_image_tools/media_create/partitions.py'
>> --- linaro_image_tools/media_create/partitions.py     2012-01-17 14:19:19 +0000
>> +++ linaro_image_tools/media_create/partitions.py     2012-04-18 13:29:21 +0000
>> @@ -590,4 +590,5 @@
>>
>>      def __init__(self, path):
>>          self.path = path
>> +        self.directory = None
>>          self.is_block_device = path.startswith('/dev/')
>
>  Hmm shouldn't you take directory in __init__?

Actually, I can't find anywhere it is used. Deleted.

>> +class Args():
>> +    def __init__(self, directory, device, board):
>> +        self.directory = directory
>> +        self.device = device
>> +        self.board = board
>
>  Don't think board is needed anymore, but you shouldn't you just use a
>  Media() instance?

It is needed as a fake version of what comes out of the arguments
parser. It isn't the same as Media.

--
James Tunnicliffe

=== modified file 'linaro-media-create'
--- linaro-media-create	2012-01-17 14:50:47 +0000
+++ linaro-media-create	2012-04-18 14:21:19 +0000
@@ -48,6 +48,9 @@
     is_arm_host,
     check_file_integrity_and_log_errors,
     path_in_tarfile_exists,
+    IncompatibleOptions,
+    prep_media_path,
+    additional_option_checks,
     )
 
 # Just define the global variables
@@ -97,6 +100,37 @@
     parser = get_args_parser()
     args = parser.parse_args()
 
+    try:
+        additional_option_checks(args)
+    except IncompatibleOptions as e:
+        parser.print_help()
+        print >> sys.stderr, "\nError:", e.value
+        sys.exit(1)
+
+    board_config = board_configs[args.board]
+    board_config.set_metadata(args.hwpacks)
+    board_config.set_board(args.board)
+    board_config.add_boot_args(args.extra_boot_args)
+    board_config.add_boot_args_from_file(args.extra_boot_args_file)
+
+    media = Media(prep_media_path(args))
+
+    if media.is_block_device:
+        if not board_config.supports_writing_to_mmc:
+            print "The board '%s' does not support the --mmc option. Please use "\
+                  "--image_file to create an image file for this board." % args.board
+            sys.exit(1)
+        if not confirm_device_selection_and_ensure_it_is_ready(args.device,
+                                                               args.nocheck_mmc):
+            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
+
     ch = logging.StreamHandler()
     ch.setLevel(logging.INFO)
     formatter = logging.Formatter("%(message)s")
@@ -116,12 +150,6 @@
     BOOT_DISK = os.path.join(TMP_DIR, 'boot-disc')
     ROOT_DISK = os.path.join(TMP_DIR, 'root-disc')
 
-    board_config = board_configs[args.board]
-    board_config.set_metadata(args.hwpacks)
-    board_config.set_board(args.board)
-    board_config.add_boot_args(args.extra_boot_args)
-    board_config.add_boot_args_from_file(args.extra_boot_args_file)
-
     ensure_required_commands(args)
 
     sig_file_list = args.hwpacksigs[:]
@@ -134,24 +162,8 @@
                                     sig_file_list, args.binary, args.hwpacks)
     if not files_ok:
         sys.exit(1)
-    
+
     atexit.register(cleanup_tempdir)
-    media = Media(args.device)
-    if media.is_block_device:
-        if not board_config.supports_writing_to_mmc:
-            print "The board '%s' does not support the --mmc option. Please use " \
-                "--image_file to create an image file for this board." % args.board
-            sys.exit(1)
-        if not confirm_device_selection_and_ensure_it_is_ready(args.device,
-                                                  args.nocheck_mmc):
-            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
 
     unpack_binary_tarball(args.binary, TMP_DIR)
 
@@ -174,7 +186,7 @@
 
     if args.should_format_bootfs:
         board_config.populate_boot(
-            ROOTFS_DIR, rootfs_uuid, boot_partition, BOOT_DISK, args.device,
+            ROOTFS_DIR, rootfs_uuid, boot_partition, BOOT_DISK, media.path,
             args.is_live, args.is_lowmem, args.consoles)
 
     if args.should_format_rootfs:
@@ -185,4 +197,4 @@
             rootfs_uuid, create_swap, str(args.swap_file),
             board_config.mmc_part_offset, board_config)
 
-    print "Done creating Linaro image on %s" % args.device
+    print "Done creating Linaro image on %s" % media.path

=== modified file 'linaro_image_tools/media_create/__init__.py'
--- linaro_image_tools/media_create/__init__.py	2012-01-19 06:31:50 +0000
+++ linaro_image_tools/media_create/__init__.py	2012-04-18 14:21:19 +0000
@@ -77,12 +77,17 @@
 def get_args_parser():
     """Get the ArgumentParser for the arguments given on the command line."""
     parser = argparse.ArgumentParser(version='%(prog)s ' + get_version())
-    group = parser.add_mutually_exclusive_group(required=True)
-    group.add_argument(
-        '--mmc', dest='device', help='The storage device to use.')
-    group.add_argument(
-        '--image-file', '--image_file', dest='device',
-        help='File where we should write the QEMU image.')
+    group = parser.add_mutually_exclusive_group()
+    group.add_argument(
+        '--mmc', dest='device', default="sd.img",
+        help='The storage device to use.')
+    group.add_argument(
+        '--image-file', '--image_file', dest='device', default="sd.img",
+        help='File where we should write an image file (defaults to sd.img '
+             'if neither --image-file or --mmc are specified.)')
+    parser.add_argument(
+        '--output-directory', dest='directory',
+        help='Directory where image and accessories should be written to.')
     parser.add_argument(
         '--dev', required=True, dest='board', choices=KNOWN_BOARDS,
         help='Generate an SD card or image for the given board.')

=== modified file 'linaro_image_tools/media_create/boards.py'
--- linaro_image_tools/media_create/boards.py	2012-04-17 16:22:09 +0000
+++ linaro_image_tools/media_create/boards.py	2012-04-18 14:21:19 +0000
@@ -1337,6 +1337,33 @@
     pass
 
 
+class FastModelConfig(BoardConfig):
+    supports_writing_to_mmc = False
+      
+    @classmethod
+    def _get_bootcmd(cls, d_img_data):
+        """Get the bootcmd for FastModel.
+
+        We override this as we don't do uboot.
+        """
+        return ""
+
+    @classmethod
+    def _make_boot_files_v2(cls, boot_env, chroot_dir, boot_dir,
+                         boot_device_or_file, k_img_data, i_img_data,
+                         d_img_data):
+        logger = logging.getLogger("linaro_image_tools")
+        logger.info("WTF=%s." % boot_device_or_file )
+        output_dir=os.path.dirname(boot_device_or_file)
+        cmd = [ "cp", "-v",  _get_file_matching("%s/boot/img.axf" % chroot_dir), output_dir ]
+        proc = cmd_runner.run(cmd, as_root=True)
+        proc.wait()
+        cmd = [ "cp", "-v",  k_img_data, i_img_data, d_img_data, output_dir ]
+        proc = cmd_runner.run(cmd, as_root=True)
+        proc.wait()
+        return
+
+
 class SamsungConfig(BoardConfig):
     @classproperty
     def extra_serial_opts(cls):
@@ -1542,6 +1569,7 @@
     'panda': PandaConfig,
     'vexpress': VexpressConfig,
     'vexpress-a9': VexpressA9Config,
+    'fastmodel': FastModelConfig,
     'ux500': Ux500Config,
     'snowball_sd': SnowballSdConfig,
     'snowball_emmc': SnowballEmmcConfig,

=== modified file 'linaro_image_tools/tests/test_utils.py'
--- linaro_image_tools/tests/test_utils.py	2011-10-12 09:54:29 +0000
+++ linaro_image_tools/tests/test_utils.py	2012-04-18 14:21:19 +0000
@@ -41,9 +41,11 @@
     verify_file_integrity,
     check_file_integrity_and_log_errors,
     path_in_tarfile_exists,
+    IncompatibleOptions,
+    prep_media_path,
+    additional_option_checks,
     )
 
-
 sudo_args = " ".join(cmd_runner.SUDO_ARGS)
 
 
@@ -260,3 +262,44 @@
             UnableToFindPackageProvidingCommand,
             install_package_providing, 'mkfs.lean')
 
+
+class Args():
+    def __init__(self, directory, device, board):
+        self.directory = directory
+        self.device = device
+        self.board = board
+
+
+class TestPrepMediaPath(TestCaseWithFixtures):
+
+    def test_prep_media_path(self):
+        self.useFixture(MockSomethingFixture(os.path, 'abspath', lambda x: x))
+        self.useFixture(MockSomethingFixture(os, "makedirs", lambda x: x))
+
+        self.assertEqual("testdevice",
+                         prep_media_path(Args(directory=None,
+                                              device="testdevice",
+                                              board="testboard")))
+
+        self.assertEqual("/foo/bar/testdevice",
+                         prep_media_path(Args(directory="/foo/bar",
+                                              device="testdevice",
+                                              board="testboard")))
+
+class TestPrepMediaPath(TestCaseWithFixtures):
+
+    def test_additional_option_checks(self):
+        self.useFixture(MockSomethingFixture(os.path, 'abspath', lambda x: x))
+        self.useFixture(MockSomethingFixture(os, "makedirs", lambda x: x))
+
+        self.assertRaises(IncompatibleOptions, additional_option_checks,
+                          Args(directory="/foo/bar",
+                               device="/testdevice",
+                               board="testboard"))
+
+        sys.argv.append("--mmc")
+        self.assertRaises(IncompatibleOptions, additional_option_checks,
+                          Args(directory="/foo/bar",
+                               device="testdevice",
+                               board="testboard"))
+        sys.argv.remove("--mmc")

=== modified file 'linaro_image_tools/utils.py'
--- linaro_image_tools/utils.py	2011-11-28 12:49:14 +0000
+++ linaro_image_tools/utils.py	2012-04-18 14:21:19 +0000
@@ -247,5 +247,41 @@
     return prefer_dir
 
 
+def prep_media_path(args):
+    if args.directory is not None:
+        loc = os.path.abspath(args.directory)
+        try:
+            os.makedirs(loc)
+        except OSError:
+            pass # Directory exists.
+
+        path = os.path.join(loc, args.device)
+    else:
+        path = args.device
+
+    return path
+
+
 class UnableToFindPackageProvidingCommand(Exception):
     """We can't find a package which provides the given command."""
+
+
+class IncompatibleOptions(Exception):
+    def __init__(self, value):
+        self.value = value
+
+    def __str__(self):
+        return repr(self.value)
+
+def additional_option_checks(args):
+    if args.directory is not None:
+    # If args.device is a path to a device (/dev/) then this is an error
+        if "--mmc" in sys.argv:
+            raise IncompatibleOptions("--directory option incompatable with "
+                                      "option --mmc")
+
+        # If directory is used as well as having a full path (rather than just
+        # a file name or relative path) in args.device, this is an error.
+        if re.search(r"^/", args.device):
+            raise IncompatibleOptions("--directory option incompatable with "
+                                      "a full path in --image-file")