On Wed, Feb 27, 2013 at 1:43 PM, Michał Górny <mgo...@gentoo.org> 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
>
>
