RE: [bitbake-devel] [PATCH] fetch2/cooker: Fix source revision handling with floating upstreams

[Peter Kjellerstedt <peter.kjellerstedt@axis.com>]

> -----Original Message-----
> From: bitbake-devel@lists.openembedded.org <bitbake-devel@lists.openembedded.org> On Behalf Of Richard Purdie
> Sent: den 10 februari 2022 18:54
> To: bitbake-devel@lists.openembedded.org
> Subject: [bitbake-devel] [PATCH] fetch2/cooker: Fix source revision handling with floating upstreams
> 
> Where a git url uses a tag instead of a full source revision, breakage
> can currently occur in builds. Issues include:
> 
> * the revision being looked up in multiple tasks (fetch and unpack)
> * the risk a different revision may be obtained in those tasks
> * that some tasks may not be allowed to access the network
> * that a revision may not be consistent throughout a given build
> * rerunning a specific task may given inconsistent results
> 
> To fix this, stop the workers from cleaning out the source revision store. This
> should only be done in the cooker itself (based on current policy).
> 
> Also, where the code "sees" an upstream access, mark the recipe as not to be
> cached. The reparse re-triggers the upstream lookup by the server.
> 
> Add a test to ensure that if get_srcrev isn't called, the user is told they're
> using a configuration that is known to break.
> 
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> ---
>  bin/bitbake-worker        | 2 +-
>  lib/bb/cookerdata.py      | 4 ++--
>  lib/bb/fetch2/__init__.py | 1 +
>  lib/bb/fetch2/git.py      | 6 ++++++
>  4 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/bin/bitbake-worker b/bin/bitbake-worker
> index 063cf37926..9d850ec77c 100755
> --- a/bin/bitbake-worker
> +++ b/bin/bitbake-worker
> @@ -440,7 +440,7 @@ class BitbakeWorker(object):
>      def handle_cookercfg(self, data):
>          self.cookercfg = pickle.loads(data)
>          self.databuilder = bb.cookerdata.CookerDataBuilder(self.cookercfg, worker=True)
> -        self.databuilder.parseBaseConfiguration()
> +        self.databuilder.parseBaseConfiguration(worker=True)
>          self.data = self.databuilder.data
> 
>      def handle_extraconfigdata(self, data):
> diff --git a/lib/bb/cookerdata.py b/lib/bb/cookerdata.py
> index 397b43dfa7..a961f36574 100644
> --- a/lib/bb/cookerdata.py
> +++ b/lib/bb/cookerdata.py
> @@ -258,12 +258,12 @@ class CookerDataBuilder(object):
>          self.data = self.basedata
>          self.mcdata = {}
> 
> -    def parseBaseConfiguration(self):
> +    def parseBaseConfiguration(self, worker=False):
>          data_hash = hashlib.sha256()
>          try:
>              self.data = self.parseConfigurationFiles(self.prefiles, self.postfiles)
> 
> -            if self.data.getVar("BB_WORKERCONTEXT", False) is None:
> +            if self.data.getVar("BB_WORKERCONTEXT", False) is None and not worker:
>                  bb.fetch.fetcher_init(self.data)
>              bb.parse.init_parser(self.data)
>              bb.codeparser.parser_cache_init(self.data)
> diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
> index 28a3e54c7f..d099cd1092 100644
> --- a/lib/bb/fetch2/__init__.py
> +++ b/lib/bb/fetch2/__init__.py
> @@ -765,6 +765,7 @@ def get_srcrev(d, method_name='sortable_revision'):
>      that fetcher provides a method with the given name and the same signature as sortable_revision.
>      """
> 
> +    d.setVar("__BBSEENSRCREV", "1")
>      recursion = d.getVar("__BBINSRCREV")
>      if recursion:
>          raise FetchError("There are recursive references in fetcher variables, likely through SRC_URI")
> diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
> index 30da8e95b7..836ef1c49a 100644
> --- a/lib/bb/fetch2/git.py
> +++ b/lib/bb/fetch2/git.py
> @@ -727,6 +727,12 @@ class Git(FetchMethod):
>          """
>          Compute the HEAD revision for the url
>          """
> +        if not d.getVar("__BBSEENSRCREV"):
> +            raise bb.fetch2.FetchError("Recipe uses a floating tag/branch without a fixed SRCREV yet doesn't call bb.fetch2.get_srcrev() (use SRCPV in PV for OE).")

This does not seem correct. We use SRCREV = "<tag>" for all our own recipes. 
However, we never use ${SRCPV} if SRCREV is set to a tag that matches PV. 
Why should we? We only use it if the SRCREV _does not_ match PV.

> +
> +        # Ensure we mark as not cached
> +        bb.fetch2.get_autorev(d)
> +

How will this affect us that use BB_SRCREV_POLICY = "cache", which we do to 
avoid unnecessary `git ls-remote` since we know our own tags do not move?

>          output = self._lsremote(ud, d, "")
>          # Tags of the form ^{} may not work, need to fallback to other form
>          if ud.unresolvedrev[name][:5] == "refs/" or ud.usehead:
> --
> 2.32.0

//Peter