Patchwork [bitbake-devel,v2] parse/ConfHander/BBHandler/utils: Fix cache dependency bugs

login
register
mail settings
Submitter Richard Purdie
Date Nov. 29, 2013, 11:15 p.m.
Message ID <1385766956.11222.80.camel@ted>
Download mbox | patch
Permalink /patch/62639/
State New
Headers show

Comments

Richard Purdie - Nov. 29, 2013, 11:15 p.m.
Currently bitbake only adds files to its dependency list if they exist.
If you add 'include foo.inc' to your recipe and the file doesn't exist,
then later you add the file, the cache will not be invalidated.

This leads to another bug which is that if files don't exist and then
you add them and they should be found first due to BBPATH, again the
cache won't invalidate.

This patch adds in tracking of files we check for the existence of so
that if they are added later, the cache correctly invalidates. This
necessitated a new version of bb.utils.which which returns a list of
files tested for.

The patch also adds in checks for duplicate file includes and for now
prints a warning about this. That will likely become a fatal error at
some point since its never usually desired to include a file twice.

The same issue is also fixed for class inheritance. Now when a class 
is added which would be found in the usual search path, it will cause
the cache to be invalidated.

Unfortunately this is old code in bitbake and the patch isn't the
neatest since we have to work within that framework.

[YOCTO #5611]
[YOCTO #4425]

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
Martin Jansa - Nov. 29, 2013, 11:27 p.m.
On Fri, Nov 29, 2013 at 11:15:56PM +0000, Richard Purdie wrote:
> 
> Currently bitbake only adds files to its dependency list if they exist.
> If you add 'include foo.inc' to your recipe and the file doesn't exist,
> then later you add the file, the cache will not be invalidated.
> 
> This leads to another bug which is that if files don't exist and then
> you add them and they should be found first due to BBPATH, again the
> cache won't invalidate.
> 
> This patch adds in tracking of files we check for the existence of so
> that if they are added later, the cache correctly invalidates. This
> necessitated a new version of bb.utils.which which returns a list of
> files tested for.
> 
> The patch also adds in checks for duplicate file includes and for now
> prints a warning about this. That will likely become a fatal error at
> some point since its never usually desired to include a file twice.
> 
> The same issue is also fixed for class inheritance. Now when a class 
> is added which would be found in the usual search path, it will cause
> the cache to be invalidated.
> 
> Unfortunately this is old code in bitbake and the patch isn't the
> neatest since we have to work within that framework.

Does it depend on any other patches? Even after removing extra bitbake/
prefix (I guess the patch is for poky not bitbake repo) it doesn't apply
in current master:

error: patch failed: lib/bb/parse/parse_py/ConfHandler.py:82
error: lib/bb/parse/parse_py/ConfHandler.py: patch does not apply
Patch failed at 0001 parse/ConfHander/BBHandler/utils: Fix cache
dependency bugs
The copy of the patch that failed is found in:
   /OE/bitbake/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.


> 
> [YOCTO #5611]
> [YOCTO #4425]
> 
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> ---
> diff --git a/bitbake/lib/bb/parse/__init__.py b/bitbake/lib/bb/parse/__init__.py
> index c973f6f..97983c9 100644
> --- a/bitbake/lib/bb/parse/__init__.py
> +++ b/bitbake/lib/bb/parse/__init__.py
> @@ -73,9 +73,17 @@ def update_mtime(f):
>  def mark_dependency(d, f):
>      if f.startswith('./'):
>          f = "%s/%s" % (os.getcwd(), f[2:])
> -    deps = (d.getVar('__depends') or []) + [(f, cached_mtime(f))]
> -    d.setVar('__depends', deps)
> -
> +    deps = (d.getVar('__depends') or [])
> +    s = (f, cached_mtime_noerror(f))
> +    if s not in deps:
> +        deps.append(s)
> +        d.setVar('__depends', deps)
> +
> +def check_dependency(d, f):
> +    s = (f, cached_mtime_noerror(f))
> +    deps = (d.getVar('__depends') or [])
> +    return s in deps
> +   
>  def supports(fn, data):
>      """Returns true if we have a handler for this file, false otherwise"""
>      for h in handlers:
> @@ -102,11 +110,14 @@ def init_parser(d):
>  def resolve_file(fn, d):
>      if not os.path.isabs(fn):
>          bbpath = d.getVar("BBPATH", True)
> -        newfn = bb.utils.which(bbpath, fn)
> +        newfn, attempts = bb.utils.which(bbpath, fn, history=True)
> +        for af in attempts:
> +            mark_dependency(d, af)
>          if not newfn:
>              raise IOError("file %s not found in %s" % (fn, bbpath))
>          fn = newfn
>  
> +    mark_dependency(d, fn)
>      if not os.path.isfile(fn):
>          raise IOError("file %s not found" % fn)
>  
> diff --git a/bitbake/lib/bb/parse/parse_py/BBHandler.py b/bitbake/lib/bb/parse/parse_py/BBHandler.py
> index 01f22d3..7cba649 100644
> --- a/bitbake/lib/bb/parse/parse_py/BBHandler.py
> +++ b/bitbake/lib/bb/parse/parse_py/BBHandler.py
> @@ -77,7 +77,10 @@ def inherit(files, fn, lineno, d):
>          if not os.path.isabs(file):
>              dname = os.path.dirname(fn)
>              bbpath = "%s:%s" % (dname, d.getVar("BBPATH", True))
> -            abs_fn = bb.utils.which(bbpath, file)
> +            abs_fn, attempts = bb.utils.which(bbpath, file, history=True)
> +            for af in attempts:
> +                if af != abs_fn:
> +                    bb.parse.mark_dependency(d, af)
>              if abs_fn:
>                  file = abs_fn
>  
> diff --git a/bitbake/lib/bb/parse/parse_py/ConfHandler.py b/bitbake/lib/bb/parse/parse_py/ConfHandler.py
> index c28f18b..f4fb2aa 100644
> --- a/bitbake/lib/bb/parse/parse_py/ConfHandler.py
> +++ b/bitbake/lib/bb/parse/parse_py/ConfHandler.py
> @@ -82,12 +82,15 @@ def include(oldfn, fn, lineno, data, error_out):
>      if not os.path.isabs(fn):
>          dname = os.path.dirname(oldfn)
>          bbpath = "%s:%s" % (dname, data.getVar("BBPATH", True))
> -        abs_fn = bb.utils.which(bbpath, fn)
> +        abs_fn, attempts = bb.utils.which(bbpath, fn, history=True)
> +        if abs_fn and bb.parse.check_dependency(data, abs_fn):
> +            bb.warn("Duplicate inclusion for %s in %s" % (abs_fn, data.getVar('FILE', True)))
> +        for af in attempts:
> +            bb.parse.mark_dependency(data, af)
>          if abs_fn:
>              fn = abs_fn
> -
> -    if fn in (data.getVar("__depends", False) or ""):
> -        bb.warn("Duplicate inclusion for %s" % fn)
> +    elif bb.parse.check_dependency(data, fn):
> +        bb.warn("Duplicate inclusion for %s in %s" % (fn, data.getVar('FILE', True)))
>  
>      from bb.parse import handle
>      try:
> @@ -96,6 +99,7 @@ def include(oldfn, fn, lineno, data, error_out):
>          if error_out:
>              raise ParseError("Could not %(error_out)s file %(fn)s" % vars(), oldfn, lineno)
>          logger.debug(2, "CONF file '%s' not found", fn)
> +        bb.parse.mark_dependency(data, fn)
>  
>  # We have an issue where a UI might want to enforce particular settings such as
>  # an empty DISTRO variable. If configuration files do something like assigning
> diff --git a/bitbake/lib/bb/utils.py b/bitbake/lib/bb/utils.py
> index 560f55a..0be45e1 100644
> --- a/bitbake/lib/bb/utils.py
> +++ b/bitbake/lib/bb/utils.py
> @@ -796,22 +796,28 @@ def copyfile(src, dest, newmtime = None, sstat = None):
>          newmtime = sstat[stat.ST_MTIME]
>      return newmtime
>  
> -def which(path, item, direction = 0):
> +def which(path, item, direction = 0, history = False):
>      """
>      Locate a file in a PATH
>      """
>  
> +    hist = []
>      paths = (path or "").split(':')
>      if direction != 0:
>          paths.reverse()
>  
>      for p in paths:
>          next = os.path.join(p, item)
> +        hist.append(next)
>          if os.path.exists(next):
>              if not os.path.isabs(next):
>                  next = os.path.abspath(next)
> +            if history:
> +                return next, hist
>              return next
>  
> +    if history:
> +        return "", hist
>      return ""
>  
>  def to_boolean(string, default=None):
> 
> 
> _______________________________________________
> bitbake-devel mailing list
> bitbake-devel@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/bitbake-devel
Richard Purdie - Nov. 29, 2013, 11:31 p.m.
On Sat, 2013-11-30 at 00:27 +0100, Martin Jansa wrote:
> On Fri, Nov 29, 2013 at 11:15:56PM +0000, Richard Purdie wrote:
> > 
> > Currently bitbake only adds files to its dependency list if they exist.
> > If you add 'include foo.inc' to your recipe and the file doesn't exist,
> > then later you add the file, the cache will not be invalidated.
> > 
> > This leads to another bug which is that if files don't exist and then
> > you add them and they should be found first due to BBPATH, again the
> > cache won't invalidate.
> > 
> > This patch adds in tracking of files we check for the existence of so
> > that if they are added later, the cache correctly invalidates. This
> > necessitated a new version of bb.utils.which which returns a list of
> > files tested for.
> > 
> > The patch also adds in checks for duplicate file includes and for now
> > prints a warning about this. That will likely become a fatal error at
> > some point since its never usually desired to include a file twice.
> > 
> > The same issue is also fixed for class inheritance. Now when a class 
> > is added which would be found in the usual search path, it will cause
> > the cache to be invalidated.
> > 
> > Unfortunately this is old code in bitbake and the patch isn't the
> > neatest since we have to work within that framework.
> 
> Does it depend on any other patches? Even after removing extra bitbake/
> prefix (I guess the patch is for poky not bitbake repo) it doesn't apply
> in current master:
> 
> error: patch failed: lib/bb/parse/parse_py/ConfHandler.py:82
> error: lib/bb/parse/parse_py/ConfHandler.py: patch does not apply
> Patch failed at 0001 parse/ConfHander/BBHandler/utils: Fix cache
> dependency bugs
> The copy of the patch that failed is found in:
>    /OE/bitbake/.git/rebase-apply/patch
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> 

Sorry, I screwed up the patch I sent out. The right version is applied
in master-next of bitbake though:

http://git.openembedded.org/bitbake/commit/?h=master-next&id=78d285871e4b8c54ccc4602d571e85f922e37ccd

Cheers,

Richard

Patch

diff --git a/bitbake/lib/bb/parse/__init__.py b/bitbake/lib/bb/parse/__init__.py
index c973f6f..97983c9 100644
--- a/bitbake/lib/bb/parse/__init__.py
+++ b/bitbake/lib/bb/parse/__init__.py
@@ -73,9 +73,17 @@  def update_mtime(f):
 def mark_dependency(d, f):
     if f.startswith('./'):
         f = "%s/%s" % (os.getcwd(), f[2:])
-    deps = (d.getVar('__depends') or []) + [(f, cached_mtime(f))]
-    d.setVar('__depends', deps)
-
+    deps = (d.getVar('__depends') or [])
+    s = (f, cached_mtime_noerror(f))
+    if s not in deps:
+        deps.append(s)
+        d.setVar('__depends', deps)
+
+def check_dependency(d, f):
+    s = (f, cached_mtime_noerror(f))
+    deps = (d.getVar('__depends') or [])
+    return s in deps
+   
 def supports(fn, data):
     """Returns true if we have a handler for this file, false otherwise"""
     for h in handlers:
@@ -102,11 +110,14 @@  def init_parser(d):
 def resolve_file(fn, d):
     if not os.path.isabs(fn):
         bbpath = d.getVar("BBPATH", True)
-        newfn = bb.utils.which(bbpath, fn)
+        newfn, attempts = bb.utils.which(bbpath, fn, history=True)
+        for af in attempts:
+            mark_dependency(d, af)
         if not newfn:
             raise IOError("file %s not found in %s" % (fn, bbpath))
         fn = newfn
 
+    mark_dependency(d, fn)
     if not os.path.isfile(fn):
         raise IOError("file %s not found" % fn)
 
diff --git a/bitbake/lib/bb/parse/parse_py/BBHandler.py b/bitbake/lib/bb/parse/parse_py/BBHandler.py
index 01f22d3..7cba649 100644
--- a/bitbake/lib/bb/parse/parse_py/BBHandler.py
+++ b/bitbake/lib/bb/parse/parse_py/BBHandler.py
@@ -77,7 +77,10 @@  def inherit(files, fn, lineno, d):
         if not os.path.isabs(file):
             dname = os.path.dirname(fn)
             bbpath = "%s:%s" % (dname, d.getVar("BBPATH", True))
-            abs_fn = bb.utils.which(bbpath, file)
+            abs_fn, attempts = bb.utils.which(bbpath, file, history=True)
+            for af in attempts:
+                if af != abs_fn:
+                    bb.parse.mark_dependency(d, af)
             if abs_fn:
                 file = abs_fn
 
diff --git a/bitbake/lib/bb/parse/parse_py/ConfHandler.py b/bitbake/lib/bb/parse/parse_py/ConfHandler.py
index c28f18b..f4fb2aa 100644
--- a/bitbake/lib/bb/parse/parse_py/ConfHandler.py
+++ b/bitbake/lib/bb/parse/parse_py/ConfHandler.py
@@ -82,12 +82,15 @@  def include(oldfn, fn, lineno, data, error_out):
     if not os.path.isabs(fn):
         dname = os.path.dirname(oldfn)
         bbpath = "%s:%s" % (dname, data.getVar("BBPATH", True))
-        abs_fn = bb.utils.which(bbpath, fn)
+        abs_fn, attempts = bb.utils.which(bbpath, fn, history=True)
+        if abs_fn and bb.parse.check_dependency(data, abs_fn):
+            bb.warn("Duplicate inclusion for %s in %s" % (abs_fn, data.getVar('FILE', True)))
+        for af in attempts:
+            bb.parse.mark_dependency(data, af)
         if abs_fn:
             fn = abs_fn
-
-    if fn in (data.getVar("__depends", False) or ""):
-        bb.warn("Duplicate inclusion for %s" % fn)
+    elif bb.parse.check_dependency(data, fn):
+        bb.warn("Duplicate inclusion for %s in %s" % (fn, data.getVar('FILE', True)))
 
     from bb.parse import handle
     try:
@@ -96,6 +99,7 @@  def include(oldfn, fn, lineno, data, error_out):
         if error_out:
             raise ParseError("Could not %(error_out)s file %(fn)s" % vars(), oldfn, lineno)
         logger.debug(2, "CONF file '%s' not found", fn)
+        bb.parse.mark_dependency(data, fn)
 
 # We have an issue where a UI might want to enforce particular settings such as
 # an empty DISTRO variable. If configuration files do something like assigning
diff --git a/bitbake/lib/bb/utils.py b/bitbake/lib/bb/utils.py
index 560f55a..0be45e1 100644
--- a/bitbake/lib/bb/utils.py
+++ b/bitbake/lib/bb/utils.py
@@ -796,22 +796,28 @@  def copyfile(src, dest, newmtime = None, sstat = None):
         newmtime = sstat[stat.ST_MTIME]
     return newmtime
 
-def which(path, item, direction = 0):
+def which(path, item, direction = 0, history = False):
     """
     Locate a file in a PATH
     """
 
+    hist = []
     paths = (path or "").split(':')
     if direction != 0:
         paths.reverse()
 
     for p in paths:
         next = os.path.join(p, item)
+        hist.append(next)
         if os.path.exists(next):
             if not os.path.isabs(next):
                 next = os.path.abspath(next)
+            if history:
+                return next, hist
             return next
 
+    if history:
+        return "", hist
     return ""
 
 def to_boolean(string, default=None):