Re: [PATCH 1/1] fetch2/__init__.py: add an "auto" policy for BB_SRCREV_POLICY

[Richard Purdie <richard.purdie@linuxfoundation.org>]

On Thu, 2012-08-09 at 16:56 +0800, Robert Yang wrote:
> The newly added "auto" policy is used for the SRCREV="${AUTOREV}" recipe,
> the current BB_SRCREV_POLICY are: "clear" (default) and "cache":
> 
> * When "clear", the "AUTOREV" recipe will not be cached in bb_cache.dat
> 
> * When "cache", the "AUTOREV" would be cached in bb_cache.dat, but it
>   doesn't try to get the latest revision.
> 
> The "auto" policy is much like the "cache" except that it will try to
> get the latest revision.
> 
> [YOCTO #2920]
> 
> Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
> Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
> ---
>  bitbake/lib/bb/fetch2/__init__.py |   12 +++++++++---
>  1 files changed, 9 insertions(+), 3 deletions(-)

I've been asked why I've not taken this. The simple answer is that I
can't figure out why we need to do this. The commit message above says
what was done but not why.

I also strongly dislike "auto" as a name for this mode. "clear" and
"cache" are at least descriptions of the behaviour, "auto" is not.

So can you please explain why we need this change? What use case does it
fix or improve or how is it useful? At the very least the commit message
needs rewriting and the option renaming.

> 
> diff --git a/bitbake/lib/bb/fetch2/__init__.py b/bitbake/lib/bb/fetch2/__init__.py
> index 12ebce2..27b73c0 100644
> --- a/bitbake/lib/bb/fetch2/__init__.py
> +++ b/bitbake/lib/bb/fetch2/__init__.py
> @@ -249,8 +249,8 @@ def fetcher_init(d):
>      Calls before this must not hit the cache.
>      """
>      # When to drop SCM head revisions controlled by user policy
> -    srcrev_policy = d.getVar('BB_SRCREV_POLICY', True) or "clear"
> -    if srcrev_policy == "cache":
> +    srcrev_policy = get_srcrev_policy(d)
> +    if srcrev_policy == "cache" or srcrev_policy == "auto":
>          logger.debug(1, "Keeping SRCREV cache due to cache policy of: %s", srcrev_policy)
>      elif srcrev_policy == "clear":
>          logger.debug(1, "Clearing SRCREV cache due to cache policy of: %s", srcrev_policy)
> @@ -384,9 +384,13 @@ def subprocess_setup():
>      # SIGPIPE errors are known issues with gzip/bash
>      signal.signal(signal.SIGPIPE, signal.SIG_DFL)
>  
> +def get_srcrev_policy(d):
> +    return d.getVar('BB_SRCREV_POLICY', True) or "auto"
> +
>  def get_autorev(d):
>      #  only not cache src rev in autorev case
> -    if d.getVar('BB_SRCREV_POLICY', True) != "cache":
> +    srcrev_policy = get_srcrev_policy(d)
> +    if srcrev_policy != "cache" and srcrev_policy != "auto":
>          d.setVar('__BB_DONT_CACHE', '1')

This looks highly suspect. If you do this, it won't check the upstream
revision at each invocation of bitbake which is how AUTOINC is meant to
behave.

Now if you want that behaviour, fine, but calling it "auto" is rather
misleading. It also seems to contradict your statement above that you
want the latest revision since with this you won't always get it.

Cheers,

Richard

>      return "AUTOINC"
>  
> @@ -1048,6 +1052,8 @@ class FetchMethod(object):
>  
>          revs = bb.persist_data.persist('BB_URI_HEADREVS', d)
>          key = self.generate_revision_key(url, ud, d, name)
> +        if get_srcrev_policy(d) == "auto":
> +            revs[key] = self._latest_revision(url, ud, d, name)
>          try:
>              return revs[key]
>          except KeyError: