[pacman-dev] [PATCH] makepkg: ensure vcs download tool are installed when required

[Allan McRae]

On 14/07/14 00:58, Andrew Gregory wrote:
> On 06/29/14 at 11:28pm, Allan McRae wrote:
>> Add an array VCSCLIENTS to makepkg.conf that matches vcs source protocols
>> to the package containing the software needed for handling the source.
>>
>> Signed-off-by: Allan McRae <allan at archlinux.org>
>> ---
>>  doc/makepkg.conf.5.txt |  5 ++++
>>  scripts/makepkg.sh.in  | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 73 insertions(+)
> 
> It's not totally clear to me what the intent of this patch is.  Is it
> to force PKGBUILD authors to include the vcs package in makedepends or
> for makepkg to automatically handle the package missing from
> makedepends?  If it's the former, the installation status of the vcs
> package shouldn't matter.  If it's the latter, it seems unnecessarily
> strict to require that VCSCLIENTS be configured and the package either
> be installed or listed as a dependency without ever just checking if
> the binary is already present.
> 

The idea is just to abort if the vcs tool is not available or going to
be installed before the source is downloaded.  You are correct that I
should explicitly check the binary too. I ignored that previously
because this is for a package manager so files should be in packages...
 I will submit an update.

>> diff --git a/doc/makepkg.conf.5.txt b/doc/makepkg.conf.5.txt
>> index b15f026..c589db6 100644
>> --- a/doc/makepkg.conf.5.txt
>> +++ b/doc/makepkg.conf.5.txt
>> @@ -46,6 +46,11 @@ Options
>>  	be replaced with the local file name, plus a ``.part'' extension, which allows
>>  	makepkg to handle resuming file downloads.
>>  
>> +**VCSCLIENTS=(**\'protocol::package' ...**)**::
>> +	Sets the packages required to fetch version controlled source files. When
>> +	required, makepkg will check that these packages are installed or are included
>> +	in the `depends` or `makedepends` arrays in the PKGBUILD.
>> +
> 
> Any reason to limit this to vcs clients?  A more generic binary ->
> package mapping could be useful for other things like the configurable
> download agents or ccache, upx, strip, etc, which are required by
> certain options.
> 

Specifying the VCS tool in makedepends is a really hangover from when
the source was downloaded within build().  But there is some
distinction.  VCS tools are not needed for every package, whereas the
other tools tend to be dealt with in global configuration files and so
required for everything.

I suppose his could be made more general, but I am really not that
interested in doing so as it would make the configuration file unwieldy.

>>  **CARCH=**"carch"::
>>  	Specifies your computer architecture; possible values include such things
>>  	as ``i686'', ``x86_64'', ``ppc'', etc. This should be automatically set on
>> diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
>> index 779ebcb..94dd98a 100644
>> --- a/scripts/makepkg.sh.in
>> +++ b/scripts/makepkg.sh.in
>> @@ -2402,6 +2402,69 @@ check_pkgver() {
>>  	return $ret
>>  }
>>  
>> +get_vcsclient() {
>> +	local proto=${1%%+*}
>> +
>> +	local i
>> +	for i in "${VCSCLIENT[@]}"; do
> 
> VCSCLIENT vs VCSCLIENTS in documentation.
> 

Thanks!   That reminds me I need to add it to the example makepkg.conf too.

>> +		local handler="${i%%::*}"
>> +		if [[ $proto = "$handler" ]]; then
>> +			local client="${i##*::}"
>> +			break
>> +		fi
>> +	done
>> +
>> +	# if we didn't find an client, return an error
>> +	if [[ -z $client ]]; then
>> +		error "$(gettext "Unknown download protocol: %s")" "$proto"
>> +		plain "$(gettext "Aborting...")"
>> +		exit 1 # $E_CONFIG_ERROR
>> +	fi
>> +
>> +	printf "%s\n" "$client"
>> +}
>> +
>> +check_vcs_software() {
>> +	local ret=0
>> +
>> +	if (( SOURCEONLY == 1 )); then
>> +		# we will not download VCS sources
>> +		return $ret
>> +	fi
>> +
>> +	if [[ -z $PACMAN_PATH ]]; then
>> +		warning "$(gettext "Cannot find the %s binary needed to check VCS source requirements.")" "$PACMAN"
>> +		return $ret
>> +	fi
>> +
>> +	for netfile in ${source[@]}; do
>> +		local proto=$(get_protocol "$netfile")
>> +
>> +		case $proto in
>> +			bzr*|git*|hg*|svn*)
>> +				local client
>> +				client=$(get_vcsclient "$proto") || exit $?
>> +				# ensure specified program is installed
>> +				local uninstalled
>> +				uninstalled="$(set +E; check_deps $client)" || exit 1
> 
> Neither this exit nor the one above appear to actually do anything.
> 

Well...  they essentially perform a return.  I copied this from
resolved_deps. It is does leave the function correctly when the VCS tool
is not installed.

@Dave: care to explain why exit is used instead of return in that function?

>> +				# if not installed, check presence in depends or makedepends
>> +				if [[ -n "$uninstalled" ]] && (( ! NODEPS )); then
>> +					if ! in_array "$client" "${depends[@]} ${makedepends}"; then
>> +						error "$(gettext "Cannot find the %s package needed to handle %s sources.")" \
>> +								"$client" "${proto%%+*}"
>> +						ret=1
>> +					fi
>> +				fi
>> +				;;
>> +			*)
>> +				# non VCS source
>> +				;;
>> +		esac
>> +	done
>> +
>> +	return $ret
>> +}
>> +
>>  check_software() {
>>  	# check for needed software
>>  	local ret=0
>> @@ -2493,6 +2556,11 @@ check_software() {
>>  		fi
>>  	fi
>>  
>> +	# tools to download vcs sources
>> +	if ! check_vcs_software; then
>> +		ret=1
>> +	fi
>> +
>>  	return $ret
>>  }
>>  
>> -- 
>> 2.0.1
> 
> 
>