Re: automating perl compile time checking
On 06/15/2018 12:21 AM, Peter Eisentraut wrote: On 6/11/18 16:06, Andrew Dunstan wrote: This affects pretty much nothing. In fact some of the other changes I've recently committed were arguably more dangerous. Do you want me to revert the whole lot? No, but this whole let's clean up the Perl code initiative seemed to have come out of nowhere after feature freeze. The Perl code was fine before, so if we're going to be polishing it around it should go into the next release. I would actually have liked to have had more time to discuss some of the changes, since I didn't seem to agree to some of them at first reading. There were changes made for perlcritic, but almost none for this check. cheers andrew -- Andrew Dunstanhttps://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: automating perl compile time checking
On 6/11/18 16:06, Andrew Dunstan wrote: > This affects pretty much nothing. In fact some of the other changes I've > recently committed were arguably more dangerous. Do you want me to > revert the whole lot? No, but this whole let's clean up the Perl code initiative seemed to have come out of nowhere after feature freeze. The Perl code was fine before, so if we're going to be polishing it around it should go into the next release. I would actually have liked to have had more time to discuss some of the changes, since I didn't seem to agree to some of them at first reading. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: automating perl compile time checking
On 06/11/2018 03:38 PM, Andrew Dunstan wrote: On 06/11/2018 02:33 PM, Alvaro Herrera wrote: On 2018-Jun-05, Daniel Gustafsson wrote: On 5 Jun 2018, at 16:31, Andrew Dunstan wrote: The patch contains a simple script to run the checks. The code that finds perl files is put in a function in a single file that is sourced by the three locations that need it. +1 on centralizing the find-files function. +1 on that. Why do we need to make the new find_perl_files file executable, given it's always sourced? (I would have given a .sh extension because it's a lib not an executable, but I suppose that's just matter of taste; we certainly don't have a policy about it). Looks fine to me either way. I've committed this, but I'm fine if people want to tweak the names. It probably doesn't need to be executable. People might be interested to see the perlcritic and 'perl -cw" checks in operation: https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=crake=2018-06-11%2021%3A47%3A18=perl-check The module isn't actually using the scripts in src/tools/perlcheck, because they are designed to be quiet and it's designed to be more verbose, but apart from that it's doing exactly the same thing. cheers andrew -- Andrew Dunstanhttps://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: automating perl compile time checking
Andrew Dunstan writes: > On 06/11/2018 04:01 PM, Peter Eisentraut wrote: >> Why is this being committed after feature freeze? > This affects pretty much nothing. In fact some of the other changes I've > recently committed were arguably more dangerous. Do you want me to > revert the whole lot? AFAICS this is adding testing, not "features", so it seems fine from here. regards, tom lane
Re: automating perl compile time checking
On 06/11/2018 04:01 PM, Peter Eisentraut wrote: On 6/11/18 15:38, Andrew Dunstan wrote: On 5 Jun 2018, at 16:31, Andrew Dunstan wrote: The patch contains a simple script to run the checks. The code that finds perl files is put in a function in a single file that is sourced by the three locations that need it. +1 on centralizing the find-files function. +1 on that. Why do we need to make the new find_perl_files file executable, given it's always sourced? (I would have given a .sh extension because it's a lib not an executable, but I suppose that's just matter of taste; we certainly don't have a policy about it). Looks fine to me either way. I've committed this, but I'm fine if people want to tweak the names. It probably doesn't need to be executable. Why is this being committed after feature freeze? This affects pretty much nothing. In fact some of the other changes I've recently committed were arguably more dangerous. Do you want me to revert the whole lot? cheers andrew -- Andrew Dunstanhttps://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: automating perl compile time checking
On 6/11/18 15:38, Andrew Dunstan wrote: On 5 Jun 2018, at 16:31, Andrew Dunstan wrote: The patch contains a simple script to run the checks. The code that finds perl files is put in a function in a single file that is sourced by the three locations that need it. >>> +1 on centralizing the find-files function. >> +1 on that. Why do we need to make the new find_perl_files file >> executable, given it's always sourced? (I would have given a .sh >> extension because it's a lib not an executable, but I suppose that's >> just matter of taste; we certainly don't have a policy about it). >> >> Looks fine to me either way. >> > > > I've committed this, but I'm fine if people want to tweak the names. It > probably doesn't need to be executable. Why is this being committed after feature freeze? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: automating perl compile time checking
On 06/11/2018 02:33 PM, Alvaro Herrera wrote: On 2018-Jun-05, Daniel Gustafsson wrote: On 5 Jun 2018, at 16:31, Andrew Dunstan wrote: The patch contains a simple script to run the checks. The code that finds perl files is put in a function in a single file that is sourced by the three locations that need it. +1 on centralizing the find-files function. +1 on that. Why do we need to make the new find_perl_files file executable, given it's always sourced? (I would have given a .sh extension because it's a lib not an executable, but I suppose that's just matter of taste; we certainly don't have a policy about it). Looks fine to me either way. I've committed this, but I'm fine if people want to tweak the names. It probably doesn't need to be executable. cheers andrew -- Andrew Dunstanhttps://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: automating perl compile time checking
On 2018-Jun-05, Daniel Gustafsson wrote: > > On 5 Jun 2018, at 16:31, Andrew Dunstan > > wrote: > > > The patch contains a simple script to run the checks. The code that finds > > perl files is put in a function in a single file that is sourced by the > > three locations that need it. > > +1 on centralizing the find-files function. +1 on that. Why do we need to make the new find_perl_files file executable, given it's always sourced? (I would have given a .sh extension because it's a lib not an executable, but I suppose that's just matter of taste; we certainly don't have a policy about it). Looks fine to me either way. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: automating perl compile time checking
On 06/05/2018 05:14 PM, Daniel Gustafsson wrote: This comment should say “perlcheck/..” and not “pgperlcritic/.." I assume: Thanks. will fix. cheers andrew -- Andrew Dunstanhttps://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: automating perl compile time checking
> On 5 Jun 2018, at 16:31, Andrew Dunstan > wrote: > The patch contains a simple script to run the checks. The code that finds > perl files is put in a function in a single file that is sourced by the three > locations that need it. +1 on centralizing the find-files function. > The directory pgperlcritic is renamed to perlcheck, as it not contains the > new script as well as pgperlcritic. This comment should say “perlcheck/..” and not “pgperlcritic/.." I assume: --- /dev/null +++ b/src/tools/perlcheck/pgperlcritic @@ -0,0 +1,20 @@ +#!/bin/sh + +# src/tools/pgperlcritic/pgperlcritic cheers ./daniel
automating perl compile time checking
Here is a follow up patch to last weeks commit allowing all perl files
to be checked clean for compile time errors and warnings.
The patch contains a simple script to run the checks. The code that
finds perl files is put in a function in a single file that is sourced
by the three locations that need it.
The directory pgperlcritic is renamed to perlcheck, as it not contains
the new script as well as pgperlcritic.
cheers
andrew
--
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 6247cd1fbba16426bb1745521d7b444d60ebbd0a Mon Sep 17 00:00:00 2001
From: Andrew Dunstan
Date: Tue, 5 Jun 2018 10:25:55 -0400
Subject: [PATCH] Extend and slightly refactor perl checking
A script is added to check for perl compile time errors and warnings.
The code to detect all the perl files is put in a central place and
included in the three places it is now needed. The directory
pgperlcritic is renamed to perlcheck, and contains the new check script
as well as pgperlcritic.
---
src/tools/perlcheck/find_perl_files| 15
src/tools/{pgperlcritic => perlcheck}/perlcriticrc | 0
src/tools/perlcheck/pgperlcritic | 20
src/tools/perlcheck/pgperlsyncheck | 17 +
src/tools/pgindent/pgperltidy | 14 +++
src/tools/pgperlcritic/pgperlcritic| 28 --
6 files changed, 55 insertions(+), 39 deletions(-)
create mode 100644 src/tools/perlcheck/find_perl_files
rename src/tools/{pgperlcritic => perlcheck}/perlcriticrc (100%)
create mode 100755 src/tools/perlcheck/pgperlcritic
create mode 100755 src/tools/perlcheck/pgperlsyncheck
delete mode 100755 src/tools/pgperlcritic/pgperlcritic
diff --git a/src/tools/perlcheck/find_perl_files b/src/tools/perlcheck/find_perl_files
new file mode 100644
index 000..e10466a
--- /dev/null
+++ b/src/tools/perlcheck/find_perl_files
@@ -0,0 +1,15 @@
+
+# src/tools/perlcheck/find_perl_files
+
+# shell function to find all perl files in the source tree
+
+find_perl_files () {
+{
+ # take all .pl and .pm files
+ find . -type f -name '*.p[lm]' -print
+ # take executable files that file(1) thinks are perl files
+ find . -type f -perm -100 -exec file {} \; -print |
+ egrep -i ':.*perl[0-9]*\>' |
+ cut -d: -f1
+ } | sort -u
+}
diff --git a/src/tools/pgperlcritic/perlcriticrc b/src/tools/perlcheck/perlcriticrc
similarity index 100%
rename from src/tools/pgperlcritic/perlcriticrc
rename to src/tools/perlcheck/perlcriticrc
diff --git a/src/tools/perlcheck/pgperlcritic b/src/tools/perlcheck/pgperlcritic
new file mode 100755
index 000..6a31f67
--- /dev/null
+++ b/src/tools/perlcheck/pgperlcritic
@@ -0,0 +1,20 @@
+#!/bin/sh
+
+# src/tools/pgperlcritic/pgperlcritic
+
+test -f src/tools/perlcheck/perlcriticrc || {
+ echo could not find src/tools/perlcheck/perlcriticrc
+ exit 1
+ }
+
+set -e
+
+# set this to override default perlcritic program:
+PERLCRITIC=${PERLCRITIC:-perlcritic}
+
+. src/tools/perlcheck/find_perl_files
+
+find_perl_files | xargs $PERLCRITIC \
+ --quiet \
+ --program-extensions .pl \
+ --profile=src/tools/perlcheck/perlcriticrc
diff --git a/src/tools/perlcheck/pgperlsyncheck b/src/tools/perlcheck/pgperlsyncheck
new file mode 100755
index 000..4595d16
--- /dev/null
+++ b/src/tools/perlcheck/pgperlsyncheck
@@ -0,0 +1,17 @@
+#!/bin/sh
+
+# script to detect compile time errors and warnings in all perl files
+
+INCLUDES="-I src/tools/msvc -I src/tools/msvc/dummylib -I src/backend/catalog"
+INCLUDES="-I src/test/perl -I src/backend/utils/mb/Unicode $INCLUDES"
+INCLUDES="-I src/bin/pg_rewind -I src/test/ssl $INCLUDES"
+
+set -e
+
+. src/tools/perlcheck/find_perl_files
+
+# for zsh
+setopt shwordsplit 2>/dev/null || true
+
+find_perl_files | xargs -L 1 perl $INCLUDES -cw 2>&1 | grep -v OK
+
diff --git a/src/tools/pgindent/pgperltidy b/src/tools/pgindent/pgperltidy
index 5d9aa7c..5e70411 100755
--- a/src/tools/pgindent/pgperltidy
+++ b/src/tools/pgindent/pgperltidy
@@ -7,14 +7,6 @@ set -e
# set this to override default perltidy program:
PERLTIDY=${PERLTIDY:-perltidy}
-# locate all Perl files in the tree
-(
- # take all .pl and .pm files
- find . -type f -a \( -name '*.pl' -o -name '*.pm' \)
- # take executable files that file(1) thinks are perl files
- find . -type f -perm -100 -exec file {} \; |
- egrep -i ':.*perl[0-9]*\>' |
- cut -d: -f1
-) |
-sort -u |
-xargs $PERLTIDY --profile=src/tools/pgindent/perltidyrc
+. src/tools/perlcheck/find_perl_files
+
+find_perl_files | xargs $PERLTIDY --profile=src/tools/pgindent/perltidyrc
diff --git a/src/tools/pgperlcritic/pgperlcritic b/src/tools/pgperlcritic/pgperlcritic
deleted file mode 100755
index 28264b1..000
--- a/src/tools/pgperlcritic/pgperlcritic
+++ /dev/null
@@ -1,28 +0,0 @@
-#!/bin/sh
-
-# src/tools/pgperlcritic/pgperlcritic
-
-test -f src/tools/pgperlcritic/perlcriticrc || {
