[PATCH 0/3] Add mechanism to catch layer branch mismatches

[PATCH 0/3] Add mechanism to catch layer branch mismatches

[Paul Eggleton]

Changes since the earlier RFC patch:
* Removed unused regex line
* Improved validation / error handling
* Added some regression tests

Note: patch 3/3 depends on patch 1/3, so they need to go in together.


The following changes since commit 207013b6dde82f9654f9be996695c8335b95a288:

  data_smart: split expanded removal values when handling _remove (2015-02-02 16:20:42 +0000)

are available in the git repository at:

  git://git.yoctoproject.org/poky-contrib paule/layerdepends
  http://git.yoctoproject.org/cgit.cgi/poky-contrib/log/?h=paule/layerdepends

Paul Eggleton (3):
  utils: ensure explode_dep_versions2 raises an exception on
    invalid/missing operator
  tests: add tests for OE pre-release version formatting
  cooker: rework LAYERDEPENDS versioning so that it is actually useful

 lib/bb/cooker.py      | 40 ++++++++++++++--------------------------
 lib/bb/tests/utils.py | 37 +++++++++++++++++++++++++++++++++++++
 lib/bb/utils.py       | 26 ++++++++++++++++++++++++++
 3 files changed, 77 insertions(+), 26 deletions(-)

-- 
1.9.3

[PATCH 1/3] utils: ensure explode_dep_versions2 raises an exception on invalid/missing operator

[Paul Eggleton]

We really want an error rather than the version to just be silently
skipped when the operator is missing (e.g. "somepackage (1.0)" was
specified instead of "somepackage (>= 1.0)".)

Signed-off-by: Paul Eggleton <paul.eggleton@linux.intel.com>
---
 lib/bb/utils.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/bb/utils.py b/lib/bb/utils.py
index 90090b2..7ba1234 100644
--- a/lib/bb/utils.py
+++ b/lib/bb/utils.py
@@ -53,6 +53,9 @@ def set_context(ctx):
 # Context used in better_exec, eval
 _context = clean_context()
 
+class VersionStringException(Exception):
+    """Exception raised when an invalid version specification is found"""
+
 def explode_version(s):
     r = []
     alpha_regexp = re.compile('^([a-zA-Z]+)(.*)$')
@@ -188,6 +191,7 @@ def explode_dep_versions2(s):
                 i = i[1:]
             else:
                 # This is an unsupported case!
+                raise VersionStringException('Invalid version specification in "(%s" - invalid or missing operator' % i)
                 lastcmp = (i or "")
                 i = ""
             i.strip()
-- 
1.9.3

[PATCH 2/3] tests: add tests for OE pre-release version formatting

[Paul Eggleton]

This scheme is used for versioning recipes that are pre-release (alpha,
beta, etc.) within OpenEmbedded, so add some tests to ensure the
appropriate comparison results still hold true.

Signed-off-by: Paul Eggleton <paul.eggleton@linux.intel.com>
---
 lib/bb/tests/utils.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/bb/tests/utils.py b/lib/bb/tests/utils.py
index 7c50b1d..cf145f0 100644
--- a/lib/bb/tests/utils.py
+++ b/lib/bb/tests/utils.py
@@ -35,6 +35,10 @@ class VerCmpString(unittest.TestCase):
         self.assertTrue(result < 0)
         result = bb.utils.vercmp_string('1.1', '1_p2')
         self.assertTrue(result < 0)
+        result = bb.utils.vercmp_string('1.0', '1.0+1.1-beta1')
+        self.assertTrue(result < 0)
+        result = bb.utils.vercmp_string('1.1', '1.0+1.1-beta1')
+        self.assertTrue(result > 0)
 
     def test_explode_dep_versions(self):
         correctresult = {"foo" : ["= 1.10"]}
-- 
1.9.3

[PATCH 3/3] cooker: rework LAYERDEPENDS versioning so that it is actually useful

[Paul Eggleton]

We've had versioned dependency support in LAYERDEPENDS for quite a long
time, but I can say with pretty good certainty that almost nobody has
used it up to now because it was too strict - the specified version had
to exactly match the version in your configuration or you would get an
error; there was no "greater than or equal" option, which is usually
what you will want given that LAYERVERSION does get bumped from time to
time.

However, users mismatching layer branches and then having their builds
fail later on with some incomprehensible error is still a pretty common
problem. We can't simply use the git branch because not everyone is
always on a branch and the branch names don't always match up (and
that's not an issue). To provide a practical means to address branch
mismatching, I have reworked LAYERDEPENDS version specifications to use
the more familiar "dependency (>= version)" syntax as used with package
dependencies, support non-integer versions, and clarified the error
message a little. If we then take care to bump the version on every
breaking change, it is at least possible to have layers depend on these
changes when they update to match; we can now even support a major.minor
scheme to allow retrospectively adding a version limiter to old branches
when a new branch is created and yet still allow the old branch minor
version to be bumped if needed.

Fixes [YOCTO #5991].

Signed-off-by: Paul Eggleton <paul.eggleton@linux.intel.com>
---
 lib/bb/cooker.py      | 40 ++++++++++++++--------------------------
 lib/bb/tests/utils.py | 33 +++++++++++++++++++++++++++++++++
 lib/bb/utils.py       | 22 ++++++++++++++++++++++
 3 files changed, 69 insertions(+), 26 deletions(-)

diff --git a/lib/bb/cooker.py b/lib/bb/cooker.py
index f77c6c0..0bbbc09 100644
--- a/lib/bb/cooker.py
+++ b/lib/bb/cooker.py
@@ -1096,42 +1096,30 @@ class BBCooker:
                 # Check dependencies and store information for priority calculation
                 deps = self.data.getVar("LAYERDEPENDS_%s" % c, True)
                 if deps:
-                    depnamelist = []
-                    deplist = deps.split()
-                    for dep in deplist:
-                        depsplit = dep.split(':')
-                        if len(depsplit) > 1:
-                            try:
-                                depver = int(depsplit[1])
-                            except ValueError:
-                                parselog.error("invalid version value in LAYERDEPENDS_%s: \"%s\"", c, dep)
-                                errors = True
-                                continue
-                        else:
-                            depver = None
-                        dep = depsplit[0]
-                        depnamelist.append(dep)
-
+                    try:
+                        deplist = bb.utils.explode_dep_versions2(deps)
+                    except bb.utils.VersionStringException as vse:
+                        bb.fatal('Error parsing LAYERDEPENDS_%s: %s' % (c, str(vse)))
+                    for dep, oplist in deplist.iteritems():
                         if dep in collection_list:
-                            if depver:
+                            for opstr in oplist:
                                 layerver = self.data.getVar("LAYERVERSION_%s" % dep, True)
+                                (op, depver) = opstr.split()
                                 if layerver:
                                     try:
-                                        lver = int(layerver)
-                                    except ValueError:
-                                        parselog.error("invalid value for LAYERVERSION_%s: \"%s\"", c, layerver)
-                                        errors = True
-                                        continue
-                                    if lver != depver:
-                                        parselog.error("Layer '%s' depends on version %d of layer '%s', but version %d is enabled in your configuration", c, depver, dep, lver)
+                                        res = bb.utils.vercmp_string_op(layerver, depver, op)
+                                    except bb.utils.VersionStringException as vse:
+                                        bb.fatal('Error parsing LAYERDEPENDS_%s: %s' % (c, str(vse)))
+                                    if not res:
+                                        parselog.error("Layer '%s' depends on version %s of layer '%s', but version %s is currently enabled in your configuration. Check that you are using the correct matching versions/branches of these two layers.", c, opstr, dep, layerver)
                                         errors = True
                                 else:
-                                    parselog.error("Layer '%s' depends on version %d of layer '%s', which exists in your configuration but does not specify a version", c, depver, dep)
+                                    parselog.error("Layer '%s' depends on version %s of layer '%s', which exists in your configuration but does not specify a version. Check that you are using the correct matching versions/branches of these two layers.", c, opstr, dep)
                                     errors = True
                         else:
                             parselog.error("Layer '%s' depends on layer '%s', but this layer is not enabled in your configuration", c, dep)
                             errors = True
-                    collection_depends[c] = depnamelist
+                    collection_depends[c] = deplist.keys()
                 else:
                     collection_depends[c] = []
 
diff --git a/lib/bb/tests/utils.py b/lib/bb/tests/utils.py
index cf145f0..507de2d 100644
--- a/lib/bb/tests/utils.py
+++ b/lib/bb/tests/utils.py
@@ -55,3 +55,36 @@ class VerCmpString(unittest.TestCase):
         result = bb.utils.explode_dep_versions2("foo ( =1.10 )")
         self.assertEqual(result, correctresult)
 
+    def test_vercmp_string_op(self):
+        compareops = [('1', '1', '=', True),
+                      ('1', '1', '==', True),
+                      ('1', '1', '!=', False),
+                      ('1', '1', '>', False),
+                      ('1', '1', '<', False),
+                      ('1', '1', '>=', True),
+                      ('1', '1', '<=', True),
+                      ('1', '0', '=', False),
+                      ('1', '0', '==', False),
+                      ('1', '0', '!=', True),
+                      ('1', '0', '>', True),
+                      ('1', '0', '<', False),
+                      ('1', '0', '>>', True),
+                      ('1', '0', '<<', False),
+                      ('1', '0', '>=', True),
+                      ('1', '0', '<=', False),
+                      ('0', '1', '=', False),
+                      ('0', '1', '==', False),
+                      ('0', '1', '!=', True),
+                      ('0', '1', '>', False),
+                      ('0', '1', '<', True),
+                      ('0', '1', '>>', False),
+                      ('0', '1', '<<', True),
+                      ('0', '1', '>=', False),
+                      ('0', '1', '<=', True)]
+
+        for arg1, arg2, op, correctresult in compareops:
+            result = bb.utils.vercmp_string_op(arg1, arg2, op)
+            self.assertEqual(result, correctresult, 'vercmp_string_op("%s", "%s", "%s") != %s' % (arg1, arg2, op, correctresult))
+
+        # Check that clearly invalid operator raises an exception
+        self.assertRaises(bb.utils.VersionStringException, bb.utils.vercmp_string_op, '0', '0', '$')
diff --git a/lib/bb/utils.py b/lib/bb/utils.py
index 7ba1234..5ac9bcf 100644
--- a/lib/bb/utils.py
+++ b/lib/bb/utils.py
@@ -131,6 +131,28 @@ def vercmp_string(a, b):
     tb = split_version(b)
     return vercmp(ta, tb)
 
+def vercmp_string_op(a, b, op):
+    """
+    Compare two versions and check if the specified comparison operator matches the result of the comparison.
+    This function is fairly liberal about what operators it will accept since there are a variety of styles
+    depending on the context.
+    """
+    res = vercmp_string(a, b)
+    if op in ('=', '=='):
+        return res == 0
+    elif op == '<=':
+        return res <= 0
+    elif op == '>=':
+        return res >= 0
+    elif op in ('>', '>>'):
+        return res > 0
+    elif op in ('<', '<<'):
+        return res < 0
+    elif op == '!=':
+        return res != 0
+    else:
+        raise VersionStringException('Unsupported comparison operator "%s"' % op)
+
 def explode_deps(s):
     """
     Take an RDEPENDS style string of format:
-- 
1.9.3