[Buildroot] [PATCH 0/2 v2] support/download: make the git helper more robust (branch yem/dl-git-robust)

[Buildroot] [PATCH 0/2 v2] support/download: make the git helper more robust (branch yem/dl-git-robust)

[Yann E. MORIN]

Hello All!

This small series is an attempt at making our download git helper be
more robust in the face of unexpected and strange errors.

What prompted the series is a set of totally weird and unexplained
download failures on my autobuilder instance.

Most notably, those two failures are the most prominent ones:

  - http://autobuild.buildroot.net/results/20f/20fd76d2256eee81837f7e9bbaefbe79d7645ae9/
    find is failing to find any file

  - http://autobuild.buildroot.org/results/018/018971ea9227b386fe25d3c264c7e80b843a9f68/
    tar is failing to stat its fd for the -T option

Although those errors are most probably caused by hardware issues (to
still be investigated), we do really need to catch those errors.

We were so far failing to do so, because the way the script was written:
we can't catch failures for left-hand sides of pipes, nor can we catch
failures of process substitutions.

This series fixes those issue.

Additionally, we have code to fetch special refs exposed by various
forges, like Github PRs or Gerrit changes. However, when using a plain
ref (e.g. a tag or a sha1), the checkout would emit a really confusing
warning message.

We also fix that by only fetching the ref if we do not already have the
necessary cset locally.


Changes v1 -> v2:
  - drop the pipe in the find command in the first patch;
  - add the second patch about special refs.


Regards,
Yann E. MORIN.


The following changes since commit 59899f40f1777c3120881947cbd078b42efb05cb

  strace: bump to version 4.13 (2016-07-31 22:22:21 +0200)


are available in the git repository at:

  https://gitlab.com/ymorin/buildroot.git

for you to fetch changes up to de318be75bdcad45f90ddb81d86911b72951e607

  support/download: be more conservative about git special refs (2016-08-01 00:19:05 +0200)


----------------------------------------------------------------
Yann E. MORIN (2):
      support/download: make the git wrapper more robust
      support/download: be more conservative about git special refs

 support/download/git | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 1/2 v2] support/download: make the git wrapper more robust

[Yann E. MORIN]

Currently, there are two failure paths in the wrapper:

  - if the tar fails, then the error is ignored because it is on the
    left-hand-side of a pipe;

  - if the find fails, then the error is ignored because it is a
    process substitution (and there is a pipe, too).

While the former could be fixed with "set -o pipefail", the latter can
not be fixed thusly and we must use an intemediate file for it.

So, fix both issues by using intermediate files, both to generate the
list of files to include in the archive, and generate the archive in a
temporary tarball.

Fixes the following build issue, where the find is failing for whatever
unknown reason:
    http://autobuild.buildroot.net/results/20f/20fd76d2256eee81837f7e9bbaefbe79d7645ae9/

And this one, where the process substitution failed, also for an unknown
reason:
    http://autobuild.buildroot.org/results/018/018971ea9227b386fe25d3c264c7e80b843a9f68/

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

---
Changes v1 -> v2:
  - also drop the pipe of the find command
---
 support/download/git | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/support/download/git b/support/download/git
index 416cd1b..e3be2e1 100755
--- a/support/download/git
+++ b/support/download/git
@@ -90,6 +90,8 @@ rm -rf .git
 popd >/dev/null
 
 # Generate the archive, sort with the C locale so that it is reproducible
+find "${basename}" -not -type d >"${basename}.list"
+LC_ALL=C sort <"${basename}.list" >"${basename}.list.sorted"
 tar cf - --numeric-owner --owner=0 --group=0 --mtime="${date}" \
-         -T <(find "${basename}" -not -type d |LC_ALL=C sort) \
-|gzip -n >"${output}"
+         -T "${basename}.list.sorted" >"${output}.tar"
+gzip -n <"${output}.tar" >"${output}"
-- 
2.7.4

[Buildroot] [PATCH 1/2 v2] support/download: make the git wrapper more robust

[Thomas Petazzoni]

Hello,

On Mon,  1 Aug 2016 00:35:55 +0200, Yann E. MORIN wrote:
> Currently, there are two failure paths in the wrapper:
> 
>   - if the tar fails, then the error is ignored because it is on the
>     left-hand-side of a pipe;
> 
>   - if the find fails, then the error is ignored because it is a
>     process substitution (and there is a pipe, too).
> 
> While the former could be fixed with "set -o pipefail", the latter can
> not be fixed thusly and we must use an intemediate file for it.
> 
> So, fix both issues by using intermediate files, both to generate the
> list of files to include in the archive, and generate the archive in a
> temporary tarball.
> 
> Fixes the following build issue, where the find is failing for whatever
> unknown reason:
>     http://autobuild.buildroot.net/results/20f/20fd76d2256eee81837f7e9bbaefbe79d7645ae9/
> 
> And this one, where the process substitution failed, also for an unknown
> reason:
>     http://autobuild.buildroot.org/results/018/018971ea9227b386fe25d3c264c7e80b843a9f68/
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> 
> ---
> Changes v1 -> v2:
>   - also drop the pipe of the find command
> ---
>  support/download/git | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

Applied to master, thanks.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH 2/2 v2] support/download: be more conservative about git special refs

[Yann E. MORIN]

The git special refs (e.g. Github PRs, Gerrit changes...) are not
retrieved on non-bare clones; we need to fetch them manually.

However, we need not fetch any cset that is not such a special ref; it
only generates warning messages that are *really* confusing.

Instead, we now check if we have the cset we need, and only when it is
missing do we explicitly request it.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 support/download/git | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/support/download/git b/support/download/git
index e3be2e1..1c08c17 100755
--- a/support/download/git
+++ b/support/download/git
@@ -66,8 +66,12 @@ pushd "${basename}" >/dev/null
 # below, if there is an issue anyway. Since most of the cset we're gonna
 # have to clone are not such special refs, consign the output to oblivion
 # so as not to alarm unsuspecting users, but still trace it as a warning.
-if ! _git fetch origin "'${cset}:${cset}'" >/dev/null 2>&1; then
-    printf "Could not fetch special ref '%s'; assuming it is not special.\n" "${cset}"
+#
+# Only try to fetch such a special ref if we do not already have the
+# cset we need locally.
+#
+if ! _git rev-parse -q --verify "'${cset}^{commit}'" >/dev/null; then
+    _git fetch origin "'${cset}:${cset}'"
 fi
 
 # Checkout the required changeset, so that we can update the required
-- 
2.7.4

[Buildroot] [PATCH 2/2 v2] support/download: be more conservative about git special refs

[Ricardo Martincoski]

Yann,

----- Original Message -----
> From: "Yann E. MORIN" <yann.morin.1998@free.fr>
> To: buildroot at buildroot.org
> Cc: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Sent: Sunday, July 31, 2016 7:35:56 PM
> Subject: [Buildroot] [PATCH 2/2 v2] support/download: be more conservative
>	about git special refs
> The git special refs (e.g. Github PRs, Gerrit changes...) are not
> retrieved on non-bare clones; we need to fetch them manually.
> 
> However, we need not fetch any cset that is not such a special ref; it
> only generates warning messages that are *really* confusing.
> 
> Instead, we now check if we have the cset we need, and only when it is
> missing do we explicitly request it.
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
> support/download/git | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/support/download/git b/support/download/git
> index e3be2e1..1c08c17 100755
> --- a/support/download/git
> +++ b/support/download/git
> @@ -66,8 +66,12 @@ pushd "${basename}" >/dev/null
> # below, if there is an issue anyway. Since most of the cset we're gonna
> # have to clone are not such special refs, consign the output to oblivion
> # so as not to alarm unsuspecting users, but still trace it as a warning.
> -if ! _git fetch origin "'${cset}:${cset}'" >/dev/null 2>&1; then
> -    printf "Could not fetch special ref '%s'; assuming it is not special.\n"
> "${cset}"
> +#
> +# Only try to fetch such a special ref if we do not already have the
> +# cset we need locally.
> +#
> +if ! _git rev-parse -q --verify "'${cset}^{commit}'" >/dev/null; then
> +    _git fetch origin "'${cset}:${cset}'"

A question more related to 13c89c2f than to this patch, but anyway...
13c89c2f support/download/git: do not use bare clones

When cset is a special ref, e.g. refs/changes/80/22580/2, this 'git fetch' works
fine.
But when cset is a sha1 of a special ref, since 13c89c2f the checkout fails
AFAIK because git server using default configuration does not upload
unreachable sha1.

Maybe we could translate the sha1 to a named ref when we assume we are about to
ask for a special ref. Something like this...

if ! _git rev-parse -q --verify "'${cset}^{commit}'" >/dev/null; then
    named_ref=$(_git ls-remote "'${repo}'" 2>&1 | grep "^${cset}" | cut -f2)
    if [ -n "$named_ref" ]; then
        _git fetch origin "'${named_ref}:${named_ref}'"
    else
        _git fetch origin "'${cset}:${cset}'"
    fi
fi

What do you think?

> fi
> 
> # Checkout the required changeset, so that we can update the required
> --
> 2.7.4

Regards,
Ricardo Martincoski

[Buildroot] [PATCH 2/2 v2] support/download: be more conservative about git special refs

[Yann E. MORIN]

Ricardo, All,

Sorry for the delay...

On 2016-08-24 10:32 -0300, Ricardo Martincoski spake thusly:
> ----- Original Message -----
> > From: "Yann E. MORIN" <yann.morin.1998@free.fr>
> > To: buildroot at buildroot.org
> > Cc: "Yann E. MORIN" <yann.morin.1998@free.fr>
> > Sent: Sunday, July 31, 2016 7:35:56 PM
> > Subject: [Buildroot] [PATCH 2/2 v2] support/download: be more conservative
> >	about git special refs
> > The git special refs (e.g. Github PRs, Gerrit changes...) are not
> > retrieved on non-bare clones; we need to fetch them manually.
> > 
> > However, we need not fetch any cset that is not such a special ref; it
> > only generates warning messages that are *really* confusing.
> > 
> > Instead, we now check if we have the cset we need, and only when it is
> > missing do we explicitly request it.
> > 
> > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> > Cc Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> > ---
> > support/download/git | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/support/download/git b/support/download/git
> > index e3be2e1..1c08c17 100755
> > --- a/support/download/git
> > +++ b/support/download/git
> > @@ -66,8 +66,12 @@ pushd "${basename}" >/dev/null
> > # below, if there is an issue anyway. Since most of the cset we're gonna
> > # have to clone are not such special refs, consign the output to oblivion
> > # so as not to alarm unsuspecting users, but still trace it as a warning.
> > -if ! _git fetch origin "'${cset}:${cset}'" >/dev/null 2>&1; then
> > -    printf "Could not fetch special ref '%s'; assuming it is not special.\n"
> > "${cset}"
> > +#
> > +# Only try to fetch such a special ref if we do not already have the
> > +# cset we need locally.
> > +#
> > +if ! _git rev-parse -q --verify "'${cset}^{commit}'" >/dev/null; then
> > +    _git fetch origin "'${cset}:${cset}'"
> 
> A question more related to 13c89c2f than to this patch, but anyway...
> 13c89c2f support/download/git: do not use bare clones
> 
> When cset is a special ref, e.g. refs/changes/80/22580/2, this 'git fetch' works
> fine.
> But when cset is a sha1 of a special ref, since 13c89c2f the checkout fails
> AFAIK because git server using default configuration does not upload
> unreachable sha1.
> 
> Maybe we could translate the sha1 to a named ref when we assume we are about to
> ask for a special ref. Something like this...
> 
> if ! _git rev-parse -q --verify "'${cset}^{commit}'" >/dev/null; then
>     named_ref=$(_git ls-remote "'${repo}'" 2>&1 | grep "^${cset}" | cut -f2)
>     if [ -n "$named_ref" ]; then
>         _git fetch origin "'${named_ref}:${named_ref}'"
>     else
>         _git fetch origin "'${cset}:${cset}'"
>     fi
> fi
> 
> What do you think?

I don't use such "special refs", so I can't really state whether it is
good or not. If there was a publicy-reachable tree I could test against,
that would be nice. Do you know of such atree (even for a package not in
Buildroot)?

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 2/2 v2] support/download: be more conservative about git special refs

[Henrique Marks]

Hello all

----- Mensagem original -----
> De: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Para: "ricardo martincoski" <ricardo.martincoski@datacom.ind.br>
> Cc: buildroot at buildroot.org
> Enviadas: S?bado, 15 de outubro de 2016 6:00:18
> Assunto: Re: [Buildroot] [PATCH 2/2 v2] support/download: be more conservative	about git special refs

> Ricardo, All,
> 
> Sorry for the delay...
> 
> On 2016-08-24 10:32 -0300, Ricardo Martincoski spake thusly:
>> ----- Original Message -----
>> > From: "Yann E. MORIN" <yann.morin.1998@free.fr>
>> > To: buildroot at buildroot.org
>> > Cc: "Yann E. MORIN" <yann.morin.1998@free.fr>
>> > Sent: Sunday, July 31, 2016 7:35:56 PM
>> > Subject: [Buildroot] [PATCH 2/2 v2] support/download: be more conservative
>> >	about git special refs
>> > The git special refs (e.g. Github PRs, Gerrit changes...) are not
>> > retrieved on non-bare clones; we need to fetch them manually.
>> > 
>> > However, we need not fetch any cset that is not such a special ref; it
>> > only generates warning messages that are *really* confusing.
>> > 
>> > Instead, we now check if we have the cset we need, and only when it is
>> > missing do we explicitly request it.
>> > 
>> > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
>> > Cc Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>> > ---
>> > support/download/git | 8 ++++++--
>> > 1 file changed, 6 insertions(+), 2 deletions(-)
>> > 
>> > diff --git a/support/download/git b/support/download/git
>> > index e3be2e1..1c08c17 100755
>> > --- a/support/download/git
>> > +++ b/support/download/git
>> > @@ -66,8 +66,12 @@ pushd "${basename}" >/dev/null
>> > # below, if there is an issue anyway. Since most of the cset we're gonna
>> > # have to clone are not such special refs, consign the output to oblivion
>> > # so as not to alarm unsuspecting users, but still trace it as a warning.
>> > -if ! _git fetch origin "'${cset}:${cset}'" >/dev/null 2>&1; then
>> > -    printf "Could not fetch special ref '%s'; assuming it is not special.\n"
>> > "${cset}"
>> > +#
>> > +# Only try to fetch such a special ref if we do not already have the
>> > +# cset we need locally.
>> > +#
>> > +if ! _git rev-parse -q --verify "'${cset}^{commit}'" >/dev/null; then
>> > +    _git fetch origin "'${cset}:${cset}'"
>> 
>> A question more related to 13c89c2f than to this patch, but anyway...
>> 13c89c2f support/download/git: do not use bare clones
>> 
>> When cset is a special ref, e.g. refs/changes/80/22580/2, this 'git fetch' works
>> fine.
>> But when cset is a sha1 of a special ref, since 13c89c2f the checkout fails
>> AFAIK because git server using default configuration does not upload
>> unreachable sha1.
>> 
>> Maybe we could translate the sha1 to a named ref when we assume we are about to
>> ask for a special ref. Something like this...
>> 
>> if ! _git rev-parse -q --verify "'${cset}^{commit}'" >/dev/null; then
>>     named_ref=$(_git ls-remote "'${repo}'" 2>&1 | grep "^${cset}" | cut -f2)
>>     if [ -n "$named_ref" ]; then
>>         _git fetch origin "'${named_ref}:${named_ref}'"
>>     else
>>         _git fetch origin "'${cset}:${cset}'"
>>     fi
>> fi
>> 
>> What do you think?
> 
> I don't use such "special refs", so I can't really state whether it is
> good or not. If there was a publicy-reachable tree I could test against,
> that would be nice. Do you know of such atree (even for a package not in
> Buildroot)?
> 
> Regards,
> Yann E. MORIN.
> 
> --
> .-----------------.--------------------.------------------.--------------------.
>|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
>| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
>| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
>| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
> '------------------------------^-------^------------------^--------------------'
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

I guess openswitch project have a lot of gerrit managed repositories. This can be used as examples and tests for Ricardo suggestion.

https://review.openswitch.net/#/admin/projects/

One particular sub-project

https://review.openswitch.net/#/admin/projects/openswitch/ops

git clone

git clone https://review.openswitch.net/openswitch/ops

And then it is possible to make a buildroot recipe to download and test this package with and without special_refs.

But it is important to say that we are using Ricardo suggestion in our buildroot-upstream cloned repository, that is based in 2016.08 tag, without any problems. Indeed, the solution has solved some problems, so i can say it has been exhaustively tested.

Our buildroot-upstream repo has this deliveries related to the subject, just for the sake of completeness:

git log -n8 --oneline

6283402 support/download/git: allow sha1 of special refs (v2)
35b9416 Revert "support/download/git: allow sha1 of special refs"
81523bc support/download/git: allow sha1 of special refs
e415a54 support/download/git: Fix compatibility issue with git older than 1.8.4
11e9a47 support/download: don't over-remove files from git archives
cf98e0a support/download/git: add support for submodules
a34d5e7 support/download/git: do not use git archive, handle it manually
5fc0731 support/download/git: do not use bare clones

The first three i think are in our tree only, and are the subject of this review.

Thanks a lot
-- 
Dr. Henrique Marks
henrique.marks at datacom.ind.br
R. Am?rica, 1000 - Eldorado do Sul - RS
CEP: 92990-000 - Brasil
Fone: +55 51 3933 3000 - Ramal 3466

[Buildroot] [PATCH 2/2 v2] support/download: be more conservative about git special refs

[Ricardo Martincoski]

Hello,

On Sat, Oct 15, 2016 at 03:35 PM, Henrique Marks wrote:

> Hello all
> 
> ----- Mensagem original -----
>> De: "Yann E. MORIN" <yann.morin.1998@free.fr>
>> Para: "ricardo martincoski" <ricardo.martincoski@datacom.ind.br>
>> Cc: buildroot at buildroot.org
>> Enviadas: S?bado, 15 de outubro de 2016 6:00:18
>> Assunto: Re: [Buildroot] [PATCH 2/2 v2] support/download: be more conservative	about git special refs
> 
>> Ricardo, All,
>> 
>> Sorry for the delay...
>> 
>> On 2016-08-24 10:32 -0300, Ricardo Martincoski spake thusly:
[snip]
>>> Maybe we could translate the sha1 to a named ref when we assume we are about to
>>> ask for a special ref. Something like this...
>>> 
>>> if ! _git rev-parse -q --verify "'${cset}^{commit}'" >/dev/null; then
>>>     named_ref=$(_git ls-remote "'${repo}'" 2>&1 | grep "^${cset}" | cut -f2)
>>>     if [ -n "$named_ref" ]; then
>>>         _git fetch origin "'${named_ref}:${named_ref}'"
>>>     else
>>>         _git fetch origin "'${cset}:${cset}'"
>>>     fi
>>> fi
>>> 
>>> What do you think?
>> 
>> I don't use such "special refs", so I can't really state whether it is
>> good or not. If there was a publicy-reachable tree I could test against,
>> that would be nice. Do you know of such atree (even for a package not in
>> Buildroot)?
>> 
[snip]

Thank you for your response.

I see Henrique already answered your question.

> 
> I guess openswitch project have a lot of gerrit managed repositories. This can
> be used as examples and tests for Ricardo suggestion.
> 
> https://review.openswitch.net/#/admin/projects/
> 
> One particular sub-project
> 
> https://review.openswitch.net/#/admin/projects/openswitch/ops
> 
> git clone
> 
> git clone https://review.openswitch.net/openswitch/ops
> 
> And then it is possible to make a buildroot recipe to download and test this
> package with and without special_refs.
> 
> But it is important to say that we are using Ricardo suggestion in our
> buildroot-upstream cloned repository, that is based in 2016.08 tag, without
> any problems. Indeed, the solution has solved some problems, so i can say it
> has been exhaustively tested.
> 
> Our buildroot-upstream repo has this deliveries related to the subject, just
> for the sake of completeness:
> 
> git log -n8 --oneline
> 
> 6283402 support/download/git: allow sha1 of special refs (v2)
> 35b9416 Revert "support/download/git: allow sha1 of special refs"
> 81523bc support/download/git: allow sha1 of special refs
> e415a54 support/download/git: Fix compatibility issue with git older than 1.8.4
> 11e9a47 support/download: don't over-remove files from git archives
> cf98e0a support/download/git: add support for submodules
> a34d5e7 support/download/git: do not use git archive, handle it manually
> 5fc0731 support/download/git: do not use bare clones
> 
> The first three i think are in our tree only, and are the subject of this review.

Actually only the topmost of the 3 local patches matters. In the first version I
didn't cover a corner case, so I reverted it and then I integrated the corrected
version ;)
The second version uses awk, just like Bryce Ferguson does in his patch [1].

I can send the full corrected patch to the mailing list for review, but I will 
do it on a weekday, since I don't have the patch at hand right now.

[1] http://patchwork.ozlabs.org/patch/681841/

Regards,
Ricardo