diff mbox series

[bitbake-devel] fetch2/git.py: fix update_mirror_links

Message ID 20251230025413.1293484-1-Qi.Chen@windriver.com
State New
Headers show
Series [bitbake-devel] fetch2/git.py: fix update_mirror_links | expand

Commit Message

ChenQi Dec. 30, 2025, 2:54 a.m. UTC 
From: Chen Qi <Qi.Chen@windriver.com>

We cannot assume ud has the same fetcher with origud. For example,
if we set map git:// to file:// in PREMIRRORS, ud is using local fetcher
and origud is using git fetcher. In such case, the ud does have the
'shallow' attribute. And we'll see the following error:

  Exception: AttributeError: 'FetchData' object has no attribute 'shallow'

Looking at the logic of this function, I think it's the origud's shallow
that should be checked. So the logic becomes: if origud is using shallow
and its full shallow tarball does not exist yet, symlink to ensure it
exists. This should make more sense.

Signed-off-by: Chen Qi <Qi.Chen@windriver.com>
---
 lib/bb/fetch2/git.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Paul Barker Feb. 11, 2026, 4:13 p.m. UTC | #1 
On Tue, 2025-12-30 at 10:54 +0800, Chen Qi via lists.openembedded.org
wrote:
> From: Chen Qi <Qi.Chen@windriver.com>
> 
> We cannot assume ud has the same fetcher with origud. For example,
> if we set map git:// to file:// in PREMIRRORS, ud is using local fetcher
> and origud is using git fetcher. In such case, the ud does have the
> 'shallow' attribute. And we'll see the following error:
> 
>   Exception: AttributeError: 'FetchData' object has no attribute 'shallow'
> 
> Looking at the logic of this function, I think it's the origud's shallow
> that should be checked. So the logic becomes: if origud is using shallow
> and its full shallow tarball does not exist yet, symlink to ensure it
> exists. This should make more sense.
> 
> Signed-off-by: Chen Qi <Qi.Chen@windriver.com>
> ---
>  lib/bb/fetch2/git.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
> index 709c692a8..f288afb51 100644
> --- a/lib/bb/fetch2/git.py
> +++ b/lib/bb/fetch2/git.py
> @@ -359,7 +359,7 @@ class Git(FetchMethod):
>          super().update_mirror_links(ud, origud)
>          # When using shallow mode, add a symlink to the original fullshallow
>          # path to ensure a valid symlink even in the `PREMIRRORS` case
> -        if ud.shallow and not os.path.exists(origud.fullshallow):
> +        if origud.shallow and not os.path.exists(origud.fullshallow):
>              self.ensure_symlink(ud.localpath, origud.fullshallow)
>  
>      def try_premirror(self, ud, d):

Hi Qi, thanks for your patience on this one as we work through the
backlog.

The only place update_mirror_links is currently called is from
try_mirror_url() in lib/bb/fetch2/__init__.py. This is called as
origud.method.update_mirror_links(ud, origud), so checking
origud.shallow instead of ud.shallow looks correct to me. I think your
patch is correct, we'll take another look at it in the next review call
and hopefully get it merged.

Best regards,
Paul Barker Feb. 19, 2026, 2:36 p.m. UTC | #2 
On Wed, 2026-02-11 at 16:13 +0000, Paul Barker wrote:
> On Tue, 2025-12-30 at 10:54 +0800, Chen Qi via lists.openembedded.org
> wrote:
> > From: Chen Qi <Qi.Chen@windriver.com>
> > 
> > We cannot assume ud has the same fetcher with origud. For example,
> > if we set map git:// to file:// in PREMIRRORS, ud is using local fetcher
> > and origud is using git fetcher. In such case, the ud does have the
> > 'shallow' attribute. And we'll see the following error:
> > 
> >   Exception: AttributeError: 'FetchData' object has no attribute 'shallow'
> > 
> > Looking at the logic of this function, I think it's the origud's shallow
> > that should be checked. So the logic becomes: if origud is using shallow
> > and its full shallow tarball does not exist yet, symlink to ensure it
> > exists. This should make more sense.
> > 
> > Signed-off-by: Chen Qi <Qi.Chen@windriver.com>
> > ---
> >  lib/bb/fetch2/git.py | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
> > index 709c692a8..f288afb51 100644
> > --- a/lib/bb/fetch2/git.py
> > +++ b/lib/bb/fetch2/git.py
> > @@ -359,7 +359,7 @@ class Git(FetchMethod):
> >          super().update_mirror_links(ud, origud)
> >          # When using shallow mode, add a symlink to the original fullshallow
> >          # path to ensure a valid symlink even in the `PREMIRRORS` case
> > -        if ud.shallow and not os.path.exists(origud.fullshallow):
> > +        if origud.shallow and not os.path.exists(origud.fullshallow):
> >              self.ensure_symlink(ud.localpath, origud.fullshallow)
> >  
> >      def try_premirror(self, ud, d):
> 
> Hi Qi, thanks for your patience on this one as we work through the
> backlog.
> 
> The only place update_mirror_links is currently called is from
> try_mirror_url() in lib/bb/fetch2/__init__.py. This is called as
> origud.method.update_mirror_links(ud, origud), so checking
> origud.shallow instead of ud.shallow looks correct to me. I think your
> patch is correct, we'll take another look at it in the next review call
> and hopefully get it merged.

We are missing some test coverage here as we've never seen the
AttributeError on the autobuilder with this patch applied. There was a
similiar error seen for v1 of the patch when it was submitted, but that
was a different implementation modifying lib/bb/fetch2/__init__.py (the
applied version was v3).

We do have a test case that looks relevant in lib/bb/tests/fetch.py:
GitShallowTest.test_shallow_mirrors.

Are you able to look in to this and extend the test coverage if needed?

Best regards,
ChenQi Feb. 26, 2026, 3:59 a.m. UTC | #3 
Hi Paul,

Yes. I'll add a test case for it.
The problem was revealed when recipes are using the same SRC_URI + git:// -> file:// PREMIRRORS settings.

Regards,
Qi

-----Original Message-----
From: Paul Barker <paul@pbarker.dev> 
Sent: Thursday, February 19, 2026 10:37 PM
To: Chen, Qi <Qi.Chen@windriver.com>; bitbake-devel@lists.openembedded.org
Subject: Re: [bitbake-devel][PATCH] fetch2/git.py: fix update_mirror_links

On Wed, 2026-02-11 at 16:13 +0000, Paul Barker wrote:
> On Tue, 2025-12-30 at 10:54 +0800, Chen Qi via lists.openembedded.org
> wrote:
> > From: Chen Qi <Qi.Chen@windriver.com>
> > 
> > We cannot assume ud has the same fetcher with origud. For example, 
> > if we set map git:// to file:// in PREMIRRORS, ud is using local 
> > fetcher and origud is using git fetcher. In such case, the ud does 
> > have the 'shallow' attribute. And we'll see the following error:
> > 
> >   Exception: AttributeError: 'FetchData' object has no attribute 'shallow'
> > 
> > Looking at the logic of this function, I think it's the origud's 
> > shallow that should be checked. So the logic becomes: if origud is 
> > using shallow and its full shallow tarball does not exist yet, 
> > symlink to ensure it exists. This should make more sense.
> > 
> > Signed-off-by: Chen Qi <Qi.Chen@windriver.com>
> > ---
> >  lib/bb/fetch2/git.py | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py index 
> > 709c692a8..f288afb51 100644
> > --- a/lib/bb/fetch2/git.py
> > +++ b/lib/bb/fetch2/git.py
> > @@ -359,7 +359,7 @@ class Git(FetchMethod):
> >          super().update_mirror_links(ud, origud)
> >          # When using shallow mode, add a symlink to the original fullshallow
> >          # path to ensure a valid symlink even in the `PREMIRRORS` case
> > -        if ud.shallow and not os.path.exists(origud.fullshallow):
> > +        if origud.shallow and not os.path.exists(origud.fullshallow):
> >              self.ensure_symlink(ud.localpath, origud.fullshallow)
> >  
> >      def try_premirror(self, ud, d):
> 
> Hi Qi, thanks for your patience on this one as we work through the 
> backlog.
> 
> The only place update_mirror_links is currently called is from
> try_mirror_url() in lib/bb/fetch2/__init__.py. This is called as 
> origud.method.update_mirror_links(ud, origud), so checking 
> origud.shallow instead of ud.shallow looks correct to me. I think your 
> patch is correct, we'll take another look at it in the next review 
> call and hopefully get it merged.

We are missing some test coverage here as we've never seen the AttributeError on the autobuilder with this patch applied. There was a similiar error seen for v1 of the patch when it was submitted, but that was a different implementation modifying lib/bb/fetch2/__init__.py (the applied version was v3).

We do have a test case that looks relevant in lib/bb/tests/fetch.py:
GitShallowTest.test_shallow_mirrors.

Are you able to look in to this and extend the test coverage if needed?

Best regards,

--
Paul Barker
diff mbox series

Patch

diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
index 709c692a8..f288afb51 100644
--- a/lib/bb/fetch2/git.py
+++ b/lib/bb/fetch2/git.py
@@ -359,7 +359,7 @@  class Git(FetchMethod):
         super().update_mirror_links(ud, origud)
         # When using shallow mode, add a symlink to the original fullshallow
         # path to ensure a valid symlink even in the `PREMIRRORS` case
-        if ud.shallow and not os.path.exists(origud.fullshallow):
+        if origud.shallow and not os.path.exists(origud.fullshallow):
             self.ensure_symlink(ud.localpath, origud.fullshallow)
 
     def try_premirror(self, ud, d):