[Buildroot] [PATCH 1/2] support/download: catch post-process errors

[Buildroot] [PATCH 1/2] support/download: catch post-process errors

[Yann E. MORIN]

Currently, when the post-process helper fails while downloading from
upstream, there is no fallback to the backup mirror.

In case the post-process helper fails, we mut consider that to be a
download failure, so we must bail out as if the download backend itself
did fail, but we fail to do so.

Duplicate the logic we have for the download helper: if the post-process
helper fails, remove the downloaded stuff, and continue on to the next
URI, which will ultimately hit the backup mirror (if one has been
configured).

Reported-by: Peter Korsgaard <peter@korsgaard.com>
Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 support/download/dl-wrapper | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/support/download/dl-wrapper b/support/download/dl-wrapper
index 09a6ac1f1a..1e8d6058f6 100755
--- a/support/download/dl-wrapper
+++ b/support/download/dl-wrapper
@@ -138,9 +138,15 @@ main() {
         fi
 
         if [ -n "${post_process}" ] ; then
-            "${OLDPWD}/support/download/${post_process}-post-process" \
-                -o "${tmpf}" \
-                -n "${raw_base_name}"
+            if ! "${OLDPWD}/support/download/${post_process}-post-process" \
+                    -o "${tmpf}" \
+                    -n "${raw_base_name}"
+            then
+                # cd back to keep path coherence
+                cd "${OLDPWD}"
+                rm -rf "${tmpd}"
+                continue
+            fi
         fi
 
         # cd back to free the temp-dir, so we can remove it later
-- 
2.25.1

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

[Buildroot] [PATCH 2/2] support/download: fix the cargo post-process in face of failed vendoring

[Yann E. MORIN]

In commit 04154a651729 (support/download/cargo-post-process: cargo
output for vendor config), we switched away from our hand-crafted
cargo.toml mangling, to use cargo itself to update that file.

In doing so, we enabled the shell pipefail option, so that we could
catch cargo failures, while redirecting its output through tee to the
cargo.toml.

However, pipefail is overzealous, and will hit us even for pipes we do
not want to globally fail, like the one that actually checks whether an
archive is already vendored or not:

    if tar tf "${output}" | grep -q "^[^/]*/VENDOR" ; then
        ...

with pipefail, the above may always fail:
  - if the tarball is already vendored, grep will exit on the first
    match because of -q (it only needs a single match to decide that its
    return code will be zero), so the | will get closed, and tar may
    get -EPIPE before it had a chance to finish listing the archive, and
    thus would terminate in error;
  - if the tarball is not vendored, grep will exit in error.

It turns out that the tee was only added so that we could see the
messages emitted by cargo, and still fill the cargo.tom with the output
of cargo.

But that's a bit overkill: the cargo messages are going to stderr, and
the blurb to add to cargo.toml to stdout, so we just need to redirect
stdout.

Yes, we do not see what cargo added to cargo.toml, but that is not so
interesting.

Still, cargo ends its messages with a suggestion for the user to modify
cargo.toml, with:

    To use vendored sources, add this to your .cargo/config.toml for this project:

But since we've already redirected that to cargo.toml, there is nothing
for the user to edit, so the above can get confusing. Emit a little
blurb that states that everything is under control.

And then we can drop pipefail.

Note: the go-post-process initially had pipefail too, but it was dropped
in bfd1a31d0e59 (support/download/go-post-process: drop -o pipefail) as
it was causing spurrious breakage when extracting the archive before
vendoring, so it is only reasonable that we also remove it from the
cargo-post-process.

Reported-by: Peter Korsgaard <peter@korsgaard.com>
Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Cc: Simon Richter <simon.richter@ptwdosimetry.com>
---
 support/download/cargo-post-process | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/support/download/cargo-post-process b/support/download/cargo-post-process
index aea2d8da7a..3f4666c006 100755
--- a/support/download/cargo-post-process
+++ b/support/download/cargo-post-process
@@ -1,7 +1,6 @@
 #!/usr/bin/env bash
 
 set -e
-set -o pipefail
 
 . "${0%/*}/helpers"
 
@@ -31,7 +30,13 @@ flock "${CARGO_HOME}"/.br-lock \
 cargo vendor \
     --manifest-path ${BR_CARGO_MANIFEST_PATH-Cargo.toml} \
     --locked VENDOR \
-    | tee .cargo/config
+    > .cargo/config
+
+# "cargo vendor' outputs on stderr a message directing to add some data
+# to the project's .cargo/config.toml, data that it outputs on stdout.
+# Since we redirect stdout to .cargo/config.toml, the message on stderr
+# gets confusing, so instruct the user that it's been handled.
+printf '(note: .cargo/config.toml automatically updated by Buildroot)\n\n'
 
 popd > /dev/null
 
-- 
2.25.1

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 2/2] support/download: fix the cargo post-process in face of failed vendoring

[Peter Korsgaard]

>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

 > In commit 04154a651729 (support/download/cargo-post-process: cargo
 > output for vendor config), we switched away from our hand-crafted
 > cargo.toml mangling, to use cargo itself to update that file.

 > In doing so, we enabled the shell pipefail option, so that we could
 > catch cargo failures, while redirecting its output through tee to the
 > cargo.toml.

 > However, pipefail is overzealous, and will hit us even for pipes we do
 > not want to globally fail, like the one that actually checks whether an
 > archive is already vendored or not:

 >     if tar tf "${output}" | grep -q "^[^/]*/VENDOR" ; then
 >         ...

 > with pipefail, the above may always fail:
 >   - if the tarball is already vendored, grep will exit on the first
 >     match because of -q (it only needs a single match to decide that its
 >     return code will be zero), so the | will get closed, and tar may
 >     get -EPIPE before it had a chance to finish listing the archive, and
 >     thus would terminate in error;
 >   - if the tarball is not vendored, grep will exit in error.

 > It turns out that the tee was only added so that we could see the
 > messages emitted by cargo, and still fill the cargo.tom with the output
 > of cargo.

 > But that's a bit overkill: the cargo messages are going to stderr, and
 > the blurb to add to cargo.toml to stdout, so we just need to redirect
 > stdout.

 > Yes, we do not see what cargo added to cargo.toml, but that is not so
 > interesting.

 > Still, cargo ends its messages with a suggestion for the user to modify
 > cargo.toml, with:

 >     To use vendored sources, add this to your .cargo/config.toml for this project:

 > But since we've already redirected that to cargo.toml, there is nothing
 > for the user to edit, so the above can get confusing. Emit a little
 > blurb that states that everything is under control.

 > And then we can drop pipefail.

 > Note: the go-post-process initially had pipefail too, but it was dropped
 > in bfd1a31d0e59 (support/download/go-post-process: drop -o pipefail) as
 > it was causing spurrious breakage when extracting the archive before
 > vendoring, so it is only reasonable that we also remove it from the
 > cargo-post-process.

 > Reported-by: Peter Korsgaard <peter@korsgaard.com>
 > Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
 > Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
 > Cc: Simon Richter <simon.richter@ptwdosimetry.com>

Committed, thanks.

-- 
Bye, Peter Korsgaard
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 1/2] support/download: catch post-process errors

[Peter Korsgaard]

>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

 > Currently, when the post-process helper fails while downloading from
 > upstream, there is no fallback to the backup mirror.

 > In case the post-process helper fails, we mut consider that to be a
 > download failure, so we must bail out as if the download backend itself
 > did fail, but we fail to do so.

 > Duplicate the logic we have for the download helper: if the post-process
 > helper fails, remove the downloaded stuff, and continue on to the next
 > URI, which will ultimately hit the backup mirror (if one has been
 > configured).

 > Reported-by: Peter Korsgaard <peter@korsgaard.com>
 > Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
 > Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

Committed after adding a fixes:
http://autobuild.buildroot.net/results/12a/12a63ae177fe3ed0c9a1ef2fa01870f334f36b0f/,
thanks!

> ---
 >  support/download/dl-wrapper | 12 +++++++++---
 >  1 file changed, 9 insertions(+), 3 deletions(-)

 > diff --git a/support/download/dl-wrapper b/support/download/dl-wrapper
 > index 09a6ac1f1a..1e8d6058f6 100755
 > --- a/support/download/dl-wrapper
 > +++ b/support/download/dl-wrapper
 > @@ -138,9 +138,15 @@ main() {
 >          fi
 
 >          if [ -n "${post_process}" ] ; then
 > -            "${OLDPWD}/support/download/${post_process}-post-process" \
 > -                -o "${tmpf}" \
 > -                -n "${raw_base_name}"
 > +            if ! "${OLDPWD}/support/download/${post_process}-post-process" \
 > +                    -o "${tmpf}" \
 > +                    -n "${raw_base_name}"
 > +            then
 > +                # cd back to keep path coherence
 > +                cd "${OLDPWD}"
 > +                rm -rf "${tmpd}"
 > +                continue
 > +            fi
 >          fi
 
 >          # cd back to free the temp-dir, so we can remove it later
 > -- 

 > 2.25.1


-- 
Bye, Peter Korsgaard
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 1/2] support/download: catch post-process errors

[Peter Korsgaard]

>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

 > Currently, when the post-process helper fails while downloading from
 > upstream, there is no fallback to the backup mirror.

 > In case the post-process helper fails, we mut consider that to be a
 > download failure, so we must bail out as if the download backend itself
 > did fail, but we fail to do so.

 > Duplicate the logic we have for the download helper: if the post-process
 > helper fails, remove the downloaded stuff, and continue on to the next
 > URI, which will ultimately hit the backup mirror (if one has been
 > configured).

 > Reported-by: Peter Korsgaard <peter@korsgaard.com>
 > Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
 > Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

Committed to 2022.11.x and 2022.02.x, thanks.

-- 
Bye, Peter Korsgaard
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot