Re: [gentoo-dev] [PATCH 2/8] Use bash redirection to run 'tee' rather than simple pipes.

[Alec Warner]

On Wed, Feb 27, 2013 at 1:43 PM, Michał Górny <mgorny@g.o> wrote:
> This allows us to spawn 'tee' as separate process while keeping
> the function code executed in the main shell.

Can you explain why this is 'better'? I'd prefer way more
documentation in the code itself. You are using a number of what I'd
consider 'banned' constructs and you don't go very far out of your way
to explain why you cannot live without them.

> ---
>  gx86/eclass/multibuild.eclass | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/gx86/eclass/multibuild.eclass b/gx86/eclass/multibuild.eclass
> index 716d34f..d42b8a7 100644
> --- a/gx86/eclass/multibuild.eclass
> +++ b/gx86/eclass/multibuild.eclass
> @@ -108,18 +108,20 @@ multibuild_foreach() {
>                 local MULTIBUILD_VARIANT=${v}
>                 local MULTIBUILD_ID=${prev_id}${v}
>                 local BUILD_DIR=${bdir%%/}-${v}
> +               local log_fd
>
> -               einfo "${v}: running ${@}" \
> -                       | tee -a "${T}/build-${MULTIBUILD_ID}.log"
> +               # redirect_alloc_fd accepts files only. so we need to open
> +               # a random file and then reuse the fd for logger process.
> +               redirect_alloc_fd log_fd /dev/null
> +               eval "exec ${log_fd}> "'>(exec tee -a "${T}/build-${MULTIBUILD_ID}.log")'
The quoting here is a nightmare.
Are the single quotes required?
Do the leading single quotes have to be *right next to* the double quotes?
Why is there a space between the first > and the "?
vim hi-lighting makes this more readable, but still weird looking.
Also please document clearly why 'eval' is required (it is not clear
to me..or I suspect most readers ;p)
Are we not just making an fd and pointing the 'read end' at a tee subprocess?

> +
> +               eval 'einfo "${v}: running ${@}" >&'${log_fd}' 2>&1'
And again here, why can't we just redirect directly to log_fd?

>
>                 # _multibuild_parallel() does redirection internally.
>                 # this is a hidden API to avoid writing multilib_foreach twice.
We do not use the _multibuild_parellel stuff anymore...is this comment relevant?

> -               if [[ ${1} == _multibuild_parallel ]]; then
> -                       "${@}"
> -               else
> -                       "${@}" 2>&1 | tee -a "${T}/build-${MULTIBUILD_ID}.log"
> -               fi
> +               eval '"${@}" >&'${log_fd}' 2>&1'
>                 lret=${?}
> +               eval "exec ${log_fd}>&-"
>         done
>         [[ ${ret} -eq 0 && ${lret} -ne 0 ]] && ret=${lret}
>
> @@ -145,8 +147,7 @@ multibuild_parallel_foreach() {
>         _multibuild_parallel() {
>                 (
>                         multijob_child_init
> -                       "${@}" 2>&1 | tee -a "${T}/build-${MULTIBUILD_ID}.log"
> -                       exit ${PIPESTATUS[0]}
> +                       "${@}"
>                 ) &
>                 multijob_post_fork
>         }
> --
> 1.8.1.4
>
>