[pacman-dev] [PATCH] makepkg: rework libdepends

[Dave Reisner]

On Sat, May 19, 2012 at 11:22:36PM +1000, Allan McRae wrote:
> Rewrite the handling of libdepends. The primary advantage are:
>  - Moves functionality from write_pkginfo() to find_libdepends().
>  - The order of the depends array in the PKGBUILD is kept in the package.
>  - An unneeded libdepends is only a warning and not an error. This allows
>    putting a libdepend on a library that is dlopened.
>  - It is now modular so can be extended to library types other than
>    ELF *.so.
>  - Finding the list of libraries a package depends only occurs when a
>    libdepend is specified in the depends array.
> 
> Signed-off-by: Allan McRae <allan at archlinux.org>
> ---

Without having tested this too thoroughly, this gets a +1 from me.
There's two small nitpicks left inline.

>  scripts/makepkg.sh.in |  126 +++++++++++++++++++++++++++++--------------------
>  1 file changed, 76 insertions(+), 50 deletions(-)
> 
> diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
> index 8d018c0..8930ec4 100644
> --- a/scripts/makepkg.sh.in
> +++ b/scripts/makepkg.sh.in
> @@ -1131,32 +1131,76 @@ tidy_install() {
>  }
>  
>  find_libdepends() {
> -	local libdepends
> -	find "$pkgdir" -type f -perm -u+x | while read filename
> -	do
> -		# get architecture of the file; if soarch is empty it's not an ELF binary
> -		soarch=$(LC_ALL=C readelf -h "$filename" 2>/dev/null | sed -n 's/.*Class.*ELF\(32\|64\)/\1/p')
> -		[ -n "$soarch" ] || continue
> -		# process all libraries needed by the binary
> -		for sofile in $(LC_ALL=C readelf -d "$filename" 2>/dev/null | sed -nr 's/.*Shared library: \[(.*)\].*/\1/p')
> -		do
> -			# extract the library name: libfoo.so
> -			soname="${sofile%.so?(+(.+([0-9])))}".so
> -			# extract the major version: 1
> -			soversion="${sofile##*\.so\.}"
> -			if in_array "${soname}" ${depends[@]}; then
> -				if ! in_array "${soname}=${soversion}-${soarch}" ${libdepends[@]}; then
> -					# libfoo.so=1-64
> -					printf "%s" "${soname}=${soversion}-${soarch}"
> -					libdepends+=("${soname}=${soversion}-${soarch}")
> +	local d sodepends;
> +
> +	sodepends=0;
> +	for d in "${depends[@]}"; do
> +		if [[ $d = *.so ]]; then
> +			sodepends=1;
> +			break;
> +		fi
> +	done
> +
> +	if (( sodepends == 0 )); then
> +		printf '%s\n' "${depends[@]}"
> +		return;
> +	fi
> +
> +	local libdeps;
> +	declare -A libdeps;
> +
> +	if (( sodepends == 1 )); then

You've already checked that $sodepends isn't 0. Can't you save a whole
level of indenting here?

> +		local filename soarch sofile soname soversion
> +
> +		while read filename; do
> +			# get architecture of the file; if soarch is empty it's not an ELF binary
> +			soarch=$(LC_ALL=C readelf -h "$filename" 2>/dev/null | sed -n 's/.*Class.*ELF\(32\|64\)/\1/p')
> +			[[ -n "$soarch" ]] || continue
> +
> +			# process all libraries needed by the binary
> +			for sofile in $(LC_ALL=C readelf -d "$filename" 2>/dev/null | sed -nr 's/.*Shared library: \[(.*)\].*/\1/p')
> +			do
> +				# extract the library name: libfoo.so
> +				soname="${sofile%.so?(+(.+([0-9])))}".so
> +				# extract the major version: 1
> +				soversion="${sofile##*\.so\.}"
> +
> +				if [[ ${libdeps[$soname]} ]]; then
> +					if [[ ! ${libdeps[$soname]} = *${soversion}-${soarch}* ]]; then

nitpick: "$foo != $bar" instead of "! $foo = $bar"

> +						libdeps[$soname]+=" ${soversion}-${soarch}"
> +					fi
> +				else
> +					libdeps[$soname]="${soversion}-${soarch}"
>  				fi
> -			fi
> -		done
> +			done
> +		done < <(find "$pkgdir" -type f -perm -u+x)
> +	fi
> +
> +	local libdepends v
> +	for d in "${depends[@]}"; do
> +		case "$d" in
> +			*.so)
> +				if [[ ${libdeps[$d]} ]]; then
> +					for v in ${libdeps[$d]}; do
> +						libdepends+=("$d=$v")
> +					done
> +				else
> +					warning "$(gettext "Library listed in %s is not required by any files: %s")" "'depends'" "$d"
> +					libdepends+=("$d")
> +				fi
> +				;;
> +			*)
> +				libdepends+=("$d")
> +				;;
> +		esac
>  	done
> +
> +	printf '%s\n' "${libdepends[@]}"
>  }
>  
> +
>  find_libprovides() {
> -	local libprovides missing
> +	local p libprovides missing
>  	for p in "${provides[@]}"; do
>  		missing=0
>  		case "$p" in
> @@ -1246,38 +1290,20 @@ write_pkginfo() {
>  	printf "size = %s\n" "$size"
>  	printf "arch = %s\n" "$pkgarch"
>  
> -	[[ $license ]]    && printf "license = %s\n"   "${license[@]}"
> -	[[ $replaces ]]   && printf "replaces = %s\n"  "${replaces[@]}"
> -	[[ $groups ]]     && printf "group = %s\n"     "${groups[@]}"
> -	[[ $optdepends ]] && printf "optdepend = %s\n" "${optdepends[@]//+([[:space:]])/ }"
> -	[[ $conflicts ]]  && printf "conflict = %s\n"  "${conflicts[@]}"
> -
>  	mapfile -t provides < <(find_libprovides)
> -	[[ $provides ]]   && printf "provides = %s\n"  "${provides[@]}"
> -
> -	[[ $backup ]]     && printf "backup = %s\n"    "${backup[@]}"
> -
> +	mapfile -t depends < <(find_libdepends)
> +
> +	[[ $license ]]     && printf "license = %s\n"    "${license[@]}"
> +	[[ $replaces ]]    && printf "replaces = %s\n"   "${replaces[@]}"
> +	[[ $groups ]]      && printf "group = %s\n"      "${groups[@]}"
> +	[[ $conflicts ]]   && printf "conflict = %s\n"   "${conflicts[@]}"
> +	[[ $provides ]]    && printf "provides = %s\n"   "${provides[@]}"
> +	[[ $backup ]]      && printf "backup = %s\n"     "${backup[@]}"
> +	[[ $depends ]]     && printf "depend = %s\n"     "${depends[@]}"
> +	[[ $optdepends ]]  && printf "optdepend = %s\n"  "${optdepends[@]//+([[:space:]])/ }"
> +	[[ $makedepends ]] && printf "makedepend = %s\n" "${makedepends[@]}"
>  
>  	local it
> -	mapfile -t libdepends < <(find_libdepends)
> -	depends+=("${libdepends[@]}")
> -
> -	for it in "${depends[@]}"; do
> -		if [[ $it = *.so ]]; then
> -			# check if the entry has been found by find_libdepends
> -			# if not, it's unneeded; tell the user so he can remove it
> -			printf -v re '(^|\s)%s=.*' "$it"
> -			if [[ ! $libdepends =~ $re ]]; then
> -				error "$(gettext "Cannot find library listed in %s: %s")" "'depends'" "$it"
> -				return 1
> -			fi
> -		else
> -			printf "depend = %s\n" "$it"
> -		fi
> -	done
> -
> -	[[ $makedepends ]]  && printf "makedepend = %s\n"  "${makedepends[@]}"
> -
>  	for it in "${packaging_options[@]}"; do
>  		check_option "$it" "y"
>  		case $? in
> -- 
> 1.7.10.2
> 
>