| Message ID | 20220525144243.19999-1-pavel@zhukoff.net |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] Honour BB_FETCH_PREMIRRORONLY option | expand |
On Wed, 2022-05-25 at 16:42 +0200, Pavel Zhukov wrote: > This should fix [Yocto 13353] and related to [Yocto 13233] as well. > Previously if git repo mirror has been updated in between of two builds > fetcher of the second build didn't try updated mirror but switched to > git clone from upstream instead. This is problem for offline builds. > Fix this to raise MirrorException if BB_FETCH_PREMIRRORONLY has been > specified by the mirror doesn't contain SRC_REV. > > Signed-off-by: Pavel Zhukov <pavel.zhukov@huawei.com> > --- > lib/bb/fetch2/__init__.py | 2 ++ > lib/bb/fetch2/git.py | 16 ++++++++++++---- > 2 files changed, 14 insertions(+), 4 deletions(-) > > diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py > index ac557176..c99cd104 100644 > --- a/lib/bb/fetch2/__init__.py > +++ b/lib/bb/fetch2/__init__.py > @@ -1745,6 +1745,8 @@ class Fetch(object): > done = False > > if premirroronly: > + if not done: > + raise FetchError("Failed to download premirror {} and BB_FETCH_PREMIRRORONLY has been set to 1.".format(self.d.getVar("PREMIRRORS"))) > self.d.setVar("BB_NO_NETWORK", "1") > > firsterr = None > diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py > index 23f8c0da..bf7c9d26 100644 > --- a/lib/bb/fetch2/git.py > +++ b/lib/bb/fetch2/git.py > @@ -353,10 +353,18 @@ 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): > - bb.utils.mkdirhier(ud.clonedir) > - runfetchcmd("tar -xzf %s" % ud.fullmirror, d, workdir=ud.clonedir) > - > + elif os.path.exists(ud.fullmirror) and self.need_update(ud, d): > + if not os.path.exists(ud.clonedir): > + bb.utils.mkdirhier(ud.clonedir) > + runfetchcmd("tar -xzf %s" % ud.fullmirror, d, workdir=ud.clonedir) > + else: > + tmpdir = tempfile.mkdtemp(dir=d.getVar('DL_DIR')) > + runfetchcmd("tar -xzf %s" % ud.fullmirror, d, workdir=tmpdir) > + fetch_cmd = "LANG=C %s fetch -f --progress %s " % (ud.basecmd, shlex.quote(tmpdir)) > + runfetchcmd(fetch_cmd, d, workdir=ud.clonedir) > + if self.try_premirror(ud, d) and self.need_update(ud, d): > + raise bb.fetch2.FetchError("Premirror doesn't contain revisions {} and "\ > + "upstream cannot be used due to BB_FETCH_PREMIRRORONLY setting".format(ud.revisions)) > repourl = self._get_repo_url(ud) > > # If the repo still doesn't exist, fallback to cloning it Sorry about the delay in reviewing this, I finally ran some tests I wanted to run to check a couple of things. I think there are two changes in here and they should really be separated out. The mirror unpack and fetch piece in a temp directory is great and just what I think is needed here. The piece I worry a little more about is the raising of fetch errrors if premirrors are configured. The code currently disables the network but continues, this changes that behaviour to a hard stop. I tested with the patch below applied and things still work as expected so if you're ok with that, I'd like to merge this with these tweaks, preserving the network disabled behaviour? Cheers, Richard diff --git a/bitbake/lib/bb/fetch2/__init__.py b/bitbake/lib/bb/fetch2/__init__.py index 9ef7fd037cc..616c67e99a0 100644 --- a/bitbake/lib/bb/fetch2/__init__.py +++ b/bitbake/lib/bb/fetch2/__init__.py @@ -1745,8 +1745,6 @@ class Fetch(object): done = False if premirroronly: - if not done: - raise FetchError("Failed to download premirror {} and BB_FETCH_PREMIRRORONLY has been set to 1.".format(self.d.getVar("PREMIRRORS"))) self.d.setVar("BB_NO_NETWORK", "1") firsterr = None diff --git a/bitbake/lib/bb/fetch2/git.py b/bitbake/lib/bb/fetch2/git.py index bf7c9d26b5c..07b3d9a7ffe 100644 --- a/bitbake/lib/bb/fetch2/git.py +++ b/bitbake/lib/bb/fetch2/git.py @@ -362,9 +362,6 @@ class Git(FetchMethod): runfetchcmd("tar -xzf %s" % ud.fullmirror, d, workdir=tmpdir) fetch_cmd = "LANG=C %s fetch -f --progress %s " % (ud.basecmd, shlex.quote(tmpdir)) runfetchcmd(fetch_cmd, d, workdir=ud.clonedir) - if self.try_premirror(ud, d) and self.need_update(ud, d): - raise bb.fetch2.FetchError("Premirror doesn't contain revisions {} and "\ - "upstream cannot be used due to BB_FETCH_PREMIRRORONLY setting".format(ud.revisions)) repourl = self._get_repo_url(ud) # If the repo still doesn't exist, fallback to cloning it diff --git a/bitbake/lib/bb/tests/fetch.py b/bitbake/lib/bb/tests/fetch.py index 56f21f8e149..7ba866520ad 100644 --- a/bitbake/lib/bb/tests/fetch.py +++ b/bitbake/lib/bb/tests/fetch.py @@ -2883,5 +2883,5 @@ class FetchPremirroronlyLocalTest(FetcherTest): def test_mirror_tarball_nonexistent(self): self.d.setVar("SRCREV", "0"*40) fetcher = bb.fetch.Fetch([self.recipe_url], self.d) - with self.assertRaises(bb.fetch2.FetchError): + with self.assertRaises(bb.fetch2.NetworkAccess): fetcher.download()
Richard Purdie <richard.purdie@linuxfoundation.org> writes: > On Wed, 2022-05-25 at 16:42 +0200, Pavel Zhukov wrote: >> This should fix [Yocto 13353] and related to [Yocto 13233] as well. >> Previously if git repo mirror has been updated in between of two builds >> fetcher of the second build didn't try updated mirror but switched to >> git clone from upstream instead. This is problem for offline builds. >> Fix this to raise MirrorException if BB_FETCH_PREMIRRORONLY has been >> specified by the mirror doesn't contain SRC_REV. >> >> Signed-off-by: Pavel Zhukov <pavel.zhukov@huawei.com> >> --- >> lib/bb/fetch2/__init__.py | 2 ++ >> lib/bb/fetch2/git.py | 16 ++++++++++++---- >> 2 files changed, 14 insertions(+), 4 deletions(-) >> >> diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py >> index ac557176..c99cd104 100644 >> --- a/lib/bb/fetch2/__init__.py >> +++ b/lib/bb/fetch2/__init__.py >> @@ -1745,6 +1745,8 @@ class Fetch(object): >> done = False >> >> if premirroronly: >> + if not done: >> + raise FetchError("Failed to download premirror {} and BB_FETCH_PREMIRRORONLY has been set to 1.".format(self.d.getVar("PREMIRRORS"))) >> self.d.setVar("BB_NO_NETWORK", "1") >> >> firsterr = None >> diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py >> index 23f8c0da..bf7c9d26 100644 >> --- a/lib/bb/fetch2/git.py >> +++ b/lib/bb/fetch2/git.py >> @@ -353,10 +353,18 @@ 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): >> - bb.utils.mkdirhier(ud.clonedir) >> - runfetchcmd("tar -xzf %s" % ud.fullmirror, d, workdir=ud.clonedir) >> - >> + elif os.path.exists(ud.fullmirror) and self.need_update(ud, d): >> + if not os.path.exists(ud.clonedir): >> + bb.utils.mkdirhier(ud.clonedir) >> + runfetchcmd("tar -xzf %s" % ud.fullmirror, d, workdir=ud.clonedir) >> + else: >> + tmpdir = tempfile.mkdtemp(dir=d.getVar('DL_DIR')) >> + runfetchcmd("tar -xzf %s" % ud.fullmirror, d, workdir=tmpdir) >> + fetch_cmd = "LANG=C %s fetch -f --progress %s " % (ud.basecmd, shlex.quote(tmpdir)) >> + runfetchcmd(fetch_cmd, d, workdir=ud.clonedir) >> + if self.try_premirror(ud, d) and self.need_update(ud, d): >> + raise bb.fetch2.FetchError("Premirror doesn't contain revisions {} and "\ >> + "upstream cannot be used due to BB_FETCH_PREMIRRORONLY setting".format(ud.revisions)) >> repourl = self._get_repo_url(ud) >> >> # If the repo still doesn't exist, fallback to cloning it > > > Sorry about the delay in reviewing this, I finally ran some tests I > wanted to run to check a couple of things. I think there are two > changes in here and they should really be separated out. The mirror > unpack and fetch piece in a temp directory is great and just what I > think is needed here. > > The piece I worry a little more about is the raising of fetch errrors > if premirrors are configured. The code currently disables the network > but continues, this changes that behaviour to a hard stop. > > I tested with the patch below applied and things still work as expected > so if you're ok with that, I'd like to merge this with these tweaks, > preserving the network disabled behaviour? I'm ok with that. However in that case *ONLY* part of the BB_FETCH_PREMIRRORONLY doesn't make much sense for me because fetcher will check (clone) upstream in case if revision is not in the mirror(s). This naming inconsistence is not so important though > > Cheers, > > Richard > > diff --git a/bitbake/lib/bb/fetch2/__init__.py b/bitbake/lib/bb/fetch2/__init__.py > index 9ef7fd037cc..616c67e99a0 100644 > --- a/bitbake/lib/bb/fetch2/__init__.py > +++ b/bitbake/lib/bb/fetch2/__init__.py > @@ -1745,8 +1745,6 @@ class Fetch(object): > done = False > > if premirroronly: > - if not done: > - raise FetchError("Failed to download premirror {} and BB_FETCH_PREMIRRORONLY has been set to 1.".format(self.d.getVar("PREMIRRORS"))) > self.d.setVar("BB_NO_NETWORK", "1") > > firsterr = None > diff --git a/bitbake/lib/bb/fetch2/git.py b/bitbake/lib/bb/fetch2/git.py > index bf7c9d26b5c..07b3d9a7ffe 100644 > --- a/bitbake/lib/bb/fetch2/git.py > +++ b/bitbake/lib/bb/fetch2/git.py > @@ -362,9 +362,6 @@ class Git(FetchMethod): > runfetchcmd("tar -xzf %s" % ud.fullmirror, d, workdir=tmpdir) > fetch_cmd = "LANG=C %s fetch -f --progress %s " % (ud.basecmd, shlex.quote(tmpdir)) > runfetchcmd(fetch_cmd, d, workdir=ud.clonedir) > - if self.try_premirror(ud, d) and self.need_update(ud, d): > - raise bb.fetch2.FetchError("Premirror doesn't contain revisions {} and "\ > - "upstream cannot be used due to BB_FETCH_PREMIRRORONLY setting".format(ud.revisions)) > repourl = self._get_repo_url(ud) > > # If the repo still doesn't exist, fallback to cloning it > diff --git a/bitbake/lib/bb/tests/fetch.py b/bitbake/lib/bb/tests/fetch.py > index 56f21f8e149..7ba866520ad 100644 > --- a/bitbake/lib/bb/tests/fetch.py > +++ b/bitbake/lib/bb/tests/fetch.py > @@ -2883,5 +2883,5 @@ class FetchPremirroronlyLocalTest(FetcherTest): > def test_mirror_tarball_nonexistent(self): > self.d.setVar("SRCREV", "0"*40) > fetcher = bb.fetch.Fetch([self.recipe_url], self.d) > - with self.assertRaises(bb.fetch2.FetchError): > + with self.assertRaises(bb.fetch2.NetworkAccess): > fetcher.download()
On Tue, 2022-06-07 at 14:57 +0200, Pavel Zhukov wrote: > Richard Purdie <richard.purdie@linuxfoundation.org> writes: > > > On Wed, 2022-05-25 at 16:42 +0200, Pavel Zhukov wrote: > > > This should fix [Yocto 13353] and related to [Yocto 13233] as well. > > > Previously if git repo mirror has been updated in between of two builds > > > fetcher of the second build didn't try updated mirror but switched to > > > git clone from upstream instead. This is problem for offline builds. > > > Fix this to raise MirrorException if BB_FETCH_PREMIRRORONLY has been > > > specified by the mirror doesn't contain SRC_REV. > > > > > > Signed-off-by: Pavel Zhukov <pavel.zhukov@huawei.com> > > > --- > > > lib/bb/fetch2/__init__.py | 2 ++ > > > lib/bb/fetch2/git.py | 16 ++++++++++++---- > > > 2 files changed, 14 insertions(+), 4 deletions(-) > > > > > > diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py > > > index ac557176..c99cd104 100644 > > > --- a/lib/bb/fetch2/__init__.py > > > +++ b/lib/bb/fetch2/__init__.py > > > @@ -1745,6 +1745,8 @@ class Fetch(object): > > > done = False > > > > > > if premirroronly: > > > + if not done: > > > + raise FetchError("Failed to download premirror {} and BB_FETCH_PREMIRRORONLY has been set to 1.".format(self.d.getVar("PREMIRRORS"))) > > > self.d.setVar("BB_NO_NETWORK", "1") > > > > > > firsterr = None > > > diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py > > > index 23f8c0da..bf7c9d26 100644 > > > --- a/lib/bb/fetch2/git.py > > > +++ b/lib/bb/fetch2/git.py > > > @@ -353,10 +353,18 @@ 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): > > > - bb.utils.mkdirhier(ud.clonedir) > > > - runfetchcmd("tar -xzf %s" % ud.fullmirror, d, workdir=ud.clonedir) > > > - > > > + elif os.path.exists(ud.fullmirror) and self.need_update(ud, d): > > > + if not os.path.exists(ud.clonedir): > > > + bb.utils.mkdirhier(ud.clonedir) > > > + runfetchcmd("tar -xzf %s" % ud.fullmirror, d, workdir=ud.clonedir) > > > + else: > > > + tmpdir = tempfile.mkdtemp(dir=d.getVar('DL_DIR')) > > > + runfetchcmd("tar -xzf %s" % ud.fullmirror, d, workdir=tmpdir) > > > + fetch_cmd = "LANG=C %s fetch -f --progress %s " % (ud.basecmd, shlex.quote(tmpdir)) > > > + runfetchcmd(fetch_cmd, d, workdir=ud.clonedir) > > > + if self.try_premirror(ud, d) and self.need_update(ud, d): > > > + raise bb.fetch2.FetchError("Premirror doesn't contain revisions {} and "\ > > > + "upstream cannot be used due to BB_FETCH_PREMIRRORONLY setting".format(ud.revisions)) > > > repourl = self._get_repo_url(ud) > > > > > > # If the repo still doesn't exist, fallback to cloning it > > > > > > Sorry about the delay in reviewing this, I finally ran some tests I > > wanted to run to check a couple of things. I think there are two > > changes in here and they should really be separated out. The mirror > > unpack and fetch piece in a temp directory is great and just what I > > think is needed here. > > > > The piece I worry a little more about is the raising of fetch errrors > > if premirrors are configured. The code currently disables the network > > but continues, this changes that behaviour to a hard stop. > > > > I tested with the patch below applied and things still work as expected > > so if you're ok with that, I'd like to merge this with these tweaks, > > preserving the network disabled behaviour? > I'm ok with that. However in that case *ONLY* part of the > BB_FETCH_PREMIRRORONLY doesn't make much sense for me because fetcher > will check (clone) upstream in case if revision is not in the mirror(s). This > naming inconsistence is not so important though It could attempt it, yes, but it would fail if it needed to access the network. I'm a bit worried that somewhere there is a corner case where there is a mirror under a different name cached locally which would succeed as the code stands today but fail under the new case. I can't quite work out that case right now but there is something bugging me a bit about it :/. Cheers, Richard
diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py index ac557176..c99cd104 100644 --- a/lib/bb/fetch2/__init__.py +++ b/lib/bb/fetch2/__init__.py @@ -1745,6 +1745,8 @@ class Fetch(object): done = False if premirroronly: + if not done: + raise FetchError("Failed to download premirror {} and BB_FETCH_PREMIRRORONLY has been set to 1.".format(self.d.getVar("PREMIRRORS"))) self.d.setVar("BB_NO_NETWORK", "1") firsterr = None diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py index 23f8c0da..bf7c9d26 100644 --- a/lib/bb/fetch2/git.py +++ b/lib/bb/fetch2/git.py @@ -353,10 +353,18 @@ 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): - bb.utils.mkdirhier(ud.clonedir) - runfetchcmd("tar -xzf %s" % ud.fullmirror, d, workdir=ud.clonedir) - + elif os.path.exists(ud.fullmirror) and self.need_update(ud, d): + if not os.path.exists(ud.clonedir): + bb.utils.mkdirhier(ud.clonedir) + runfetchcmd("tar -xzf %s" % ud.fullmirror, d, workdir=ud.clonedir) + else: + tmpdir = tempfile.mkdtemp(dir=d.getVar('DL_DIR')) + runfetchcmd("tar -xzf %s" % ud.fullmirror, d, workdir=tmpdir) + fetch_cmd = "LANG=C %s fetch -f --progress %s " % (ud.basecmd, shlex.quote(tmpdir)) + runfetchcmd(fetch_cmd, d, workdir=ud.clonedir) + if self.try_premirror(ud, d) and self.need_update(ud, d): + raise bb.fetch2.FetchError("Premirror doesn't contain revisions {} and "\ + "upstream cannot be used due to BB_FETCH_PREMIRRORONLY setting".format(ud.revisions)) repourl = self._get_repo_url(ud) # If the repo still doesn't exist, fallback to cloning it
This should fix [Yocto 13353] and related to [Yocto 13233] as well. Previously if git repo mirror has been updated in between of two builds fetcher of the second build didn't try updated mirror but switched to git clone from upstream instead. This is problem for offline builds. Fix this to raise MirrorException if BB_FETCH_PREMIRRORONLY has been specified by the mirror doesn't contain SRC_REV. Signed-off-by: Pavel Zhukov <pavel.zhukov@huawei.com> --- lib/bb/fetch2/__init__.py | 2 ++ lib/bb/fetch2/git.py | 16 ++++++++++++---- 2 files changed, 14 insertions(+), 4 deletions(-)