Re: use Getopt::Long for catalog scripts
On 2/12/19, Alvaro Herrera wrote: > Pushed, thanks. Thank you. -- John Naylorhttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: use Getopt::Long for catalog scripts
On 2019-Feb-12, Alvaro Herrera wrote: > On 2019-Feb-08, John Naylor wrote: > > > On Thu, Feb 7, 2019 at 6:53 PM David Fetter wrote: > > > [some style suggestions] > > > > I've included these in v2, and also some additional tweaks for consistency. > > I noticed that because we have this line in the scripts, > BEGIN { use lib File::Spec->rel2abs(dirname(__FILE__)); } > the -I switch to Perl is no longer needed in the makefiles. Pushed, thanks. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: use Getopt::Long for catalog scripts
On 2019-Feb-08, John Naylor wrote: > On Thu, Feb 7, 2019 at 6:53 PM David Fetter wrote: > > [some style suggestions] > > I've included these in v2, and also some additional tweaks for consistency. I noticed that because we have this line in the scripts, BEGIN { use lib File::Spec->rel2abs(dirname(__FILE__)); } the -I switch to Perl is no longer needed in the makefiles. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: use Getopt::Long for catalog scripts
On Thu, Feb 7, 2019 at 6:53 PM David Fetter wrote: > [some style suggestions] I've included these in v2, and also some additional tweaks for consistency. -- John Naylorhttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services From 0bf2b3ffdaa16d85856074dda8e15c09fc99d0fe Mon Sep 17 00:00:00 2001 From: John Naylor Date: Thu, 7 Feb 2019 20:10:50 +0100 Subject: [PATCH v2] Use Getopt::Long for catalog scripts Replace hand-rolled option parsing with the Getopt module. This is shorter and easier to read. In passing, make some cosmetic adjustments for consistency. --- src/backend/catalog/Makefile | 2 +- src/backend/catalog/genbki.pl| 49 +--- src/backend/utils/Gen_fmgrtab.pl | 38 - src/backend/utils/Makefile | 2 +- 4 files changed, 28 insertions(+), 63 deletions(-) diff --git a/src/backend/catalog/Makefile b/src/backend/catalog/Makefile index d9f92ba701..b06ee49ca8 100644 --- a/src/backend/catalog/Makefile +++ b/src/backend/catalog/Makefile @@ -89,7 +89,7 @@ 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..4935e00fb2 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,43 +24,21 @@ 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, + 'set-version:s' => \$major_version, + 'include-path:s' => \$include_path) || usage(); # Sanity check arguments. -die "No input files.\n" if !@input_files; -die "--set-version must be specified.\n" if !defined $major_version; -die "-I, the header include path, must be specified.\n" if !$include_path; +die "No input files.\n" unless @ARGV; +die "--set-version must be specified.\n" unless $major_version; +die "Invalid version string: $major_version\n" + unless $major_version =~ /^\d+$/; +die "--include-path must be specified.\n" unless $include_path; # Make sure paths end with a slash. if ($output_path ne '' && substr($output_path, -1) ne '/') @@ -79,7 +58,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,12 +896,12 @@ sub form_pg_type_symbol sub usage { die <] [--include-path/-i ] header... Options: --I include path --o output path +--output Output directory (default '.') --set-versionPostgreSQL version number for initdb cross-check +--include-path Include path in source tree genbki.pl generates BKI files and symbol definition headers from specially formatted header files and .dat diff --git a/src/backend/utils/Gen_fmgrtab.pl b/src/backend/utils/Gen_fmgrtab.pl index f17a78ebcd..2ffacae666 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-path:s' => \$include_path) || usage(); # Make sure output_path ends in a slash. if ($output_path ne '' && substr($output_path, -1) ne '/') @@ -52,8 +34,8 @@ if ($output_path ne '' && substr($output_path, -1) ne '/') } # Sanity check arguments. -die "No input files.\n" if !@input_files; -die "No include path; you must
Re: use Getopt::Long for catalog scripts
Why is this script talking to me? On 2019-Feb-07, David Fetter wrote: > Similarly, > > die "-I, the header include path, must be specified.\n" unless $include_path; But why must thee, oh mighty header include path, be specified? -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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 Naylorhttps://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 < -Usage: genbki.pl [options] header... > +Usage: perl -I [directory of Catalog.pm] genbki.pl [--output/-o ] > [--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-versionPostgreSQL 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