[PATCH] fetch2/git: always use premirror first if update is required

[PATCH] fetch2/git: always use premirror first if update is required

[Pascal Bach]

When an update of the local copy is required always try to fetch the update
from a premirror first.
The premirror should always have precedence over the upstream repository
as updating from there might not work if BB_FETCH_PREMIRRORONLY,
BB_NO_NBB_ALLOWED_NETWORKS or BB_NO_NETWORK is set.

This also gets rid of the special case for BB_FETCH_PREMIRRORONLY.

Signed-off-by: Pascal Bach <pascal.bach@siemens.com>
---
 lib/bb/fetch2/git.py | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
index 1bec60a..cac2a87 100644
--- a/lib/bb/fetch2/git.py
+++ b/lib/bb/fetch2/git.py
@@ -227,13 +227,8 @@ class Git(FetchMethod):
         return False
 
     def try_premirror(self, ud, d):
-        # If we don't do this, updating an existing checkout with only premirrors
-        # is not possible
-        if d.getVar("BB_FETCH_PREMIRRORONLY", True) is not None:
-            return True
-        if os.path.exists(ud.clonedir):
-            return False
-        return True
+        # Always try premirror if an update is required
+        return self.need_update(ud, d)
 
     def download(self, ud, d):
         """Fetch url"""
-- 
2.1.4

Re: [PATCH] fetch2/git: always use premirror first if update is required

[Christopher Larson]

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

On Fri, Sep 16, 2016 at 1:58 AM, Pascal Bach <pascal.bach@siemens.com>
wrote:

> When an update of the local copy is required always try to fetch the update
> from a premirror first.
> The premirror should always have precedence over the upstream repository
> as updating from there might not work if BB_FETCH_PREMIRRORONLY,
> BB_NO_NBB_ALLOWED_NETWORKS or BB_NO_NETWORK is set.
>
> This also gets rid of the special case for BB_FETCH_PREMIRRORONLY.
>
> Signed-off-by: Pascal Bach <pascal.bach@siemens.com>
> ---
>  lib/bb/fetch2/git.py | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
> index 1bec60a..cac2a87 100644
> --- a/lib/bb/fetch2/git.py
> +++ b/lib/bb/fetch2/git.py
> @@ -227,13 +227,8 @@ class Git(FetchMethod):
>          return False
>
>      def try_premirror(self, ud, d):
> -        # If we don't do this, updating an existing checkout with only
> premirrors
> -        # is not possible
> -        if d.getVar("BB_FETCH_PREMIRRORONLY", True) is not None:
> -            return True
> -        if os.path.exists(ud.clonedir):
> -            return False
> -        return True
> +        # Always try premirror if an update is required
> +        return self.need_update(ud, d)
>

Will this result in fetching a mirror tarball even if you already have a
clone in ud.clonedir? The previous logic always returned False if the
clonedir existed..
-- 
Christopher Larson
clarson at kergoth dot com
Founder - BitBake, OpenEmbedded, OpenZaurus
Maintainer - Tslib
Senior Software Engineer, Mentor Graphics

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

Re: [PATCH] fetch2/git: always use premirror first if update is required

[Pascal Bach]

On 16.09.2016 18:20, Christopher Larson wrote:

>
> On Fri, Sep 16, 2016 at 1:58 AM, Pascal Bach <pascal.bach@siemens.com <mailto:pascal.bach@siemens.com>> wrote:
>
>     When an update of the local copy is required always try to fetch the update
>     from a premirror first.
>     The premirror should always have precedence over the upstream repository
>     as updating from there might not work if BB_FETCH_PREMIRRORONLY,
>     BB_NO_NBB_ALLOWED_NETWORKS or BB_NO_NETWORK is set.
>
>     This also gets rid of the special case for BB_FETCH_PREMIRRORONLY.
>
>     Signed-off-by: Pascal Bach <pascal.bach@siemens.com <mailto:pascal.bach@siemens.com>>
>     ---
>      lib/bb/fetch2/git.py | 9 ++-------
>      1 file changed, 2 insertions(+), 7 deletions(-)
>
>     diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
>     index 1bec60a..cac2a87 100644
>     --- a/lib/bb/fetch2/git.py
>     +++ b/lib/bb/fetch2/git.py
>     @@ -227,13 +227,8 @@ class Git(FetchMethod):
>              return False
>
>          def try_premirror(self, ud, d):
>     -        # If we don't do this, updating an existing checkout with only premirrors
>     -        # is not possible
>     -        if d.getVar("BB_FETCH_PREMIRRORONLY", True) is not None:
>     -            return True
>     -        if os.path.exists(ud.clonedir):
>     -            return False
>     -        return True
>     +        # Always try premirror if an update is required
>     +        return self.need_update(ud, d)
>
>
> Will this result in fetching a mirror tarball even if you already have a clone in ud.clonedir? The previous logic always returned False if the clonedir existed..
The tarball should only be fetched if the local ud.clonedir is out of date, this means the revision that is requested via SRCREV is not in the local repository.
In the case the revision is present the tarball should not be fetched again, at least that was the intention and my local tests didn't indicate otherwise.

Without this change the git fetcher would directly fall back to do a git fetch from the original source, which doesn't work if the build machine doesn't have access to the internet.

Re: [PATCH] fetch2/git: always use premirror first if update is required

[Christopher Larson]

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

On Mon, Sep 19, 2016 at 12:22 AM, Pascal Bach <pascal.bach@siemens.com>
wrote:

> On 16.09.2016 18:20, Christopher Larson wrote:
>
> >
> > On Fri, Sep 16, 2016 at 1:58 AM, Pascal Bach <pascal.bach@siemens.com
> <mailto:pascal.bach@siemens.com>> wrote:
> >
> >     When an update of the local copy is required always try to fetch the
> update
> >     from a premirror first.
> >     The premirror should always have precedence over the upstream
> repository
> >     as updating from there might not work if BB_FETCH_PREMIRRORONLY,
> >     BB_NO_NBB_ALLOWED_NETWORKS or BB_NO_NETWORK is set.
> >
> >     This also gets rid of the special case for BB_FETCH_PREMIRRORONLY.
> >
> >     Signed-off-by: Pascal Bach <pascal.bach@siemens.com <mailto:
> pascal.bach@siemens.com>>
> >     ---
> >      lib/bb/fetch2/git.py | 9 ++-------
> >      1 file changed, 2 insertions(+), 7 deletions(-)
> >
> >     diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
> >     index 1bec60a..cac2a87 100644
> >     --- a/lib/bb/fetch2/git.py
> >     +++ b/lib/bb/fetch2/git.py
> >     @@ -227,13 +227,8 @@ class Git(FetchMethod):
> >              return False
> >
> >          def try_premirror(self, ud, d):
> >     -        # If we don't do this, updating an existing checkout with
> only premirrors
> >     -        # is not possible
> >     -        if d.getVar("BB_FETCH_PREMIRRORONLY", True) is not None:
> >     -            return True
> >     -        if os.path.exists(ud.clonedir):
> >     -            return False
> >     -        return True
> >     +        # Always try premirror if an update is required
> >     +        return self.need_update(ud, d)
> >
> >
> > Will this result in fetching a mirror tarball even if you already have a
> clone in ud.clonedir? The previous logic always returned False if the
> clonedir existed..
> The tarball should only be fetched if the local ud.clonedir is out of
> date, this means the revision that is requested via SRCREV is not in the
> local repository.
> In the case the revision is present the tarball should not be fetched
> again, at least that was the intention and my local tests didn't indicate
> otherwise.
>

Yes, but it’s better to run ‘git fetch’ in an out of date clone than to
re-download a 1gb mirror tarball which may well be out of date too.
-- 
Christopher Larson
clarson at kergoth dot com
Founder - BitBake, OpenEmbedded, OpenZaurus
Maintainer - Tslib
Senior Software Engineer, Mentor Graphics

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

Re: [PATCH] fetch2/git: always use premirror first if update is required

[Pascal Bach]

On 19.09.2016 17:10, Christopher Larson wrote:
>
> On Mon, Sep 19, 2016 at 12:22 AM, Pascal Bach <pascal.bach@siemens.com <mailto:pascal.bach@siemens.com>> wrote:
>
>     On 16.09.2016 18:20, Christopher Larson wrote:
>
>     >
>     > On Fri, Sep 16, 2016 at 1:58 AM, Pascal Bach <pascal.bach@siemens.com <mailto:pascal.bach@siemens.com> <mailto:pascal.bach@siemens.com <mailto:pascal.bach@siemens.com>>> wrote:
>     >
>     >     When an update of the local copy is required always try to fetch the update
>     >     from a premirror first.
>     >     The premirror should always have precedence over the upstream repository
>     >     as updating from there might not work if BB_FETCH_PREMIRRORONLY,
>     >     BB_NO_NBB_ALLOWED_NETWORKS or BB_NO_NETWORK is set.
>     >
>     >     This also gets rid of the special case for BB_FETCH_PREMIRRORONLY.
>     >
>     >     Signed-off-by: Pascal Bach <pascal.bach@siemens.com <mailto:pascal.bach@siemens.com> <mailto:pascal.bach@siemens.com <mailto:pascal.bach@siemens.com>>>
>     >     ---
>     >      lib/bb/fetch2/git.py | 9 ++-------
>     >      1 file changed, 2 insertions(+), 7 deletions(-)
>     >
>     >     diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
>     >     index 1bec60a..cac2a87 100644
>     >     --- a/lib/bb/fetch2/git.py
>     >     +++ b/lib/bb/fetch2/git.py
>     >     @@ -227,13 +227,8 @@ class Git(FetchMethod):
>     >              return False
>     >
>     >          def try_premirror(self, ud, d):
>     >     -        # If we don't do this, updating an existing checkout with only premirrors
>     >     -        # is not possible
>     >     -        if d.getVar("BB_FETCH_PREMIRRORONLY", True) is not None:
>     >     -            return True
>     >     -        if os.path.exists(ud.clonedir):
>     >     -            return False
>     >     -        return True
>     >     +        # Always try premirror if an update is required
>     >     +        return self.need_update(ud, d)
>     >
>     >
>     > Will this result in fetching a mirror tarball even if you already have a clone in ud.clonedir? The previous logic always returned False if the clonedir existed..
>     The tarball should only be fetched if the local ud.clonedir is out of date, this means the revision that is requested via SRCREV is not in the local repository.
>     In the case the revision is present the tarball should not be fetched again, at least that was the intention and my local tests didn't indicate otherwise.
>
>
> Yes, but it’s better to run ‘git fetch’ in an out of date clone than to re-download a 1gb mirror tarball which may well be out of date too.
Agreed, but this doesn't work in the case where the machine doesn't have access to the upstream git repository. For example in the case where BB_ALLOWED_NETWORKS = "*.my.company". The fetcher would still got to to github.com which is not allowed, in that case it should fall back to fetch the tarball from "mirror.my.company".

I'm open for suggestions how to make this more efficient.

Pascal

Re: [PATCH] fetch2/git: always use premirror first if update is required

[Pascal Bach]

>>     The tarball should only be fetched if the local ud.clonedir is out of date, this means the revision that is requested via SRCREV is not in the local repository.
>>     In the case the revision is present the tarball should not be fetched again, at least that was the intention and my local tests didn't indicate otherwise.
>>
>>
>> Yes, but it’s better to run ‘git fetch’ in an out of date clone than to re-download a 1gb mirror tarball which may well be out of date too.
> Agreed, but this doesn't work in the case where the machine doesn't have access to the upstream git repository. For example in the case where BB_ALLOWED_NETWORKS = "*.my.company". The fetcher would still got to to github.com which is not allowed, in that case it should fall back to fetch the tarball from "mirror.my.company".
>
> I'm open for suggestions how to make this more efficient.
Does anyone have a suggestion how this should behave in the ideal case? In my opinion if there is an internal mirror defined it should always have precedence even if it might be less efficient.

Pascal

Re: [PATCH] fetch2/git: always use premirror first if update is required

[Richard Purdie]

On Thu, 2016-09-22 at 09:45 +0200, Pascal Bach wrote:
> > 
> > > 
> > >     The tarball should only be fetched if the local ud.clonedir
> > > is out of date, this means the revision that is requested via
> > > SRCREV is not in the local repository.
> > >     In the case the revision is present the tarball should not be
> > > fetched again, at least that was the intention and my local tests
> > > didn't indicate otherwise.
> > > 
> > > 
> > > Yes, but it’s better to run ‘git fetch’ in an out of date clone
> > > than to re-download a 1gb mirror tarball which may well be out of
> > > date too.
> > Agreed, but this doesn't work in the case where the machine doesn't
> > have access to the upstream git repository. For example in the case
> > where BB_ALLOWED_NETWORKS = "*.my.company". The fetcher would still
> > got to to github.com which is not allowed, in that case it should
> > fall back to fetch the tarball from "mirror.my.company".
> > 
> > I'm open for suggestions how to make this more efficient.
> Does anyone have a suggestion how this should behave in the ideal
> case? In my opinion if there is an internal mirror defined it should
> always have precedence even if it might be less efficient.

"less efficient" in this case translates a few seconds of git fetch
operation on something like linux-yocto into a hundreds of megabytes
download.

Much as you might not like it, I really don't think we can change the
code as your patch does as it would badly affect standard usage for
many users.

We need to find an alternative which allows your use case to work but
I'm not sure what that is. Much as I hate it, we may just have to have
a setting which you can set which changes the behaviour. If we do that,
I will want to see test cases added for it though as the fetcher code
is a mass of different configurations and its very very difficult to
maintain as it is without yet more different code paths :(.

Cheers,

Richard

Re: [PATCH] fetch2/git: always use premirror first if update is required

[Richard Purdie]

On Thu, 2016-09-22 at 11:17 +0100, Richard Purdie wrote:
> On Thu, 2016-09-22 at 09:45 +0200, Pascal Bach wrote:
> > 
> > > 
> > > 
> > > > 
> > > > 
> > > >     The tarball should only be fetched if the local ud.clonedir
> > > > is out of date, this means the revision that is requested via
> > > > SRCREV is not in the local repository.
> > > >     In the case the revision is present the tarball should not
> > > > be
> > > > fetched again, at least that was the intention and my local
> > > > tests
> > > > didn't indicate otherwise.
> > > > 
> > > > 
> > > > Yes, but it’s better to run ‘git fetch’ in an out of date clone
> > > > than to re-download a 1gb mirror tarball which may well be out
> > > > of
> > > > date too.
> > > Agreed, but this doesn't work in the case where the machine
> > > doesn't
> > > have access to the upstream git repository. For example in the
> > > case
> > > where BB_ALLOWED_NETWORKS = "*.my.company". The fetcher would
> > > still
> > > got to to github.com which is not allowed, in that case it should
> > > fall back to fetch the tarball from "mirror.my.company".
> > > 
> > > I'm open for suggestions how to make this more efficient.
> > Does anyone have a suggestion how this should behave in the ideal
> > case? In my opinion if there is an internal mirror defined it
> > should
> > always have precedence even if it might be less efficient.
> "less efficient" in this case translates a few seconds of git fetch
> operation on something like linux-yocto into a hundreds of megabytes
> download.
> 
> Much as you might not like it, I really don't think we can change the
> code as your patch does as it would badly affect standard usage for
> many users.
> 
> We need to find an alternative which allows your use case to work but
> I'm not sure what that is. Much as I hate it, we may just have to
> have
> a setting which you can set which changes the behaviour. If we do
> that,
> I will want to see test cases added for it though as the fetcher code
> is a mass of different configurations and its very very difficult to
> maintain as it is without yet more different code paths :(.


FWIW there is an open bug which I believe is related to this:

https://bugzilla.yoctoproject.org/show_bug.cgi?id=9061

It might be worth making the code aware of git aware premirror sources
compared to other things like mirror tarballs as a start at fixing
this.

Cheers,

Richard