[PATCH] fetch2/git: make sure clonedir is empty when trying next fullmirror

[PATCH] fetch2/git: make sure clonedir is empty when trying next fullmirror

[Denys Dmytriyenko]

When first mirror returns an outdated fullmirror tarball, its clonedir stays
behind for successive mirror tries. Even when latest tarball gets fetched
next, it doesn't get unpacked, as clonedir already exists. Cleanup old
clonedir and unpack the new fullmirror tarball, when the old clonedir is
no good and requires update.

Signed-off-by: Denys Dmytriyenko <denis@denix.org>
---
 lib/bb/fetch2/git.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
index e3ba80a..cb62dcc 100644
--- a/lib/bb/fetch2/git.py
+++ b/lib/bb/fetch2/git.py
@@ -337,7 +337,8 @@ class Git(FetchMethod):
         if ud.shallow and os.path.exists(ud.fullshallow) and self.need_update(ud, d):
             ud.localpath = ud.fullshallow
             return
-        elif os.path.exists(ud.fullmirror) and not os.path.exists(ud.clonedir):
+        elif os.path.exists(ud.fullmirror) and (not os.path.exists(ud.clonedir) or self.clonedir_need_update(ud, d)):
+            bb.utils.remove(ud.clonedir, recurse=True)
             bb.utils.mkdirhier(ud.clonedir)
             runfetchcmd("tar -xzf %s" % ud.fullmirror, d, workdir=ud.clonedir)
 
-- 
2.7.4

Re: [bitbake-devel] [PATCH] fetch2/git: make sure clonedir is empty when trying next fullmirror

[Christopher Larson]

[-- Attachment #1: Type: text/plain, Size: 1565 bytes --]

Nicely spotted. Do we have a test for this case?

On Thu, Mar 18, 2021 at 1:40 PM Denys Dmytriyenko <denis@denix.org> wrote:

> When first mirror returns an outdated fullmirror tarball, its clonedir
> stays
> behind for successive mirror tries. Even when latest tarball gets fetched
> next, it doesn't get unpacked, as clonedir already exists. Cleanup old
> clonedir and unpack the new fullmirror tarball, when the old clonedir is
> no good and requires update.
>
> Signed-off-by: Denys Dmytriyenko <denis@denix.org>
> ---
>  lib/bb/fetch2/git.py | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
> index e3ba80a..cb62dcc 100644
> --- a/lib/bb/fetch2/git.py
> +++ b/lib/bb/fetch2/git.py
> @@ -337,7 +337,8 @@ class Git(FetchMethod):
>          if ud.shallow and os.path.exists(ud.fullshallow) and
> self.need_update(ud, d):
>              ud.localpath = ud.fullshallow
>              return
> -        elif os.path.exists(ud.fullmirror) and not
> os.path.exists(ud.clonedir):
> +        elif os.path.exists(ud.fullmirror) and (not
> os.path.exists(ud.clonedir) or self.clonedir_need_update(ud, d)):
> +            bb.utils.remove(ud.clonedir, recurse=True)
>              bb.utils.mkdirhier(ud.clonedir)
>              runfetchcmd("tar -xzf %s" % ud.fullmirror, d,
> workdir=ud.clonedir)
>
> --
> 2.7.4
>
>
> 
>
>

-- 
Christopher Larson
kergoth at gmail dot com
Founder - BitBake, OpenEmbedded, OpenZaurus
Senior Software Engineer, Mentor Graphics

[-- Attachment #2: Type: text/html, Size: 2142 bytes --]

Re: [bitbake-devel] [PATCH] fetch2/git: make sure clonedir is empty when trying next fullmirror

[Richard Purdie]

On Thu, 2021-03-18 at 16:39 -0400, Denys Dmytriyenko wrote:
> When first mirror returns an outdated fullmirror tarball, its clonedir stays
> behind for successive mirror tries. Even when latest tarball gets fetched
> next, it doesn't get unpacked, as clonedir already exists. Cleanup old
> clonedir and unpack the new fullmirror tarball, when the old clonedir is
> no good and requires update.
> 
> Signed-off-by: Denys Dmytriyenko <denis@denix.org>
> ---
>  lib/bb/fetch2/git.py | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
> index e3ba80a..cb62dcc 100644
> --- a/lib/bb/fetch2/git.py
> +++ b/lib/bb/fetch2/git.py
> @@ -337,7 +337,8 @@ class Git(FetchMethod):
>          if ud.shallow and os.path.exists(ud.fullshallow) and self.need_update(ud, d):
>              ud.localpath = ud.fullshallow
>              return
> -        elif os.path.exists(ud.fullmirror) and not os.path.exists(ud.clonedir):
> +        elif os.path.exists(ud.fullmirror) and (not os.path.exists(ud.clonedir) or self.clonedir_need_update(ud, d)):
> +            bb.utils.remove(ud.clonedir, recurse=True)
>              bb.utils.mkdirhier(ud.clonedir)
>              runfetchcmd("tar -xzf %s" % ud.fullmirror, d, workdir=ud.clonedir)

I talked a little with Denys about this. I'm worried that if you specify 
SRCREV incorrectly it will start blowing away clone directories and
for large ones like linux-yocto, the failure mode is *really* annoying 
and has the potential for data loss (you shouldn't be putting important data
here, but...).

I agree that we do need a test case. We can put artefacts on yp.org urls
if needed.

I also wonder if we can't check the new mirror tarball is any better before
we blow away the existing directory.

This isn't at all straightforward and I suspect we've been here before.

Cheers,

Richard

Re: [bitbake-devel] [PATCH] fetch2/git: make sure clonedir is empty when trying next fullmirror

[Christopher Larson]

[-- Attachment #1: Type: text/plain, Size: 2573 bytes --]

On Thu, Mar 18, 2021 at 3:23 PM Richard Purdie <
richard.purdie@linuxfoundation.org> wrote:

> On Thu, 2021-03-18 at 16:39 -0400, Denys Dmytriyenko wrote:
> > When first mirror returns an outdated fullmirror tarball, its clonedir
> stays
> > behind for successive mirror tries. Even when latest tarball gets fetched
> > next, it doesn't get unpacked, as clonedir already exists. Cleanup old
> > clonedir and unpack the new fullmirror tarball, when the old clonedir is
> > no good and requires update.
> >
> > Signed-off-by: Denys Dmytriyenko <denis@denix.org>
> > ---
> >  lib/bb/fetch2/git.py | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
> > index e3ba80a..cb62dcc 100644
> > --- a/lib/bb/fetch2/git.py
> > +++ b/lib/bb/fetch2/git.py
> > @@ -337,7 +337,8 @@ class Git(FetchMethod):
> >          if ud.shallow and os.path.exists(ud.fullshallow) and
> self.need_update(ud, d):
> >              ud.localpath = ud.fullshallow
> >              return
> > -        elif os.path.exists(ud.fullmirror) and not
> os.path.exists(ud.clonedir):
> > +        elif os.path.exists(ud.fullmirror) and (not
> os.path.exists(ud.clonedir) or self.clonedir_need_update(ud, d)):
> > +            bb.utils.remove(ud.clonedir, recurse=True)
> >              bb.utils.mkdirhier(ud.clonedir)
> >              runfetchcmd("tar -xzf %s" % ud.fullmirror, d,
> workdir=ud.clonedir)
>
> I talked a little with Denys about this. I'm worried that if you specify
> SRCREV incorrectly it will start blowing away clone directories and
> for large ones like linux-yocto, the failure mode is *really* annoying
> and has the potential for data loss (you shouldn't be putting important
> data
> here, but...).
>
> I agree that we do need a test case. We can put artefacts on yp.org urls
> if needed.
>
> I also wonder if we can't check the new mirror tarball is any better before
> we blow away the existing directory.
>

I'm assuming this is for the case where the clonedir can't be updated
directly, ie premirrorsonly?

That's a good idea, unpack the new one to a temporary location, see if that
one is good enough first. It makes me wonder if there's a way to use git
bundles or something that can be interacted with more directly. I'm
assuming this is for the case where the clonedir can't be directly updated,
i.e. premirrorsonly?
-- 
Christopher Larson
kergoth at gmail dot com
Founder - BitBake, OpenEmbedded, OpenZaurus
Senior Software Engineer, Mentor Graphics

[-- Attachment #2: Type: text/html, Size: 3516 bytes --]

Re: [bitbake-devel] [PATCH] fetch2/git: make sure clonedir is empty when trying next fullmirror

[Richard Purdie]

On Thu, 2021-03-18 at 15:28 -0700, Christopher Larson wrote:
> 
> 
> On Thu, Mar 18, 2021 at 3:23 PM Richard Purdie <richard.purdie@linuxfoundation.org> wrote:
> > On Thu, 2021-03-18 at 16:39 -0400, Denys Dmytriyenko wrote:
> > > When first mirror returns an outdated fullmirror tarball, its clonedir stays
> > > behind for successive mirror tries. Even when latest tarball gets fetched
> > > next, it doesn't get unpacked, as clonedir already exists. Cleanup old
> > > clonedir and unpack the new fullmirror tarball, when the old clonedir is
> > > no good and requires update.
> > > 
> > > Signed-off-by: Denys Dmytriyenko <denis@denix.org>
> > > ---
> > >  lib/bb/fetch2/git.py | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
> > > index e3ba80a..cb62dcc 100644
> > > --- a/lib/bb/fetch2/git.py
> > > +++ b/lib/bb/fetch2/git.py
> > > @@ -337,7 +337,8 @@ class Git(FetchMethod):
> > >          if ud.shallow and os.path.exists(ud.fullshallow) and self.need_update(ud, d):
> > >              ud.localpath = ud.fullshallow
> > >              return
> > > -        elif os.path.exists(ud.fullmirror) and not os.path.exists(ud.clonedir):
> > > +        elif os.path.exists(ud.fullmirror) and (not os.path.exists(ud.clonedir) or
> > self.clonedir_need_update(ud, d)):
> > > +            bb.utils.remove(ud.clonedir, recurse=True)
> > >              bb.utils.mkdirhier(ud.clonedir)
> > >              runfetchcmd("tar -xzf %s" % ud.fullmirror, d, workdir=ud.clonedir)
> > 
> > I talked a little with Denys about this. I'm worried that if you specify 
> > SRCREV incorrectly it will start blowing away clone directories and
> > for large ones like linux-yocto, the failure mode is *really* annoying 
> > and has the potential for data loss (you shouldn't be putting important data
> > here, but...).
> > 
> > I agree that we do need a test case. We can put artefacts on yp.org urls
> > if needed.
> > 
> > I also wonder if we can't check the new mirror tarball is any better before
> > we blow away the existing directory.
> > 
> 
> 
> I'm assuming this is for the case where the clonedir can't be updated
> directly, ie premirrorsonly?

Effectively. If I understand Denys' case, the upstream is down, the first mirror
tarball it out of date and the second mirror tarball found is not.

> That's a good idea, unpack the new one to a temporary location, see if 
> that one is good enough first. It makes me wonder if there's a way to 
> use git bundles or something that can be interacted with more directly. 
> I'm assuming this is for the case where the clonedir can't be directly 
> updated, i.e. premirrorsonly?

Right, yes. For whatever reason the original source is unavailable and we 
have multiple conflicting mirror data sources we can't immediately inspect.

Cheers,

Richard

Re: [bitbake-devel] [PATCH] fetch2/git: make sure clonedir is empty when trying next fullmirror

[Denys Dmytriyenko]

On Thu, Mar 18, 2021 at 10:32:34PM +0000, Richard Purdie wrote:
> On Thu, 2021-03-18 at 15:28 -0700, Christopher Larson wrote:
> > 
> > 
> > On Thu, Mar 18, 2021 at 3:23 PM Richard Purdie <richard.purdie@linuxfoundation.org> wrote:
> > > On Thu, 2021-03-18 at 16:39 -0400, Denys Dmytriyenko wrote:
> > > > When first mirror returns an outdated fullmirror tarball, its clonedir stays
> > > > behind for successive mirror tries. Even when latest tarball gets fetched
> > > > next, it doesn't get unpacked, as clonedir already exists. Cleanup old
> > > > clonedir and unpack the new fullmirror tarball, when the old clonedir is
> > > > no good and requires update.
> > > > 
> > > > Signed-off-by: Denys Dmytriyenko <denis@denix.org>
> > > > ---
> > > >  lib/bb/fetch2/git.py | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
> > > > index e3ba80a..cb62dcc 100644
> > > > --- a/lib/bb/fetch2/git.py
> > > > +++ b/lib/bb/fetch2/git.py
> > > > @@ -337,7 +337,8 @@ class Git(FetchMethod):
> > > >          if ud.shallow and os.path.exists(ud.fullshallow) and self.need_update(ud, d):
> > > >              ud.localpath = ud.fullshallow
> > > >              return
> > > > -        elif os.path.exists(ud.fullmirror) and not os.path.exists(ud.clonedir):
> > > > +        elif os.path.exists(ud.fullmirror) and (not os.path.exists(ud.clonedir) or
> > > self.clonedir_need_update(ud, d)):
> > > > +            bb.utils.remove(ud.clonedir, recurse=True)
> > > >              bb.utils.mkdirhier(ud.clonedir)
> > > >              runfetchcmd("tar -xzf %s" % ud.fullmirror, d, workdir=ud.clonedir)
> > > 
> > > I talked a little with Denys about this. I'm worried that if you specify 
> > > SRCREV incorrectly it will start blowing away clone directories and
> > > for large ones like linux-yocto, the failure mode is *really* annoying 
> > > and has the potential for data loss (you shouldn't be putting important data
> > > here, but...).
> > > 
> > > I agree that we do need a test case. We can put artefacts on yp.org urls
> > > if needed.
> > > 
> > > I also wonder if we can't check the new mirror tarball is any better before
> > > we blow away the existing directory.
> > > 
> > 
> > 
> > I'm assuming this is for the case where the clonedir can't be updated
> > directly, ie premirrorsonly?
> 
> Effectively. If I understand Denys' case, the upstream is down, the first mirror
> tarball it out of date and the second mirror tarball found is not.

Yes, that was exactly my case.


> > That's a good idea, unpack the new one to a temporary location, see if 
> > that one is good enough first. It makes me wonder if there's a way to 
> > use git bundles or something that can be interacted with more directly. 
> > I'm assuming this is for the case where the clonedir can't be directly 
> > updated, i.e. premirrorsonly?
> 
> Right, yes. For whatever reason the original source is unavailable and we 
> have multiple conflicting mirror data sources we can't immediately inspect.

Shouldn't need_update() be sufficient indication that clonedir doesn't have 
the required SRCREV we are looking for?

I believe you also mentioned cases of locally-modified clonedir that we don't 
want to blow away here. Is there an execution path that would come here to 
do_fetch() with fullmirror? E.g. you got it previously fetched and unpacked 
successfully, modified the recipe with new (potentially bad) SRCREV, and now 
bitbake while trying to fetch that, ends up blowing away good clonedir and 
replace it with potentially outdated mirror clone, correct?

So, how about we always untar fullmirror into a new clonedir2, then run 
need_update() on it and if False, replace clonedir with clonedir2, otherwise 
wipe out bad clonedir2. Would that work?

Are there any other checks besides need_update() that determine if clonedir 
is good or not?

-- 
Denys