diff mbox series

base.bbclass: Do not fail during parsing if ${SRCREV} does not exist

Message ID 20230825171625.1213968-1-pkj@axis.com
State New
Headers show
Series base.bbclass: Do not fail during parsing if ${SRCREV} does not exist | expand

Commit Message

Peter Kjellerstedt Aug. 25, 2023, 5:16 p.m. UTC
After commit a8e7b0f932 (base/package: Move source revision information
from PV to PKGV) was integrated, having a recipe with a SRCREV that
currently cannot be fetched would lead to an exception during parsing.
Catch that exception and instead raise bb.parse.SkipRecipe. That way
the parsing continues as it should. Instead you now get a meaningful
error if you try build a recipe with a SRCREV that cannot be fetched,
e.g.:

  ERROR: Nothing PROVIDES 'psplash'
  psplash was skipped: Fetcher failure: Unable to resolve 'unknown-ref'
  in upstream git repository in git ls-remote output for
  git.yoctoproject.org/psplash

Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
---
 meta/classes-global/base.bbclass | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Richard Purdie Aug. 30, 2023, 9:04 a.m. UTC | #1
On Fri, 2023-08-25 at 19:16 +0200, Peter Kjellerstedt wrote:
> After commit a8e7b0f932 (base/package: Move source revision information
> from PV to PKGV) was integrated, having a recipe with a SRCREV that
> currently cannot be fetched would lead to an exception during parsing.
> Catch that exception and instead raise bb.parse.SkipRecipe. That way
> the parsing continues as it should. Instead you now get a meaningful
> error if you try build a recipe with a SRCREV that cannot be fetched,
> e.g.:
> 
>   ERROR: Nothing PROVIDES 'psplash'
>   psplash was skipped: Fetcher failure: Unable to resolve 'unknown-ref'
>   in upstream git repository in git ls-remote output for
>   git.yoctoproject.org/psplash

Something doesn't sound quite right in this description. You've said "a
SRCREV that currently cannot be fetched" but "unknown-ref" will never
be fetchable and is completely invalid as a revision. I'd guess bitbake
is assuming it is a tag and trying to resolve it.

I really don't like complicating the core code when it doesn't make
sense to, particularly when it relates to cornercases most people don't
hit. I think SkipRecipe is also the incorrect thing to do here,
particularly when no reason is being passed back to bitbake to give
back to the user.

I suspect if you set the SRCREV to something that looks like a
revision, like "badbadbadbadbadbadbadbadbadbadbadbadbadb" it will avoid
the problems. Alternatively, just add the SkipRecipe locally along with
your unknown-ref.

Cheers,

Richard
Martin Jansa Aug. 30, 2023, 10:01 a.m. UTC | #2
FWIW: I have seen this in some meta-evil-bsp and the unfortunate part is
that only the first parsing fatal error is shown at time, so to find all
recipes which now need SkipRecipe, because your bsp vendor is evil, takes
possibly many parsing cycles.

But in the end nobody should work with evil vendors, so maybe we should
take the sticks to them not to core.

On Wed, Aug 30, 2023 at 11:04 AM Richard Purdie <
richard.purdie@linuxfoundation.org> wrote:

> On Fri, 2023-08-25 at 19:16 +0200, Peter Kjellerstedt wrote:
> > After commit a8e7b0f932 (base/package: Move source revision information
> > from PV to PKGV) was integrated, having a recipe with a SRCREV that
> > currently cannot be fetched would lead to an exception during parsing.
> > Catch that exception and instead raise bb.parse.SkipRecipe. That way
> > the parsing continues as it should. Instead you now get a meaningful
> > error if you try build a recipe with a SRCREV that cannot be fetched,
> > e.g.:
> >
> >   ERROR: Nothing PROVIDES 'psplash'
> >   psplash was skipped: Fetcher failure: Unable to resolve 'unknown-ref'
> >   in upstream git repository in git ls-remote output for
> >   git.yoctoproject.org/psplash
>
> Something doesn't sound quite right in this description. You've said "a
> SRCREV that currently cannot be fetched" but "unknown-ref" will never
> be fetchable and is completely invalid as a revision. I'd guess bitbake
> is assuming it is a tag and trying to resolve it.
>
> I really don't like complicating the core code when it doesn't make
> sense to, particularly when it relates to cornercases most people don't
> hit. I think SkipRecipe is also the incorrect thing to do here,
> particularly when no reason is being passed back to bitbake to give
> back to the user.
>
> I suspect if you set the SRCREV to something that looks like a
> revision, like "badbadbadbadbadbadbadbadbadbadbadbadbadb" it will avoid
> the problems. Alternatively, just add the SkipRecipe locally along with
> your unknown-ref.
>
> Cheers,
>
> Richard
>
>
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#186874):
> https://lists.openembedded.org/g/openembedded-core/message/186874
> Mute This Topic: https://lists.openembedded.org/mt/100960059/3617156
> Group Owner: openembedded-core+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [
> Martin.Jansa@gmail.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>
>
Peter Kjellerstedt Sept. 1, 2023, 4:44 p.m. UTC | #3
> -----Original Message-----
> From: Richard Purdie <richard.purdie@linuxfoundation.org>
> Sent: den 30 augusti 2023 11:04
> To: Peter Kjellerstedt <peter.kjellerstedt@axis.com>; openembedded-core@lists.openembedded.org
> Subject: Re: [OE-core] [PATCH] base.bbclass: Do not fail during parsing if ${SRCREV} does not exist
> 
> On Fri, 2023-08-25 at 19:16 +0200, Peter Kjellerstedt wrote:
> > After commit a8e7b0f932 (base/package: Move source revision information
> > from PV to PKGV) was integrated, having a recipe with a SRCREV that
> > currently cannot be fetched would lead to an exception during parsing.
> > Catch that exception and instead raise bb.parse.SkipRecipe. That way
> > the parsing continues as it should. Instead you now get a meaningful
> > error if you try build a recipe with a SRCREV that cannot be fetched,
> > e.g.:
> >
> >   ERROR: Nothing PROVIDES 'psplash'
> >   psplash was skipped: Fetcher failure: Unable to resolve 'unknown-ref'
> >   in upstream git repository in git ls-remote output for
> >   git.yoctoproject.org/psplash
> 
> Something doesn't sound quite right in this description. You've said "a
> SRCREV that currently cannot be fetched" but "unknown-ref" will never
> be fetchable and is completely invalid as a revision. I'd guess bitbake
> is assuming it is a tag and trying to resolve it.

You are correct in that the problem only occurs when non-SHA1 references 
are used in SRCREV and that should probably be mentioned in the commit 
message. 

> I really don't like complicating the core code when it doesn't make
> sense to, particularly when it relates to cornercases most people don't
> hit.

It is true that the use of non-SHA1 references in SRCREV is not 
recommended, but the bitbake code does have explicit support for it. 
And while we are working on changing our recipes to not use tags in 
SRCREV for released code, it is unfortunately not trivial to do so 
as we have other tooling that currently relies on being able to find 
the tag names in SRCREV. :(

However, not doing anything to improve the OE code is not an option 
I would like to see, because the following is the output I get with 
the current code (modified to remove the actual names):

WARNING: .../meta-axis/recipes-foo/bar/recipe1_git.bb: Exception during build_dependencies for fetcher_hashes_dummyfunc
WARNING: .../meta-axis/recipes-foo/bar/recipe1_git.bb: Error during finalise of .../meta-axis/recipes-foo/bar/recipe1_git.bb
WARNING: .../meta-axis/recipes-foo/bar/recipe2_git.bb: Exception during build_dependencies for fetcher_hashes_dummyfunc
WARNING: .../meta-axis/recipes-foo/bar/recipe2_git.bb: Error during finalise of .../meta-axis/recipes-foo/bar/recipe2_git.bb
WARNING: .../meta-axis/recipes-foo/bar/recipe3_git.bb: Exception during build_dependencies for fetcher_hashes_dummyfunc
WARNING: .../meta-axis/recipes-foo/bar/recipe3_git.bb: Error during finalise of .../meta-axis/recipes-foo/bar/recipe3_git.bb
WARNING: .../meta-axis/recipes-foo/bar/recipe4_git.bb: Exception during build_dependencies for fetcher_hashes_dummyfunc
WARNING: .../meta-axis/recipes-foo/bar/recipe4_git.bb: Error during finalise of .../meta-axis/recipes-foo/bar/recipe4_git.bb
WARNING: .../meta-axis/recipes-foo/bar/recipe5_git.bb: Exception during build_dependencies for fetcher_hashes_dummyfunc
WARNING: .../meta-axis/recipes-foo/bar/recipe5_git.bb: Error during finalise of .../meta-axis/recipes-foo/bar/recipe5_git.bb
WARNING: .../meta-axis/recipes-foo/bar/recipe6_git.bb: Exception during build_dependencies for fetcher_hashes_dummyfunc
WARNING: .../meta-axis/recipes-foo/bar/recipe6_git.bb: Error during finalise of .../meta-axis/recipes-foo/bar/recipe6_git.bb
WARNING: .../meta-axis/recipes-foo/bar/recipe7_git.bb: Exception during build_dependencies for fetcher_hashes_dummyfunc
WARNING: .../meta-axis/recipes-foo/bar/recipe7_git.bb: Error during finalise of .../meta-axis/recipes-foo/bar/recipe7_git.bb
WARNING: .../meta-axis/recipes-foo/bar/recipe8_git.bb: Exception during build_dependencies for fetcher_hashes_dummyfunc
WARNING: .../meta-axis/recipes-foo/bar/recipe8_git.bb: Error during finalise of .../meta-axis/recipes-foo/bar/recipe8_git.bb
WARNING: .../meta-axis/recipes-foo/bar/recipe9_git.bb: Exception during build_dependencies for fetcher_hashes_dummyfunc
WARNING: .../meta-axis/recipes-foo/bar/recipe9_git.bb: Error during finalise of .../meta-axis/recipes-foo/bar/recipe9_git.bb
WARNING: .../meta-axis/recipes-foo/bar/recipe10_git.bb: Exception during build_dependencies for fetcher_hashes_dummyfunc
WARNING: .../meta-axis/recipes-foo/bar/recipe10_git.bb: Error during finalise of .../meta-axis/recipes-foo/bar/recipe10_git.bb
WARNING: .../meta-axis/recipes-foo/bar/recipe11_git.bb: Exception during build_dependencies for fetcher_hashes_dummyfunc
WARNING: .../meta-axis/recipes-foo/bar/recipe11_git.bb: Error during finalise of .../meta-axis/recipes-foo/bar/recipe11_git.bb
WARNING: .../meta-axis/recipes-foo/bar/recipe12_git.bb: Exception during build_dependencies for fetcher_hashes_dummyfunc
WARNING: .../meta-axis/recipes-foo/bar/recipe12_git.bb: Error during finalise of .../meta-axis/recipes-foo/bar/recipe12_git.bb
WARNING: .../meta-axis/recipes-foo/bar/recipe13_git.bb: Exception during build_dependencies for fetcher_hashes_dummyfunc
WARNING: .../meta-axis/recipes-foo/bar/recipe13_git.bb: Error during finalise of .../meta-axis/recipes-foo/bar/recipe13_git.bb
WARNING: .../meta-axis/recipes-foo/bar/recipe14_git.bb: Exception during build_dependencies for fetcher_hashes_dummyfunc
WARNING: .../meta-axis/recipes-foo/bar/recipe14_git.bb: Error during finalise of .../meta-axis/recipes-foo/bar/recipe14_git.bb
ERROR: ExpansionError during parsing .../meta-axis/recipes-foo/bar/recipe2_git.bb
Traceback (most recent call last):
  File "/home/pkj/dists/cfp-master/poky/bitbake/lib/bb/fetch2/__init__.py", line 1718, in Fetch.__init__(urls=['git://gitserver.axis.com:29418/recipe2.git;protocol=ssh;nobranch=1'], d=<bb.data_smart.DataSmart object at 0x7f55f95cd2a0>, cache=True, localonly=False, connection_cache=None):
                     try:
    >                    self.ud[url] = FetchData(url, d, localonly)
                     except NonLocalMethod:
  File "/home/pkj/dists/cfp-master/poky/bitbake/lib/bb/fetch2/__init__.py", line 1347, in FetchData.__init__(url='git://gitserver.axis.com:29418/recipe2.git;protocol=ssh;nobranch=1', d=<bb.data_smart.DataSmart object at 0x7f55f95cd2a0>, localonly=False):
             if hasattr(self.method, "urldata_init"):
    >            self.method.urldata_init(self, d)

  File "/home/pkj/dists/cfp-master/poky/bitbake/lib/bb/fetch2/git.py", line 271, in Git.urldata_init(ud=<bb.fetch2.FetchData object at 0x7f5606ba3880>, d=<bb.data_smart.DataSmart object at 0x7f55f95cd2a0>):
                         ud.unresolvedrev[name] = ud.revisions[name]
    >                ud.revisions[name] = self.latest_revision(ud, d, name)

  File "/home/pkj/dists/cfp-master/poky/bitbake/lib/bb/fetch2/__init__.py", line 1662, in Git.latest_revision(ud=<bb.fetch2.FetchData object at 0x7f5606ba3880>, d=<bb.data_smart.DataSmart object at 0x7f55f95cd2a0>, name='default'):
             except KeyError:
    >            revs[key] = rev = self._latest_revision(ud, d, name)
                 return rev
  File "/home/pkj/dists/cfp-master/poky/bitbake/lib/bb/fetch2/git.py", line 799, in Git._latest_revision(ud=<bb.fetch2.FetchData object at 0x7f5606ba3880>, d=<bb.data_smart.DataSmart object at 0x7f55f95cd2a0>, name='default'):
                         return sha1
    >        raise bb.fetch2.FetchError("Unable to resolve '%s' in upstream git repository in git ls-remote output for %s" % \
                 (ud.unresolvedrev[name], ud.host+ud.path))
bb.data_smart.ExpansionError: Failure expanding variable fetcher_hashes_dummyfunc[vardepvalue], expression was ${@bb.fetch.get_hashvalue(d)} which triggered exception FetchError: Fetcher failure: Unable to resolve 'v1.2.3' in upstream git repository in git ls-remote output for gitserver.axis.com:29418/recipe2.git
The variable dependency chain for the failure is: fetcher_hashes_dummyfunc[vardepvalue]

That is not very user friendly, especially as the user running into 
this may not even be interested in building any of the failing recipes.
And since this happens during parsing, it also prevents the user from, 
e.g., running `bitbake -e` on the failing recipes.

> I think SkipRecipe is also the incorrect thing to do here,
> particularly when no reason is being passed back to bitbake to give
> back to the user.

After having given this some more thought and testing, I believe it 
actually is as simple as to just ignore the FetchError instead with:

def fetcher_get_hashvalue(d):
    # Catch and ignore any fetcher errors here to avoid errors during recipe
    # parsing due to references (e.g., tags) that cannot be retrieved. Any
    # error will be reported if the fetch task for the recipe is run.
    try:
        return bb.fetch.get_hashvalue(d)
    except bb.fetch2.FetchError:
        return ""

With that applied, I get the exact same behavior as we have in Mickledore, 
i.e., no errors during recipe parsing, and the following error if I 
actually try to build one of the recipes that cannot be fetched (with an 
unknown reference):

ERROR: psplash-0.1+git-r0 do_fetch: Bitbake Fetcher Error: FetchError("Unable to resolve 'unknown-ref' in upstream git repository in git ls-remote output for git.yoctoproject.org/psplash", None)

or with an unknown SHA-1:

ERROR: psplash-0.1+git-r0 do_fetch: Fetcher failure: Unable to find revision badbadbadbadbadbadbadbadbadbadbadbadbadb in branch master even from upstream
ERROR: psplash-0.1+git-r0 do_fetch: Bitbake Fetcher Error: FetchError('Unable to fetch URL from any source.', 'git://git.yoctoproject.org/psplash;branch=master;protocol=https')

An alternative to adding the fetcher_get_hashvalue() function to 
base.bbclass would be to add a safe version of bb.fetch.get_hashvalue() 
that does not raise FetchError and thus can be used directly in 
the fetcher_hashes_dummyfunc[vardepvalue], e.g.:

def get_safe_hashvalue(d, method_name='sortable_revision'):
    try:
        return get_hashvalue(d, method_name=method_name)
    except FetchError:
        return ""

Another alternative would be to be able to pass an option to 
get_hashvalue() to ignore errors, e.g.:

def get_hashvalue(d, method_name='sortable_revision', ignore_errors=False):
    try:
        pkgv, revs = _get_srcrev(d, method_name=method_name)
        return " ".join(revs)
    except FetchError as e:
        if ignore_errors:
            return ""
        raise e

I guess that the most Pythonic is to have the function in base.bbclass, 
but from a Bitbake perspective it may be cleaner to keep the error 
handling in the fetcher. My personal choice would be the last alternative.

> I suspect if you set the SRCREV to something that looks like a
> revision, like "badbadbadbadbadbadbadbadbadbadbadbadbadb" it will avoid
> the problems. Alternatively, just add the SkipRecipe locally along with
> your unknown-ref.
> 
> Cheers,
> 
> Richard

I hope this can persuade you to accept an updated patch with either of 
the suggested alternatives above as I believe this is an improvement to 
the code and not just complicating it.

//Peter
diff mbox series

Patch

diff --git a/meta/classes-global/base.bbclass b/meta/classes-global/base.bbclass
index 7c774d250f..fb55a6eee9 100644
--- a/meta/classes-global/base.bbclass
+++ b/meta/classes-global/base.bbclass
@@ -131,7 +131,13 @@  def setup_hosttools_dir(dest, toolsvar, d, fatal=True):
 python fetcher_hashes_dummyfunc() {
     return
 }
-fetcher_hashes_dummyfunc[vardepvalue] = "${@bb.fetch.get_hashvalue(d)}"
+fetcher_hashes_dummyfunc[vardepvalue] = "${@validate_hashvalue(d)}"
+
+def validate_hashvalue(d):
+    try:
+        bb.fetch.get_hashvalue(d)
+    except bb.fetch2.FetchError as e:
+        raise bb.parse.SkipRecipe(e)
 
 addtask fetch
 do_fetch[dirs] = "${DL_DIR}"