[pacman-dev] [PATCH] makepkg: refactor absolute filename detection

[Allan McRae]

On 28/05/10 00:35, Cedric Staniewski wrote:
> Move the absolute filename detection to a new function to reduce code
> duplication.
>
> This patch also fixes the --allsource option that did not include remote
> source files if they reside in $startdir instead of $SRCDEST.
>
> Signed-off-by: Cedric Staniewski<cedric at gmx.ca>
> ---
>
> Changes:
> - renamed error_source_file_not_found to missing_source_exit
> - removed differentiation in download_sources: now it prints "Found %s" instead
>    of "Found %s in build dir" and "Using cached copy of %s"
>

I have very minor comments below.  Suggested naming comments are 
debatable...


> scripts/makepkg.sh.in |   99 ++++++++++++++++++++++++-------------------------
>   1 files changed, 48 insertions(+), 51 deletions(-)
>
> diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
> index 8c0da8b..00167f7 100644
> --- a/scripts/makepkg.sh.in
> +++ b/scripts/makepkg.sh.in
> @@ -179,6 +179,33 @@ trap 'trap_exit "$(gettext "An unknown error has occurred. Exiting...")"' ERR
>   # 1) "filename::http://path/to/file"
>   # 2) "http://path/to/file"
>
> +# Return the absolute filename of a source entry
> +#
> +# This function accepts a source entry or the already extracted filename of a
> +# source entry as input
> +get_absolute_filename() {

get_filepath()?

> +	local file="$(get_filename "$1")"
> +
> +	if [[ -f "$startdir/$file" ]] ; then
> +		file="$startdir/$file"
> +	else

I'd prefer and "elif" here as it seems cleaner.

> +		if [[ -f "$SRCDEST/$file" ]] ; then
> +			file="$SRCDEST/$file"
> +		else
> +			return 1
> +		fi
> +	fi
> +
> +	echo "$file"
> +}
> +
> +# Print 'source not found' error message and exit makepkg
> +missing_source_exit() {

I do not really think a three line function needs documented.  Also, I 
think "missing_source_file" sounds a better name especially when you 
read it after a "||" as it is used.

> +	error "$(gettext "Unable to find source file %s.")" "$(get_filename "$1")"
> +	plain "$(gettext "Aborting...")"
> +	exit 1 # $E_MISSING_FILE
> +}
> +
>   # extract the filename from a source entry
>   get_filename() {
>   	# if a filename is specified, use it

<snip>

> @@ -645,25 +653,17 @@ extract_sources() {
>   	msg "$(gettext "Extracting Sources...")"
>   	local netfile
>   	for netfile in "${source[@]}"; do
> -		file=$(get_filename "$netfile")
> +		absfile="$(get_absolute_filename "$netfile")" || missing_source_exit "$netfile"
> +		file=${absfile##*/}
>   		if in_array "$file" ${noextract[@]}; then
>   			#skip source files in the noextract=() array
>   			#  these are marked explicitly to NOT be extracted
>   			continue
>   		fi
>
> -		if [[ ! -f $file ]] ; then
> -			if [[ ! -f $SRCDEST/$file ]] ; then
> -				error "$(gettext "Unable to find source file %s for extraction.")" "$file"
> -				plain "$(gettext "Aborting...")"
> -				exit 1
> -			else
> -				file="$SRCDEST/$file"
> -			fi
> -		fi
>
>   		# fix flyspray #6246
> -		local file_type=$(file -bizL "$file")
> +		local file_type=$(file -bizL "$absfile")

I do not think that this change is not needed.

>   		local ext=${file##*.}
>   		local cmd=''
>   		case "$file_type" in
> @@ -693,10 +693,10 @@ extract_sources() {
>   		local ret=0
>   		msg2 "$(gettext "Extracting %s with %s")" "$file" "$cmd"
>   		if [[ $cmd = bsdtar ]]; then
> -			$cmd -xf "$file" || ret=$?
> +			$cmd -xf "$absfile" || ret=$?
>   		else
>   			rm -f "${file%.*}"
> -			$cmd -dcf "$file">  "${file%.*}" || ret=$?
> +			$cmd -dcf "$absfile">  "${file%.*}" || ret=$?

or these two.  We have set up symlinks in $srcdir so we should just use them

>   		fi
>   		if (( ret )); then
>   			error "$(gettext "Failed to extract %s")" "$file"
> @@ -1097,13 +1097,10 @@ create_srcpackage() {
>
>   	local netfile
>   	for netfile in "${source[@]}"; do
> -		local file=$(get_filename "$netfile")
> -		if [[ -f $netfile ]]; then
> -			msg2 "$(gettext "Adding %s...")" "$netfile"
> -			ln -s "${startdir}/$netfile" "${srclinks}/${pkgbase}"
> -		elif (( SOURCEONLY == 2 ))&&  [[ -f $SRCDEST/$file ]]; then
> -			msg2 "$(gettext "Adding %s...")" "$file"
> -			ln -s "$SRCDEST/$file" "${srclinks}/${pkgbase}/"
> +		if [[ -f $netfile ]] || (( SOURCEONLY == 2 )); then
> +			local file=$(get_absolute_filename "$netfile") || missing_source_exit "$netfile"
> +			msg2 "$(gettext "Adding %s...")" "${file##*/}"
> +			ln -s "$file" "${srclinks}/${pkgbase}"
>   		fi
>   	done
>

While this works, I would prefer to keep the distinction between local 
and downloaded files.   So something like:

   local file
   for file in "${source[@]}"; do
     if [[ -f $file ]]; then
       msg2 "$(gettext "Adding %s...")" "$file"
       ln -s "${startdir}/$file" "${srclinks}/${pkgbase}"
     elif (( SOURCEONLY == 2 )); then
       local absfile=$(get_absolute_filename "$file") || 
missing_source_exit "$file"
       msg2 "$(gettext "Adding %s...")" "${absfile##*/}"
       ln -s "$absfile" "${srclinks}/${pkgbase}"
     fi
   done

Note the renaming of "netfile" as the name makes little sense anymore...

With those comments addressed, this patch is good to go.

I would like this in the 3.4 release if possible since it is a bug fix. 
  Given it has string changes, that would mean it would need resubmitted 
soon.  I apologize for rushing you after taking so long to comment, but 
I can take care of the changes if you are busy at the moment.

Allan