[pacman-dev] [PATCH] makepkg: use tput for terminal-safe colored and bold text

[Allan McRae]

Cedric Staniewski wrote:
> Allan McRae wrote:
>   
>> Dan McGee wrote:
>>     
>>> On Thu, Oct 22, 2009 at 7:41 PM, Allan McRae <allan at archlinux.org> wrote:
>>>  
>>>       
>>>> Cedric Staniewski wrote:
>>>>    
>>>>         
>>>>> Suggested-by: Dan McGee <dan at archlinux.org>
>>>>> Signed-off-by: Cedric Staniewski <cedric at gmx.ca>
>>>>> ---
>>>>>
>>>>> I do not know if this patch is usefull at all, because I do not
>>>>> notice any
>>>>> change. Just grabbed this from Dan's TODO list and wanted to play a bit
>>>>> with tput.
>>>>>
>>>>>       
>>>>>           
>>>> It looks fine to me...
>>>> I just have no idea on the advantage we are achieving with this
>>>> change apart
>>>> from the apparent terminal safeness.  Was the old version not safe?
>>>>
>>>> Dan: comments?
>>>>     
>>>>         
>>> First comment- that TODO list is still huge, wow. :)
>>>       
>
> Hehe. :)
>
>
>   
>>> Anyway, this seems pretty reasonable to me, but not sure it is worth
>>> it. At the least, we should capture these sequences once on script
>>> startup, and then use the global variable in each function. And does
>>> $(tput offbold) make more sense for the reset?
>>>   
>>>       
>> There does not appear to be such a thing as "tput offbold".
>>
>> Anyway, I like the idea of setting these all at the start.
>>
>> if [ $COLORMSG -eq 1 ]; then
>>  BOLD_ON=$(tput bold)
>>  BOLD_OFF=${tput srg0)
>>  GREEN_ON...
>>  ...
>> fi
>>
>> then the messages can just be
>>
>> plain() {
>>     local mesg=$1; shift
>>         printf "${BOLD_ON}    ${mesg}${BOLD_OFF}\n" "$@" >&2
>>     fi
>> }
>>
>>
>> with none of the tests needed.
>>
>> Allan
>>     
>
> That definitely makes sense.
> If you think the attached patch is fine, I plan to change the order of these two ones and resubmit them.
>
>
> >From 50901c551ddefb9a5eca500ac63ab0a0c7eef167 Mon Sep 17 00:00:00 2001
> From: Cedric Staniewski <cedric at gmx.ca>
> Date: Fri, 23 Oct 2009 12:27:54 +0200
> -- 8< --
> Subject: [PATCH] makepkg: define the required escape sequences globally
>
> In doing so, it is possible to get rid of all the tests for colored
> messages except for one global one.
>
> Signed-off-by: Cedric Staniewski <cedric at gmx.ca>
> ---
>  scripts/makepkg.sh.in |   42 +++++++++++++++---------------------------
>  1 files changed, 15 insertions(+), 27 deletions(-)
>
> diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
> index a64a867..b89acdb 100644
> --- a/scripts/makepkg.sh.in
> +++ b/scripts/makepkg.sh.in
> @@ -72,7 +72,6 @@ IGNOREARCH=0
>  HOLDVER=0
>  PKGFUNC=0
>  SPLITPKG=0
> -COLORMSG=0
>  
>  # Forces the pkgver of the current PKGBUILD. Used by the fakeroot call
>  # when dealing with svn/cvs/etc PKGBUILDs.
> @@ -84,47 +83,27 @@ PACMAN_OPTS=
>  
>  plain() {
>  	local mesg=$1; shift
> -	if [ $COLORMSG -eq 1 ]; then
> -		printf "$(tput bold)    ${mesg}$(tput sgr0)\n" "$@" >&2
> -	else
> -		printf "    ${mesg}\n" "$@" >&2
> -	fi
> +	printf "${BOLD}    ${mesg}${ALL_OFF}\n" "$@" >&2
>  }
>  
>  msg() {
>  	local mesg=$1; shift
> -	if [ $COLORMSG -eq 1 ]; then
> -		printf "$(echo -e "bold\nsetaf 2" | tput -S)==>$(echo -e "sgr0\nbold" | tput -S) ${mesg}$(tput sgr0)\n" "$@" >&2
> -	else
> -		printf "==> ${mesg}\n" "$@" >&2
> -	fi
> +	printf "${GREEN}==>${ALL_OFF_BOLD} ${mesg}${ALL_OFF}\n" "$@" >&2
>  }
>  
>  msg2() {
>  	local mesg=$1; shift
> -	if [ $COLORMSG -eq 1 ]; then
> -		printf "$(echo -e "bold\nsetaf 4" | tput -S)  ->$(echo -e "sgr0\nbold" | tput -S) ${mesg}$(tput sgr0)\n" "$@" >&2
> -	else
> -		printf "  -> ${mesg}\n" "$@" >&2
> -	fi
> +	printf "${BLUE}  ->${ALL_OFF_BOLD} ${mesg}${ALL_OFF}\n" "$@" >&2
>  }
>  
>  warning() {
>  	local mesg=$1; shift
> -	if [ $COLORMSG -eq 1 ]; then
> -		printf "$(echo -e "bold\nsetaf 3" | tput -S)==> $(gettext "WARNING:")$(echo -e "sgr0\nbold" | tput -S) ${mesg}$(tput sgr0)\n" "$@" >&2
> -	else
> -		printf "==> $(gettext "WARNING:") ${mesg}\n" "$@" >&2
> -	fi
> +	printf "${YELLOW}==> $(gettext "WARNING:")${ALL_OFF_BOLD} ${mesg}${ALL_OFF}\n" "$@" >&2
>  }
>  
>  error() {
>  	local mesg=$1; shift
> -	if [ $COLORMSG -eq 1 ]; then
> -		printf "$(echo -e "bold\nsetaf 1" | tput -S)==> $(gettext "ERROR:")$(echo -e "sgr0\nbold" | tput -S) ${mesg}$(tput sgr0)\n" "$@" >&2
> -	else
> -		printf "==> $(gettext "ERROR:") ${mesg}\n" "$@" >&2
> -	fi
> +	printf "${RED}==> $(gettext "ERROR:")${ALL_OFF_BOLD} ${mesg}${ALL_OFF}\n" "$@" >&2
>  }
>  
>  
> @@ -1576,7 +1555,16 @@ fi
>  
>  # check if messages are to be printed using color
>  if [ -t 2 -a ! "$USE_COLOR" = "n" -a "$(check_buildenv color)" = "y" ]; then
> -	COLORMSG=1
> +	readonly ALL_OFF="$(tput sgr0)" \
> +						BOLD="$(tput bold)"
> +	readonly ALL_OFF_BOLD="${ALL_OFF}${BOLD}" \
> +						RED="${BOLD}$(tput setaf 1)" \
> +						BLUE="${BOLD}$(tput setaf 4)" \
> +						GREEN="${BOLD}$(tput setaf 2)" \
> +						YELLOW="${BOLD}$(tput setaf 3)"
> +else
> +	unset ALL_OFF ALL_OFF_BOLD BOLD RED BLUE GREEN YELLOW
> +	readonly ALL_OFF ALL_OFF_BOLD BOLD RED BLUE GREEN YELLOW
>  fi
>  
>  # override settings with an environment variable for batch processing
>   

That looks good to me.  Feel free to tidy it as you see fit and forward 
the patch.

Allan