Re: [gentoo-dev] [PATCH 02/12] python-utils-r1.eclass: New python_fix_shebang approach

[Michał Górny]

On Thu, 2019-01-03 at 21:39 +0000, James Le Cuirot wrote:
> The previous approach would erroneously match foopython. The new
> approach requires the match to start the string or be preceeded by a
> slash, the only two cases we actually want. It does this with slightly
> less code and allows the replacement of whole path strings that would
> be problematic when passed to sed. This will be needed when
> cross-compiling is addressed.
>
> Signed-off-by: James Le Cuirot <chewi@g.o>
> ---
>  eclass/python-utils-r1.eclass | 78 ++++++++++++++---------------------
>  1 file changed, 31 insertions(+), 47 deletions(-)
>
> diff --git a/eclass/python-utils-r1.eclass b/eclass/python-utils-r1.eclass
> index da76a755fb34..1eca0764a202 100644
> --- a/eclass/python-utils-r1.eclass
> +++ b/eclass/python-utils-r1.eclass
> @@ -1165,8 +1165,7 @@ python_fix_shebang() {
>  		[[ -d ${path} ]] && is_recursive=1
>
>  		while IFS= read -r -d '' f; do
> -			local shebang i
> -			local error= from=
> +			local shebang i= error= fix=
>
>  			# note: we can't ||die here since read will fail if file
>  			# has no newline characters
> @@ -1175,65 +1174,56 @@ python_fix_shebang() {
>  			# First, check if it's shebang at all...
>  			if [[ ${shebang} == '#!'* ]]; then
>  				local split_shebang=()
> -				read -r -a split_shebang <<<${shebang} || die
> +				read -r -a split_shebang <<<${shebang#\#\!} || die

Does '!' really need to be escaped?

>
>  				# Match left-to-right in a loop, to avoid matching random
>  				# repetitions like 'python2.7 python2'.
> -				for i in "${split_shebang[@]}"; do
> -					case "${i}" in
> -						*"${EPYTHON}")
> +				for i in $(seq 0 $((${#split_shebang[@]} - 1))); do

for i in "${!split_shebang[@]}"; do

> +					case "/${split_shebang[${i}]}" in

case "/${split_shebang[i]}" in

Also below.

> +						*/${EPYTHON})
>  							debug-print "${FUNCNAME}: in file ${f#${D%/}}"
>  							debug-print "${FUNCNAME}: shebang matches EPYTHON: ${shebang}"
>
>  							# Nothing to do, move along.
>  							any_correct=1
> -							from=${EPYTHON}
> +							continue 2
> +							;;
> +						*/python)
> +							fix=1
>  							break
>  							;;
> -						*python|*python[23])
> -							debug-print "${FUNCNAME}: in file ${f#${D%/}}"
> -							debug-print "${FUNCNAME}: rewriting shebang: ${shebang}"
> -
> -							if [[ ${i} == *python2 ]]; then
> -								from=python2
> -								if [[ ! ${force} ]]; then
> -									python_is_python3 "${EPYTHON}" && error=1
> -								fi
> -							elif [[ ${i} == *python3 ]]; then
> -								from=python3
> -								if [[ ! ${force} ]]; then
> -									python_is_python3 "${EPYTHON}" || error=1
> -								fi
> -							else
> -								from=python
> +						*/python2)
> +							if [[ ! ${force} ]]; then
> +								python_is_python3 "${EPYTHON}" && error=1
>  							fi
> +							fix=1
>  							break
>  							;;
> -						*python[23].[0123456789]|*pypy|*pypy3|*jython[23].[0123456789])
> -							# Explicit mismatch.
> +						*/python3)
>  							if [[ ! ${force} ]]; then
> -								error=1
> -							else
> -								case "${i}" in
> -									*python[23].[0123456789])
> -										from="python[23].[0123456789]";;
> -									*pypy)
> -										from="pypy";;
> -									*pypy3)
> -										from="pypy3";;
> -									*jython[23].[0123456789])
> -										from="jython[23].[0123456789]";;
> -									*)
> -										die "${FUNCNAME}: internal error in 2nd pattern match";;
> -								esac
> +								python_is_python3 "${EPYTHON}" || error=1
>  							fi
> +							fix=1
> +							break
> +							;;
> +						*/python[2-3].[0-9]|*/pypy|*/pypy3|*/jython[2-3].[0-9])
> +							# Explicit mismatch.
> +							[[ ! ${force} ]] && error=1
> +							fix=1
>  							break
>  							;;
>  					esac
>  				done
>  			fi
>
> -			if [[ ! ${error} && ! ${from} ]]; then
> +			if [[ ${fix} ]]; then

Doesn't this mean errors from above code will be ignored?  Diff makes it
really hard to follow the logic here but I'm pretty sure you set both
error=1 fix=1, then check for fix first.

> +				debug-print "${FUNCNAME}: in file ${f#${D%/}}"
> +				debug-print "${FUNCNAME}: rewriting shebang: ${shebang}"
> +
> +				split_shebang[${i}]=/${split_shebang[${i}]}

A comment above this hack would be helpful.

> +				split_shebang[${i}]=${split_shebang[${i}]%/*}
> +				split_shebang[${i}]=${split_shebang[${i}]#/}${split_shebang[${i}]:+/}${EPYTHON}
> +			elif [[ ! ${error} ]]; then
>  				# Non-Python shebang. Allowed in recursive mode,
>  				# disallowed when specifying file explicitly.
>  				[[ ${is_recursive} ]] && continue
> @@ -1245,13 +1235,7 @@ python_fix_shebang() {
>  			fi
>
>  			if [[ ! ${error} ]]; then
> -				# We either want to match ${from} followed by space
> -				# or at end-of-string.
> -				if [[ ${shebang} == *${from}" "* ]]; then
> -					sed -i -e "1s:${from} :${EPYTHON} :" "${f}" || die
> -				else
> -					sed -i -e "1s:${from}$:${EPYTHON}:" "${f}" || die
> -				fi
> +				sed -i -e "1c#\!${split_shebang[*]}" "${f}" || die
>  				any_fixed=1
>  			else
>  				eerror "The file has incompatible shebang:"

I'll get to other patches later.

-- 
Best regards,
Michał Górny