[bitbake-devel] [PATCH 2/2] fetch2: Unify BB_FETCH_PREMIRRORONLY

[richard.purdie at linuxfoundation.org]

On Fri, 2019-03-22 at 11:47 +0800, Robert Yang wrote:
> Hi RP,
> 
> On 3/22/19 7:35 AM, Richard Purdie wrote:
> > On Wed, 2019-03-20 at 14:40 +0800, Robert Yang wrote:
> > > The fetch2/__init__.py checks whether "BB_FETCH_PREMIRRORONLY" ==
> > > "1", but
> > > fetch2/git.py and hg.py checks whether it is None, this makes it
> > > discontinuous,
> > > and BB_FETCH_PREMIRRORONLY = "0" doens't work as expected in the
> > > later case,
> > > so unify it to the previous one. (As BB_NO_NETWORK does).
> > > 
> > > Signed-off-by: Robert Yang <liezhi.yang at windriver.com>
> > > ---
> > >   bitbake/lib/bb/fetch2/git.py | 2 +-
> > >   bitbake/lib/bb/fetch2/hg.py  | 2 +-
> > >   2 files changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/bitbake/lib/bb/fetch2/git.py
> > > b/bitbake/lib/bb/fetch2/git.py
> > > index 1a8ebe3..e021f33 100644
> > > --- a/bitbake/lib/bb/fetch2/git.py
> > > +++ b/bitbake/lib/bb/fetch2/git.py
> > > @@ -318,7 +318,7 @@ class Git(FetchMethod):
> > >       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") is not None:
> > > +        if d.getVar("BB_FETCH_PREMIRRORONLY") == "1":
> > >               return True
> > >           if os.path.exists(ud.clonedir):
> > >               return False
> > > diff --git a/bitbake/lib/bb/fetch2/hg.py
> > > b/bitbake/lib/bb/fetch2/hg.py
> > > index 936d043..5a3db92 100644
> > > --- a/bitbake/lib/bb/fetch2/hg.py
> > > +++ b/bitbake/lib/bb/fetch2/hg.py
> > > @@ -99,7 +99,7 @@ class Hg(FetchMethod):
> > >       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") is not None:
> > > +        if d.getVar("BB_FETCH_PREMIRRORONLY") == "1":
> > >               return True
> > >           if os.path.exists(ud.moddir):
> > >               return False
> > 
> > Can we use bb.utils.to_boolean() in all cases to be consistent
> > please?
> 
> Thanks, I will update it.
> 
> I have another question about try_premirror(), are there any side
> effects
> if we make try_premirror() always return true? I mean that:
> 
> def try_premirror(self, ud, d):
>      return True
> 
> I've met a problem recently, when rework in an old build, e.g,
> suppose
> there is a foo_git.bb:
> 
> 1) Add foo.git to PREMIRROR
> 
> 2) In builddir
> $ bitbake foo
> Then foo.git will be fetched into DL_DIR, which is premirror_foo.git.
> 
> 3) Upgrade foo_git.bb's SRCREV, and this SRCREV isn't in
> premirror_foo.git since
>     it's very new.
> 
> 4) Upgrade foo.git in PREMIRROR, now it contains the SRCREV of step 3
> 
> 5) Go to the old builddir
> $ bitbake foo
> 
> Then it won't update from PREMIRROR if BB_FETCH_PREMIRRORONLY is not
> set,
> and it would fail to build if BB_NO_NETWORK = "1" since SRCREV can't
> be found
> in DL_DIR. Now the problem is:
> 
> - Set BB_FETCH_PREMIRRORONLY = "0" and BB_NO_NETWORK = "1" will make
> foo fail to build.

That is expected to fail as the user has said specifically not to use
premirrors.

> - Set BB_FETCH_PREMIRRORONLY = "1" will make it work for foo_git.bb,
> but it
>    may not work for another recipe such as foo_X_git.bb even if
>    BB_NO_NETWORK = "0".(Imagine the PREMIRROR doesn't contain
> foo_X_git.bb's
>    SRCREV)

Here, the user has said to use premirrors *only* so that is what the
code should do.

> - Set BB_FETCH_PREMIRRORONLY = "0" and BB_NO_NETWORK = "0" will make
> foo work,
>    but it won't upgrade from PREMIRROR, though everything is up-to-
> date there.
>    It will update DL_DIR from SRC_URI, this isn't good when network
> is slow.

The behaviour of BB_FETCH_PREMIRRORONLY = "0" will change after your
patch, it will make it behave as if BB_FETCH_PREMIRRORONLY was unset so
I think your patch will fix this case?

> Things will be much easier if make try_premirror() always return
> True.

This gets a little complex, for example if premirrors is a tarball
mirror of a git repo rather than a full repo.

If you change this to always return True, it would always download the
mirror tarball for a git repo rather than trying to do a git pull in
the main repo which would just download the delta.

The correct behaviour would probably be to work out whether we can do a
"git fetch" update from any source, then go back to pulling mirror
tarballs if we can't get the revision by the other means.

We should probably file an enhancement bug for that and put a comment
about this in the code as this is a key behaviour we rely upon. I don't
want to change this part of the fetcher at this point in the release
cycle though as this kind of change is fraught with risk :(.

Cheers,

Richard