[pacman-dev] [PATCH v2] makepkg-template: support multiple --template-dirs

[Florian Pritz]

On 23.04.2015 13:59, Dominik Fischer wrote:
> diff --git a/scripts/makepkg-template.pl.in b/scripts/makepkg-template.pl.in
> index 71d2aae..49e0e37 100755
> --- a/scripts/makepkg-template.pl.in
> +++ b/scripts/makepkg-template.pl.in
> @@ -98,26 +98,32 @@ sub load_template {
>  
>  	my $ret = "";
>  
> -	my $path;
> +	my $template_name = "$values->{name}";
>  	if (!$opts{newest} and $values->{version}) {
> -		$path = "$opts{template_dir}/$values->{name}-$values->{version}.template";
> -	} else {
> -		$path = "$opts{template_dir}/$values->{name}.template";
> +		$template_name .= "-$values->{version}";
>  	}
> +	$template_name .= ".template";
>  
> -	# resolve symlink(s) and use the real file's name for version detection
> -	my ($version) = (abs_path($path) =~ /-([0-9.]+)[.]template$/);
> -
> -	if (!$version) {
> -		die sprintf(gettext("Couldn't detect version for template '%s'\n"), $values->{name});
> -	}
> +	my $path;

Please declare $path inside the loop.

> +	foreach my $dir (reverse @{$opts{template_dir}}) {
> +		$path = "$dir/$template_name";
> +		if ( -e $path ) {
> +			# resolve symlink(s) and use the real file's name for version detection
> +			my ($version) = (abs_path($path) =~ /-([0-9.]+)[.]template$/);
> +
> +			if (!$version) {
> +				die sprintf(gettext("Couldn't detect version for template '%s'\n"), $values->{name});

Since we now have multiple dirs this could be ambiguous and should
probably use $path in the error instead of $values->{name}.

> +			}
>  
> -	my $parsed = process_file($path);
> +			my $parsed = process_file($path);
>  
> -	$ret .= "# template start; name=$values->{name}; version=$version;\n";
> -	$ret .= $parsed;
> -	$ret .= "# template end;\n";
> -	return $ret;
> +			$ret .= "# template start; name=$values->{name}; version=$version;\n";
> +			$ret .= $parsed;
> +			$ret .= "# template end;\n";
> +			return $ret;
> +		}
> +	}
> +	die sprintf(gettext("failed to open '%s': %s\n"), $template_name, $!)

You don't actually fail to open anything, you fail to find a matching
template. die sprintf(gettext("Failed to find template file matching
'%s'), $template_name); would better reflect that.

> @@ -128,7 +134,7 @@ sub process_file {
>  	my $nesting_level = 0;
>  	my $linenumber = 0;
>  
> -	open (my $fh, "<", $filename) or die sprintf(gettext("failed to open '%s': %s\n"), $filename, $!);
> +	open (my $fh, "<", $filename);

Removing this die introduces a race condition and issues when the file
has wrong permissions and is not readable (-e will succeed, open will
not). I'd say keep it here (the way it was) in addition to the die after
the for loop above. They catch different kinds of errors.


The rest of the patch and the tests look good.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://lists.archlinux.org/pipermail/pacman-dev/attachments/20150423/aff7b4e8/attachment.asc>