[PATCH v2 00/10] devtool refactoring

[PATCH v2 00/10] devtool refactoring

[Markus Lehtonen]

Second iteration of my refactoring patchset. Fixed mistakes in
- "devtool: simplify few conditionals a bit"
- "devtool: slight simplification of path splitting logic"
which caused the unit tests to fail.

The following changes since commit 7f85e74d5c53b34e5f470967fdbdbd19fed1929a:

  sysvinit: Only enable recipe in builds where its applicable (2015-06-10 12:03:14 +0100)

are available in the git repository at:

  git://git.openembedded.org/openembedded-core-contrib marquiz/devtool/refactor
  http://git.openembedded.org/openembedded-core-contrib/commit/?h=marquiz/devtool/refactor

----------------------------------------------------------------
Markus Lehtonen (10):
  devtool: fix wrong indentation
  devtool: refactor bb task execution into a separate class
  devtool: update-recipe: do rev parsing in a separate function
  devtool: simplify the logic of determining patches to be removed
  devtool: simplify few conditionals a bit
  devtool: slight simplification of path splitting logic
  devtool: split out 'srcrev' update mode into a separate function
  devtool: split out 'patch' update mode into a separate function
  devtool: remove some unused return values
  devtool: use DevtoolError for error handling

 scripts/devtool                 |   9 +-
 scripts/lib/devtool/__init__.py |   6 +
 scripts/lib/devtool/deploy.py   |  31 +-
 scripts/lib/devtool/standard.py | 650 +++++++++++++++++++++-------------------
 4 files changed, 372 insertions(+), 324 deletions(-)

-- 
2.1.4

[PATCH v2 01/10] devtool: fix wrong indentation

[Markus Lehtonen]

Signed-off-by: Markus Lehtonen <markus.lehtonen@linux.intel.com>
---
 scripts/lib/devtool/standard.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/lib/devtool/standard.py b/scripts/lib/devtool/standard.py
index c5b32d8..1e99413 100644
--- a/scripts/lib/devtool/standard.py
+++ b/scripts/lib/devtool/standard.py
@@ -657,8 +657,8 @@ def update_recipe(args, config, basepath, workspace):
                         for newpatch in newpatches:
                             if seqpatch_re.search(newpatch) and patchfile[5:] == newpatch[5:]:
                                 break
-                        else:
-                            removepatches.append(patch)
+                            else:
+                                removepatches.append(patch)
                     elif patchfile not in newpatches:
                         removepatches.append(patch)
             finally:
-- 
2.1.4

[PATCH v2 02/10] devtool: refactor bb task execution into a separate class

[Markus Lehtonen]

Signed-off-by: Markus Lehtonen <markus.lehtonen@linux.intel.com>
---
 scripts/lib/devtool/standard.py | 50 ++++++++++++++++++++++++-----------------
 1 file changed, 30 insertions(+), 20 deletions(-)

diff --git a/scripts/lib/devtool/standard.py b/scripts/lib/devtool/standard.py
index 1e99413..aed76ea 100644
--- a/scripts/lib/devtool/standard.py
+++ b/scripts/lib/devtool/standard.py
@@ -213,6 +213,32 @@ def extract(args, config, basepath, workspace):
     else:
         return -1
 
+class BbTaskExecutor(object):
+    """Class for executing bitbake tasks for a recipe
+
+    FIXME: This is very awkward. Unfortunately it's not currently easy to
+    properly execute tasks outside of bitbake itself, until then this has to
+    suffice if we are to handle e.g. linux-yocto's extra tasks
+    """
+
+    def __init__(self, rdata):
+        self.rdata = rdata
+        self.executed = []
+
+    def exec_func(self, func, report):
+        """Run bitbake task function"""
+        if not func in self.executed:
+            deps = self.rdata.getVarFlag(func, 'deps')
+            if deps:
+                for taskdepfunc in deps:
+                    self.exec_func(taskdepfunc, True)
+            if report:
+                logger.info('Executing %s...' % func)
+            fn = self.rdata.getVar('FILE', True)
+            localdata = bb.build._task_data(fn, func, self.rdata)
+            bb.build.exec_func(func, localdata)
+            self.executed.append(func)
+
 
 def _extract_source(srctree, keep_temp, devbranch, d):
     """Extract sources of a recipe"""
@@ -270,28 +296,12 @@ def _extract_source(srctree, keep_temp, devbranch, d):
             # We don't want to move the source to STAGING_KERNEL_DIR here
             crd.setVar('STAGING_KERNEL_DIR', '${S}')
 
-        # FIXME: This is very awkward. Unfortunately it's not currently easy to properly
-        # execute tasks outside of bitbake itself, until then this has to suffice if we
-        # are to handle e.g. linux-yocto's extra tasks
-        executed = []
-        def exec_task_func(func, report):
-            """Run specific bitbake task for a recipe"""
-            if not func in executed:
-                deps = crd.getVarFlag(func, 'deps')
-                if deps:
-                    for taskdepfunc in deps:
-                        exec_task_func(taskdepfunc, True)
-                if report:
-                    logger.info('Executing %s...' % func)
-                fn = d.getVar('FILE', True)
-                localdata = bb.build._task_data(fn, func, crd)
-                bb.build.exec_func(func, localdata)
-                executed.append(func)
+        task_executor = BbTaskExecutor(crd)
 
         logger.info('Fetching %s...' % pn)
-        exec_task_func('do_fetch', False)
+        task_executor.exec_func('do_fetch', False)
         logger.info('Unpacking...')
-        exec_task_func('do_unpack', False)
+        task_executor.exec_func('do_unpack', False)
         srcsubdir = crd.getVar('S', True)
         if srcsubdir == workdir:
             # Find non-patch sources that were "unpacked" to srctree directory
@@ -343,7 +353,7 @@ def _extract_source(srctree, keep_temp, devbranch, d):
             crd.setVar('PATCHTOOL', 'git')
 
         logger.info('Patching...')
-        exec_task_func('do_patch', False)
+        task_executor.exec_func('do_patch', False)
 
         bb.process.run('git tag -f devtool-patched', cwd=srcsubdir)
 
-- 
2.1.4

[PATCH v2 03/10] devtool: update-recipe: do rev parsing in a separate function

[Markus Lehtonen]

Split out the logic of determining "initial rev" and "update rev" into a
separate function.

Signed-off-by: Markus Lehtonen <markus.lehtonen@linux.intel.com>
---
 scripts/lib/devtool/standard.py | 55 +++++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 24 deletions(-)

diff --git a/scripts/lib/devtool/standard.py b/scripts/lib/devtool/standard.py
index aed76ea..c8ba247 100644
--- a/scripts/lib/devtool/standard.py
+++ b/scripts/lib/devtool/standard.py
@@ -514,6 +514,36 @@ def modify(args, config, basepath, workspace):
 
     return 0
 
+def _get_patchset_revs(args, srctree, recipe_path):
+    """Get initial and update rev of a recipe. These are the start point of the
+    whole patchset and start point for the patches to be re-generated/updated.
+    """
+    import bb
+
+    if args.initial_rev:
+        return args.initial_rev, args.initial_rev
+
+    # Parse initial rev from recipe
+    commits = []
+    initial_rev = None
+    with open(recipe_path, 'r') as f:
+        for line in f:
+            if line.startswith('# initial_rev:'):
+                initial_rev = line.split(':')[-1].strip()
+            elif line.startswith('# commit:'):
+                commits.append(line.split(':')[-1].strip())
+
+    update_rev = initial_rev
+    if initial_rev:
+        # Find first actually changed revision
+        stdout, _ = bb.process.run('git rev-list --reverse %s..HEAD' %
+                                   initial_rev, cwd=srctree)
+        newcommits = stdout.split()
+        for i in xrange(min(len(commits), len(newcommits))):
+            if newcommits[i] == commits[i]:
+                update_rev = commits[i]
+
+    return initial_rev, update_rev
 
 def update_recipe(args, config, basepath, workspace):
     """Entry point for the devtool 'update-recipe' subcommand"""
@@ -618,34 +648,11 @@ def update_recipe(args, config, basepath, workspace):
             logger.info('You will need to update SRC_URI within the recipe to point to a git repository where you have pushed your changes')
 
     elif mode == 'patch':
-        commits = []
-        update_rev = None
-        if args.initial_rev:
-            initial_rev = args.initial_rev
-        else:
-            initial_rev = None
-            with open(append, 'r') as f:
-                for line in f:
-                    if line.startswith('# initial_rev:'):
-                        initial_rev = line.split(':')[-1].strip()
-                    elif line.startswith('# commit:'):
-                        commits.append(line.split(':')[-1].strip())
-
-            if initial_rev:
-                # Find first actually changed revision
-                (stdout, _) = bb.process.run('git rev-list --reverse %s..HEAD' % initial_rev, cwd=srctree)
-                newcommits = stdout.split()
-                for i in xrange(min(len(commits), len(newcommits))):
-                    if newcommits[i] == commits[i]:
-                        update_rev = commits[i]
-
+        initial_rev, update_rev = _get_patchset_revs(args, srctree, append)
         if not initial_rev:
             logger.error('Unable to find initial revision - please specify it with --initial-rev')
             return -1
 
-        if not update_rev:
-            update_rev = initial_rev
-
         # Find list of existing patches in recipe file
         existing_patches = oe.recipeutils.get_recipe_patches(rd)
 
-- 
2.1.4

[PATCH v2 04/10] devtool: simplify the logic of determining patches to be removed

[Markus Lehtonen]

A slight simplification of the code.

Signed-off-by: Markus Lehtonen <markus.lehtonen@linux.intel.com>
---
 scripts/lib/devtool/standard.py | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/scripts/lib/devtool/standard.py b/scripts/lib/devtool/standard.py
index c8ba247..aa95e6e 100644
--- a/scripts/lib/devtool/standard.py
+++ b/scripts/lib/devtool/standard.py
@@ -657,26 +657,23 @@ def update_recipe(args, config, basepath, workspace):
         existing_patches = oe.recipeutils.get_recipe_patches(rd)
 
         removepatches = []
-        seqpatch_re = re.compile('^[0-9]{4}-')
+        seqpatch_re = re.compile('^([0-9]{4}-)?(.+)')
         if not args.no_remove:
             # Get all patches from source tree and check if any should be removed
             tempdir = tempfile.mkdtemp(prefix='devtool')
             try:
                 GitApplyTree.extractPatches(srctree, initial_rev, tempdir)
-                newpatches = os.listdir(tempdir)
+                # Strip numbering from patch names. If it's a git sequence
+                # named patch, the numbers might not match up since we are
+                # starting from a different revision This does assume that
+                # people are using unique shortlog values, but they ought to be
+                # anyway...
+                newpatches = [seqpatch_re.match(fname).group(2) for fname in
+                              os.listdir(tempdir)]
                 for patch in existing_patches:
-                    # If it's a git sequence named patch, the numbers might not match up
-                    # since we are starting from a different revision
-                    # This does assume that people are using unique shortlog values, but
-                    # they ought to be anyway...
-                    patchfile = os.path.basename(patch)
-                    if seqpatch_re.search(patchfile):
-                        for newpatch in newpatches:
-                            if seqpatch_re.search(newpatch) and patchfile[5:] == newpatch[5:]:
-                                break
-                            else:
-                                removepatches.append(patch)
-                    elif patchfile not in newpatches:
+                    basename = seqpatch_re.match(
+                                    os.path.basename(patch)).group(2)
+                    if basename not in newpatches:
                         removepatches.append(patch)
             finally:
                 shutil.rmtree(tempdir)
-- 
2.1.4

[PATCH v2 05/10] devtool: simplify few conditionals a bit

[Markus Lehtonen]

Just refactor the code.

Signed-off-by: Markus Lehtonen <markus.lehtonen@linux.intel.com>
---
 scripts/lib/devtool/standard.py | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/scripts/lib/devtool/standard.py b/scripts/lib/devtool/standard.py
index aa95e6e..4b9cebb 100644
--- a/scripts/lib/devtool/standard.py
+++ b/scripts/lib/devtool/standard.py
@@ -57,10 +57,9 @@ def add(args, config, basepath, workspace):
             elif os.listdir(srctree):
                 logger.error("Cannot fetch into source tree path %s as it already exists and is non-empty" % srctree)
                 return 1
-    else:
-        if not args.fetch:
-            logger.error("Specified source tree %s could not be found" % srctree)
-            return 1
+    elif not args.fetch:
+        logger.error("Specified source tree %s could not be found" % srctree)
+        return 1
 
     appendpath = os.path.join(config.workspace_path, 'appends')
     if not os.path.exists(appendpath):
@@ -424,10 +423,9 @@ def modify(args, config, basepath, workspace):
         logger.error("recipe %s is already in your workspace" % args.recipename)
         return -1
 
-    if not args.extract:
-        if not os.path.isdir(args.srctree):
-            logger.error("directory %s does not exist or not a directory (specify -x to extract source from recipe)" % args.srctree)
-            return -1
+    if not args.extract and not os.path.isdir(args.srctree):
+        logger.error("directory %s does not exist or not a directory (specify -x to extract source from recipe)" % args.srctree)
+        return -1
 
     tinfoil = setup_tinfoil()
 
-- 
2.1.4

[PATCH v2 06/10] devtool: slight simplification of path splitting logic

[Markus Lehtonen]

Signed-off-by: Markus Lehtonen <markus.lehtonen@linux.intel.com>
---
 scripts/lib/devtool/standard.py | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/scripts/lib/devtool/standard.py b/scripts/lib/devtool/standard.py
index 4b9cebb..c92c9ae 100644
--- a/scripts/lib/devtool/standard.py
+++ b/scripts/lib/devtool/standard.py
@@ -464,13 +464,12 @@ def modify(args, config, basepath, workspace):
                 initial_rev = stdout.rstrip()
 
     # Check that recipe isn't using a shared workdir
-    s = rd.getVar('S', True)
-    workdir = rd.getVar('WORKDIR', True)
-    if s.startswith(workdir):
+    s = os.path.abspath(rd.getVar('S', True))
+    workdir = os.path.abspath(rd.getVar('WORKDIR', True))
+    if s.startswith(workdir) and s != workdir and os.path.dirname(s) != workdir:
         # Handle if S is set to a subdirectory of the source
-        if s != workdir and os.path.dirname(s) != workdir:
-            srcsubdir = os.sep.join(os.path.relpath(s, workdir).split(os.sep)[1:])
-            srctree = os.path.join(srctree, srcsubdir)
+        srcsubdir = os.path.relpath(s, workdir).split(os.sep, 1)[1]
+        srctree = os.path.join(srctree, srcsubdir)
 
     appendpath = os.path.join(config.workspace_path, 'appends')
     if not os.path.exists(appendpath):
-- 
2.1.4

[PATCH v2 07/10] devtool: split out 'srcrev' update mode into a separate function

[Markus Lehtonen]

Refactor update_recipe() (i.e. the implementation of the update-recipe
command)  by splitting out the 'srcrev' into a distinct function.

Signed-off-by: Markus Lehtonen <markus.lehtonen@linux.intel.com>
---
 scripts/lib/devtool/standard.py | 188 ++++++++++++++++++++++++----------------
 1 file changed, 111 insertions(+), 77 deletions(-)

diff --git a/scripts/lib/devtool/standard.py b/scripts/lib/devtool/standard.py
index c92c9ae..af73009 100644
--- a/scripts/lib/devtool/standard.py
+++ b/scripts/lib/devtool/standard.py
@@ -29,6 +29,12 @@ from devtool import exec_build_env_command, setup_tinfoil
 
 logger = logging.getLogger('devtool')
 
+
+class DevtoolError(Exception):
+    """Exception for handling devtool errors"""
+    pass
+
+
 def plugin_init(pluginlist):
     """Plugin initialization"""
     pass
@@ -542,6 +548,103 @@ def _get_patchset_revs(args, srctree, recipe_path):
 
     return initial_rev, update_rev
 
+def _remove_patch_entries(srcuri, patchlist):
+    """Remove patch entries from SRC_URI"""
+    remaining = patchlist[:]
+    entries = []
+    for patch in patchlist:
+        patchfile = os.path.basename(patch)
+        for i in xrange(len(srcuri)):
+            if srcuri[i].startswith('file://') and os.path.basename(srcuri[i].split(';')[0]) == patchfile:
+                entries.append(srcuri[i])
+                remaining.remove(patch)
+                srcuri.pop(i)
+                break
+    return entries, remaining
+
+def _remove_patch_files(args, patches, destpath):
+    """Unlink existing patch files"""
+    for patchfile in patches:
+        if args.append:
+            if not destpath:
+                raise Exception('destpath should be set here')
+            patchfile = os.path.join(destpath, os.path.basename(patchfile))
+
+        if os.path.exists(patchfile):
+            logger.info('Removing patch %s' % patchfile)
+            # FIXME "git rm" here would be nice if the file in question is
+            #       tracked
+            # FIXME there's a chance that this file is referred to by
+            #       another recipe, in which case deleting wouldn't be the
+            #       right thing to do
+            os.remove(patchfile)
+            # Remove directory if empty
+            try:
+                os.rmdir(os.path.dirname(patchfile))
+            except OSError as ose:
+                if ose.errno != errno.ENOTEMPTY:
+                    raise
+
+def _update_recipe_srcrev(args, srctree, rd, config_data):
+    """Implement the 'srcrev' mode of update-recipe"""
+    import bb
+    import oe.recipeutils
+    from oe.patch import GitApplyTree
+
+    recipefile = rd.getVar('FILE', True)
+    logger.info('Updating SRCREV in recipe %s' % os.path.basename(recipefile))
+
+    # Get HEAD revision
+    try:
+        stdout, _ = bb.process.run('git rev-parse HEAD', cwd=srctree)
+    except bb.process.ExecutionError as err:
+        raise DevtoolError('Failed to get HEAD revision in %s: %s' %
+                           (srctree, err))
+    srcrev = stdout.strip()
+    if len(srcrev) != 40:
+        raise DevtoolError('Invalid hash returned by git: %s' % stdout)
+
+    destpath = None
+    removepatches = []
+    patchfields = {}
+    patchfields['SRCREV'] = srcrev
+    orig_src_uri = rd.getVar('SRC_URI', False) or ''
+    if not args.no_remove:
+        # Find list of existing patches in recipe file
+        existing_patches = oe.recipeutils.get_recipe_patches(rd)
+
+        old_srcrev = (rd.getVar('SRCREV', False) or '')
+        tempdir = tempfile.mkdtemp(prefix='devtool')
+        try:
+            GitApplyTree.extractPatches(srctree, old_srcrev, tempdir)
+            newpatches = os.listdir(tempdir)
+            for patch in existing_patches:
+                patchfile = os.path.basename(patch)
+                if patchfile in newpatches:
+                    removepatches.append(patch)
+        finally:
+            shutil.rmtree(tempdir)
+
+        if removepatches:
+            srcuri = orig_src_uri.split()
+            removedentries, _ = _remove_patch_entries(srcuri, removepatches)
+            if removedentries:
+                patchfields['SRC_URI'] = ' '.join(srcuri)
+
+    if args.append:
+        _, destpath = oe.recipeutils.bbappend_recipe(
+                rd, args.append, None, wildcardver=args.wildcard_version,
+                extralines=patchfields)
+    else:
+        oe.recipeutils.patch_recipe(config_data, recipefile, patchfields)
+
+    if not 'git://' in orig_src_uri:
+        logger.info('You will need to update SRC_URI within the recipe to '
+                    'point to a git repository where you have pushed your '
+                    'changes')
+
+    _remove_patch_files(args, removepatches, destpath)
+
 def update_recipe(args, config, basepath, workspace):
     """Entry point for the devtool 'update-recipe' subcommand"""
     if not args.recipename in workspace:
@@ -581,69 +684,16 @@ def update_recipe(args, config, basepath, workspace):
     else:
         mode = args.mode
 
-    def remove_patch_entries(srcuri, patchlist):
-        """Remove patch entries from SRC_URI"""
-        remaining = patchlist[:]
-        entries = []
-        for patch in patchlist:
-            patchfile = os.path.basename(patch)
-            for i in xrange(len(srcuri)):
-                if srcuri[i].startswith('file://') and os.path.basename(srcuri[i].split(';')[0]) == patchfile:
-                    entries.append(srcuri[i])
-                    remaining.remove(patch)
-                    srcuri.pop(i)
-                    break
-        return entries, remaining
-
     srctree = workspace[args.recipename]
 
-    # Get HEAD revision
-    try:
-        (stdout, _) = bb.process.run('git rev-parse HEAD', cwd=srctree)
-    except bb.process.ExecutionError as err:
-        print('Failed to get HEAD revision in %s: %s' % (srctree, err))
-        return 1
-    srcrev = stdout.strip()
-    if len(srcrev) != 40:
-        logger.error('Invalid hash returned by git: %s' % stdout)
-        return 1
-
     removepatches = []
     destpath = None
     if mode == 'srcrev':
-        logger.info('Updating SRCREV in recipe %s' % os.path.basename(recipefile))
-        removevalues = None
-        patchfields = {}
-        patchfields['SRCREV'] = srcrev
-        if not args.no_remove:
-            # Find list of existing patches in recipe file
-            existing_patches = oe.recipeutils.get_recipe_patches(rd)
-
-            old_srcrev = (rd.getVar('SRCREV', False) or '')
-            tempdir = tempfile.mkdtemp(prefix='devtool')
-            try:
-                GitApplyTree.extractPatches(srctree, old_srcrev, tempdir)
-                newpatches = os.listdir(tempdir)
-                for patch in existing_patches:
-                    patchfile = os.path.basename(patch)
-                    if patchfile in newpatches:
-                        removepatches.append(patch)
-            finally:
-                shutil.rmtree(tempdir)
-            if removepatches:
-                srcuri = (rd.getVar('SRC_URI', False) or '').split()
-                removedentries, _ = remove_patch_entries(srcuri, removepatches)
-                if removedentries:
-                    patchfields['SRC_URI'] = ' '.join(srcuri)
-
-        if args.append:
-            (appendfile, destpath) = oe.recipeutils.bbappend_recipe(rd, args.append, None, wildcardver=args.wildcard_version, extralines=patchfields)
-        else:
-            oe.recipeutils.patch_recipe(tinfoil.config_data, recipefile, patchfields)
-
-        if not 'git://' in orig_src_uri:
-            logger.info('You will need to update SRC_URI within the recipe to point to a git repository where you have pushed your changes')
-
+        try:
+            _update_recipe_srcrev(args, srctree, rd, tinfoil.config_data)
+        except DevtoolError as err:
+            logger.error(err)
+            return 1
     elif mode == 'patch':
         initial_rev, update_rev = _get_patchset_revs(args, srctree, append)
         if not initial_rev:
@@ -698,7 +748,7 @@ def update_recipe(args, config, basepath, workspace):
                     removevalues = None
                     if removepatches:
                         srcuri = (rd.getVar('SRC_URI', False) or '').split()
-                        removedentries, remaining = remove_patch_entries(srcuri, removepatches)
+                        removedentries, remaining = _remove_patch_entries(srcuri, removepatches)
                         if removedentries or remaining:
                             removevalues = {'SRC_URI': removedentries + ['file://' + os.path.basename(item) for item in remaining]}
                     (appendfile, destpath) = oe.recipeutils.bbappend_recipe(rd, args.append, patchfiles, removevalues=removevalues)
@@ -737,28 +787,12 @@ def update_recipe(args, config, basepath, workspace):
         finally:
             shutil.rmtree(tempdir)
 
+        _remove_patch_files(args, removepatches, destpath)
+
     else:
         logger.error('update_recipe: invalid mode %s' % mode)
         return 1
 
-    if removepatches:
-        for patchfile in removepatches:
-            if args.append:
-                if not destpath:
-                    raise Exception('destpath should be set here')
-                patchfile = os.path.join(destpath, os.path.basename(patchfile))
-
-            if os.path.exists(patchfile):
-                logger.info('Removing patch %s' % patchfile)
-                # FIXME "git rm" here would be nice if the file in question is tracked
-                # FIXME there's a chance that this file is referred to by another recipe, in which case deleting wouldn't be the right thing to do
-                os.remove(patchfile)
-                # Remove directory if empty
-                try:
-                    os.rmdir(os.path.dirname(patchfile))
-                except OSError as ose:
-                    if ose.errno != errno.ENOTEMPTY:
-                        raise
     return 0
 
 
-- 
2.1.4

[PATCH v2 08/10] devtool: split out 'patch' update mode into a separate function

[Markus Lehtonen]

Continue refactoring of update_recipe() by splitting out the 'patch'
mode into a separate function.

Signed-off-by: Markus Lehtonen <markus.lehtonen@linux.intel.com>
---
 scripts/lib/devtool/standard.py | 236 +++++++++++++++++++++-------------------
 1 file changed, 122 insertions(+), 114 deletions(-)

diff --git a/scripts/lib/devtool/standard.py b/scripts/lib/devtool/standard.py
index af73009..fb3cc78 100644
--- a/scripts/lib/devtool/standard.py
+++ b/scripts/lib/devtool/standard.py
@@ -645,6 +645,119 @@ def _update_recipe_srcrev(args, srctree, rd, config_data):
 
     _remove_patch_files(args, removepatches, destpath)
 
+def _update_recipe_patch(args, config, srctree, rd, config_data):
+    """Implement the 'patch' mode of update-recipe"""
+    import bb
+    import oe.recipeutils
+    from oe.patch import GitApplyTree
+
+    recipefile = rd.getVar('FILE', True)
+    append = os.path.join(config.workspace_path, 'appends', '%s.bbappend' %
+                          os.path.splitext(os.path.basename(recipefile))[0])
+    if not os.path.exists(append):
+        raise DevtoolError('unable to find workspace bbappend for recipe %s' %
+                           args.recipename)
+
+    initial_rev, update_rev = _get_patchset_revs(args, srctree, append)
+    if not initial_rev:
+        raise DevtoolError('Unable to find initial revision - please specify '
+                           'it with --initial-rev')
+
+    # Find list of existing patches in recipe file
+    existing_patches = oe.recipeutils.get_recipe_patches(rd)
+
+    removepatches = []
+    seqpatch_re = re.compile('^([0-9]{4}-)?(.+)')
+    if not args.no_remove:
+        # Get all patches from source tree and check if any should be removed
+        tempdir = tempfile.mkdtemp(prefix='devtool')
+        try:
+            GitApplyTree.extractPatches(srctree, initial_rev, tempdir)
+            # Strip numbering from patch names. If it's a git sequence named
+            # patch, the numbers might not match up since we are starting from
+            # a different revision This does assume that people are using
+            # unique shortlog values, but they ought to be anyway...
+            newpatches = [seqpatch_re.match(fname).group(2) for fname in
+                          os.listdir(tempdir)]
+            for patch in existing_patches:
+                basename = seqpatch_re.match(
+                                os.path.basename(patch)).group(2)
+                if basename not in newpatches:
+                    removepatches.append(patch)
+        finally:
+            shutil.rmtree(tempdir)
+
+    # Get updated patches from source tree
+    tempdir = tempfile.mkdtemp(prefix='devtool')
+    try:
+        GitApplyTree.extractPatches(srctree, update_rev, tempdir)
+
+        # Match up and replace existing patches with corresponding new patches
+        updatepatches = False
+        updaterecipe = False
+        destpath = None
+        newpatches = os.listdir(tempdir)
+        if args.append:
+            patchfiles = {}
+            for patch in existing_patches:
+                patchfile = os.path.basename(patch)
+                if patchfile in newpatches:
+                    patchfiles[os.path.join(tempdir, patchfile)] = patchfile
+                    newpatches.remove(patchfile)
+            for patchfile in newpatches:
+                patchfiles[os.path.join(tempdir, patchfile)] = None
+
+            if patchfiles or removepatches:
+                removevalues = None
+                if removepatches:
+                    srcuri = (rd.getVar('SRC_URI', False) or '').split()
+                    removedentries, remaining = _remove_patch_entries(
+                                                    srcuri, removepatches)
+                    if removedentries or remaining:
+                        remaining = ['file://' + os.path.basename(item) for
+                                     item in remaining]
+                        removevalues = {'SRC_URI': removedentries + remaining}
+                _, destpath = oe.recipeutils.bbappend_recipe(
+                                rd, args.append, patchfiles,
+                                removevalues=removevalues)
+            else:
+                logger.info('No patches needed updating')
+        else:
+            for patch in existing_patches:
+                patchfile = os.path.basename(patch)
+                if patchfile in newpatches:
+                    logger.info('Updating patch %s' % patchfile)
+                    shutil.move(os.path.join(tempdir, patchfile), patch)
+                    newpatches.remove(patchfile)
+                    updatepatches = True
+            srcuri = (rd.getVar('SRC_URI', False) or '').split()
+            if newpatches:
+                # Add any patches left over
+                patchdir = os.path.join(os.path.dirname(recipefile),
+                                        rd.getVar('BPN', True))
+                bb.utils.mkdirhier(patchdir)
+                for patchfile in newpatches:
+                    logger.info('Adding new patch %s' % patchfile)
+                    shutil.move(os.path.join(tempdir, patchfile),
+                                os.path.join(patchdir, patchfile))
+                    srcuri.append('file://%s' % patchfile)
+                    updaterecipe = True
+            if removepatches:
+                removedentries, _ = _remove_patch_entries(srcuri, removepatches)
+                if removedentries:
+                    updaterecipe = True
+            if updaterecipe:
+                logger.info('Updating recipe %s' % os.path.basename(recipefile))
+                oe.recipeutils.patch_recipe(config_data, recipefile,
+                                            {'SRC_URI': ' '.join(srcuri)})
+            elif not updatepatches:
+                # Neither patches nor recipe were updated
+                logger.info('No patches need updating')
+    finally:
+        shutil.rmtree(tempdir)
+
+    _remove_patch_files(args, removepatches, destpath)
+
 def update_recipe(args, config, basepath, workspace):
     """Entry point for the devtool 'update-recipe' subcommand"""
     if not args.recipename in workspace:
@@ -660,20 +773,10 @@ def update_recipe(args, config, basepath, workspace):
             return 2
 
     tinfoil = setup_tinfoil()
-    import bb
-    from oe.patch import GitApplyTree
-    import oe.recipeutils
 
     rd = _parse_recipe(config, tinfoil, args.recipename, True)
     if not rd:
         return -1
-    recipefile = rd.getVar('FILE', True)
-
-    # Get initial revision from bbappend
-    append = os.path.join(config.workspace_path, 'appends', '%s.bbappend' % os.path.splitext(os.path.basename(recipefile))[0])
-    if not os.path.exists(append):
-        logger.error('unable to find workspace bbappend for recipe %s' % args.recipename)
-        return -1
 
     orig_src_uri = rd.getVar('SRC_URI', False) or ''
     if args.mode == 'auto':
@@ -686,111 +789,16 @@ def update_recipe(args, config, basepath, workspace):
 
     srctree = workspace[args.recipename]
 
-    removepatches = []
-    destpath = None
-    if mode == 'srcrev':
-        try:
+    try:
+        if mode == 'srcrev':
             _update_recipe_srcrev(args, srctree, rd, tinfoil.config_data)
-        except DevtoolError as err:
-            logger.error(err)
-            return 1
-    elif mode == 'patch':
-        initial_rev, update_rev = _get_patchset_revs(args, srctree, append)
-        if not initial_rev:
-            logger.error('Unable to find initial revision - please specify it with --initial-rev')
-            return -1
-
-        # Find list of existing patches in recipe file
-        existing_patches = oe.recipeutils.get_recipe_patches(rd)
-
-        removepatches = []
-        seqpatch_re = re.compile('^([0-9]{4}-)?(.+)')
-        if not args.no_remove:
-            # Get all patches from source tree and check if any should be removed
-            tempdir = tempfile.mkdtemp(prefix='devtool')
-            try:
-                GitApplyTree.extractPatches(srctree, initial_rev, tempdir)
-                # Strip numbering from patch names. If it's a git sequence
-                # named patch, the numbers might not match up since we are
-                # starting from a different revision This does assume that
-                # people are using unique shortlog values, but they ought to be
-                # anyway...
-                newpatches = [seqpatch_re.match(fname).group(2) for fname in
-                              os.listdir(tempdir)]
-                for patch in existing_patches:
-                    basename = seqpatch_re.match(
-                                    os.path.basename(patch)).group(2)
-                    if basename not in newpatches:
-                        removepatches.append(patch)
-            finally:
-                shutil.rmtree(tempdir)
-
-        # Get updated patches from source tree
-        tempdir = tempfile.mkdtemp(prefix='devtool')
-        try:
-            GitApplyTree.extractPatches(srctree, update_rev, tempdir)
-
-            # Match up and replace existing patches with corresponding new patches
-            updatepatches = False
-            updaterecipe = False
-            newpatches = os.listdir(tempdir)
-            if args.append:
-                patchfiles = {}
-                for patch in existing_patches:
-                    patchfile = os.path.basename(patch)
-                    if patchfile in newpatches:
-                        patchfiles[os.path.join(tempdir, patchfile)] = patchfile
-                        newpatches.remove(patchfile)
-                for patchfile in newpatches:
-                    patchfiles[os.path.join(tempdir, patchfile)] = None
-
-                if patchfiles or removepatches:
-                    removevalues = None
-                    if removepatches:
-                        srcuri = (rd.getVar('SRC_URI', False) or '').split()
-                        removedentries, remaining = _remove_patch_entries(srcuri, removepatches)
-                        if removedentries or remaining:
-                            removevalues = {'SRC_URI': removedentries + ['file://' + os.path.basename(item) for item in remaining]}
-                    (appendfile, destpath) = oe.recipeutils.bbappend_recipe(rd, args.append, patchfiles, removevalues=removevalues)
-                else:
-                    logger.info('No patches needed updating')
-            else:
-                for patch in existing_patches:
-                    patchfile = os.path.basename(patch)
-                    if patchfile in newpatches:
-                        logger.info('Updating patch %s' % patchfile)
-                        shutil.move(os.path.join(tempdir, patchfile), patch)
-                        newpatches.remove(patchfile)
-                        updatepatches = True
-                srcuri = (rd.getVar('SRC_URI', False) or '').split()
-                if newpatches:
-                    # Add any patches left over
-                    patchdir = os.path.join(os.path.dirname(recipefile), rd.getVar('BPN', True))
-                    bb.utils.mkdirhier(patchdir)
-                    for patchfile in newpatches:
-                        logger.info('Adding new patch %s' % patchfile)
-                        shutil.move(os.path.join(tempdir, patchfile), os.path.join(patchdir, patchfile))
-                        srcuri.append('file://%s' % patchfile)
-                        updaterecipe = True
-                if removepatches:
-                    removedentries, _ = remove_patch_entries(srcuri, removepatches)
-                    if removedentries:
-                        updaterecipe = True
-                if updaterecipe:
-                    logger.info('Updating recipe %s' % os.path.basename(recipefile))
-                    oe.recipeutils.patch_recipe(tinfoil.config_data,
-                            recipefile, {'SRC_URI': ' '.join(srcuri)})
-                elif not updatepatches:
-                    # Neither patches nor recipe were updated
-                    logger.info('No patches need updating')
-
-        finally:
-            shutil.rmtree(tempdir)
-
-        _remove_patch_files(args, removepatches, destpath)
-
-    else:
-        logger.error('update_recipe: invalid mode %s' % mode)
+        elif mode == 'patch':
+            _update_recipe_patch(args, config, srctree, rd,
+                                 tinfoil.config_data)
+        else:
+            raise DevtoolError('update_recipe: invalid mode %s' % mode)
+    except DevtoolError as err:
+        logger.error(err)
         return 1
 
     return 0
-- 
2.1.4

[PATCH v2 09/10] devtool: remove some unused return values

[Markus Lehtonen]

Signed-off-by: Markus Lehtonen <markus.lehtonen@linux.intel.com>
---
 scripts/devtool                 | 1 -
 scripts/lib/devtool/standard.py | 3 ---
 2 files changed, 4 deletions(-)

diff --git a/scripts/devtool b/scripts/devtool
index 0100eb8..307846a 100755
--- a/scripts/devtool
+++ b/scripts/devtool
@@ -157,7 +157,6 @@ def _enable_workspace_layer(workspacedir, config, basepath):
     bblayers_conf = os.path.join(basepath, 'conf', 'bblayers.conf')
     if not os.path.exists(bblayers_conf):
         logger.error('Unable to find bblayers.conf')
-        return -1
     _, added = bb.utils.edit_bblayers_conf(bblayers_conf, workspacedir, config.workspace_path)
     if added:
         logger.info('Enabling workspace layer in bblayers.conf')
diff --git a/scripts/lib/devtool/standard.py b/scripts/lib/devtool/standard.py
index fb3cc78..14912a9 100644
--- a/scripts/lib/devtool/standard.py
+++ b/scripts/lib/devtool/standard.py
@@ -417,9 +417,6 @@ def _check_preserve(config, recipename):
                     tf.write(line)
     os.rename(newfile, origfile)
 
-    return False
-
-
 def modify(args, config, basepath, workspace):
     """Entry point for the devtool 'modify' subcommand"""
     import bb
-- 
2.1.4

[PATCH v2 10/10] devtool: use DevtoolError for error handling

[Markus Lehtonen]

Use DevtoolError exception more widely for handling error cases. This
exception is now caught in the main script and raising it can be used to
exit with an error. This hopefully simplifies error handling. The
change also makes exit codes more consistent, always returning '1' when
an error occurs.

Signed-off-by: Markus Lehtonen <markus.lehtonen@linux.intel.com>
---
 scripts/devtool                 |   8 ++-
 scripts/lib/devtool/__init__.py |   6 ++
 scripts/lib/devtool/deploy.py   |  31 ++++----
 scripts/lib/devtool/standard.py | 154 ++++++++++++++++++----------------------
 4 files changed, 99 insertions(+), 100 deletions(-)

diff --git a/scripts/devtool b/scripts/devtool
index 307846a..7fd4da3 100755
--- a/scripts/devtool
+++ b/scripts/devtool
@@ -35,6 +35,7 @@ context = None
 scripts_path = os.path.dirname(os.path.realpath(__file__))
 lib_path = scripts_path + '/lib'
 sys.path = sys.path + [lib_path]
+from devtool import DevtoolError
 import scriptutils
 logger = scriptutils.logger_create('devtool')
 
@@ -249,7 +250,12 @@ def main():
     if args.subparser_name != 'create-workspace':
         read_workspace()
 
-    ret = args.func(args, config, basepath, workspace)
+    try:
+        ret = args.func(args, config, basepath, workspace)
+    except DevtoolError as err:
+        if str(err):
+            logger.error(str(err))
+        ret = 1
 
     return ret
 
diff --git a/scripts/lib/devtool/__init__.py b/scripts/lib/devtool/__init__.py
index 9ec1ef6..ea0b63e 100644
--- a/scripts/lib/devtool/__init__.py
+++ b/scripts/lib/devtool/__init__.py
@@ -25,6 +25,12 @@ import logging
 
 logger = logging.getLogger('devtool')
 
+
+class DevtoolError(Exception):
+    """Exception for handling devtool errors"""
+    pass
+
+
 def exec_build_env_command(init_path, builddir, cmd, watch=False, **options):
     """Run a program in bitbake build context"""
     import bb
diff --git a/scripts/lib/devtool/deploy.py b/scripts/lib/devtool/deploy.py
index 92a3cb4..ca74a8e 100644
--- a/scripts/lib/devtool/deploy.py
+++ b/scripts/lib/devtool/deploy.py
@@ -19,7 +19,7 @@
 import os
 import subprocess
 import logging
-from devtool import exec_build_env_command, setup_tinfoil
+from devtool import exec_build_env_command, setup_tinfoil, DevtoolError
 
 logger = logging.getLogger('devtool')
 
@@ -34,8 +34,8 @@ def deploy(args, config, basepath, workspace):
     import oe.recipeutils
 
     if not args.recipename in workspace:
-        logger.error("no recipe named %s in your workspace" % args.recipename)
-        return -1
+        raise DevtoolError("no recipe named %s in your workspace" %
+                           args.recipename)
     try:
         host, destdir = args.target.split(':')
     except ValueError:
@@ -50,12 +50,13 @@ def deploy(args, config, basepath, workspace):
     try:
         rd = oe.recipeutils.parse_recipe_simple(tinfoil.cooker, args.recipename, tinfoil.config_data)
     except Exception as e:
-        logger.error('Exception parsing recipe %s: %s' % (args.recipename, e))
-        return 2
+        raise DevtoolError('Exception parsing recipe %s: %s' %
+                           (args.recipename, e))
     recipe_outdir = rd.getVar('D', True)
     if not os.path.exists(recipe_outdir) or not os.listdir(recipe_outdir):
-        logger.error('No files to deploy - have you built the %s recipe? If so, the install step has not installed any files.' % args.recipename)
-        return -1
+        raise DevtoolError('No files to deploy - have you built the %s '
+                           'recipe? If so, the install step has not installed '
+                           'any files.' % args.recipename)
 
     if args.dry_run:
         print('Files to be deployed for %s on target %s:' % (args.recipename, args.target))
@@ -67,7 +68,7 @@ def deploy(args, config, basepath, workspace):
     if os.path.exists(deploy_file):
         if undeploy(args, config, basepath, workspace):
             # Error already shown
-            return -1
+            return 1
 
     extraoptions = ''
     if args.no_host_check:
@@ -76,8 +77,8 @@ def deploy(args, config, basepath, workspace):
         extraoptions += ' -q'
     ret = subprocess.call('scp -r %s %s/* %s:%s' % (extraoptions, recipe_outdir, args.target, destdir), shell=True)
     if ret != 0:
-        logger.error('Deploy failed - rerun with -s to get a complete error message')
-        return ret
+        raise DevtoolError('Deploy failed - rerun with -s to get a complete '
+                           'error message')
 
     logger.info('Successfully deployed %s' % recipe_outdir)
 
@@ -99,8 +100,7 @@ def undeploy(args, config, basepath, workspace):
     """Entry point for the devtool 'undeploy' subcommand"""
     deploy_file = os.path.join(basepath, 'target_deploy', args.target, args.recipename + '.list')
     if not os.path.exists(deploy_file):
-        logger.error('%s has not been deployed' % args.recipename)
-        return -1
+        raise DevtoolError('%s has not been deployed' % args.recipename)
 
     if args.dry_run:
         print('Previously deployed files to be un-deployed for %s on target %s:' % (args.recipename, args.target))
@@ -117,15 +117,16 @@ def undeploy(args, config, basepath, workspace):
 
     ret = subprocess.call("scp %s %s %s:/tmp" % (extraoptions, deploy_file, args.target), shell=True)
     if ret != 0:
-        logger.error('Failed to copy file list to %s - rerun with -s to get a complete error message' % args.target)
-        return -1
+        raise DevtoolError('Failed to copy file list to %s - rerun with -s to '
+                           'get a complete error message' % args.target)
 
     ret = subprocess.call("ssh %s %s 'xargs -n1 rm -f </tmp/%s'" % (extraoptions, args.target, os.path.basename(deploy_file)), shell=True)
     if ret == 0:
         logger.info('Successfully undeployed %s' % args.recipename)
         os.remove(deploy_file)
     else:
-        logger.error('Undeploy failed - rerun with -s to get a complete error message')
+        raise DevtoolError('Undeploy failed - rerun with -s to get a complete '
+                           'error message')
 
     return ret
 
diff --git a/scripts/lib/devtool/standard.py b/scripts/lib/devtool/standard.py
index 14912a9..4d3ff02 100644
--- a/scripts/lib/devtool/standard.py
+++ b/scripts/lib/devtool/standard.py
@@ -25,16 +25,11 @@ import logging
 import argparse
 import scriptutils
 import errno
-from devtool import exec_build_env_command, setup_tinfoil
+from devtool import exec_build_env_command, setup_tinfoil, DevtoolError
 
 logger = logging.getLogger('devtool')
 
 
-class DevtoolError(Exception):
-    """Exception for handling devtool errors"""
-    pass
-
-
 def plugin_init(pluginlist):
     """Plugin initialization"""
     pass
@@ -46,26 +41,27 @@ def add(args, config, basepath, workspace):
     import oe.recipeutils
 
     if args.recipename in workspace:
-        logger.error("recipe %s is already in your workspace" % args.recipename)
-        return -1
+        raise DevtoolError("recipe %s is already in your workspace" %
+                            args.recipename)
 
     reason = oe.recipeutils.validate_pn(args.recipename)
     if reason:
-        logger.error(reason)
-        return -1
+        raise DevtoolError(reason)
 
     srctree = os.path.abspath(args.srctree)
     if os.path.exists(srctree):
         if args.fetch:
             if not os.path.isdir(srctree):
-                logger.error("Cannot fetch into source tree path %s as it exists and is not a directory" % srctree)
-                return 1
+                raise DevtoolError("Cannot fetch into source tree path %s as "
+                                   "it exists and is not a directory" %
+                                   srctree)
             elif os.listdir(srctree):
-                logger.error("Cannot fetch into source tree path %s as it already exists and is non-empty" % srctree)
-                return 1
+                raise DevtoolError("Cannot fetch into source tree path %s as "
+                                   "it already exists and is non-empty" %
+                                   srctree)
     elif not args.fetch:
-        logger.error("Specified source tree %s could not be found" % srctree)
-        return 1
+        raise DevtoolError("Specified source tree %s could not be found" %
+                           srctree)
 
     appendpath = os.path.join(config.workspace_path, 'appends')
     if not os.path.exists(appendpath):
@@ -76,8 +72,7 @@ def add(args, config, basepath, workspace):
     rfv = None
     if args.version:
         if '_' in args.version or ' ' in args.version:
-            logger.error('Invalid version string "%s"' % args.version)
-            return -1
+            raise DevtoolError('Invalid version string "%s"' % args.version)
         rfv = args.version
     if args.fetch:
         if args.fetch.startswith('git://'):
@@ -107,8 +102,7 @@ def add(args, config, basepath, workspace):
         stdout, _ = exec_build_env_command(config.init_path, basepath, 'recipetool --color=%s create -o %s "%s" %s' % (color, recipefile, source, extracmdopts))
         logger.info('Recipe %s has been automatically created; further editing may be required to make it fully functional' % recipefile)
     except bb.process.ExecutionError as e:
-        logger.error('Command \'%s\' failed:\n%s' % (e.command, e.stdout))
-        return 1
+        raise DevtoolError('Command \'%s\' failed:\n%s' % (e.command, e.stdout))
 
     _add_md5(config, args.recipename, recipefile)
 
@@ -134,35 +128,33 @@ def add(args, config, basepath, workspace):
 def _check_compatible_recipe(pn, d):
     """Check if the recipe is supported by devtool"""
     if pn == 'perf':
-        logger.error("The perf recipe does not actually check out source and thus cannot be supported by this tool")
-        return False
+        raise DevtoolError("The perf recipe does not actually check out "
+                           "source and thus cannot be supported by this tool")
 
     if pn in ['kernel-devsrc', 'package-index'] or pn.startswith('gcc-source'):
-        logger.error("The %s recipe is not supported by this tool" % pn)
-        return False
+        raise DevtoolError("The %s recipe is not supported by this tool" % pn)
 
     if bb.data.inherits_class('image', d):
-        logger.error("The %s recipe is an image, and therefore is not supported by this tool" % pn)
-        return False
+        raise DevtoolError("The %s recipe is an image, and therefore is not "
+                           "supported by this tool" % pn)
 
     if bb.data.inherits_class('populate_sdk', d):
-        logger.error("The %s recipe is an SDK, and therefore is not supported by this tool" % pn)
-        return False
+        raise DevtoolError("The %s recipe is an SDK, and therefore is not "
+                           "supported by this tool" % pn)
 
     if bb.data.inherits_class('packagegroup', d):
-        logger.error("The %s recipe is a packagegroup, and therefore is not supported by this tool" % pn)
-        return False
+        raise DevtoolError("The %s recipe is a packagegroup, and therefore is "
+                           "not supported by this tool" % pn)
 
     if bb.data.inherits_class('meta', d):
-        logger.error("The %s recipe is a meta-recipe, and therefore is not supported by this tool" % pn)
-        return False
+        raise DevtoolError("The %s recipe is a meta-recipe, and therefore is "
+                           "not supported by this tool" % pn)
 
     if bb.data.inherits_class('externalsrc', d) and d.getVar('EXTERNALSRC', True):
-        logger.error("externalsrc is currently enabled for the %s recipe. This prevents the normal do_patch task from working. You will need to disable this first." % pn)
-        return False
-
-    return True
-
+        raise DevtoolError("externalsrc is currently enabled for the %s "
+                           "recipe. This prevents the normal do_patch task "
+                           "from working. You will need to disable this "
+                           "first." % pn)
 
 def _get_recipe_file(cooker, pn):
     """Find recipe file corresponding a package name"""
@@ -209,14 +201,14 @@ def extract(args, config, basepath, workspace):
 
     rd = _parse_recipe(config, tinfoil, args.recipename, True)
     if not rd:
-        return -1
+        return 1
 
     srctree = os.path.abspath(args.srctree)
     initial_rev = _extract_source(srctree, args.keep_temp, args.branch, rd)
     if initial_rev:
         return 0
     else:
-        return -1
+        return 1
 
 class BbTaskExecutor(object):
     """Class for executing bitbake tasks for a recipe
@@ -262,16 +254,15 @@ def _extract_source(srctree, keep_temp, devbranch, d):
 
     pn = d.getVar('PN', True)
 
-    if not _check_compatible_recipe(pn, d):
-        return None
+    _check_compatible_recipe(pn, d)
 
     if os.path.exists(srctree):
         if not os.path.isdir(srctree):
-            logger.error("output path %s exists and is not a directory" % srctree)
-            return None
+            raise DevtoolError("output path %s exists and is not a directory" %
+                               srctree)
         elif os.listdir(srctree):
-            logger.error("output path %s already exists and is non-empty" % srctree)
-            return None
+            raise DevtoolError("output path %s already exists and is "
+                               "non-empty" % srctree)
 
     # Prepare for shutil.move later on
     bb.utils.mkdirhier(srctree)
@@ -341,8 +332,8 @@ def _extract_source(srctree, keep_temp, devbranch, d):
             initial_rev = stdout.rstrip()
         else:
             if not os.listdir(srcsubdir):
-                logger.error("no source unpacked to S, perhaps the %s recipe doesn't use any source?" % pn)
-                return None
+                raise DevtoolError("no source unpacked to S, perhaps the %s "
+                                   "recipe doesn't use any source?" % pn)
 
             if not os.path.exists(os.path.join(srcsubdir, '.git')):
                 bb.process.run('git init', cwd=srcsubdir)
@@ -423,22 +414,22 @@ def modify(args, config, basepath, workspace):
     import oe.recipeutils
 
     if args.recipename in workspace:
-        logger.error("recipe %s is already in your workspace" % args.recipename)
-        return -1
+        raise DevtoolError("recipe %s is already in your workspace" %
+                           args.recipename)
 
     if not args.extract and not os.path.isdir(args.srctree):
-        logger.error("directory %s does not exist or not a directory (specify -x to extract source from recipe)" % args.srctree)
-        return -1
+        raise DevtoolError("directory %s does not exist or not a directory "
+                           "(specify -x to extract source from recipe)" %
+                           args.srctree)
 
     tinfoil = setup_tinfoil()
 
     rd = _parse_recipe(config, tinfoil, args.recipename, True)
     if not rd:
-        return -1
+        return 1
     recipefile = rd.getVar('FILE', True)
 
-    if not _check_compatible_recipe(args.recipename, rd):
-        return -1
+    _check_compatible_recipe(args.recipename, rd)
 
     initial_rev = None
     commits = []
@@ -446,7 +437,7 @@ def modify(args, config, basepath, workspace):
     if args.extract:
         initial_rev = _extract_source(args.srctree, False, args.branch, rd)
         if not initial_rev:
-            return -1
+            return 1
         # Get list of commits since this revision
         (stdout, _) = bb.process.run('git rev-list --reverse %s..HEAD' % initial_rev, cwd=args.srctree)
         commits = stdout.split()
@@ -758,22 +749,22 @@ def _update_recipe_patch(args, config, srctree, rd, config_data):
 def update_recipe(args, config, basepath, workspace):
     """Entry point for the devtool 'update-recipe' subcommand"""
     if not args.recipename in workspace:
-        logger.error("no recipe named %s in your workspace" % args.recipename)
-        return -1
+        raise DevtoolError("no recipe named %s in your workspace" %
+                           args.recipename)
 
     if args.append:
         if not os.path.exists(args.append):
-            logger.error('bbappend destination layer directory "%s" does not exist' % args.append)
-            return 2
+            raise DevtoolError('bbappend destination layer directory "%s" '
+                               'does not exist' % args.append)
         if not os.path.exists(os.path.join(args.append, 'conf', 'layer.conf')):
-            logger.error('conf/layer.conf not found in bbappend destination layer "%s"' % args.append)
-            return 2
+            raise DevtoolError('conf/layer.conf not found in bbappend '
+                               'destination layer "%s"' % args.append)
 
     tinfoil = setup_tinfoil()
 
     rd = _parse_recipe(config, tinfoil, args.recipename, True)
     if not rd:
-        return -1
+        return 1
 
     orig_src_uri = rd.getVar('SRC_URI', False) or ''
     if args.mode == 'auto':
@@ -786,17 +777,12 @@ def update_recipe(args, config, basepath, workspace):
 
     srctree = workspace[args.recipename]
 
-    try:
-        if mode == 'srcrev':
-            _update_recipe_srcrev(args, srctree, rd, tinfoil.config_data)
-        elif mode == 'patch':
-            _update_recipe_patch(args, config, srctree, rd,
-                                 tinfoil.config_data)
-        else:
-            raise DevtoolError('update_recipe: invalid mode %s' % mode)
-    except DevtoolError as err:
-        logger.error(err)
-        return 1
+    if mode == 'srcrev':
+        _update_recipe_srcrev(args, srctree, rd, tinfoil.config_data)
+    elif mode == 'patch':
+        _update_recipe_patch(args, config, srctree, rd, tinfoil.config_data)
+    else:
+        raise DevtoolError('update_recipe: invalid mode %s' % mode)
 
     return 0
 
@@ -816,15 +802,13 @@ def reset(args, config, basepath, workspace):
     import bb
     if args.recipename:
         if args.all:
-            logger.error("Recipe cannot be specified if -a/--all is used")
-            return -1
+            raise DevtoolError("Recipe cannot be specified if -a/--all is used")
         elif not args.recipename in workspace:
-            logger.error("no recipe named %s in your workspace" % args.recipename)
-            return -1
+            raise DevtoolError("no recipe named %s in your workspace" %
+                               args.recipename)
     elif not args.all:
-        logger.error("Recipe must be specified, or specify -a/--all to reset all recipes")
-        return -1
-
+        raise DevtoolError("Recipe must be specified, or specify -a/--all to "
+                           "reset all recipes")
     if args.all:
         recipes = workspace
     else:
@@ -836,8 +820,10 @@ def reset(args, config, basepath, workspace):
             try:
                 exec_build_env_command(config.init_path, basepath, 'bitbake -c clean %s' % pn)
             except bb.process.ExecutionError as e:
-                logger.error('Command \'%s\' failed, output:\n%s\nIf you wish, you may specify -n/--no-clean to skip running this command when resetting' % (e.command, e.stdout))
-                return 1
+                raise DevtoolError('Command \'%s\' failed, output:\n%s\nIf you '
+                                   'wish, you may specify -n/--no-clean to '
+                                   'skip running this command when resetting' %
+                                   (e.command, e.stdout))
 
         _check_preserve(config, pn)
 
@@ -861,8 +847,8 @@ def build(args, config, basepath, workspace):
     """Entry point for the devtool 'build' subcommand"""
     import bb
     if not args.recipename in workspace:
-        logger.error("no recipe named %s in your workspace" % args.recipename)
-        return -1
+        raise DevtoolError("no recipe named %s in your workspace" %
+                           args.recipename)
     build_task = config.get('Build', 'build_task', 'populate_sysroot')
     try:
         exec_build_env_command(config.init_path, basepath, 'bitbake -c %s %s' % (build_task, args.recipename), watch=True)
-- 
2.1.4

Re: [PATCH v2 09/10] devtool: remove some unused return values

[Paul Eggleton]

On Thursday 11 June 2015 14:34:15 Markus Lehtonen wrote:
> Signed-off-by: Markus Lehtonen <markus.lehtonen@linux.intel.com>
> ---
>  scripts/devtool                 | 1 -
>  scripts/lib/devtool/standard.py | 3 ---
>  2 files changed, 4 deletions(-)
> 
> diff --git a/scripts/devtool b/scripts/devtool
> index 0100eb8..307846a 100755
> --- a/scripts/devtool
> +++ b/scripts/devtool
> @@ -157,7 +157,6 @@ def _enable_workspace_layer(workspacedir, config,
> basepath): bblayers_conf = os.path.join(basepath, 'conf', 'bblayers.conf')
> if not os.path.exists(bblayers_conf):
>          logger.error('Unable to find bblayers.conf')
> -        return -1

I appreciate the actually returned value might not be used, but simply 
dropping the return entirely allows the function to continue after the error, 
which is wrong.

Cheers,
Paul

-- 

Paul Eggleton
Intel Open Source Technology Centre

Re: [PATCH v2 09/10] devtool: remove some unused return values

[Markus Lehtonen]

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

Hi Paul,

On Wed, 2015-06-17 at 10:36 +0100, Paul Eggleton wrote:
> On Thursday 11 June 2015 14:34:15 Markus Lehtonen wrote:
> > Signed-off-by: Markus Lehtonen <markus.lehtonen@linux.intel.com>
> > ---
> >  scripts/devtool                 | 1 -
> >  scripts/lib/devtool/standard.py | 3 ---
> >  2 files changed, 4 deletions(-)
> > 
> > diff --git a/scripts/devtool b/scripts/devtool
> > index 0100eb8..307846a 100755
> > --- a/scripts/devtool
> > +++ b/scripts/devtool
> > @@ -157,7 +157,6 @@ def _enable_workspace_layer(workspacedir, config,
> > basepath): bblayers_conf = os.path.join(basepath, 'conf', 'bblayers.conf')
> > if not os.path.exists(bblayers_conf):
> >          logger.error('Unable to find bblayers.conf')
> > -        return -1
> 
> I appreciate the actually returned value might not be used, but simply 
> dropping the return entirely allows the function to continue after the error, 
> which is wrong.

Uh oh, stupid mistake. Thanks for spotting that!

You can find a fixed patch, attached. I also pushed a fixed version of
the patchset to
git://git.openembedded.org/openembedded-core-contrib
marquiz/devtool/refactor



Thanks,
  Markus

[-- Attachment #2: v3-0009-devtool-remove-some-unused-return-values.patch --]
[-- Type: text/x-patch, Size: 1487 bytes --]

From 30401a4f075057e0c8eca9aaaae7e45591baf7c4 Mon Sep 17 00:00:00 2001
From: Markus Lehtonen <markus.lehtonen@linux.intel.com>
Date: Wed, 27 May 2015 17:40:49 +0300
Subject: [PATCH v3 09/10] devtool: remove some unused return values

Signed-off-by: Markus Lehtonen <markus.lehtonen@linux.intel.com>
---
 scripts/devtool                 | 2 +-
 scripts/lib/devtool/standard.py | 3 ---
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/scripts/devtool b/scripts/devtool
index 0100eb8..fd4af98 100755
--- a/scripts/devtool
+++ b/scripts/devtool
@@ -157,7 +157,7 @@ def _enable_workspace_layer(workspacedir, config, basepath):
     bblayers_conf = os.path.join(basepath, 'conf', 'bblayers.conf')
     if not os.path.exists(bblayers_conf):
         logger.error('Unable to find bblayers.conf')
-        return -1
+        return
     _, added = bb.utils.edit_bblayers_conf(bblayers_conf, workspacedir, config.workspace_path)
     if added:
         logger.info('Enabling workspace layer in bblayers.conf')
diff --git a/scripts/lib/devtool/standard.py b/scripts/lib/devtool/standard.py
index fb3cc78..14912a9 100644
--- a/scripts/lib/devtool/standard.py
+++ b/scripts/lib/devtool/standard.py
@@ -417,9 +417,6 @@ def _check_preserve(config, recipename):
                     tf.write(line)
     os.rename(newfile, origfile)
 
-    return False
-
-
 def modify(args, config, basepath, workspace):
     """Entry point for the devtool 'modify' subcommand"""
     import bb
-- 
2.1.4