Re: use Getopt::Long for catalog scripts

On Thu, Feb 07, 2019 at 10:09:08AM +0100, John Naylor wrote:
> This was suggested in
> 
> https://www.postgresql.org/message-id/11766.1545942853%40sss.pgh.pa.us
> 
> I also adjusted usage() to match. There might be other scripts that
> could use this treatment, but I figure this is a good start.
> 
> -- 
> John Naylor                https://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

> diff --git a/src/backend/catalog/Makefile b/src/backend/catalog/Makefile
> index d9f92ba701..8c4bd0c7ae 100644
> --- a/src/backend/catalog/Makefile
> +++ b/src/backend/catalog/Makefile
> @@ -89,7 +89,8 @@ generated-header-symlinks: $(top_builddir)/src/include/catalog/header-stamp
>  # version number when it changes.
>  bki-stamp: genbki.pl Catalog.pm $(POSTGRES_BKI_SRCS) $(POSTGRES_BKI_DATA) $(top_srcdir)/configure.in
>      $(PERL) -I $(catalogdir) $< \
> -        -I $(top_srcdir)/src/include/ --set-version=$(MAJORVERSION) \
> +        --include-path=$(top_srcdir)/src/include/ \
> +        --set-version=$(MAJORVERSION) \
>          $(POSTGRES_BKI_SRCS)
>      touch $@
>  
> diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
> index be81094ffb..48ba69cd0a 100644
> --- a/src/backend/catalog/genbki.pl
> +++ b/src/backend/catalog/genbki.pl
> @@ -16,6 +16,7 @@
>  
>  use strict;
>  use warnings;
> +use Getopt::Long;
>  
>  use File::Basename;
>  use File::Spec;
> @@ -23,41 +24,20 @@ BEGIN  { use lib File::Spec->rel2abs(dirname(__FILE__)); }
>  
>  use Catalog;
>  
> -my @input_files;
>  my $output_path = '';
>  my $major_version;
>  my $include_path;
>  
> -# Process command line switches.
> -while (@ARGV)
> -{
> -    my $arg = shift @ARGV;
> -    if ($arg !~ /^-/)
> -    {
> -        push @input_files, $arg;
> -    }
> -    elsif ($arg =~ /^-I/)
> -    {
> -        $include_path = length($arg) > 2 ? substr($arg, 2) : shift @ARGV;
> -    }
> -    elsif ($arg =~ /^-o/)
> -    {
> -        $output_path = length($arg) > 2 ? substr($arg, 2) : shift @ARGV;
> -    }
> -    elsif ($arg =~ /^--set-version=(.*)$/)
> -    {
> -        $major_version = $1;
> -        die "Invalid version string.\n"
> -          if !($major_version =~ /^\d+$/);
> -    }
> -    else
> -    {
> -        usage();
> -    }
> -}
> +GetOptions(
> +    'output:s'        => \$output_path,
> +    'include-path:s'  => \$include_path,
> +    'set-version:s'   => \$major_version) || usage();
> +
> +die "Invalid version string.\n"
> +  if !($major_version =~ /^\d+$/);

Maybe this would be clearer as:

die "Invalid version string.\n"
    unless $major_version =~ /^\d+$/;

>  # Sanity check arguments.
> -die "No input files.\n" if !@input_files;
> +die "No input files.\n" if !@ARGV;
>  die "--set-version must be specified.\n" if !defined $major_version;
>  die "-I, the header include path, must be specified.\n" if !$include_path;

Similarly,

die "No input files.\n" unless @ARGV;
die "--set-version must be specified.\n" unless defined $major_version;
die "-I, the header include path, must be specified.\n" unless $include_path;

>  
> @@ -79,7 +59,7 @@ my @toast_decls;
>  my @index_decls;
>  my %oidcounts;
>  
> -foreach my $header (@input_files)
> +foreach my $header (@ARGV)
>  {
>      $header =~ /(.+)\.h$/
>        or die "Input files need to be header files.\n";
> @@ -917,11 +897,11 @@ sub form_pg_type_symbol
>  sub usage
>  {
>      die <<EOM;
> -Usage: genbki.pl [options] header...
> +Usage: perl -I [directory of Catalog.pm] genbki.pl [--output/-o <path>] [--include-path/-i include path] header...
>  
>  Options:
> -    -I               include path
> -    -o               output path
> +    --output         Output directory (default '.')
> +    --include-path   Include path for source directory
>      --set-version    PostgreSQL version number for initdb cross-check
>  
>  genbki.pl generates BKI files and symbol definition
> diff --git a/src/backend/utils/Gen_fmgrtab.pl b/src/backend/utils/Gen_fmgrtab.pl
> index f17a78ebcd..9aa8714840 100644
> --- a/src/backend/utils/Gen_fmgrtab.pl
> +++ b/src/backend/utils/Gen_fmgrtab.pl
> @@ -18,32 +18,14 @@ use Catalog;
>  
>  use strict;
>  use warnings;
> +use Getopt::Long;
>  
> -# Collect arguments
> -my @input_files;
>  my $output_path = '';
>  my $include_path;
>  
> -while (@ARGV)
> -{
> -    my $arg = shift @ARGV;
> -    if ($arg !~ /^-/)
> -    {
> -        push @input_files, $arg;
> -    }
> -    elsif ($arg =~ /^-o/)
> -    {
> -        $output_path = length($arg) > 2 ? substr($arg, 2) : shift @ARGV;
> -    }
> -    elsif ($arg =~ /^-I/)
> -    {
> -        $include_path = length($arg) > 2 ? substr($arg, 2) : shift @ARGV;
> -    }
> -    else
> -    {
> -        usage();
> -    }
> -}
> +GetOptions(
> +    'output:s'    => \$output_path,
> +    'include:s'   => \$include_path) || usage();
>  
>  # Make sure output_path ends in a slash.
>  if ($output_path ne '' && substr($output_path, -1) ne '/')
> @@ -52,7 +34,7 @@ if ($output_path ne '' && substr($output_path, -1) ne '/')
>  }
>  
>  # Sanity check arguments.

And here:

> -die "No input files.\n"                       if !@input_files;
> +die "No input files.\n"                       if !@ARGV;
>  die "No include path; you must specify -I.\n" if !$include_path;

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate