[PATCH 0/2] fetch2: Unify BB_FETCH_PREMIRRORONLY

[PATCH 0/2] fetch2: Unify BB_FETCH_PREMIRRORONLY

[Robert Yang]

The following changes since commit 753e2a0ede4449917c75353b57f13bbafe70fac8:

  build-appliance-image: Update to master head revision (2019-03-19 23:56:29 +0000)

are available in the git repository at:

  git://git.pokylinux.org/poky-contrib rbt/premirror
  http://git.pokylinux.org/cgit.cgi//log/?h=rbt/premirror

Robert Yang (2):
  fetch2: runfetchcmd(): Print workdir in debug message
  fetch2: Unify BB_FETCH_PREMIRRORONLY

 bitbake/lib/bb/fetch2/__init__.py | 5 ++++-
 bitbake/lib/bb/fetch2/git.py      | 2 +-
 bitbake/lib/bb/fetch2/hg.py       | 2 +-
 3 files changed, 6 insertions(+), 3 deletions(-)

-- 
2.7.4

[PATCH 1/2] fetch2: runfetchcmd(): Print workdir in debug message

[Robert Yang]

The workdir is an important message when run git command, for exmample, before the patch:
Running git -c core.fsyncobjectfiles=0 branch --contains [snip]

We don't know where it is running, now it is:
Running 'git -c core.fsyncobjectfiles=0 branch --contains [snip]' in /path/to/git.opensvc.com.multipath-tools..git

Which is easier to debug.

Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
---
 bitbake/lib/bb/fetch2/__init__.py | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/bitbake/lib/bb/fetch2/__init__.py b/bitbake/lib/bb/fetch2/__init__.py
index 5c2a1c7..f5a5928 100644
--- a/bitbake/lib/bb/fetch2/__init__.py
+++ b/bitbake/lib/bb/fetch2/__init__.py
@@ -858,7 +858,10 @@ def runfetchcmd(cmd, d, quiet=False, cleanup=None, log=None, workdir=None):
     # Disable pseudo as it may affect ssh, potentially causing it to hang.
     cmd = 'export PSEUDO_DISABLED=1; ' + cmd
 
-    logger.debug(1, "Running %s", cmd)
+    if workdir:
+        logger.debug(1, "Running '%s' in %s" % (cmd, workdir))
+    else:
+        logger.debug(1, "Running %s", cmd)
 
     success = False
     error_message = ""
-- 
2.7.4

[PATCH 2/2] fetch2: Unify BB_FETCH_PREMIRRORONLY

[Robert Yang]

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@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
-- 
2.7.4

Re: [PATCH 2/2] fetch2: Unify BB_FETCH_PREMIRRORONLY

[Richard Purdie]

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@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?

Cheers,

Richard

Re: [PATCH 2/2] fetch2: Unify BB_FETCH_PREMIRRORONLY

[Robert Yang]

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@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.

- 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)

- 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.

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

// Robert

> 
> Cheers,
> 
> Richard
> 
> 
> 

Re: [PATCH 2/2] fetch2: Unify BB_FETCH_PREMIRRORONLY

[richard.purdie]

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@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

Re: [PATCH 2/2] fetch2: Unify BB_FETCH_PREMIRRORONLY

[Robert Yang]

Hi RP,

On 3/22/19 7:18 PM, richard.purdie@linuxfoundation.org wrote:
> 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@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?

Unfortunately, no, my patch doesn't fix this problem. My patches fixes
BB_FETCH_PREMIRRORONLY's inconsistent behavior between fetch2/__init__.py
and fetch2/git.py or hg.py.

> 
>> 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

Sound good, I filed a enhancement bug for 2.8 M1:

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

Bug 13233 - fetch2: try_premirror(): improve on updating repo from mirror


I will send a V2 to use bb.utils.to_boolean() atm.

// Robert

> 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
> 
> 
> 
>