[pacman-dev] [PATCHv3] Add makepkg-template

[Andrew Gregory]

A few more comments now that I've had time to do a more in-depth
review.

On 05/02/13 at 09:36am, Florian Pritz wrote:
> This allows for somewhat easy templating for PKGBUILDs.
> 
> Signed-off-by: Florian Pritz <bluewind at xinu.at>
> ---
> 
> v3:
>  - improved manpage
>  - added --with-template-dir to configure
>  - use @BUILDSCRIPT@ instead of hardcoded default
>  - switched to same filename format as makepkg/pacman
>    (templatename-version.template) and allowed some more chars
>    for templatename
>  - added some sanity checks for templatename/version
> 
>  configure.ac                   |   9 +++
>  doc/.gitignore                 |   1 +
>  doc/Makefile.am                |   4 +
>  doc/makepkg-template.1.txt     | 132 ++++++++++++++++++++++++++++++++
>  scripts/.gitignore             |   1 +
>  scripts/Makefile.am            |  11 +++
>  scripts/makepkg-template.pl.in | 170 +++++++++++++++++++++++++++++++++++++++++
>  7 files changed, 328 insertions(+)
>  create mode 100644 doc/makepkg-template.1.txt
>  create mode 100755 scripts/makepkg-template.pl.in
> 

<snip>

> diff --git a/scripts/makepkg-template.pl.in b/scripts/makepkg-template.pl.in
> new file mode 100755
> index 0000000..c7e753e
> --- /dev/null
> +++ b/scripts/makepkg-template.pl.in
> @@ -0,0 +1,170 @@
> +#!/usr/bin/perl
> +use warnings;
> +use strict;
> +use v5.14;
> +use Cwd qw(abs_path);
> +use File::Spec;
> +use Getopt::Long;
> +use Pod::Usage;
> +
> +my %opts = (
> +	input => '@BUILDSCRIPT@',
> +	template_dir => '@TEMPLATE_DIR@',
> +);
> +
> +my $template_name_charset = qr/[[:alnum:]+_\. at -]/;

No need to escape the period, they lose their magic inside [].
Let's also add a $template_marker = qr/# template/ here.

> +
> +sub burp {
> +	my ($file_name) = shift;

Let's be consistent in unpacking @_.
my ($file_name, @lines) = @_;
...
print $fh @lines;

> +	open (my $fh, ">", $file_name) || die "can't create $file_name $!" ;
> +	print $fh @_;

Let's explicitly close $fh.

> +}
> +
> +# read a template marker line and parse values into a hash
> +# format is "# template (start|input); key=value; key2=value2; ..."
> +sub parse_template_line {
> +	my ($line, $filename, $linenumber) = @_;
> +	my %values;
> +
> +	my @elements = split(/;\s?/, $line);
> +
> +	foreach my $element (@elements) {
> +		given($element) {
> +			when (/^# template (.*)$/) {

The first element is always going to be special. Let's just grab it
separately and handle it upfront.

my ($marker, @elements) = split...
($values{command}) = ($marker =~ /$template_marker (.*)/);

Then this loop can be reduced to something like this (note the split
above will take care of any semi-colons):

my ($key, $val) = ($element =~ /^([a-z0-9]+)=(.*)$/)
die "invalid key/value pair $filename:linenumber: $line"
    unless $key and $val;
$values{$key} = $val;

> +				$values{template} = $1;
> +			}
> +			when (/^([a-z0-9_]+)=(.*);?$/) {
> +				$values{$1} = $2;
> +			}
> +		}
> +	}
> +
> +	if (!$values{name}) {
> +		die "invalid template line: can't find template name\n",
> +		    "$filename:$linenumber: $line";
> +	}
> +
> +	unless ($values{name} =~ /^$template_name_charset+$/) {
> +		die "invalid chars used in name '$values{name}'. allowed: [:alnum:]+_. at -\n",
> +		    "$filename:$linenumber: $line";
> +	}
> +
> +	return \%values;
> +}
> +
> +# load a template, process possibly existing markers (nested templates)
> +sub load_template {
> +	my ($values) = @_;
> +
> +	my $ret = "";
> +
> +	my $path;
> +	if (!$opts{newest} and $values->{version}) {
> +		$path = "$opts{template_dir}/$values->{name}-$values->{version}.template";
> +	} else {
> +		$path = "$opts{template_dir}/$values->{name}.template";
> +	}
> +
> +	# resolve symlink(s) and use the real file's basename for version detection
> +	my $basename = (File::Spec->splitpath(abs_path($path)))[2];
> +	my ($version) = ($basename =~ /^$template_name_charset+-([0-9\.]+)\.template$/);

We're already anchored at the end of the path and I don't see any
reason to check for a valid template name at this stage, so no need
for the splitpath call here or including $template_name_charset in the
regex.

my ($version) = (abs_path($path) =~ /-([0-9.]+)[.]template$/);

> +
> +	if (!$version) {
> +		die "Couldn't detect version for template '$values->{name}'";
> +	}
> +
> +	my $parsed = process_file($path);
> +
> +	$ret .= "# template start; name=$values->{name}; version=$version;\n";
> +	$ret .= $parsed;
> +	$ret .= "# template end;\n";
> +	return $ret;
> +}
> +
> +# process input file and load templates for all markers found
> +sub process_file {
> +	my ($filename) = @_;
> +
> +	my $ret = "";
> +	my $in_block = 0;
> +	my $linenumber = 0;
> +
> +	open FH, "<", $filename or die "failed to open '$filename': $!";
> +	my @lines = <FH>;
> +	close FH;

We may as well be consistent about using simple scalars with open.
open (my $fh...

> +
> +	foreach my $line (@lines) {
> +		my $add_line = 1;
> +		$linenumber++;
> +
> +		if ($line =~ /^# template ([^;]+);?/) {

I don't think we should partially parse the line here only to parse it
fully in parse_template_line.  That will just require us to keep this
regex in sync with the real parser.

if($line =~ $template_marker) {
    my $values = parse_template_line(...);
    # either move the checks for template name here or skip them in
    # parse_template_line for 'template end'
    given ($value->{command}) {
        ...

> +			given ($1) {
> +				# special marker to insert a template for the first time
> +				when ('input') {
> +					my $values = parse_template_line($line, $filename, $linenumber);
> +					$ret .= load_template($values);
> +					$add_line = 0;
> +				}
> +
> +				# marker followed by code from the template and an end
> +				when ('start') {
> +					my $values = parse_template_line($line, $filename, $linenumber);
> +
> +					# only process the first template level here
> +					# load_template calls us again if templates are nested
> +					if ($in_block == 0) {
> +						$ret .= load_template($values);
> +					}
> +					$in_block++;
> +				}
> +
> +				when ('end') {
> +					$in_block--;
> +					$add_line = 0;
> +				}
> +
> +				default {
> +					die "Unknown template marker '$1'",
> +					    "$filename:$linenumber: $line";
> +				}
> +			}
> +		}
> +
> +		if ($in_block > 0) {
> +			$add_line = 0;
> +		}
> +
> +		$ret .= "$line" if $add_line;

This $in_block/$add_line mechanism for tracking whether or not to
append the line is a little odd.  I think it would be better to
either: convert this loop to a c-style loop and immediately skip to
the end of the template block as soon as we see 'template start' or
use next to skip to the next line instead of setting $add_line = 0.

Also, as it stands now, it will skip over a nested 'template start'
inside a block, but it will expand 'template input' inside the block.

> +	}
> +	return $ret;
> +}
> +
> +Getopt::Long::Configure ("bundling");
> +GetOptions(
> +	"help" => sub {pod2usage(-exitval => 0, -verbose => 1); },
> +	"h" => sub {pod2usage(-exitval => 0, -verbose => 0); },
> +	"input|p=s" => \$opts{input},
> +	"output|o=s" => \$opts{output},
> +	"newest|n" => \$opts{newest},
> +	"template-dir=s" => \$opts{template_dir},
> +) or pod2usage(1);
> +
> +$opts{output} = $opts{input} unless $opts{output};
> +
> +my $output = process_file($opts{input});

This temporary variable doesn't hurt anything, but it doesn't serve
any purpose either

> +
> +burp($opts{output}, $output);
> +
> +__END__
> +=head1 SYNOPSIS
> +
> +makepkg-template [options]
> +
> + Options:
> +   --input, -p <file>     PKGBUILD to read (default: @BUILDSCRIPT@)
> +   --output, -o <file>    file to output to (default: input file)
> +   --newest, -n           update templates to newest possible version
> +                          (default: use specified version in the template markers)
> +   --template-dir <dir>   dir to search for templates (default: @TEMPLATE_DIR@)
> +
> +=cut

vim modeline:
# vim: set noet:

> -- 
> 1.8.2.2
>