[PATCH 1/7] fetch2: Avoid incorrect getVarFlag call

[PATCH 1/7] fetch2: Avoid incorrect getVarFlag call

[Richard Purdie]

Calling getVarFlag with flag=None makes no sense, don't do it. Bitbake
used to silently ignore this, it now warns so avoid the warning.

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
 lib/bb/fetch2/__init__.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
index 9aea08b387..2b62b41618 100644
--- a/lib/bb/fetch2/__init__.py
+++ b/lib/bb/fetch2/__init__.py
@@ -1097,7 +1097,9 @@ def trusted_network(d, url):
         return True
 
     pkgname = d.expand(d.getVar('PN', False))
-    trusted_hosts = d.getVarFlag('BB_ALLOWED_NETWORKS', pkgname, False)
+    trusted_hosts = None
+    if pkgname:
+        trusted_hosts = d.getVarFlag('BB_ALLOWED_NETWORKS', pkgname, False)
 
     if not trusted_hosts:
         trusted_hosts = d.getVar('BB_ALLOWED_NETWORKS')
-- 
2.17.1

[PATCH 2/7] data_smart: Micro optimise _remove handling

[Richard Purdie]

This brings _remove handling into line with _append/_prepend with regard
to the parsing flag to getVarFlag.

This is an internal flag and the only times this is used is through getVar
during renameVar operations and when processing ?= operations to see if
a variable is set. In either case we don't need to process remove operations.

Therefore take the minor speedup and skip processing for parsing=True.

[YOCTO #10945]

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
 lib/bb/data_smart.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/bb/data_smart.py b/lib/bb/data_smart.py
index 0a8488ca1b..7b2c0a8943 100644
--- a/lib/bb/data_smart.py
+++ b/lib/bb/data_smart.py
@@ -805,7 +805,7 @@ class DataSmart(MutableMapping):
                 cachename = var + "[" + flag + "]"
             value = self.expand(value, cachename)
 
-        if value and flag == "_content" and local_var is not None and "_remove" in local_var:
+        if value and flag == "_content" and local_var is not None and "_remove" in local_var and not parsing:
             removes = []
             self.need_overrides()
             for (r, o) in local_var["_remove"]:
-- 
2.17.1

[PATCH 3/7] data_smart: Fix expand_cache and _remove operator interaction issues

[Richard Purdie]

The contents of the expand_cache is meant to match the return value of
getVarFlag() but the implementation was mostly in expandWithRefs(). If
an incorrect key was passed to expandWithRefs(), or a variable was only
partially expanded with no remove processing, the cache could become
corrupted.

Move the code to getVarFlag making the data lifecycle very clear, meaning
other calls to expandWithRefs() cannot corrupt the cache.

The expand_cache reset code needs to be moved ahead of any remote data
connectors too, since the expand_cache is now on the local side of the
connection.

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
 lib/bb/data_smart.py | 50 ++++++++++++++++++++------------------------
 1 file changed, 23 insertions(+), 27 deletions(-)

diff --git a/lib/bb/data_smart.py b/lib/bb/data_smart.py
index 7b2c0a8943..ecc71bd644 100644
--- a/lib/bb/data_smart.py
+++ b/lib/bb/data_smart.py
@@ -105,11 +105,7 @@ class VariableParse:
             if self.varname and key:
                 if self.varname == key:
                     raise Exception("variable %s references itself!" % self.varname)
-            if key in self.d.expand_cache:
-                varparse = self.d.expand_cache[key]
-                var = varparse.value
-            else:
-                var = self.d.getVarFlag(key, "_content")
+            var = self.d.getVarFlag(key, "_content")
             self.references.add(key)
             if var is not None:
                 return var
@@ -412,9 +408,6 @@ class DataSmart(MutableMapping):
         if not isinstance(s, str): # sanity check
             return VariableParse(varname, self, s)
 
-        if varname and varname in self.expand_cache:
-            return self.expand_cache[varname]
-
         varparse = VariableParse(varname, self)
 
         while s.find('${') != -1:
@@ -438,9 +431,6 @@ class DataSmart(MutableMapping):
 
         varparse.value = s
 
-        if varname:
-            self.expand_cache[varname] = varparse
-
         return varparse
 
     def expand(self, s, varname = None):
@@ -509,6 +499,7 @@ class DataSmart(MutableMapping):
 
     def setVar(self, var, value, **loginfo):
         #print("var=" + str(var) + "  val=" + str(value))
+        self.expand_cache = {}
         parsing=False
         if 'parsing' in loginfo:
             parsing=True
@@ -521,7 +512,7 @@ class DataSmart(MutableMapping):
 
         if 'op' not in loginfo:
             loginfo['op'] = "set"
-        self.expand_cache = {}
+
         match  = __setvar_regexp__.match(var)
         if match and match.group("keyword") in __setvar_keyword__:
             base = match.group('base')
@@ -672,6 +663,7 @@ class DataSmart(MutableMapping):
         self.setVar(var + "_prepend", value, ignore=True, parsing=True)
 
     def delVar(self, var, **loginfo):
+        self.expand_cache = {}
         if '_remote_data' in self.dict:
             connector = self.dict["_remote_data"]["_content"]
             res = connector.delVar(var)
@@ -681,7 +673,6 @@ class DataSmart(MutableMapping):
         loginfo['detail'] = ""
         loginfo['op'] = 'del'
         self.varhistory.record(**loginfo)
-        self.expand_cache = {}
         self.dict[var] = {}
         if var in self.overridedata:
             del self.overridedata[var]
@@ -704,13 +695,13 @@ class DataSmart(MutableMapping):
                          override = None
 
     def setVarFlag(self, var, flag, value, **loginfo):
+        self.expand_cache = {}
         if '_remote_data' in self.dict:
             connector = self.dict["_remote_data"]["_content"]
             res = connector.setVarFlag(var, flag, value)
             if not res:
                 return
 
-        self.expand_cache = {}
         if 'op' not in loginfo:
             loginfo['op'] = "set"
         loginfo['flag'] = flag
@@ -732,6 +723,17 @@ class DataSmart(MutableMapping):
             self.dict["__exportlist"]["_content"].add(var)
 
     def getVarFlag(self, var, flag, expand=True, noweakdefault=False, parsing=False):
+        if flag == "_content":
+            cachename = var
+        else:
+            if not flag:
+                bb.warn("Calling getVarFlag with flag unset is invalid")
+                return None
+            cachename = var + "[" + flag + "]"
+
+        if expand and cachename in self.expand_cache:
+            return self.expand_cache[cachename].value
+
         local_var, overridedata = self._findVar(var)
         value = None
         if flag == "_content" and overridedata is not None and not parsing:
@@ -796,14 +798,9 @@ class DataSmart(MutableMapping):
                 if match:
                     value = r + value
 
-        if expand and value:
-            # Only getvar (flag == _content) hits the expand cache
-            cachename = None
-            if flag == "_content":
-                cachename = var
-            else:
-                cachename = var + "[" + flag + "]"
-            value = self.expand(value, cachename)
+        if expand:
+            self.expand_cache[cachename] = self.expandWithRefs(value, cachename)
+            value = self.expand_cache[cachename].value
 
         if value and flag == "_content" and local_var is not None and "_remove" in local_var and not parsing:
             removes = []
@@ -821,20 +818,19 @@ class DataSmart(MutableMapping):
                 filtered = filter(lambda v: v not in removes,
                                   __whitespace_split__.split(value))
                 value = "".join(filtered)
-                if expand and var in self.expand_cache:
-                    # We need to ensure the expand cache has the correct value
-                    # flag == "_content" here
-                    self.expand_cache[var].value = value
+                if expand and cachename in self.expand_cache:
+                    self.expand_cache[cachename].value = value
+
         return value
 
     def delVarFlag(self, var, flag, **loginfo):
+        self.expand_cache = {}
         if '_remote_data' in self.dict:
             connector = self.dict["_remote_data"]["_content"]
             res = connector.delVarFlag(var, flag)
             if not res:
                 return
 
-        self.expand_cache = {}
         local_var, _ = self._findVar(var)
         if not local_var:
             return
-- 
2.17.1

[PATCH 4/7] data/data_smart: Allow getVarFlag to return the variable parser object

[Richard Purdie]

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
 lib/bb/data.py       | 11 +++++------
 lib/bb/data_smart.py | 18 +++++++++++++-----
 2 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/lib/bb/data.py b/lib/bb/data.py
index 80a7879cb6..e2700077c3 100644
--- a/lib/bb/data.py
+++ b/lib/bb/data.py
@@ -283,14 +283,12 @@ def build_dependencies(key, keys, shelldeps, varflagsexcl, d):
     try:
         if key[-1] == ']':
             vf = key[:-1].split('[')
-            value = d.getVarFlag(vf[0], vf[1], False)
-            parser = d.expandWithRefs(value, key)
+            value, parser = d.getVarFlag(vf[0], vf[1], False, retparser=True)
             deps |= parser.references
             deps = deps | (keys & parser.execs)
             return deps, value
         varflags = d.getVarFlags(key, ["vardeps", "vardepvalue", "vardepsexclude", "exports", "postfuncs", "prefuncs", "lineno", "filename"]) or {}
         vardeps = varflags.get("vardeps")
-        value = d.getVarFlag(key, "_content", False)
 
         def handle_contains(value, contains, d):
             newvalue = ""
@@ -310,9 +308,10 @@ def build_dependencies(key, keys, shelldeps, varflagsexcl, d):
             return value + newvalue
 
         if "vardepvalue" in varflags:
-           value = varflags.get("vardepvalue")
+            value = varflags.get("vardepvalue")
         elif varflags.get("func"):
             if varflags.get("python"):
+                value = d.getVarFlag(key, "_content", False)
                 parser = bb.codeparser.PythonParser(key, logger)
                 if value and "\t" in value:
                     logger.warning("Variable %s contains tabs, please remove these (%s)" % (key, d.getVar("FILE")))
@@ -321,7 +320,7 @@ def build_dependencies(key, keys, shelldeps, varflagsexcl, d):
                 deps = deps | (keys & parser.execs)
                 value = handle_contains(value, parser.contains, d)
             else:
-                parsedvar = d.expandWithRefs(value, key)
+                value, parsedvar = d.getVarFlag(key, "_content", False, retparser=True)
                 parser = bb.codeparser.ShellParser(key, logger)
                 parser.parse_shell(parsedvar.value)
                 deps = deps | shelldeps
@@ -337,7 +336,7 @@ def build_dependencies(key, keys, shelldeps, varflagsexcl, d):
             if "exports" in varflags:
                 deps = deps | set(varflags["exports"].split())
         else:
-            parser = d.expandWithRefs(value, key)
+            value, parser = d.getVarFlag(key, "_content", False, retparser=True)
             deps |= parser.references
             deps = deps | (keys & parser.execs)
             value = handle_contains(value, parser.contains, d)
diff --git a/lib/bb/data_smart.py b/lib/bb/data_smart.py
index ecc71bd644..4ad0567c9a 100644
--- a/lib/bb/data_smart.py
+++ b/lib/bb/data_smart.py
@@ -722,7 +722,7 @@ class DataSmart(MutableMapping):
                 self.dict["__exportlist"]["_content"] = set()
             self.dict["__exportlist"]["_content"].add(var)
 
-    def getVarFlag(self, var, flag, expand=True, noweakdefault=False, parsing=False):
+    def getVarFlag(self, var, flag, expand=True, noweakdefault=False, parsing=False, retparser=False):
         if flag == "_content":
             cachename = var
         else:
@@ -798,9 +798,11 @@ class DataSmart(MutableMapping):
                 if match:
                     value = r + value
 
+        parser = None
+        if expand or retparser:
+            parser = self.expandWithRefs(value, cachename)
         if expand:
-            self.expand_cache[cachename] = self.expandWithRefs(value, cachename)
-            value = self.expand_cache[cachename].value
+            value = parser.value
 
         if value and flag == "_content" and local_var is not None and "_remove" in local_var and not parsing:
             removes = []
@@ -818,8 +820,14 @@ class DataSmart(MutableMapping):
                 filtered = filter(lambda v: v not in removes,
                                   __whitespace_split__.split(value))
                 value = "".join(filtered)
-                if expand and cachename in self.expand_cache:
-                    self.expand_cache[cachename].value = value
+                if parser:
+                    parser.value = value
+
+        if parser:
+            self.expand_cache[cachename] = parser
+
+        if retparser:
+            return value, parser
 
         return value
 
-- 
2.17.1

[PATCH 5/7] bitbake: data: Ensure task checksums account for remove data

[Richard Purdie]

Currently remove operations are not being accounted for in the task
checksums. This is a fairly serious oversight and needs to be fixed.

To do so, we need internal data from getVarFlag combined with the
expanded variable data so that only "active" remove operators are
accounted for in the task checksum. We can get this from the new
optional removes attribute in the returned parser object.

The code can then use the data on active remove operators to account
for the removals in task checksum but only when the removal is active.

We have to be careful here not to reference any expanded data since this
may for example contain build paths. This means we can only map back
and reference the unsplit (and hence unexpanded) remove string which may
expand to multiple removal values.

[YOCTO #12913]

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
 lib/bb/data.py       | 12 ++++++++++++
 lib/bb/data_smart.py | 27 ++++++++++++++++++---------
 2 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/lib/bb/data.py b/lib/bb/data.py
index e2700077c3..fde4cba6bb 100644
--- a/lib/bb/data.py
+++ b/lib/bb/data.py
@@ -307,6 +307,14 @@ def build_dependencies(key, keys, shelldeps, varflagsexcl, d):
                 return newvalue
             return value + newvalue
 
+        def handle_remove(value, deps, removes, d):
+            for r in sorted(removes):
+                r2 = d.expandWithRefs(r, None)
+                value += "\n_remove of %s" % r
+                deps |= r2.references
+                deps = deps | (keys & r2.execs)
+            return value
+
         if "vardepvalue" in varflags:
             value = varflags.get("vardepvalue")
         elif varflags.get("func"):
@@ -327,6 +335,8 @@ def build_dependencies(key, keys, shelldeps, varflagsexcl, d):
                 deps = deps | parsedvar.references
                 deps = deps | (keys & parser.execs) | (keys & parsedvar.execs)
                 value = handle_contains(value, parsedvar.contains, d)
+                if hasattr(parsedvar, "removes"):
+                    value = handle_remove(value, deps, parsedvar.removes, d)
             if vardeps is None:
                 parser.log.flush()
             if "prefuncs" in varflags:
@@ -340,6 +350,8 @@ def build_dependencies(key, keys, shelldeps, varflagsexcl, d):
             deps |= parser.references
             deps = deps | (keys & parser.execs)
             value = handle_contains(value, parser.contains, d)
+            if hasattr(parser, "removes"):
+                value = handle_remove(value, deps, parser.removes, d)
 
         if "vardepvalueexclude" in varflags:
             exclude = varflags.get("vardepvalueexclude")
diff --git a/lib/bb/data_smart.py b/lib/bb/data_smart.py
index 4ad0567c9a..8c4c6a9a32 100644
--- a/lib/bb/data_smart.py
+++ b/lib/bb/data_smart.py
@@ -805,7 +805,7 @@ class DataSmart(MutableMapping):
             value = parser.value
 
         if value and flag == "_content" and local_var is not None and "_remove" in local_var and not parsing:
-            removes = []
+            removes = {}
             self.need_overrides()
             for (r, o) in local_var["_remove"]:
                 match = True
@@ -814,14 +814,23 @@ class DataSmart(MutableMapping):
                         if not o2 in self.overrides:
                             match = False                            
                 if match:
-                    removes.extend(self.expand(r).split())
-
-            if removes:
-                filtered = filter(lambda v: v not in removes,
-                                  __whitespace_split__.split(value))
-                value = "".join(filtered)
-                if parser:
-                    parser.value = value
+                    removes[r] = self.expand(r).split()
+
+            if removes and parser:
+                parser.removes = set()
+                val = ""
+                for v in __whitespace_split__.split(parser.value):
+                    skip = False
+                    for r in removes:
+                        if v in removes[r]:
+                            parser.removes.add(r)
+                            skip = True
+                    if skip:
+                        continue
+                    val = val + v
+                parser.value = val
+                if expand:
+                    value = parser.value
 
         if parser:
             self.expand_cache[cachename] = parser
-- 
2.17.1

[PATCH 6/7] data/siggen: Extract task hash generation code into a function

[Richard Purdie]

By creating a standalone function, we can add better functional testing
of this code.

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
 lib/bb/data.py   | 38 ++++++++++++++++++++++++++++++++++++++
 lib/bb/siggen.py | 37 ++++---------------------------------
 2 files changed, 42 insertions(+), 33 deletions(-)

diff --git a/lib/bb/data.py b/lib/bb/data.py
index fde4cba6bb..d66d98cc8b 100644
--- a/lib/bb/data.py
+++ b/lib/bb/data.py
@@ -38,6 +38,7 @@ the speed is more critical here.
 # Based on functions from the base bb module, Copyright 2003 Holger Schurig
 
 import sys, os, re
+import hashlib
 if sys.argv[0][-5:] == "pydoc":
     path = os.path.dirname(os.path.dirname(sys.argv[1]))
 else:
@@ -405,6 +406,43 @@ def generate_dependencies(d):
         #print "For %s: %s" % (task, str(deps[task]))
     return tasklist, deps, values
 
+def generate_dependency_hash(tasklist, gendeps, lookupcache, whitelist, fn):
+    taskdeps = {}
+    basehash = {}
+
+    for task in tasklist:
+        data = lookupcache[task]
+
+        if data is None:
+            bb.error("Task %s from %s seems to be empty?!" % (task, fn))
+            data = ''
+
+        gendeps[task] -= whitelist
+        newdeps = gendeps[task]
+        seen = set()
+        while newdeps:
+            nextdeps = newdeps
+            seen |= nextdeps
+            newdeps = set()
+            for dep in nextdeps:
+                if dep in whitelist:
+                    continue
+                gendeps[dep] -= whitelist
+                newdeps |= gendeps[dep]
+            newdeps -= seen
+
+        alldeps = sorted(seen)
+        for dep in alldeps:
+            data = data + dep
+            var = lookupcache[dep]
+            if var is not None:
+                data = data + str(var)
+        k = fn + "." + task
+        basehash[k] = hashlib.md5(data.encode("utf-8")).hexdigest()
+        taskdeps[task] = alldeps
+
+    return taskdeps, basehash
+
 def inherits_class(klass, d):
     val = d.getVar('__inherit_cache', False) or []
     needle = os.path.join('classes', '%s.bbclass' % klass)
diff --git a/lib/bb/siggen.py b/lib/bb/siggen.py
index e9bb51d736..03c824ec38 100644
--- a/lib/bb/siggen.py
+++ b/lib/bb/siggen.py
@@ -110,42 +110,13 @@ class SignatureGeneratorBasic(SignatureGenerator):
         ignore_mismatch = ((d.getVar("BB_HASH_IGNORE_MISMATCH") or '') == '1')
         tasklist, gendeps, lookupcache = bb.data.generate_dependencies(d)
 
-        taskdeps = {}
-        basehash = {}
+        taskdeps, basehash = bb.data.generate_dependency_hash(tasklist, gendeps, lookupcache, self.basewhitelist, fn)
 
         for task in tasklist:
-            data = lookupcache[task]
-
-            if data is None:
-                bb.error("Task %s from %s seems to be empty?!" % (task, fn))
-                data = ''
-
-            gendeps[task] -= self.basewhitelist
-            newdeps = gendeps[task]
-            seen = set()
-            while newdeps:
-                nextdeps = newdeps
-                seen |= nextdeps
-                newdeps = set()
-                for dep in nextdeps:
-                    if dep in self.basewhitelist:
-                        continue
-                    gendeps[dep] -= self.basewhitelist
-                    newdeps |= gendeps[dep]
-                newdeps -= seen
-
-            alldeps = sorted(seen)
-            for dep in alldeps:
-                data = data + dep
-                var = lookupcache[dep]
-                if var is not None:
-                    data = data + str(var)
-            datahash = hashlib.md5(data.encode("utf-8")).hexdigest()
             k = fn + "." + task
-            if not ignore_mismatch and k in self.basehash and self.basehash[k] != datahash:
-                bb.error("When reparsing %s, the basehash value changed from %s to %s. The metadata is not deterministic and this needs to be fixed." % (k, self.basehash[k], datahash))
-            self.basehash[k] = datahash
-            taskdeps[task] = alldeps
+            if not ignore_mismatch and k in self.basehash and self.basehash[k] != basehash[k]:
+                bb.error("When reparsing %s, the basehash value changed from %s to %s. The metadata is not deterministic and this needs to be fixed." % (k, self.basehash[k], basehash[k]))
+            self.basehash[k] = basehash[k]
 
         self.taskdeps[fn] = taskdeps
         self.gendeps[fn] = gendeps
-- 
2.17.1

[PATCH 7/7] test/data: Add new tests for task checksum changing/not changing

[Richard Purdie]

This adds some basic tests for task checksums to ensure that the
checksums:

* change when variables change
* change when active _remove operators are present
* don't change when the _remove operators are not active
* change when an active contains() expression is present
* dont' change a contains() expression isn't active

There is a lot of other functionality which should be added to this
test but its a start.

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
 lib/bb/tests/data.py | 48 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/lib/bb/tests/data.py b/lib/bb/tests/data.py
index 8279115e03..9ac78e3683 100644
--- a/lib/bb/tests/data.py
+++ b/lib/bb/tests/data.py
@@ -455,6 +455,54 @@ class Contains(unittest.TestCase):
         self.assertFalse(bb.utils.contains_any("SOMEFLAG", "x y z", True, False, self.d))
 
 
+class TaskHash(unittest.TestCase):
+    def test_taskhashes(self):
+        def gettask_bashhash(taskname, d):
+            tasklist, gendeps, lookupcache = bb.data.generate_dependencies(d)
+            taskdeps, basehash = bb.data.generate_dependency_hash(tasklist, gendeps, lookupcache, set(), "somefile")
+            bb.warn(str(lookupcache))
+            return basehash["somefile." + taskname]
+
+        d = bb.data.init()
+        d.setVar("__BBTASKS", ["mytask"])
+        d.setVar("__exportlist", [])
+        d.setVar("mytask", "${MYCOMMAND}")
+        d.setVar("MYCOMMAND", "${VAR}; foo; bar; exit 0")
+        d.setVar("VAR", "val")
+        orighash = gettask_bashhash("mytask", d)
+
+        # Changing a variable should change the hash
+        d.setVar("VAR", "val2")
+        nexthash = gettask_bashhash("mytask", d)
+        self.assertNotEqual(orighash, nexthash)
+
+        d.setVar("VAR", "val")
+        # Adding an inactive removal shouldn't change the hash
+        d.setVar("BAR", "notbar")
+        d.setVar("MYCOMMAND_remove", "${BAR}")
+        nexthash = gettask_bashhash("mytask", d)
+        self.assertEqual(orighash, nexthash)
+
+        # Adding an active removal should change the hash
+        d.setVar("BAR", "bar;")
+        nexthash = gettask_bashhash("mytask", d)
+        self.assertNotEqual(orighash, nexthash)
+
+        # Setup an inactive contains()
+        d.setVar("VAR", "${@bb.utils.contains('VAR2', 'A', 'val', '', d)}")
+        orighash = gettask_bashhash("mytask", d)
+
+        # Activate the contains() and the hash should change
+        d.setVar("VAR2", "A")
+        nexthash = gettask_bashhash("mytask", d)
+        self.assertNotEqual(orighash, nexthash)
+
+        # The contains should be inactive but even though VAR2 has a
+        # different value the hash should match the original
+        d.setVar("VAR2", "B")
+        nexthash = gettask_bashhash("mytask", d)
+        self.assertEqual(orighash, nexthash)
+
 class Serialize(unittest.TestCase):
 
     def test_serialize(self):
-- 
2.17.1

Re: [PATCH 2/7] data_smart: Micro optimise _remove handling

[Martin Jansa]

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

This one and "data_smart: Fix expand_cache and _remove operator interaction
issues" are slightly changing the behavior, not sure if this change was
intended.

Since last bitbake upgrade I've started seeing:
ERROR: ca-certificates-20180409-r0 do_package_qa: QA Issue: ca-certificates
rdepends on openssl, but it isn't a build dependency? [build-deps]
ERROR: nativesdk-bash-completion-2.8-r0 do_package_qa: QA Issue:
nativesdk-bash-completion rdepends on bash, but it isn't a build
dependency? [build-deps]

with bitbake 5463c16e3619d324aed137f47f93f0997a227d29
# $RDEPENDS_ca-certificates [4 operations]
#   _remove[pn-iputils] conf/distro/include/some-distro.inc:52
#     " ${PN}-ping6"
#   append oe-core/meta/recipes-support/ca-certificates/
ca-certificates_20180409.bb:85
#     "openssl"
#   _remove
meta-webos/recipes-support/ca-certificates/ca-certificates_%.bbappend:24
#     "openssl"
#   rename from RDEPENDS_${PN} data.py:117 [expandKeys]
#     " openssl"
# pre-expansion value:
#   " openssl"
RDEPENDS_ca-certificates=" "

and do_package_qa sees the openssl in rdepends:

NOTE: recipe ca-certificates-20180409-r0: task do_package_qa: Started
WARNING: ca-certificates-20180409-r0 do_package_qa: build-dep:
ca-certificates-dev
WARNING: ca-certificates-20180409-r0 do_package_qa: build-dep: openssl
WARNING: ca-certificates-20180409-r0 do_package_qa: rdep_data for rdepend:
openssl are {}
WARNING: ca-certificates-20180409-r0 do_package_qa: trying
/jenkins/mjansa/build-signage-master/BUILD/pkgdata/m16pp/runtime-rprovides/openssl/
ERROR: ca-certificates-20180409-r0 do_package_qa: QA Issue: ca-certificates
rdepends on openssl, but it isn't a build dependency? [build-deps]
WARNING: ca-certificates-20180409-r0 do_package_qa: build-dep:
ca-certificates
ERROR: ca-certificates-20180409-r0 do_package_qa: QA run found fatal
errors. Please consider fixing them.
ERROR: ca-certificates-20180409-r0 do_package_qa: Function failed:
do_package_qa
ERROR: Logfile of failure stored in:
/jenkins/mjansa/build-signage-master/BUILD/work/all-signage-linux/ca-certificates/20180409-r0/temp/log.do_package_qa.46878
NOTE: recipe ca-certificates-20180409-r0: task do_package_qa: Failed

with bitbake a68de8ace62eaba23856bfb301efbbe1824322aa
# $RDEPENDS_ca-certificates [4 operations]
#   _remove[pn-iputils] conf/distro/include/some-distro.inc:52
#     " ${PN}-ping6"
#   append oe-core/meta/recipes-support/ca-certificates/
ca-certificates_20180409.bb:85
#     "openssl"
#   _remove
meta-webos/recipes-support/ca-certificates/ca-certificates_%.bbappend:24
#     "openssl"
#   rename from RDEPENDS_${PN} data.py:116 [expandKeys]
#     " "
# pre-expansion value:
#   " "
RDEPENDS_ca-certificates=" "

git log --oneline a68de8a..5463c16e | tee
5463c16e test/data: Add new tests for task checksum changing/not changing
796a20d2 data/siggen: Extract task hash generation code into a function
57d2ee17 bitbake: data: Ensure task checksums account for remove data
136100dc data/data_smart: Allow getVarFlag to return the variable parser
object
a039052f data_smart: Fix expand_cache and _remove operator interaction
issues
6d19eb32 data_smart: Micro optimise _remove handling

With the micro optimise commit 6d19eb32 it already has openssl in "rename
from RDEPENDS_${PN}":
# $RDEPENDS_ca-certificates [4 operations]
#   _remove[pn-iputils] conf/distro/include/some-disto.inc:52
#     " ${PN}-ping6"
#   append oe-core/meta/recipes-support/ca-certificates/
ca-certificates_20180409.bb:85
#     "openssl"
#   _remove
meta-webos/recipes-support/ca-certificates/ca-certificates_%.bbappend:24
#     "openssl"
#   rename from RDEPENDS_${PN} data.py:116 [expandKeys]
#     " openssl"
# pre-expansion value:
#   " "
RDEPENDS_ca-certificates=" "

The bbappend doesn't do anything special, just this:
# ca-certificates is allarch, so it either has to RDEPENDS on
${MLPREFIX}openssl or
# in this case it's easier to just remove the dependency and pull it from
somewhere
# else
RDEPENDS_${PN}_remove = "openssl"

and similar _remove bbappend for bash-completion:
VIRTUAL-RUNTIME_bash ?= "bash"
RDEPENDS_${PN}_append_class-target = " ${VIRTUAL-RUNTIME_bash}"
RDEPENDS_${PN}_remove = "bash"

The reason for bash-completion is probably the same as in ca-certificates,
but it's surprising that I have maybe a 100 of bbappends like above with
bash, but only these 2 are causing the QA error.

Will dig a bit more where exactly it was introduced and will try to
reproduce with just oe-core.

On Thu, Oct 18, 2018 at 11:23 AM Richard Purdie <
richard.purdie@linuxfoundation.org> wrote:

> This brings _remove handling into line with _append/_prepend with regard
> to the parsing flag to getVarFlag.
>
> This is an internal flag and the only times this is used is through getVar
> during renameVar operations and when processing ?= operations to see if
> a variable is set. In either case we don't need to process remove
> operations.
>
> Therefore take the minor speedup and skip processing for parsing=True.
>
> [YOCTO #10945]
>
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> ---
>  lib/bb/data_smart.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/bb/data_smart.py b/lib/bb/data_smart.py
> index 0a8488ca1b..7b2c0a8943 100644
> --- a/lib/bb/data_smart.py
> +++ b/lib/bb/data_smart.py
> @@ -805,7 +805,7 @@ class DataSmart(MutableMapping):
>                  cachename = var + "[" + flag + "]"
>              value = self.expand(value, cachename)
>
> -        if value and flag == "_content" and local_var is not None and
> "_remove" in local_var:
> +        if value and flag == "_content" and local_var is not None and
> "_remove" in local_var and not parsing:
>              removes = []
>              self.need_overrides()
>              for (r, o) in local_var["_remove"]:
> --
> 2.17.1
>
> --
> _______________________________________________
> bitbake-devel mailing list
> bitbake-devel@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/bitbake-devel
>

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

Re: [PATCH 2/7] data_smart: Micro optimise _remove handling

[richard.purdie]

On Fri, 2018-10-19 at 10:57 +0200, Martin Jansa wrote:
> This one and "data_smart: Fix expand_cache and _remove operator
> interaction issues" are slightly changing the behavior, not sure if
> this change was intended.

I think this is a bug, thanks for reporting. I was able to reprocduce
with OE-Core and a bbappend containing the line you mentioned. The way
RDEPENDS are handled in insane.bbclass is a little unique and I was
able to reduce it to a test case:

    def test_remove_with_override(self):
        self.d.setVar("TEST_bar", "testvalue2")
        self.d.setVar("TEST_some_val", "testvalue3 testvalue5")
        self.d.setVar("TEST_some_val_remove", "testvalue3")
        self.d.setVar("TEST_foo", "testvalue4")
        self.d.setVar("OVERRIDES", "foo:bar:some_val")
        self.assertEqual(self.d.getVar("TEST"), " testvalue5")

and this gives a good pointer to where/how the code is breaking. I'll
try and figure out how to fix it (and add a test case for it!).

Cheers,

Richard