[wic patch 0/5] Add option to wic and use argparse

[wic patch 0/5] Add option to wic and use argparse

[Andreas J. Reichel]

This series consists of the following changes to wic:

    * wic previously introduced deletion of all single partition
        image files. In some cases, this deletion can fail, however
        this is no reason for wic to fail. Thus, exception handling
        was added to the clean-up function of the direct imager
        plugin.
    
    * wic uses optparse and a rather complicated approach of its
        initialization. It is much easier to switch from the
        deprecated optparse to the argparse library, which also
        supports sub-commands, which wic heavily makes use of.
        The second patch replaces optparse by argparse.

    * For some embedded projects it is necessary to keep the single
        partition image files to further process them. This is
        the case if a user has no rights to mount and cannot extract
        the images from the already merged .direct file, for example.

        For this purpose, a new option is introduced:

            -P, --keep-partition-images

        which prevents deletion of .p* files created by the direct
        imager plugin.

    * Maintainability improvement in engine.py by comparing parameters
        with enum-like string constants rather than repeating the
        strings and fixes in help texts.

The new argument parser was tested with random sampling manually, since
variable names were not changed inside the core of the subcommands and
transition from optparse to argparse is straight forward regarding access
to argument values.

Signed-off-by: Andreas Reichel <andreas.reichel.ext@siemens.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Signed-off-by: Daniel Wagner <daniel.wagner@siemens.com>

Reichel Andreas (5):
  wic: Catch errors during image files clean-up
  wic: Use argparse instead of optparse
  wic: Add missing text to usage and help strings
  wic: Add option to keep partition images
  wic: Use enum like dicts for string constants

 scripts/lib/wic/engine.py                |  21 ++-
 scripts/lib/wic/help.py                  |  30 +++--
 scripts/lib/wic/plugins/imager/direct.py |  23 +++-
 scripts/wic                              | 215 +++++++++++++++++++------------
 4 files changed, 191 insertions(+), 98 deletions(-)

-- 
2.11.0

[wic patch 1/5] wic: Catch errors during image files clean-up

[Andreas J. Reichel]

Handle exception if a file could not be deleted during clean-up of
unwanted files, thus preventing a failure of wic in this case.

Signed-off-by: Andreas Reichel <andreas.reichel.ext@siemens.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Signed-off-by: Daniel Wagner <daniel.wagner@siemens.com>

---
 scripts/lib/wic/plugins/imager/direct.py | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/scripts/lib/wic/plugins/imager/direct.py b/scripts/lib/wic/plugins/imager/direct.py
index f2e6127331..d6b47ff0bb 100644
--- a/scripts/lib/wic/plugins/imager/direct.py
+++ b/scripts/lib/wic/plugins/imager/direct.py
@@ -541,7 +541,12 @@ class PartitionedImage():
     def cleanup(self):
         # remove partition images
         for image in set(self.partimages):
-            os.remove(image)
+            try:
+                os.remove(image)
+            except IOError as e:
+                logger.warning(
+                    "Could not delete file. {0}: I/O error ({1}): {2}\n".format(
+                    image, e.errno, e.strerror))
 
     def assemble(self):
         logger.debug("Installing partitions")
-- 
2.11.0

[wic patch 2/5] wic: Use argparse instead of optparse

[Andreas J. Reichel]

* optparse is deprecated and will not be developed further
    (see: https://docs.python.org/2/library/optparse.html)
* argparse supports subcommands, which simplifies definition of
    arguments and options
* reimplement help mechanism through sub-subcommands

Signed-off-by: Andreas Reichel <andreas.reichel.ext@siemens.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Signed-off-by: Daniel Wagner <daniel.wagner@siemens.com>

---
 scripts/lib/wic/engine.py |  11 +--
 scripts/lib/wic/help.py   |  23 +++--
 scripts/wic               | 213 ++++++++++++++++++++++++++++------------------
 3 files changed, 154 insertions(+), 93 deletions(-)

diff --git a/scripts/lib/wic/engine.py b/scripts/lib/wic/engine.py
index f59821fea6..647358287f 100644
--- a/scripts/lib/wic/engine.py
+++ b/scripts/lib/wic/engine.py
@@ -201,17 +201,18 @@ def wic_list(args, scripts_path):
     """
     Print the list of images or source plugins.
     """
-    if len(args) < 1:
+    if args.list_type is None:
         return False
 
-    if args == ["images"]:
+    if args.list_type == "images":
+
         list_canned_images(scripts_path)
         return True
-    elif args == ["source-plugins"]:
+    elif args.list_type == "source-plugins":
         list_source_plugins()
         return True
-    elif len(args) == 2 and args[1] == "help":
-        wks_file = args[0]
+    elif len(args.help_for) == 1 and args.help_for[0] == 'help':
+        wks_file = args.list_type
         fullpath = find_canned_image(scripts_path, wks_file)
         if not fullpath:
             raise WicError("No image named %s found, exiting. "
diff --git a/scripts/lib/wic/help.py b/scripts/lib/wic/help.py
index aee2451a72..2c2dbdc3b5 100644
--- a/scripts/lib/wic/help.py
+++ b/scripts/lib/wic/help.py
@@ -56,7 +56,7 @@ def wic_help(args, usage_str, subcommands):
     """
     Subcommand help dispatcher.
     """
-    if len(args) == 1 or not display_help(args[1], subcommands):
+    if args.help_topic == None or not display_help(args.help_topic, subcommands):
         print(usage_str)
 
 
@@ -82,19 +82,20 @@ def invoke_subcommand(args, parser, main_command_usage, subcommands):
     Dispatch to subcommand handler borrowed from combo-layer.
     Should use argparse, but has to work in 2.6.
     """
-    if not args:
+    if not args.command:
         logger.error("No subcommand specified, exiting")
         parser.print_help()
         return 1
-    elif args[0] == "help":
+    elif args.command == "help":
         wic_help(args, main_command_usage, subcommands)
-    elif args[0] not in subcommands:
-        logger.error("Unsupported subcommand %s, exiting\n", args[0])
+    elif args.command not in subcommands:
+        logger.error("Unsupported subcommand %s, exiting\n", args.command)
         parser.print_help()
         return 1
     else:
-        usage = subcommands.get(args[0], subcommand_error)[1]
-        subcommands.get(args[0], subcommand_error)[0](args[1:], usage)
+        subcmd = subcommands.get(args.command, subcommand_error)
+        usage = subcmd[1]
+        subcmd[0](args, usage)
 
 
 ##
@@ -795,3 +796,11 @@ DESCRIPTION
       .wks files.
 
 """
+
+wic_help_help = """
+NAME
+    wic help - display a help topic
+
+DESCRIPTION
+    Specify a help topic to display it. Topics are shown above.
+"""
diff --git a/scripts/wic b/scripts/wic
index a5f2dbfc6f..49cad869e2 100755
--- a/scripts/wic
+++ b/scripts/wic
@@ -33,7 +33,7 @@ __version__ = "0.2.0"
 # Python Standard Library modules
 import os
 import sys
-import optparse
+import argparse
 import logging
 from distutils import spawn
 
@@ -85,66 +85,30 @@ def rootfs_dir_to_args(krootfs_dir):
         rootfs_dir += '='.join([key, val])
     return rootfs_dir.strip()
 
-def callback_rootfs_dir(option, opt, value, parser):
-    """
-    Build a dict using --rootfs_dir connection=dir
-    """
-    if not type(parser.values.rootfs_dir) is dict:
-        parser.values.rootfs_dir = dict()
 
-    if '=' in value:
-        (key, rootfs_dir) = value.split('=')
-    else:
-        key = 'ROOTFS_DIR'
-        rootfs_dir = value
+class RootfsArgAction(argparse.Action):
+    def __init__(self, **kwargs):
+        super().__init__(**kwargs)
+
+    def __call__(self, parser, namespace, value, option_string=None):
+        if not "rootfs_dir" in vars(namespace) or \
+           not type(namespace.__dict__['rootfs_dir']) is dict:
+            namespace.__dict__['rootfs_dir'] = {}
+
+        if '=' in value:
+            (key, rootfs_dir) = value.split('=')
+        else:
+            key = 'ROOTFS_DIR'
+            rootfs_dir = value
 
-    parser.values.rootfs_dir[key] = rootfs_dir
+        namespace.__dict__['rootfs_dir'][key] = rootfs_dir
 
-def wic_create_subcommand(args, usage_str):
+
+def wic_create_subcommand(options, usage_str):
     """
     Command-line handling for image creation.  The real work is done
     by image.engine.wic_create()
     """
-    parser = optparse.OptionParser(usage=usage_str)
-
-    parser.add_option("-o", "--outdir", dest="outdir", default='.',
-                      help="name of directory to create image in")
-    parser.add_option("-e", "--image-name", dest="image_name",
-                      help="name of the image to use the artifacts from "
-                           "e.g. core-image-sato")
-    parser.add_option("-r", "--rootfs-dir", dest="rootfs_dir", type="string",
-                      action="callback", callback=callback_rootfs_dir,
-                      help="path to the /rootfs dir to use as the "
-                           ".wks rootfs source")
-    parser.add_option("-b", "--bootimg-dir", dest="bootimg_dir",
-                      help="path to the dir containing the boot artifacts "
-                           "(e.g. /EFI or /syslinux dirs) to use as the "
-                           ".wks bootimg source")
-    parser.add_option("-k", "--kernel-dir", dest="kernel_dir",
-                      help="path to the dir containing the kernel to use "
-                           "in the .wks bootimg")
-    parser.add_option("-n", "--native-sysroot", dest="native_sysroot",
-                      help="path to the native sysroot containing the tools "
-                           "to use to build the image")
-    parser.add_option("-s", "--skip-build-check", dest="build_check",
-                      action="store_false", default=True, help="skip the build check")
-    parser.add_option("-f", "--build-rootfs", action="store_true", help="build rootfs")
-    parser.add_option("-c", "--compress-with", choices=("gzip", "bzip2", "xz"),
-                      dest='compressor',
-                      help="compress image with specified compressor")
-    parser.add_option("-m", "--bmap", action="store_true", help="generate .bmap")
-    parser.add_option("-v", "--vars", dest='vars_dir',
-                      help="directory with <image>.env files that store "
-                           "bitbake variables")
-    parser.add_option("-D", "--debug", dest="debug", action="store_true",
-                      default=False, help="output debug information")
-
-    (options, args) = parser.parse_args(args)
-
-    if len(args) != 1:
-        parser.print_help()
-        raise WicError("Wrong number of arguments, exiting")
-
     if options.build_rootfs and not bitbake_main:
         raise WicError("Can't build rootfs as bitbake is not in the $PATH")
 
@@ -206,14 +170,14 @@ def wic_create_subcommand(args, usage_str):
             raise WicError("Unable to find the location of the native "
                            "tools sysroot to use")
 
-    wks_file = args[0]
+    wks_file = options.wks_file
 
     if not wks_file.endswith(".wks"):
         wks_file = engine.find_canned_image(scripts_path, wks_file)
         if not wks_file:
             raise WicError("No image named %s found, exiting.  (Use 'wic list images' "
                            "to list available images, or specify a fully-qualified OE "
-                           "kickstart (.wks) filename)" % args[0])
+                           "kickstart (.wks) filename)" % options.wks_file)
 
     if not options.image_name:
         rootfs_dir = ''
@@ -264,59 +228,146 @@ def wic_list_subcommand(args, usage_str):
     Command-line handling for listing available images.
     The real work is done by image.engine.wic_list()
     """
-    parser = optparse.OptionParser(usage=usage_str)
-    args = parser.parse_args(args)[1]
-
     if not engine.wic_list(args, scripts_path):
-        parser.print_help()
         raise WicError("Bad list arguments, exiting")
 
 
-def wic_help_topic_subcommand(args, usage_str):
+def wic_help_subcommand(args, usage_str):
     """
-    Command-line handling for help-only 'subcommands'.  This is
-    essentially a dummy command that doesn nothing but allow users to
-    use the existing subcommand infrastructure to display help on a
-    particular topic not attached to any particular subcommand.
+    Command-line handling for help subcommand to keep the current
+    structure of the function definitions.
     """
     pass
 
 
+def wic_help_topic_subcommand(usage_str, help_str):
+    """
+    Display function for help 'sub-subcommands'.
+    """
+    print(help_str)
+    return
+
+
 wic_help_topic_usage = """
 """
 
-subcommands = {
-    "create":    [wic_create_subcommand,
-                  hlp.wic_create_usage,
-                  hlp.wic_create_help],
-    "list":      [wic_list_subcommand,
-                  hlp.wic_list_usage,
-                  hlp.wic_list_help],
+helptopics = {
     "plugins":   [wic_help_topic_subcommand,
                   wic_help_topic_usage,
-                  hlp.get_wic_plugins_help],
+                  hlp.wic_plugins_help],
     "overview":  [wic_help_topic_subcommand,
                   wic_help_topic_usage,
                   hlp.wic_overview_help],
     "kickstart": [wic_help_topic_subcommand,
                   wic_help_topic_usage,
                   hlp.wic_kickstart_help],
+    "create":    [wic_help_topic_subcommand,
+                  wic_help_topic_usage,
+                  hlp.wic_create_help],
+    "list":      [wic_help_topic_subcommand,
+                  wic_help_topic_usage,
+                  hlp.wic_list_help]
 }
 
 
+def wic_init_parser_create(subparser):
+    subparser.add_argument("wks_file")
+
+    subparser.add_argument("-o", "--outdir", dest="outdir", default='.',
+                      help="name of directory to create image in")
+    subparser.add_argument("-e", "--image-name", dest="image_name",
+                      help="name of the image to use the artifacts from "
+                           "e.g. core-image-sato")
+    subparser.add_argument("-r", "--rootfs-dir", action=RootfsArgAction,
+                      help="path to the /rootfs dir to use as the "
+                           ".wks rootfs source")
+    subparser.add_argument("-b", "--bootimg-dir", dest="bootimg_dir",
+                      help="path to the dir containing the boot artifacts "
+                           "(e.g. /EFI or /syslinux dirs) to use as the "
+                           ".wks bootimg source")
+    subparser.add_argument("-k", "--kernel-dir", dest="kernel_dir",
+                      help="path to the dir containing the kernel to use "
+                           "in the .wks bootimg")
+    subparser.add_argument("-n", "--native-sysroot", dest="native_sysroot",
+                      help="path to the native sysroot containing the tools "
+                           "to use to build the image")
+    subparser.add_argument("-s", "--skip-build-check", dest="build_check",
+                      action="store_false", default=True, help="skip the build check")
+    subparser.add_argument("-f", "--build-rootfs", action="store_true", help="build rootfs")
+    subparser.add_argument("-c", "--compress-with", choices=("gzip", "bzip2", "xz"),
+                      dest='compressor',
+                      help="compress image with specified compressor")
+    subparser.add_argument("-m", "--bmap", action="store_true", help="generate .bmap")
+    subparser.add_argument("-v", "--vars", dest='vars_dir',
+                      help="directory with <image>.env files that store "
+                           "bitbake variables")
+    subparser.add_argument("-D", "--debug", dest="debug", action="store_true",
+                      default=False, help="output debug information")
+    return
+
+
+def wic_init_parser_list(subparser):
+    subparser.add_argument("list_type",
+                        help="can be 'images' or 'source-plugins' "
+                             "to obtain a list. "
+                             "If value is a valid .wks image file")
+    subparser.add_argument("help_for", default=[], nargs='*',
+                        help="If 'list_type' is a valid .wks image file "
+                             "this value can be 'help' to show the help information "
+                             "defined inside the .wks file")
+    return
+
+
+def wic_init_parser_help(subparser):
+    helpparsers = subparser.add_subparsers(dest='help_topic', help=hlp.wic_usage)
+    for helptopic in helptopics:
+        helpparsers.add_parser(helptopic, help=helptopics[helptopic][2])
+    return
+
+
+subcommands = {
+    "create":    [wic_create_subcommand,
+                  hlp.wic_create_usage,
+                  hlp.wic_create_help,
+                  wic_init_parser_create],
+    "list":      [wic_list_subcommand,
+                  hlp.wic_list_usage,
+                  hlp.wic_list_help,
+                  wic_init_parser_list],
+    "help":      [wic_help_subcommand,
+                  wic_help_topic_usage,
+                  hlp.wic_help_help,
+                  wic_init_parser_help]
+}
+
+
+def init_parser(parser):
+    parser.add_argument("--version", action="version",
+        version="%(prog)s {version}".format(version=__version__))
+    subparsers = parser.add_subparsers(dest='command', help=hlp.wic_usage)
+    for subcmd in subcommands:
+        subparser = subparsers.add_parser(subcmd, help=subcommands[subcmd][2])
+        subcommands[subcmd][3](subparser)
+
+
 def main(argv):
-    parser = optparse.OptionParser(version="wic version %s" % __version__,
-                                   usage=hlp.wic_usage)
+    parser = argparse.ArgumentParser(
+        description="wic version %s" % __version__)
 
-    parser.disable_interspersed_args()
+    init_parser(parser)
 
-    args = parser.parse_args(argv)[1]
+    args = parser.parse_args(argv)
 
-    if len(args):
-        if args[0] == "help":
-            if len(args) == 1:
+    if "command" in vars(args):
+        if args.command == "help":
+            if args.help_topic is None:
                 parser.print_help()
-                raise WicError("help command requires parameter")
+                print()
+                print("Please specify a help topic")
+            elif args.help_topic in helptopics:
+                hlpt = helptopics[args.help_topic]
+                hlpt[0](hlpt[1], hlpt[2])
+            return 0
 
     return hlp.invoke_subcommand(args, parser, hlp.wic_help_usage, subcommands)
 
-- 
2.11.0

[wic patch 3/5] wic: Add missing text to usage and help strings

[Andreas J. Reichel]

Add missing parameters -c to and remove non-existent
parameter -i from usage string for 'wic create'.

Signed-off-by: Andreas Reichel <andreas.reichel.ext@siemens.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Signed-off-by: Daniel Wagner <daniel.wagner@siemens.com>

---
 scripts/lib/wic/help.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/lib/wic/help.py b/scripts/lib/wic/help.py
index 2c2dbdc3b5..ba0c8e5eea 100644
--- a/scripts/lib/wic/help.py
+++ b/scripts/lib/wic/help.py
@@ -131,10 +131,10 @@ wic_create_usage = """
  Create a new OpenEmbedded image
 
  usage: wic create <wks file or image name> [-o <DIRNAME> | --outdir <DIRNAME>]
-            [-i <JSON PROPERTY FILE> | --infile <JSON PROPERTY_FILE>]
             [-e | --image-name] [-s, --skip-build-check] [-D, --debug]
             [-r, --rootfs-dir] [-b, --bootimg-dir]
             [-k, --kernel-dir] [-n, --native-sysroot] [-f, --build-rootfs]
+            [-c, --compress-with] [-m, --bmap]
 
  This command creates an OpenEmbedded image based on the 'OE kickstart
  commands' found in the <wks file>.
-- 
2.11.0

[wic patch 4/5] wic: Add option to keep partition images

[Andreas J. Reichel]

Add new option -P, --keep-partition-images to prevent
deletion of .p* files created by wic's direct imager plugin.

Signed-off-by: Andreas Reichel <andreas.reichel.ext@siemens.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Signed-off-by: Daniel Wagner <daniel.wagner@siemens.com>

---
 scripts/lib/wic/help.py                  |  5 +++++
 scripts/lib/wic/plugins/imager/direct.py | 28 +++++++++++++++++++---------
 scripts/wic                              |  2 ++
 3 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/scripts/lib/wic/help.py b/scripts/lib/wic/help.py
index ba0c8e5eea..026a383155 100644
--- a/scripts/lib/wic/help.py
+++ b/scripts/lib/wic/help.py
@@ -134,6 +134,7 @@ wic_create_usage = """
             [-e | --image-name] [-s, --skip-build-check] [-D, --debug]
             [-r, --rootfs-dir] [-b, --bootimg-dir]
             [-k, --kernel-dir] [-n, --native-sysroot] [-f, --build-rootfs]
+            [-P, --keep-partition-images]
             [-c, --compress-with] [-m, --bmap]
 
  This command creates an OpenEmbedded image based on the 'OE kickstart
@@ -155,6 +156,7 @@ SYNOPSIS
         [-e | --image-name] [-s, --skip-build-check] [-D, --debug]
         [-r, --rootfs-dir] [-b, --bootimg-dir]
         [-k, --kernel-dir] [-n, --native-sysroot] [-f, --build-rootfs]
+        [-P, --keep-partition-images]
         [-c, --compress-with] [-m, --bmap]
 
 DESCRIPTION
@@ -227,6 +229,9 @@ DESCRIPTION
 
     The -m option is used to produce .bmap file for the image. This file
     can be used to flash image using bmaptool utility.
+
+    The -p option is used to keep the individual partition image files .p*,
+    created by the direct imager plugin.
 """
 
 wic_list_usage = """
diff --git a/scripts/lib/wic/plugins/imager/direct.py b/scripts/lib/wic/plugins/imager/direct.py
index d6b47ff0bb..b27584bdbe 100644
--- a/scripts/lib/wic/plugins/imager/direct.py
+++ b/scripts/lib/wic/plugins/imager/direct.py
@@ -84,8 +84,12 @@ class DirectPlugin(ImagerPlugin):
                 break
 
         image_path = self._full_path(self.workdir, self.parts[0].disk, "direct")
+        self.keep_partimages = options.keep_partimages
+
         self._image = PartitionedImage(image_path, self.ptable_format,
-                                       self.parts, self.native_sysroot)
+                                       self.parts, self.native_sysroot,
+                                       keep_partimages=self.keep_partimages)
+
 
     def do_create(self):
         """
@@ -287,7 +291,9 @@ class PartitionedImage():
     Partitioned image in a file.
     """
 
-    def __init__(self, path, ptable_format, partitions, native_sysroot=None):
+    def __init__(self, path, ptable_format, partitions, native_sysroot=None,
+        keep_partimages=False):
+
         self.path = path  # Path to the image file
         self.numpart = 0  # Number of allocated partitions
         self.realpart = 0 # Number of partitions in the partition table
@@ -325,6 +331,8 @@ class PartitionedImage():
                 else: # msdos partition table
                     part.uuid = '%08x-%02d' % (self.identifier, part.realnum)
 
+        self.keep_partimages = keep_partimages
+
     def prepare(self, imager):
         """Prepare an image. Call prepare method of all image partitions."""
         for part in self.partitions:
@@ -540,13 +548,15 @@ class PartitionedImage():
 
     def cleanup(self):
         # remove partition images
-        for image in set(self.partimages):
-            try:
-                os.remove(image)
-            except IOError as e:
-                logger.warning(
-                    "Could not delete file. {0}: I/O error ({1}): {2}\n".format(
-                    image, e.errno, e.strerror))
+        if not self.keep_partimages:
+            # remove partition images
+            for image in set(self.partimages):
+                try:
+                    os.remove(image)
+                except IOError as e:
+                    logger.warning(
+                        "Could not delete file. {0}: I/O error ({1}): {2}\n".format(
+                        image, e.errno, e.strerror))
 
     def assemble(self):
         logger.debug("Installing partitions")
diff --git a/scripts/wic b/scripts/wic
index 49cad869e2..98f7405702 100755
--- a/scripts/wic
+++ b/scripts/wic
@@ -303,6 +303,8 @@ def wic_init_parser_create(subparser):
                            "bitbake variables")
     subparser.add_argument("-D", "--debug", dest="debug", action="store_true",
                       default=False, help="output debug information")
+    subparser.add_argument("-P", "--keep-partition-images", dest="keep_partimages",
+                      action="store_true", default=False)
     return
 
 
-- 
2.11.0

[wic patch 5/5] wic: Use enum like dicts for string constants

[Andreas J. Reichel]

To increase code maintainability, use dictionaries
as enum-like container for parameter string comparisons.

Signed-off-by: Andreas Reichel <andreas.reichel.ext@siemens.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Signed-off-by: Daniel Wagner <daniel.wagner@siemens.com>

---
 scripts/lib/wic/engine.py | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/scripts/lib/wic/engine.py b/scripts/lib/wic/engine.py
index 647358287f..1428a73ba8 100644
--- a/scripts/lib/wic/engine.py
+++ b/scripts/lib/wic/engine.py
@@ -35,6 +35,16 @@ from wic import WicError
 from wic.pluginbase import PluginMgr
 from wic.utils.misc import get_bitbake_var
 
+class StrEnum(dict):
+    __getattr__ = dict.get
+
+ListType = StrEnum({
+    'LIST_IMAGES': "images",
+    'LIST_SRC_PLUGINS': "source-plugins" })
+
+HelpArg = StrEnum({
+    'HELP': "help" })
+
 logger = logging.getLogger('wic')
 
 def verify_build_env():
@@ -204,14 +214,14 @@ def wic_list(args, scripts_path):
     if args.list_type is None:
         return False
 
-    if args.list_type == "images":
+    if args.list_type == ListType.LIST_IMAGES:
 
         list_canned_images(scripts_path)
         return True
-    elif args.list_type == "source-plugins":
+    elif args.list_type == ListType.LIST_SRC_PLUGINS:
         list_source_plugins()
         return True
-    elif len(args.help_for) == 1 and args.help_for[0] == 'help':
+    elif len(args.help_for) == 1 and args.help_for[0] == HelpArg.HELP:
         wks_file = args.list_type
         fullpath = find_canned_image(scripts_path, wks_file)
         if not fullpath:
-- 
2.11.0

Re: [wic patch 2/5] wic: Use argparse instead of optparse

[Burton, Ross]

[-- Attachment #1: Type: text/plain, Size: 1006 bytes --]

On 21 April 2017 at 13:11, Andreas J. Reichel <andreas.reichel@tngtech.com>
wrote:

> * optparse is deprecated and will not be developed further
>     (see: https://docs.python.org/2/library/optparse.html)
> * argparse supports subcommands, which simplifies definition of
>     arguments and options
> * reimplement help mechanism through sub-subcommands
>

This results in a behaviour change and breaks the selftest:

FAIL [0.535s]: test_unsupported_subcommand (oeqa.selftest.wic.Wic)
----------------------------------------------------------------------
Traceback (most recent call last):
  File
"/home/pokybuild/yocto-autobuilder/yocto-worker/nightly-oe-selftest/build/meta/lib/oeqa/utils/decorators.py",
line 109, in wrapped_f
    return func(*args, **kwargs)
  File
"/home/pokybuild/yocto-autobuilder/yocto-worker/nightly-oe-selftest/build/meta/lib/oeqa/selftest/wic.py",
line 163, in test_unsupported_subcommand
    ignore_status=True).status)
AssertionError: 1 != 2

Ross

[-- Attachment #2: Type: text/html, Size: 1680 bytes --]

Re: [wic patch 2/5] wic: Use argparse instead of optparse

[Andreas Reichel]

On Sun, Apr 23, 2017 at 08:58:44PM +0100, Burton, Ross wrote:

> 
> On 21 April 2017 at 13:11, Andreas J. Reichel <andreas.reichel@tngtech.com>
> wrote:
> 
>     * optparse is deprecated and will not be developed further
>         (see: https://docs.python.org/2/library/optparse.html)
>     * argparse supports subcommands, which simplifies definition of
>         arguments and options
>     * reimplement help mechanism through sub-subcommands
> 
> 
> This results in a behaviour change and breaks the selftest:
> 
> FAIL [0.535s]: test_unsupported_subcommand (oeqa.selftest.wic.Wic)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
>   File "/home/pokybuild/yocto-autobuilder/yocto-worker/nightly-oe-selftest/
> build/meta/lib/oeqa/utils/decorators.py", line 109, in wrapped_f
>     return func(*args, **kwargs)
>   File "/home/pokybuild/yocto-autobuilder/yocto-worker/nightly-oe-selftest/
> build/meta/lib/oeqa/selftest/wic.py", line 163, in test_unsupported_subcommand
>     ignore_status=True).status)
> AssertionError: 1 != 2
> 
Dear Ross,

I see three possible strategies in fixing this problem:
- Change the code to keep current return values / behavior
- Adapt the unit test to fit the new code
- Write a less complex patch for just adding the option
  "--keep-partition-images" without migration to argparse

Which way would you prefer it?

Andreas

> Ross

-- 
Andreas Reichel 
Dipl.-Phys. (Univ.) 
Software Consultant

Andreas.Reichel@tngtech.com 
+49-174-3180074

TNG Technology Consulting GmbH, Betastr. 13a, 85774 Unterföhring
Geschäftsführer: Henrik Klagges, Christoph Stock, Dr. Robert Dahlke
Sitz: Unterföhring * Amtsgericht München * HRB 135082

Re: [wic patch 2/5] wic: Use argparse instead of optparse

[Burton, Ross]

[-- Attachment #1: Type: text/plain, Size: 526 bytes --]

On 26 April 2017 at 11:34, Andreas Reichel <Andreas.Reichel@tngtech.com>
wrote:

> - Change the code to keep current return values / behavior
> - Adapt the unit test to fit the new code
> - Write a less complex patch for just adding the option
>   "--keep-partition-images" without migration to argparse
>

It looks like the exit 2 is deeply nested into the argparse code, so I
guess the test should be updated.  The test could be made more resilient by
checking that the output wasn't 0 instead of was 1.

Ross

[-- Attachment #2: Type: text/html, Size: 1038 bytes --]

Re: [wic patch 1/5] wic: Catch errors during image files clean-up

[Ed Bartosh]

On Fri, Apr 21, 2017 at 02:11:41PM +0200, Andreas J. Reichel wrote:
> Handle exception if a file could not be deleted during clean-up of
> unwanted files, thus preventing a failure of wic in this case.

Can you explain why partition images can't be deleted?
As wic creates them it's not obvious why it can't remove them.

> Signed-off-by: Andreas Reichel <andreas.reichel.ext@siemens.com>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> Signed-off-by: Daniel Wagner <daniel.wagner@siemens.com>
> 
> ---
>  scripts/lib/wic/plugins/imager/direct.py | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/lib/wic/plugins/imager/direct.py b/scripts/lib/wic/plugins/imager/direct.py
> index f2e6127331..d6b47ff0bb 100644
> --- a/scripts/lib/wic/plugins/imager/direct.py
> +++ b/scripts/lib/wic/plugins/imager/direct.py
> @@ -541,7 +541,12 @@ class PartitionedImage():
>      def cleanup(self):
>          # remove partition images
>          for image in set(self.partimages):
> -            os.remove(image)
> +            try:
> +                os.remove(image)
> +            except IOError as e:
> +                logger.warning(
> +                    "Could not delete file. {0}: I/O error ({1}): {2}\n".format(
> +                    image, e.errno, e.strerror))
>  
>      def assemble(self):
>          logger.debug("Installing partitions")
> -- 
> 2.11.0
> 
> -- 
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-core

-- 
--
Regards,
Ed

Re: [wic patch 5/5] wic: Use enum like dicts for string constants

[Ed Bartosh]

On Fri, Apr 21, 2017 at 02:11:45PM +0200, Andreas J. Reichel wrote:
> To increase code maintainability, use dictionaries
> as enum-like container for parameter string comparisons.
> 
> Signed-off-by: Andreas Reichel <andreas.reichel.ext@siemens.com>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> Signed-off-by: Daniel Wagner <daniel.wagner@siemens.com>
> 
> ---
>  scripts/lib/wic/engine.py | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/scripts/lib/wic/engine.py b/scripts/lib/wic/engine.py
> index 647358287f..1428a73ba8 100644
> --- a/scripts/lib/wic/engine.py
> +++ b/scripts/lib/wic/engine.py
> @@ -35,6 +35,16 @@ from wic import WicError
>  from wic.pluginbase import PluginMgr
>  from wic.utils.misc import get_bitbake_var
>  
> +class StrEnum(dict):
> +    __getattr__ = dict.get
> +
> +ListType = StrEnum({
> +    'LIST_IMAGES': "images",
> +    'LIST_SRC_PLUGINS': "source-plugins" })
> +
> +HelpArg = StrEnum({
> +    'HELP': "help" })
> +
>  logger = logging.getLogger('wic')
>  
>  def verify_build_env():
> @@ -204,14 +214,14 @@ def wic_list(args, scripts_path):
>      if args.list_type is None:
>          return False
>  
> -    if args.list_type == "images":
> +    if args.list_type == ListType.LIST_IMAGES:
>  
>          list_canned_images(scripts_path)
>          return True
> -    elif args.list_type == "source-plugins":
> +    elif args.list_type == ListType.LIST_SRC_PLUGINS:
>          list_source_plugins()
>          return True
> -    elif len(args.help_for) == 1 and args.help_for[0] == 'help':
> +    elif len(args.help_for) == 1 and args.help_for[0] == HelpArg.HELP:
>          wks_file = args.list_type
>          fullpath = find_canned_image(scripts_path, wks_file)
>          if not fullpath:

I'm not sure if this increases maintainability, but it definitely increases code complexity and
decreases readability. Can you explain the idea in a bit more detailed way?

--
Regards,
Ed

Re: [wic patch 0/5] Add option to wic and use argparse

[Ed Bartosh]

On Fri, Apr 21, 2017 at 02:11:40PM +0200, Andreas J. Reichel wrote:
>     * wic uses optparse and a rather complicated approach of its
>         initialization. It is much easier to switch from the
>         deprecated optparse to the argparse library, which also
>         supports sub-commands, which wic heavily makes use of.
>         The second patch replaces optparse by argparse.

Thank you for the patchset!

I'm going to fix failing test case and test argparse-related patches
as a separate patchset.

Feel free to update and re-send the rest of the patches.

--
Regards,
Ed

Re: [wic patch 1/5] wic: Catch errors during image files clean-up

[Andreas Reichel]

[-- Attachment #1: message --]
[-- Type: text/plain, Size: 3069 bytes --]

On Tue, May 02, 2017 at 03:56:38PM +0300, Ed Bartosh wrote:
> On Fri, Apr 21, 2017 at 02:11:41PM +0200, Andreas J. Reichel wrote:
> > Handle exception if a file could not be deleted during clean-up of
> > unwanted files, thus preventing a failure of wic in this case.
> 
> Can you explain why partition images can't be deleted?
> As wic creates them it's not obvious why it can't remove them.
> 
As part of our internal project, we generate artifacts to be packaged
in further archives and only contain one single partition image. The
image generation process is solely done by wic and post-processing
scripts, therefore we always used the .p2 file for the root partition,
that had already peen patched (for example fstab by wic). Suddenly the
file was missing and CI was not working anymore. Here, instead of
just stating, that *nobody* needs these files, my idea was to
introduce a parameter, so that people can chose if they want them or
not. 
We do not want to generate any image artifacts with bitbake to keep
things separated better.
Just saving space is not a valid argument to me because building
a system with bitbake needs up to 50 GB and if you do it for several
machines, a few hundred MB should not matter. However, no problem for me
to delete them per standard, if you say *most* people don' use them.

> > Signed-off-by: Andreas Reichel <andreas.reichel.ext@siemens.com>
> > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > Signed-off-by: Daniel Wagner <daniel.wagner@siemens.com>
> > 
> > ---
> >  scripts/lib/wic/plugins/imager/direct.py | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/scripts/lib/wic/plugins/imager/direct.py b/scripts/lib/wic/plugins/imager/direct.py
> > index f2e6127331..d6b47ff0bb 100644
> > --- a/scripts/lib/wic/plugins/imager/direct.py
> > +++ b/scripts/lib/wic/plugins/imager/direct.py
> > @@ -541,7 +541,12 @@ class PartitionedImage():
> >      def cleanup(self):
> >          # remove partition images
> >          for image in set(self.partimages):
> > -            os.remove(image)
> > +            try:
> > +                os.remove(image)
> > +            except IOError as e:
> > +                logger.warning(
> > +                    "Could not delete file. {0}: I/O error ({1}): {2}\n".format(
> > +                    image, e.errno, e.strerror))
> >  
> >      def assemble(self):
> >          logger.debug("Installing partitions")
> > -- 
> > 2.11.0
> > 
> > -- 
> > _______________________________________________
> > Openembedded-core mailing list
> > Openembedded-core@lists.openembedded.org
> > http://lists.openembedded.org/mailman/listinfo/openembedded-core
> 
> -- 
> --
> Regards,
> Ed

-- 
Andreas Reichel 
Dipl.-Phys. (Univ.) 
Software Consultant

Andreas.Reichel@tngtech.com 
+49-174-3180074

TNG Technology Consulting GmbH, Betastr. 13a, 85774 Unterföhring
Geschäftsführer: Henrik Klagges, Christoph Stock, Dr. Robert Dahlke
Sitz: Unterföhring * Amtsgericht München * HRB 135082

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 849 bytes --]

Re: [wic patch 5/5] wic: Use enum like dicts for string constants

[Andreas Reichel]

[-- Attachment #1: message --]
[-- Type: text/plain, Size: 2816 bytes --]

On Tue, May 02, 2017 at 04:36:22PM +0300, Ed Bartosh wrote:
> On Fri, Apr 21, 2017 at 02:11:45PM +0200, Andreas J. Reichel wrote:
> > To increase code maintainability, use dictionaries
> > as enum-like container for parameter string comparisons.
> > 
> > Signed-off-by: Andreas Reichel <andreas.reichel.ext@siemens.com>
> > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > Signed-off-by: Daniel Wagner <daniel.wagner@siemens.com>
> > 
> > ---
> >  scripts/lib/wic/engine.py | 16 +++++++++++++---
> >  1 file changed, 13 insertions(+), 3 deletions(-)
> > 
> > diff --git a/scripts/lib/wic/engine.py b/scripts/lib/wic/engine.py
> > index 647358287f..1428a73ba8 100644
> > --- a/scripts/lib/wic/engine.py
> > +++ b/scripts/lib/wic/engine.py
> > @@ -35,6 +35,16 @@ from wic import WicError
> >  from wic.pluginbase import PluginMgr
> >  from wic.utils.misc import get_bitbake_var
> >  
> > +class StrEnum(dict):
> > +    __getattr__ = dict.get
> > +
> > +ListType = StrEnum({
> > +    'LIST_IMAGES': "images",
> > +    'LIST_SRC_PLUGINS': "source-plugins" })
> > +
> > +HelpArg = StrEnum({
> > +    'HELP': "help" })
> > +
> >  logger = logging.getLogger('wic')
> >  
> >  def verify_build_env():
> > @@ -204,14 +214,14 @@ def wic_list(args, scripts_path):
> >      if args.list_type is None:
> >          return False
> >  
> > -    if args.list_type == "images":
> > +    if args.list_type == ListType.LIST_IMAGES:
> >  
> >          list_canned_images(scripts_path)
> >          return True
> > -    elif args.list_type == "source-plugins":
> > +    elif args.list_type == ListType.LIST_SRC_PLUGINS:
> >          list_source_plugins()
> >          return True
> > -    elif len(args.help_for) == 1 and args.help_for[0] == 'help':
> > +    elif len(args.help_for) == 1 and args.help_for[0] == HelpArg.HELP:
> >          wks_file = args.list_type
> >          fullpath = find_canned_image(scripts_path, wks_file)
> >          if not fullpath:
> 
> I'm not sure if this increases maintainability, but it definitely increases code complexity and
> decreases readability. Can you explain the idea in a bit more detailed way?
> 
In general, if string constants are used as keys, it is better do define them as
constants. Maintainability increases because if you have to change them,
you only change them in one place. You have a central place for this
definition and not scattered them all over the code. That's the idea.

> --
> Regards,
> Ed

-- 
Andreas Reichel 
Dipl.-Phys. (Univ.) 
Software Consultant

Andreas.Reichel@tngtech.com 
+49-174-3180074

TNG Technology Consulting GmbH, Betastr. 13a, 85774 Unterföhring
Geschäftsführer: Henrik Klagges, Christoph Stock, Dr. Robert Dahlke
Sitz: Unterföhring * Amtsgericht München * HRB 135082

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 849 bytes --]

Re: [wic patch 0/5] Add option to wic and use argparse

[Andreas Reichel]

[-- Attachment #1: message --]
[-- Type: text/plain, Size: 1115 bytes --]

On Tue, May 02, 2017 at 05:37:31PM +0300, Ed Bartosh wrote:
> On Fri, Apr 21, 2017 at 02:11:40PM +0200, Andreas J. Reichel wrote:
> >     * wic uses optparse and a rather complicated approach of its
> >         initialization. It is much easier to switch from the
> >         deprecated optparse to the argparse library, which also
> >         supports sub-commands, which wic heavily makes use of.
> >         The second patch replaces optparse by argparse.
> 
> Thank you for the patchset!
> 
> I'm going to fix failing test case and test argparse-related patches
> as a separate patchset.
> 
> Feel free to update and re-send the rest of the patches.
> 
Thank you. So then I wait until argparse is successfully integrated
and rebase the option-patch.

Andreas
> --
> Regards,
> Ed

-- 
Andreas Reichel 
Dipl.-Phys. (Univ.) 
Software Consultant

Andreas.Reichel@tngtech.com 
+49-174-3180074

TNG Technology Consulting GmbH, Betastr. 13a, 85774 Unterföhring
Geschäftsführer: Henrik Klagges, Christoph Stock, Dr. Robert Dahlke
Sitz: Unterföhring * Amtsgericht München * HRB 135082

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 849 bytes --]

Re: [wic patch 1/5] wic: Catch errors during image files clean-up

[Ed Bartosh]

On Wed, May 03, 2017 at 10:45:52AM +0200, Andreas Reichel wrote:
> On Tue, May 02, 2017 at 03:56:38PM +0300, Ed Bartosh wrote:
> > On Fri, Apr 21, 2017 at 02:11:41PM +0200, Andreas J. Reichel wrote:
> > > Handle exception if a file could not be deleted during clean-up of
> > > unwanted files, thus preventing a failure of wic in this case.
> > 
> > Can you explain why partition images can't be deleted?
> > As wic creates them it's not obvious why it can't remove them.
> > 
> As part of our internal project, we generate artifacts to be packaged
> in further archives and only contain one single partition image. The
> image generation process is solely done by wic and post-processing
> scripts, therefore we always used the .p2 file for the root partition,
> that had already peen patched (for example fstab by wic). Suddenly the
> file was missing and CI was not working anymore.
I'd suggest to investigate this further and find out why it disappeared.

> Here, instead of
> just stating, that *nobody* needs these files, my idea was to
> introduce a parameter, so that people can chose if they want them or
> not.
This makes sense to do. However, it doesn't explain nor justify this
particual change.

> We do not want to generate any image artifacts with bitbake to keep
> things separated better.
> Just saving space is not a valid argument to me because building
> a system with bitbake needs up to 50 GB and if you do it for several
> machines, a few hundred MB should not matter. However, no problem for me
> to delete them per standard, if you say *most* people don' use them.
I'm ok with the option to preserve partition files. I don't understand
the reason for this change though. It looks like a workaround to me.
It's better to find a real reason and fix it.


> > > Signed-off-by: Andreas Reichel <andreas.reichel.ext@siemens.com>
> > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > > Signed-off-by: Daniel Wagner <daniel.wagner@siemens.com>
> > > 
> > > ---
> > >  scripts/lib/wic/plugins/imager/direct.py | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/scripts/lib/wic/plugins/imager/direct.py b/scripts/lib/wic/plugins/imager/direct.py
> > > index f2e6127331..d6b47ff0bb 100644
> > > --- a/scripts/lib/wic/plugins/imager/direct.py
> > > +++ b/scripts/lib/wic/plugins/imager/direct.py
> > > @@ -541,7 +541,12 @@ class PartitionedImage():
> > >      def cleanup(self):
> > >          # remove partition images
> > >          for image in set(self.partimages):
> > > -            os.remove(image)
> > > +            try:
> > > +                os.remove(image)
> > > +            except IOError as e:
> > > +                logger.warning(
> > > +                    "Could not delete file. {0}: I/O error ({1}): {2}\n".format(
> > > +                    image, e.errno, e.strerror))
> > >  
> > >      def assemble(self):
> > >          logger.debug("Installing partitions")
> > > -- 
> > > 2.11.0
> > > 
> > > -- 
> > > _______________________________________________
> > > Openembedded-core mailing list
> > > Openembedded-core@lists.openembedded.org
> > > http://lists.openembedded.org/mailman/listinfo/openembedded-core
> > 
> > -- 
> > --
> > Regards,
> > Ed
> 
> -- 
> Andreas Reichel 
> Dipl.-Phys. (Univ.) 
> Software Consultant
> 
> Andreas.Reichel@tngtech.com 
> +49-174-3180074
> 
> TNG Technology Consulting GmbH, Betastr. 13a, 85774 Unterföhring
> Geschäftsführer: Henrik Klagges, Christoph Stock, Dr. Robert Dahlke
> Sitz: Unterföhring * Amtsgericht München * HRB 135082



-- 
--
Regards,
Ed

Re: [wic patch 5/5] wic: Use enum like dicts for string constants

[Ed Bartosh]

On Wed, May 03, 2017 at 10:47:45AM +0200, Andreas Reichel wrote:
> On Tue, May 02, 2017 at 04:36:22PM +0300, Ed Bartosh wrote:
> > On Fri, Apr 21, 2017 at 02:11:45PM +0200, Andreas J. Reichel wrote:
> > > To increase code maintainability, use dictionaries
> > > as enum-like container for parameter string comparisons.
> > > 
> > > Signed-off-by: Andreas Reichel <andreas.reichel.ext@siemens.com>
> > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > > Signed-off-by: Daniel Wagner <daniel.wagner@siemens.com>
> > > 
> > > ---
> > >  scripts/lib/wic/engine.py | 16 +++++++++++++---
> > >  1 file changed, 13 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/scripts/lib/wic/engine.py b/scripts/lib/wic/engine.py
> > > index 647358287f..1428a73ba8 100644
> > > --- a/scripts/lib/wic/engine.py
> > > +++ b/scripts/lib/wic/engine.py
> > > @@ -35,6 +35,16 @@ from wic import WicError
> > >  from wic.pluginbase import PluginMgr
> > >  from wic.utils.misc import get_bitbake_var
> > >  
> > > +class StrEnum(dict):
> > > +    __getattr__ = dict.get
> > > +
> > > +ListType = StrEnum({
> > > +    'LIST_IMAGES': "images",
> > > +    'LIST_SRC_PLUGINS': "source-plugins" })
> > > +
> > > +HelpArg = StrEnum({
> > > +    'HELP': "help" })
> > > +
> > >  logger = logging.getLogger('wic')
> > >  
> > >  def verify_build_env():
> > > @@ -204,14 +214,14 @@ def wic_list(args, scripts_path):
> > >      if args.list_type is None:
> > >          return False
> > >  
> > > -    if args.list_type == "images":
> > > +    if args.list_type == ListType.LIST_IMAGES:
> > >  
> > >          list_canned_images(scripts_path)
> > >          return True
> > > -    elif args.list_type == "source-plugins":
> > > +    elif args.list_type == ListType.LIST_SRC_PLUGINS:
> > >          list_source_plugins()
> > >          return True
> > > -    elif len(args.help_for) == 1 and args.help_for[0] == 'help':
> > > +    elif len(args.help_for) == 1 and args.help_for[0] == HelpArg.HELP:
> > >          wks_file = args.list_type
> > >          fullpath = find_canned_image(scripts_path, wks_file)
> > >          if not fullpath:
> > 
> > I'm not sure if this increases maintainability, but it definitely increases code complexity and
> > decreases readability. Can you explain the idea in a bit more detailed way?
> > 
> In general, if string constants are used as keys, it is better do define them as
> constants. Maintainability increases because if you have to change them,
> you only change them in one place. You have a central place for this
> definition and not scattered them all over the code. That's the idea.
>

Not sure I'm convinced. From my point of view this patch changes quite
simple and readable code to something more complex and less readable for
no visible reason.

If I have to change 'images' or 'source-plugins'(why would I want to do that?)
I'd just go and change them in one place as they're not used anywhere else.

--
Regards,
Ed