diff mbox series

[v2] fetch2/git.py: Check if clonedir is correct after shallow tarball clone

Message ID 20220210132714.22921-1-tomasz.dziendzielski@gmail.com
State New
Headers show
Series [v2] fetch2/git.py: Check if clonedir is correct after shallow tarball clone | expand

Commit Message

Tomasz Dziendzielski Feb. 10, 2022, 1:27 p.m. UTC 
If shallow tarball clone is used and if tar unpacking fails for any
reason it might end up with empty directory created. Since runfetchcmd
does not check any return code we might not detect the issue and we will
not clone from git. Then due to missing hash it will replace main
repository's git remote with the remote of failing recipe.

To fix this behaviour we'll check if clonedir is not empty and if git
dir is the same as clonedir.

Signed-off-by: Tomasz Dziendzielski <tomasz.dziendzielski@gmail.com>
Signed-off-by: Jan Brzezanski <jan.brzezanski@gmail.com>
---
 lib/bb/fetch2/git.py | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Alexander Kanavin Feb. 10, 2022, 1:31 p.m. UTC | #1 
Wait, doesn't this mask the real issue? Is it ok that the tarball
unpacking failed 'for any reason'?

Alex

On Thu, 10 Feb 2022 at 14:27, Tomasz Dziendzielski
<tomasz.dziendzielski@gmail.com> wrote:
>
> If shallow tarball clone is used and if tar unpacking fails for any
> reason it might end up with empty directory created. Since runfetchcmd
> does not check any return code we might not detect the issue and we will
> not clone from git. Then due to missing hash it will replace main
> repository's git remote with the remote of failing recipe.
>
> To fix this behaviour we'll check if clonedir is not empty and if git
> dir is the same as clonedir.
>
> Signed-off-by: Tomasz Dziendzielski <tomasz.dziendzielski@gmail.com>
> Signed-off-by: Jan Brzezanski <jan.brzezanski@gmail.com>
> ---
>  lib/bb/fetch2/git.py | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
> index 30da8e95..ed6ecb64 100644
> --- a/lib/bb/fetch2/git.py
> +++ b/lib/bb/fetch2/git.py
> @@ -356,8 +356,15 @@ class Git(FetchMethod):
>
>          repourl = self._get_repo_url(ud)
>
> -        # If the repo still doesn't exist, fallback to cloning it
> -        if not os.path.exists(ud.clonedir):
> +        # Clean repo if git dir is different than ud.clonedir, which means tar unpacking failed
> +        if os.path.exists(ud.clonedir):
> +            if ud.clonedir != runfetchcmd("%s -C %s rev-parse --git-dir" % (ud.basecmd, ud.clonedir), d):
> +                bb.utils.remove(ud.clonedir, recurse=True)
> +                bb.utils.mkdirhier(ud.clonedir)
> +                os.chdir(ud.clonedir)
> +
> +        # If the repo still doesn't exist or is empty, fallback to cloning it
> +        if not os.path.exists(ud.clonedir) or not os.listdir(ud.clonedir):
>              # We do this since git will use a "-l" option automatically for local urls where possible
>              if repourl.startswith("file://"):
>                  repourl = repourl[7:]
> --
> 2.34.1
>
>
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#13328): https://lists.openembedded.org/g/bitbake-devel/message/13328
> Mute This Topic: https://lists.openembedded.org/mt/89045076/1686489
> Group Owner: bitbake-devel+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/bitbake-devel/unsub [alex.kanavin@gmail.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>
Tomasz Dziendzielski Feb. 10, 2022, 1:39 p.m. UTC | #2 
Hi,
right now code accepts failing tar and just runs fallback (which is git
clone). If there is any tar failure then it also needs to be fixed but even
so we don't want bitbake to override our git remote.

Best regards,
Tomasz Dziendzielski

czw., 10 lut 2022 o 14:31 Alexander Kanavin <alex.kanavin@gmail.com>
napisał(a):

> Wait, doesn't this mask the real issue? Is it ok that the tarball
> unpacking failed 'for any reason'?
>
> Alex
>
> On Thu, 10 Feb 2022 at 14:27, Tomasz Dziendzielski
> <tomasz.dziendzielski@gmail.com> wrote:
> >
> > If shallow tarball clone is used and if tar unpacking fails for any
> > reason it might end up with empty directory created. Since runfetchcmd
> > does not check any return code we might not detect the issue and we will
> > not clone from git. Then due to missing hash it will replace main
> > repository's git remote with the remote of failing recipe.
> >
> > To fix this behaviour we'll check if clonedir is not empty and if git
> > dir is the same as clonedir.
> >
> > Signed-off-by: Tomasz Dziendzielski <tomasz.dziendzielski@gmail.com>
> > Signed-off-by: Jan Brzezanski <jan.brzezanski@gmail.com>
> > ---
> >  lib/bb/fetch2/git.py | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
> > index 30da8e95..ed6ecb64 100644
> > --- a/lib/bb/fetch2/git.py
> > +++ b/lib/bb/fetch2/git.py
> > @@ -356,8 +356,15 @@ class Git(FetchMethod):
> >
> >          repourl = self._get_repo_url(ud)
> >
> > -        # If the repo still doesn't exist, fallback to cloning it
> > -        if not os.path.exists(ud.clonedir):
> > +        # Clean repo if git dir is different than ud.clonedir, which
> means tar unpacking failed
> > +        if os.path.exists(ud.clonedir):
> > +            if ud.clonedir != runfetchcmd("%s -C %s rev-parse
> --git-dir" % (ud.basecmd, ud.clonedir), d):
> > +                bb.utils.remove(ud.clonedir, recurse=True)
> > +                bb.utils.mkdirhier(ud.clonedir)
> > +                os.chdir(ud.clonedir)
> > +
> > +        # If the repo still doesn't exist or is empty, fallback to
> cloning it
> > +        if not os.path.exists(ud.clonedir) or not
> os.listdir(ud.clonedir):
> >              # We do this since git will use a "-l" option automatically
> for local urls where possible
> >              if repourl.startswith("file://"):
> >                  repourl = repourl[7:]
> > --
> > 2.34.1
> >
> >
> > -=-=-=-=-=-=-=-=-=-=-=-
> > Links: You receive all messages sent to this group.
> > View/Reply Online (#13328):
> https://lists.openembedded.org/g/bitbake-devel/message/13328
> > Mute This Topic: https://lists.openembedded.org/mt/89045076/1686489
> > Group Owner: bitbake-devel+owner@lists.openembedded.org
> > Unsubscribe: https://lists.openembedded.org/g/bitbake-devel/unsub [
> alex.kanavin@gmail.com]
> > -=-=-=-=-=-=-=-=-=-=-=-
> >
>
Tomasz Dziendzielski Feb. 10, 2022, 2:22 p.m. UTC | #3 
Then the first patch is safer, as it will not break build with tarballs
without vcs.

Best regards,
Tomasz Dziendzielski

czw., 10 lut 2022 o 15:08 Pavel Zhukov <pavel@zhukoff.net> napisał(a):

> This is good question.
> It is possible to provide bitbake with mailformed git repo mirror tarball
> (manually created one for example with --exclude-vcs flag set or from wrong
> subdirectory) in that case tar will not return any return code but
> git.download() will try to replace remote with origin from recipe_url. In
> worse case it will replace origin of "upper" git repo (poky?) with recipe's
> one.
>
>
> --
> Pavel
>
>
>
> 10.02.2022, 14:31, "Alexander Kanavin" <alex.kanavin@gmail.com>:
>
> Wait, doesn't this mask the real issue? Is it ok that the tarball
> unpacking failed 'for any reason'?
>
> Alex
>
> On Thu, 10 Feb 2022 at 14:27, Tomasz Dziendzielski
> <tomasz.dziendzielski@gmail.com> wrote:
>
>
>  If shallow tarball clone is used and if tar unpacking fails for any
>  reason it might end up with empty directory created. Since runfetchcmd
>  does not check any return code we might not detect the issue and we will
>  not clone from git. Then due to missing hash it will replace main
>  repository's git remote with the remote of failing recipe.
>
>  To fix this behaviour we'll check if clonedir is not empty and if git
>  dir is the same as clonedir.
>
>  Signed-off-by: Tomasz Dziendzielski <tomasz.dziendzielski@gmail.com>
>  Signed-off-by: Jan Brzezanski <jan.brzezanski@gmail.com>
>  ---
>   lib/bb/fetch2/git.py | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
>
>  diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
>  index 30da8e95..ed6ecb64 100644
>  --- a/lib/bb/fetch2/git.py
>  +++ b/lib/bb/fetch2/git.py
>  @@ -356,8 +356,15 @@ class Git(FetchMethod):
>
>           repourl = self._get_repo_url(ud)
>
>  - # If the repo still doesn't exist, fallback to cloning it
>  - if not os.path.exists(ud.clonedir):
>  + # Clean repo if git dir is different than ud.clonedir, which means tar
> unpacking failed
>  + if os.path.exists(ud.clonedir):
>  + if ud.clonedir != runfetchcmd("%s -C %s rev-parse --git-dir" %
> (ud.basecmd, ud.clonedir), d):
>  + bb.utils.remove(ud.clonedir, recurse=True)
>  + bb.utils.mkdirhier(ud.clonedir)
>  + os.chdir(ud.clonedir)
>  +
>  + # If the repo still doesn't exist or is empty, fallback to cloning it
>  + if not os.path.exists(ud.clonedir) or not os.listdir(ud.clonedir):
>               # We do this since git will use a "-l" option automatically
> for local urls where possible
>               if repourl.startswith("file://"):
>                   repourl = repourl[7:]
>  --
>  2.34.1
>
>
>  -=-=-=-=-=-=-=-=-=-=-=-
>  Links: You receive all messages sent to this group.
>  View/Reply Online (#13328):
> https://lists.openembedded.org/g/bitbake-devel/message/13328
>  Mute This Topic: https://lists.openembedded.org/mt/89045076/1686489
>  Group Owner: bitbake-devel+owner@lists.openembedded.org
>  Unsubscribe: https://lists.openembedded.org/g/bitbake-devel/unsub [
> alex.kanavin@gmail.com]
>  -=-=-=-=-=-=-=-=-=-=-=-
>
>
>
Tomasz Dziendzielski Feb. 10, 2022, 2:58 p.m. UTC | #4 
Is this bitbake-selftest output? I tried that but got network issues and
missing branch tag only.
Maybe I'll check it on different machine.

Best regards,
Tomasz Dziendzielski

czw., 10 lut 2022 o 15:40 Pavel Zhukov <pavel@zhukoff.net> napisał(a):

> Not really.
> it will either complain [1] if there's not git repository in the parent's
> dirs or replace git remote with the one from recipe_url and fail with [2]
> if build directory is under some git managed directory.
>
> [1]
> Fetcher failure: Fetch command export PSEUDO_DISABLED=1; git -c
> core.fsyncobjectfiles=0 -c gc.autoDetach=false remote failed with exit code
> 128, output:
> fatal: not a git repository (or any parent up to mount point /)
>
> [2]
> bb.fetch2.FetchError: Fetcher failure: Fetch command export
> PSEUDO_DISABLED=1; git -c core.fsyncobjectfiles=0 -c gc.autoDetach=false
> clone -n -s
> /tmp/bitbake-fetch-hyrcqkrw/download/git2/git.openembedded.org.bitbake/
> /tmp/bitbake-fetch-hyrcqkrw/unpacked/git/ failed with exit code 128, output:
> fatal: repository
> '/tmp/bitbake-fetch-hyrcqkrw/download/git2/git.openembedded.org.bitbake/'
> does not exist
>
>
> --
> Pavel
>
>
>
> 10.02.2022, 15:19, "Tomasz Dziendzielski" <tomasz.dziendzielski@gmail.com
> >:
>
> Then the first patch is safer, as it will not break build with tarballs
> without vcs.
>
> Best regards,
> Tomasz Dziendzielski
>
> czw., 10 lut 2022 o 15:08 Pavel Zhukov <pavel@zhukoff.net> napisał(a):
>
> This is good question.
> It is possible to provide bitbake with mailformed git repo mirror tarball
> (manually created one for example with --exclude-vcs flag set or from wrong
> subdirectory) in that case tar will not return any return code but
> git.download() will try to replace remote with origin from recipe_url. In
> worse case it will replace origin of "upper" git repo (poky?) with recipe's
> one.
>
>
> --
> Pavel
>
>
>
> 10.02.2022, 14:31, "Alexander Kanavin" <alex.kanavin@gmail.com>:
>
> Wait, doesn't this mask the real issue? Is it ok that the tarball
> unpacking failed 'for any reason'?
>
> Alex
>
> On Thu, 10 Feb 2022 at 14:27, Tomasz Dziendzielski
> <tomasz.dziendzielski@gmail.com> wrote:
>
>
>  If shallow tarball clone is used and if tar unpacking fails for any
>  reason it might end up with empty directory created. Since runfetchcmd
>  does not check any return code we might not detect the issue and we will
>  not clone from git. Then due to missing hash it will replace main
>  repository's git remote with the remote of failing recipe.
>
>  To fix this behaviour we'll check if clonedir is not empty and if git
>  dir is the same as clonedir.
>
>  Signed-off-by: Tomasz Dziendzielski <tomasz.dziendzielski@gmail.com>
>  Signed-off-by: Jan Brzezanski <jan.brzezanski@gmail.com>
>  ---
>   lib/bb/fetch2/git.py | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
>
>  diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
>  index 30da8e95..ed6ecb64 100644
>  --- a/lib/bb/fetch2/git.py
>  +++ b/lib/bb/fetch2/git.py
>  @@ -356,8 +356,15 @@ class Git(FetchMethod):
>
>           repourl = self._get_repo_url(ud)
>
>  - # If the repo still doesn't exist, fallback to cloning it
>  - if not os.path.exists(ud.clonedir):
>  + # Clean repo if git dir is different than ud.clonedir, which means tar
> unpacking failed
>  + if os.path.exists(ud.clonedir):
>  + if ud.clonedir != runfetchcmd("%s -C %s rev-parse --git-dir" %
> (ud.basecmd, ud.clonedir), d):
>  + bb.utils.remove(ud.clonedir, recurse=True)
>  + bb.utils.mkdirhier(ud.clonedir)
>  + os.chdir(ud.clonedir)
>  +
>  + # If the repo still doesn't exist or is empty, fallback to cloning it
>  + if not os.path.exists(ud.clonedir) or not os.listdir(ud.clonedir):
>               # We do this since git will use a "-l" option automatically
> for local urls where possible
>               if repourl.startswith("file://"):
>                   repourl = repourl[7:]
>  --
>  2.34.1
>
>
>  -=-=-=-=-=-=-=-=-=-=-=-
>  Links: You receive all messages sent to this group.
>  View/Reply Online (#13328):
> https://lists.openembedded.org/g/bitbake-devel/message/13328
>  Mute This Topic: https://lists.openembedded.org/mt/89045076/1686489
>  Group Owner: bitbake-devel+owner@lists.openembedded.org
>  Unsubscribe: https://lists.openembedded.org/g/bitbake-devel/unsub [
> alex.kanavin@gmail.com]
>  -=-=-=-=-=-=-=-=-=-=-=-
>
>
>
Tomasz Dziendzielski Feb. 10, 2022, 3:24 p.m. UTC | #5 
I just double checked and with the first patch bitbake-selftest is failing
with two errors that appear also without my changes, something about
missing hash on branch. Where did you take that logs so I could check it
and fix the patch?

Best regards,
Tomasz Dziendzielski

czw., 10 lut 2022 o 15:55 Tomasz Dziendzielski via lists.openembedded.org
<tomasz.dziendzielski=gmail.com@lists.openembedded.org> napisał(a):

> Is this bitbake-selftest output? I tried that but got network issues and
> missing branch tag only.
> Maybe I'll check it on different machine.
>
> Best regards,
> Tomasz Dziendzielski
>
> czw., 10 lut 2022 o 15:40 Pavel Zhukov <pavel@zhukoff.net> napisał(a):
>
>> Not really.
>> it will either complain [1] if there's not git repository in the parent's
>> dirs or replace git remote with the one from recipe_url and fail with [2]
>> if build directory is under some git managed directory.
>>
>> [1]
>> Fetcher failure: Fetch command export PSEUDO_DISABLED=1; git -c
>> core.fsyncobjectfiles=0 -c gc.autoDetach=false remote failed with exit code
>> 128, output:
>> fatal: not a git repository (or any parent up to mount point /)
>>
>> [2]
>> bb.fetch2.FetchError: Fetcher failure: Fetch command export
>> PSEUDO_DISABLED=1; git -c core.fsyncobjectfiles=0 -c gc.autoDetach=false
>> clone -n -s
>> /tmp/bitbake-fetch-hyrcqkrw/download/git2/git.openembedded.org.bitbake/
>> /tmp/bitbake-fetch-hyrcqkrw/unpacked/git/ failed with exit code 128, output:
>> fatal: repository
>> '/tmp/bitbake-fetch-hyrcqkrw/download/git2/git.openembedded.org.bitbake/'
>> does not exist
>>
>>
>> --
>> Pavel
>>
>>
>>
>> 10.02.2022, 15:19, "Tomasz Dziendzielski" <tomasz.dziendzielski@gmail.com
>> >:
>>
>> Then the first patch is safer, as it will not break build with tarballs
>> without vcs.
>>
>> Best regards,
>> Tomasz Dziendzielski
>>
>> czw., 10 lut 2022 o 15:08 Pavel Zhukov <pavel@zhukoff.net> napisał(a):
>>
>> This is good question.
>> It is possible to provide bitbake with mailformed git repo mirror tarball
>> (manually created one for example with --exclude-vcs flag set or from wrong
>> subdirectory) in that case tar will not return any return code but
>> git.download() will try to replace remote with origin from recipe_url. In
>> worse case it will replace origin of "upper" git repo (poky?) with recipe's
>> one.
>>
>>
>> --
>> Pavel
>>
>>
>>
>> 10.02.2022, 14:31, "Alexander Kanavin" <alex.kanavin@gmail.com>:
>>
>> Wait, doesn't this mask the real issue? Is it ok that the tarball
>> unpacking failed 'for any reason'?
>>
>> Alex
>>
>> On Thu, 10 Feb 2022 at 14:27, Tomasz Dziendzielski
>> <tomasz.dziendzielski@gmail.com> wrote:
>>
>>
>>  If shallow tarball clone is used and if tar unpacking fails for any
>>  reason it might end up with empty directory created. Since runfetchcmd
>>  does not check any return code we might not detect the issue and we will
>>  not clone from git. Then due to missing hash it will replace main
>>  repository's git remote with the remote of failing recipe.
>>
>>  To fix this behaviour we'll check if clonedir is not empty and if git
>>  dir is the same as clonedir.
>>
>>  Signed-off-by: Tomasz Dziendzielski <tomasz.dziendzielski@gmail.com>
>>  Signed-off-by: Jan Brzezanski <jan.brzezanski@gmail.com>
>>  ---
>>   lib/bb/fetch2/git.py | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>>  diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
>>  index 30da8e95..ed6ecb64 100644
>>  --- a/lib/bb/fetch2/git.py
>>  +++ b/lib/bb/fetch2/git.py
>>  @@ -356,8 +356,15 @@ class Git(FetchMethod):
>>
>>           repourl = self._get_repo_url(ud)
>>
>>  - # If the repo still doesn't exist, fallback to cloning it
>>  - if not os.path.exists(ud.clonedir):
>>  + # Clean repo if git dir is different than ud.clonedir, which means tar
>> unpacking failed
>>  + if os.path.exists(ud.clonedir):
>>  + if ud.clonedir != runfetchcmd("%s -C %s rev-parse --git-dir" %
>> (ud.basecmd, ud.clonedir), d):
>>  + bb.utils.remove(ud.clonedir, recurse=True)
>>  + bb.utils.mkdirhier(ud.clonedir)
>>  + os.chdir(ud.clonedir)
>>  +
>>  + # If the repo still doesn't exist or is empty, fallback to cloning it
>>  + if not os.path.exists(ud.clonedir) or not os.listdir(ud.clonedir):
>>               # We do this since git will use a "-l" option automatically
>> for local urls where possible
>>               if repourl.startswith("file://"):
>>                   repourl = repourl[7:]
>>  --
>>  2.34.1
>>
>>
>>
>>
>>
>>
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#13335):
> https://lists.openembedded.org/g/bitbake-devel/message/13335
> Mute This Topic: https://lists.openembedded.org/mt/89045076/3619514
> Group Owner: bitbake-devel+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/bitbake-devel/unsub [
> tomasz.dziendzielski@gmail.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>
>
diff mbox series

Patch

diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
index 30da8e95..ed6ecb64 100644
--- a/lib/bb/fetch2/git.py
+++ b/lib/bb/fetch2/git.py
@@ -356,8 +356,15 @@  class Git(FetchMethod):
 
         repourl = self._get_repo_url(ud)
 
-        # If the repo still doesn't exist, fallback to cloning it
-        if not os.path.exists(ud.clonedir):
+        # Clean repo if git dir is different than ud.clonedir, which means tar unpacking failed
+        if os.path.exists(ud.clonedir):
+            if ud.clonedir != runfetchcmd("%s -C %s rev-parse --git-dir" % (ud.basecmd, ud.clonedir), d):
+                bb.utils.remove(ud.clonedir, recurse=True)
+                bb.utils.mkdirhier(ud.clonedir)
+                os.chdir(ud.clonedir)
+
+        # If the repo still doesn't exist or is empty, fallback to cloning it
+        if not os.path.exists(ud.clonedir) or not os.listdir(ud.clonedir):
             # We do this since git will use a "-l" option automatically for local urls where possible
             if repourl.startswith("file://"):
                 repourl = repourl[7:]