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@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:]+_\.@-]/;
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:]+_.@-\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