use Getopt::Long for catalog scripts
This was suggested in
/messages/by-id/11766.1545942853@sss.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
Attachments:
use-getopt-for-catalog-scripts.patchtext/x-patch; charset=US-ASCII; name=use-getopt-for-catalog-scripts.patchDownload
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+$/);
# 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;
@@ -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.
-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;
# Read all the input files into internal data structures.
@@ -63,7 +45,7 @@ die "No include path; you must specify -I.\n" if !$include_path;
# more than one data file.
my %catalogs;
my %catalog_data;
-foreach my $datfile (@input_files)
+foreach my $datfile (@ARGV)
{
$datfile =~ /(.+)\.dat$/
or die "Input files need to be data (.dat) files.\n";
@@ -292,7 +274,7 @@ Catalog::RenameTempFile($tabfile, $tmpext);
sub usage
{
die <<EOM;
-Usage: perl -I [directory of Catalog.pm] Gen_fmgrtab.pl -I [include path] [path to pg_proc.dat]
+Usage: perl -I [directory of Catalog.pm] Gen_fmgrtab.pl [--include-path/-i include path] [path to pg_proc.dat]
Gen_fmgrtab.pl generates fmgroids.h, fmgrprotos.h, and fmgrtab.c from
pg_proc.dat
diff --git a/src/backend/utils/Makefile b/src/backend/utils/Makefile
index a5251327e2..c2618e5272 100644
--- a/src/backend/utils/Makefile
+++ b/src/backend/utils/Makefile
@@ -35,7 +35,7 @@ $(SUBDIRS:%=%-recursive): fmgr-stamp errcodes.h
# the timestamps of the individual output files, because the Perl script
# won't update them if they didn't change (to avoid unnecessary recompiles).
fmgr-stamp: Gen_fmgrtab.pl $(catalogdir)/Catalog.pm $(top_srcdir)/src/include/catalog/pg_proc.dat $(top_srcdir)/src/include/access/transam.h
- $(PERL) -I $(catalogdir) $< -I $(top_srcdir)/src/include/ $(top_srcdir)/src/include/catalog/pg_proc.dat
+ $(PERL) -I $(catalogdir) $< --include-path=$(top_srcdir)/src/include/ $(top_srcdir)/src/include/catalog/pg_proc.dat
touch $@
errcodes.h: $(top_srcdir)/src/backend/utils/errcodes.txt generate-errcodes.pl
On Thu, Feb 07, 2019 at 10:09:08AM +0100, John Naylor wrote:
This was suggested in
/messages/by-id/11766.1545942853@sss.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-checkgenbki.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
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 Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Feb 7, 2019 at 6:53 PM David Fetter <david@fetter.org> wrote:
[some style suggestions]
I've included these in v2, and also some additional tweaks for consistency.
--
John Naylor https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
v2-0001-Use-Getopt-Long-for-catalog-scripts.patchtext/x-patch; charset=US-ASCII; name=v2-0001-Use-Getopt-Long-for-catalog-scripts.patchDownload
From 0bf2b3ffdaa16d85856074dda8e15c09fc99d0fe Mon Sep 17 00:00:00 2001
From: John Naylor <jcnaylor@gmail.com>
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 <<EOM;
-Usage: genbki.pl [options] header...
+Usage: perl -I [directory of Catalog.pm] genbki.pl [--output/-o <path>] [--include-path/-i <path>] header...
Options:
- -I include path
- -o output path
+ --output Output directory (default '.')
--set-version PostgreSQL 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 specify -I.\n" if !$include_path;
+die "No input files.\n" unless @ARGV;
+die "--include-path must be specified.\n" unless $include_path;
# Read all the input files into internal data structures.
# Note: We pass data file names as arguments and then look for matching
@@ -63,7 +45,7 @@ die "No include path; you must specify -I.\n" if !$include_path;
# more than one data file.
my %catalogs;
my %catalog_data;
-foreach my $datfile (@input_files)
+foreach my $datfile (@ARGV)
{
$datfile =~ /(.+)\.dat$/
or die "Input files need to be data (.dat) files.\n";
@@ -292,7 +274,11 @@ Catalog::RenameTempFile($tabfile, $tmpext);
sub usage
{
die <<EOM;
-Usage: perl -I [directory of Catalog.pm] Gen_fmgrtab.pl -I [include path] [path to pg_proc.dat]
+Usage: perl -I [directory of Catalog.pm] Gen_fmgrtab.pl [--include-path/-i <path>] [path to pg_proc.dat]
+
+Options:
+ --output Output directory (default '.')
+ --include-path Include path in source tree
Gen_fmgrtab.pl generates fmgroids.h, fmgrprotos.h, and fmgrtab.c from
pg_proc.dat
diff --git a/src/backend/utils/Makefile b/src/backend/utils/Makefile
index a5251327e2..c2618e5272 100644
--- a/src/backend/utils/Makefile
+++ b/src/backend/utils/Makefile
@@ -35,7 +35,7 @@ $(SUBDIRS:%=%-recursive): fmgr-stamp errcodes.h
# the timestamps of the individual output files, because the Perl script
# won't update them if they didn't change (to avoid unnecessary recompiles).
fmgr-stamp: Gen_fmgrtab.pl $(catalogdir)/Catalog.pm $(top_srcdir)/src/include/catalog/pg_proc.dat $(top_srcdir)/src/include/access/transam.h
- $(PERL) -I $(catalogdir) $< -I $(top_srcdir)/src/include/ $(top_srcdir)/src/include/catalog/pg_proc.dat
+ $(PERL) -I $(catalogdir) $< --include-path=$(top_srcdir)/src/include/ $(top_srcdir)/src/include/catalog/pg_proc.dat
touch $@
errcodes.h: $(top_srcdir)/src/backend/utils/errcodes.txt generate-errcodes.pl
--
2.17.1
On 2019-Feb-08, John Naylor wrote:
On Thu, Feb 7, 2019 at 6:53 PM David Fetter <david@fetter.org> 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 Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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 <david@fetter.org> 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 Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2/12/19, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
Pushed, thanks.
Thank you.
--
John Naylor https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services