[pacman-dev] [PATCH] makepkg: Progress bar at package compression stage

[Dave Reisner]

On Sat, Jun 07, 2014 at 11:25:53PM +0400, Yanus Poluektovich wrote:
> If pv is installed, use it to provide progress indication for package compression stage.

So, I like this idea, but there's some issues I'd like to raise with
your approach, mostly stylistic:

> ---
>  scripts/makepkg.sh.in | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
> index 96e5349..0bb619f 100644
> --- a/scripts/makepkg.sh.in
> +++ b/scripts/makepkg.sh.in
> @@ -2006,10 +2006,21 @@ create_package() {
>  	msg2 "$(gettext "Compressing package...")"
>  	# TODO: Maybe this can be set globally for robustness
>  	shopt -s -o pipefail
> +
> +	comp_files+=( $(ls) )

You should be using globbing here -- exactly what you took away later on
from the bsdtar call. But, I'd go one step further and just add this to
the original declaration of comp_files.

> +	local pv_uncompressed="cat"
> +	local pv_compressed="cat"
> +	if [[ -f "/usr/bin/pv" ]]; then

You don't need to assume pv is in /usr/bin. To check existence, use:

  if type -P pv >/dev/null; then

> +		local uncompressed_size=$( du -sb "${comp_files[@]}" | awk 'BEGIN {s=0} {s += $1} END {print s}' )

Other usage of du in this script are necessarily done via a preprocessor
replacement, i.e. @DUPATH@ @DUFLAGS@

You'll need to follow this. There's also no need for your BEGIN block --
variables are implicitly defined in awk.

> +		pv_uncompressed="/usr/bin/pv -c -N uncompressed -perab -s $uncompressed_size"
> +		pv_compressed="/usr/bin/pv -c -N compressed -trab"

Commands should always be declared in an array, not as a simple string.
I'd opt for slightly different variable names, too:

  if type -P pv >/dev/null; then
    uncompressed_pipe=(pv -c -N uncompressed -perab -s "$uncompressed_size")
    compressed_pipe=(pv -c -N compressed -trab)
  else
    uncompressed_pipe=(cat)
    compressed_pipe=(cat)
  fi

> +	fi
> +
>  	# bsdtar's gzip compression always saves the time stamp, making one
>  	# archive created using the same command line distinct from another.
>  	# Disable bsdtar compression and use gzip -n for now.
> -	bsdtar -cf - "${comp_files[@]}" * |
> +	bsdtar -cf - "${comp_files[@]}" |
>
> +	$pv_uncompressed |

This would be "${uncompressed_pipe[@]}"

>  	case "$PKGEXT" in
>  		*tar.gz)  ${COMPRESSGZ[@]:-gzip -c -f -n} ;;
>  		*tar.bz2) ${COMPRESSBZ2[@]:-bzip2 -c -f} ;;
> @@ -2020,7 +2031,7 @@ create_package() {
>  		*tar)     cat ;;
>  		*) warning "$(gettext "'%s' is not a valid archive extension.")" \
>  			"$PKGEXT"; cat ;;
> -	esac > "${pkg_file}" || ret=$?
> +	esac | $pv_compressed > "${pkg_file}" || ret=$?

This would be "${compressed_pipe[@]}"

>  
>  	shopt -u nullglob
>  	shopt -u -o pipefail
> -- 
> 2.0.0
> 
>