[prev in list] [next in list] [prev in thread] [next in thread] 

List:       gentoo-dev
Subject:    Re: [gentoo-dev] [PATCH 02/12] python-utils-r1.eclass: New python_fix_shebang approach
From:       James Le Cuirot <chewi () gentoo ! org>
Date:       2019-01-06 17:20:59
Message-ID: 20190106172059.5d57ffb9 () symphony ! aura-online ! co ! uk
[Download RAW message or body]


On Fri, 04 Jan 2019 07:02:40 +0100
Michał Górny <mgorny@gentoo.org> wrote:

> 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@gentoo.org>
> > ---
> >  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?

Seemingly only in an interactive shell.

> >  
> >  				# 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

Interesting, didn't know about that.

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

...or that.

> 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.

It will still hit the error clause below regardless. The debug-print
lines were refactored from further up and they were printed even in
error cases. We could skip the split_shebang lines but it would just
make the code longer.

> > +				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.

Okay.
 
> > +				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.
> 


-- 
James Le Cuirot (chewi)
Gentoo Linux Developer

[Attachment #3 (application/pgp-signature)]

[prev in list] [next in list] [prev in thread] [next in thread]