Bug 3890 - @contains does not create vardep automatically
Summary: @contains does not create vardep automatically
Status: VERIFIED FIXED
Alias: None
Product: OE-Core
Classification: Build System, Metadata & Runtime
Component: core (show other bugs)
Version: unspecified
Hardware: x86 Multiple
: Medium enhancement
Target Milestone: 1.4
Assignee: Richard Purdie
QA Contact:
URL:
Whiteboard:
Depends on:
Blocks:
 
Reported: 2013-02-14 11:24 UTC by Martin Jansa
Modified: 2013-04-12 16:50 UTC (History)
3 users (show)

See Also:
OS type for building Yocto: ---
Type of Regression: ---
Verified:
Documentation change: ---


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Jansa 2013-02-14 11:24:17 UTC
tested with all 3 "contains" variants
oe.utils.contains
base_contains (calls oe.utils.contains)
bb.utils.contains (same code as oe.utils.contains)

When I use 
FOO="${@oe.utils.contains('BAR', '1', 'YES', 'NO')}"
it would be convenient to automatically detect FOO dependency on BAR as variable.

Real world example:

foo.bb:
RDEPENDS_${PN} = "${@oe.utils.contains('DISTRO_FEATURES', 'bar', 'man', '', d)}"

This scenario currently works very badly:
1) do_package checksum is not changed when DISTRO_FEATURES gets or loose 'bar', because expression looks the same and foo.do_package does not depend on DISTRO_FEATURES variable
2) do_package_write checksum is changed, because new deptask is found as reported by bitbake-diffsigs:
Dependency on task man.do_packagedata was added with hash deadcafe 
so user sees do_package_write and e.g. do_package_write_ipk executed, but 'Depends' field in .ipk is the same, because do_packagedata/emit_pkgdata wasn't reexecuted to write new RDEPENDS value.

There is simple work around to add explicit vardep
RDEPENDS_foo[vardep] += "DISTRO_FEATURES"
but that's not really intuitive, easy to forget and hard to find out later when you just notice on target that some incremental builds behave different then other.

Also RDEPENDS_${PN}[vardep] += "DISTRO_FEATURES" does not seem to work.

Can we record this dependency somehow automatically?

I can probably add something like
def append_contains(variable, checkvariable, checkvalues, truevalue, falsevalue, d):
    d.appendVar(variable, base_contains(checkvariable, checkvalues, truevalue, falsevalue, d)
    d.appendVarFlag(variable, 'vardeps', checkvariable)

or

def base_contains_with_vardep(variable, checkvariable, checkvalues, truevalue, falsevalue, d):
    d.appendVarFlag(variable, 'vardeps', checkvariable)
    base_contains(checkvariable, checkvalues, truevalue, falsevalue, d)

RDEPENDS_${PN} = "${@base_contains_wit_vardep('RDEPENDS_foo', 'DISTRO_FEATURES', 'bar', 'man', '', d)}"

But I hope there is better simpler solution :).
Comment 1 Martin Jansa 2013-02-14 11:37:47 UTC
This one is simpler for sure..
(the same as base_contains without first line to getVar(variable))

${@val_contains("${DISTRO_FEATURES}", 'bar', 'man', '', d)}

Other ideas?
Comment 2 Richard Purdie 2013-02-14 21:51:30 UTC
See how this works:

diff --git a/bitbake/lib/bb/codeparser.py b/bitbake/lib/bb/codeparser.py
index 979e6bd..78f82e1 100644
--- a/bitbake/lib/bb/codeparser.py
+++ b/bitbake/lib/bb/codeparser.py
@@ -101,6 +101,7 @@ class BufferedLogger(Logger):
 
 class PythonParser():
     getvars = ("d.getVar", "bb.data.getVar", "data.getVar", "d.appendVar", "d.prependVar")
+    containsfuncs = ("bb.utils.contains", "base_contains", "oe.utils.contains")
     execfuncs = ("bb.build.exec_func", "bb.build.exec_task")
 
     def warn(self, func, arg):
@@ -119,7 +120,7 @@ class PythonParser():
 
     def visit_Call(self, node):
         name = self.called_node_name(node.func)
-        if name in self.getvars:
+        if name in self.getvars or name in self.containsfuncs:
             if isinstance(node.args[0], ast.Str):
                 self.var_references.add(node.args[0].s)
             else:
Comment 3 Martin Jansa 2013-02-15 04:20:50 UTC
For some reason this isn't enough, bitbake-diffsigs does not show new dependency and task checksum is the same with or without bar in DISTRO_FEATURES.
Comment 4 Richard Purdie 2013-02-15 11:35:02 UTC
I did some experimentation. I can't make it work properly with base_contains since it doesn't track the variable reference properly however if I instead do:

--- a/meta/recipes-kernel/latencytop/latencytop_0.5.bb
+++ b/meta/recipes-kernel/latencytop/latencytop_0.5.bb
@@ -15,7 +15,7 @@ SRC_URI = "http://www.latencytop.org/download/latencytop-${PV}.tar.gz \
 SRC_URI[md5sum] = "73bb3371c6ee0b0e68e25289027e865c"
 SRC_URI[sha256sum] = "9e7f72fbea7bd918e71212a1eabaad8488d2c602205d2e3c95d62cd57e9203ef"
 
-EXTRA_OEMAKE_X = "${@base_contains('DISTRO_FEATURES', 'x11', 'HAS_GTK_GUI=1', '', d)}"
+EXTRA_OEMAKE_X = "${@oe.utils.contains('DISTRO_FEATURES', 'x11', 'HAS_GTK_GUI=1', '', d)}"
 
 CFLAGS += "${LDFLAGS}"
 
then this does track the reference properly with the patch I attached. I'm tempted just to remove base_contains since it is just an old wrapper...
Comment 5 Richard Purdie 2013-02-15 11:42:57 UTC
A further update - there is a cache issue here. The patch does work and *does* need the base_contains. You need to rm tmp/cache/bb_codeparser.dat for the changes to be visible though since the cache gets in the way.
Comment 6 Richard Purdie 2013-02-15 12:33:20 UTC
Fixed with http://git.yoctoproject.org/cgit.cgi/poky/commit/?id=f966c544aa4da61a19bc0d751c2bbf0f66028791 (now including cache version bump)
Comment 7 Martin Jansa 2013-02-15 21:42:23 UTC
Confirmed on my use-case with base_contains. Thanks!
Comment 8 Martin Jansa 2013-02-15 21:42:55 UTC
Is this something we can backport to danny?
Comment 9 Richard Purdie 2013-02-15 23:00:50 UTC
cc'd to the danny maintainer. We need to test this in master a while first but in theory the patch would apply easily enough. Probably some other bitbake fixes we could pull onto the stable branch I guess.

Only concern is this will change all the sstate checksums but we've probably changed enough in the point release anyway to make that a lesser consideration...
Comment 10 Martin Jansa 2013-04-12 16:13:08 UTC
(In reply to comment #9)
> cc'd to the danny maintainer. We need to test this in master a while first
> but in theory the patch would apply easily enough. Probably some other
> bitbake fixes we could pull onto the stable branch I guess.
> 
> Only concern is this will change all the sstate checksums but we've probably
> changed enough in the point release anyway to make that a lesser
> consideration...

Ross, can you cherry-pick 
767b4751232f4ee3979deb4d3f733fcf9ee2bd44
0b9d117631ce909312d53b93289e61defc6be01c
to bitbake-1.16?
Comment 11 Ross Burton 2013-04-12 16:24:49 UTC
Richard has to do the bitbake cherry-picking.
Comment 12 Richard Purdie 2013-04-12 16:50:32 UTC
cherry-picked to 1.16