Add pgindent test to check if codebase is correctly formatted
Had some time to watch code run through an extensive test suite, so
thought I would propose this patch that is probably about 75% of the way
to the stated $subject. I had to add in a hack for Meson, and I couldn't
figure out a good hack for autotools.
I think a good solution would be to distribute pgindent and
pg_bsd_indent. At Neon, we are trying to format our extension code using
pgindent. I am sure there are other extension authors out there too that
format using pgindent. Distributing pg_bsd_indent and pgindent in the
postgresql-devel package would be a great help to those of us that
pgindent out of tree code. It would also have the added benefit of
adding the tools to $PREFIX/bin, which would make the test that I added
not need a hack to get the pg_bsd_indent executable.
--
Tristan Partin
Neon (https://neon.tech)
Attachments:
v1-0001-Allow-tests-to-register-command-line-arguments-in.patchtext/x-patch; charset=utf-8; name=v1-0001-Allow-tests-to-register-command-line-arguments-in.patchDownload
From 6e9ca366b6e4976ae591012150fe77729e37c503 Mon Sep 17 00:00:00 2001
From: Tristan Partin <tristan@neon.tech>
Date: Tue, 16 Jan 2024 19:00:44 -0600
Subject: [PATCH v1 1/2] Allow tests to register command line arguments in
Meson
---
meson.build | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/meson.build b/meson.build
index c317144b6b..12ba91109b 100644
--- a/meson.build
+++ b/meson.build
@@ -3284,6 +3284,8 @@ foreach test_dir : tests
'env': env,
} + t.get('test_kwargs', {})
+ test_args = t.get('args', [])
+
foreach onetap : t['tests']
# Make tap test names prettier, remove t/ and .pl
onetap_p = onetap
@@ -3302,7 +3304,7 @@ foreach test_dir : tests
'--testname', onetap_p,
'--', test_command,
test_dir['sd'] / onetap,
- ],
+ ] + test_args,
)
endforeach
install_suites += test_group
--
Tristan Partin
Neon (https://neon.tech)
v1-0002-Add-pgindent-test-to-check-if-codebase-is-correct.patchtext/x-patch; charset=utf-8; name=v1-0002-Add-pgindent-test-to-check-if-codebase-is-correct.patchDownload
From 13902328b24984ab2d18914b63c6874143715f48 Mon Sep 17 00:00:00 2001
From: Tristan Partin <tristan@neon.tech>
Date: Tue, 16 Jan 2024 19:03:42 -0600
Subject: [PATCH v1 2/2] Add pgindent test to check if codebase is correctly
formatted
Should be useful for developers and committers to test code as they
develop, commit, or prepare patches.
---
src/meson.build | 2 +-
src/tools/pgindent/Makefile | 24 ++++++++++++++++++++++++
src/tools/pgindent/meson.build | 13 +++++++++++++
src/tools/pgindent/t/001_pgindent.pl | 19 +++++++++++++++++++
4 files changed, 57 insertions(+), 1 deletion(-)
create mode 100644 src/tools/pgindent/Makefile
create mode 100644 src/tools/pgindent/meson.build
create mode 100644 src/tools/pgindent/t/001_pgindent.pl
diff --git a/src/meson.build b/src/meson.build
index 65c7d17d08..0345d2fa79 100644
--- a/src/meson.build
+++ b/src/meson.build
@@ -14,7 +14,7 @@ subdir('pl')
subdir('interfaces')
subdir('tools/pg_bsd_indent')
-
+subdir('tools/pgindent')
### Generate a Makefile.global that's complete enough for PGXS to work.
#
diff --git a/src/tools/pgindent/Makefile b/src/tools/pgindent/Makefile
new file mode 100644
index 0000000000..45d6b71a12
--- /dev/null
+++ b/src/tools/pgindent/Makefile
@@ -0,0 +1,24 @@
+#-------------------------------------------------------------------------
+#
+# src/tools/pgindent/Makefile
+#
+# Copyright (c) 2024, PostgreSQL Global Development Group
+#
+#-------------------------------------------------------------------------
+
+subdir = src/tools/pgindent
+top_builddir = ../../..
+include $(top_builddir)/src/Makefile.global
+
+clean distclean:
+ rm -rf log/ tmp_check/
+
+# Provide this alternate test name to allow testing pgindent without building
+# all of the surrounding Postgres installation.
+.PHONY: test
+
+pg_bsd_indent:
+ $(MAKE) -C ../pg_bsd_indent pg_bsd_indent
+
+test: pg_bsd_indent
+ $(prove_installcheck)
diff --git a/src/tools/pgindent/meson.build b/src/tools/pgindent/meson.build
new file mode 100644
index 0000000000..d31ade11ce
--- /dev/null
+++ b/src/tools/pgindent/meson.build
@@ -0,0 +1,13 @@
+tests += {
+ 'name': 'pgindent',
+ 'sd': meson.current_source_dir(),
+ 'bd': meson.current_build_dir(),
+ 'tap': {
+ 'args': [
+ pg_bsd_indent.path(),
+ ],
+ 'tests': [
+ 't/001_pgindent.pl',
+ ],
+ },
+}
diff --git a/src/tools/pgindent/t/001_pgindent.pl b/src/tools/pgindent/t/001_pgindent.pl
new file mode 100644
index 0000000000..8ee93f4789
--- /dev/null
+++ b/src/tools/pgindent/t/001_pgindent.pl
@@ -0,0 +1,19 @@
+# Copyright (c) 2024, PostgreSQL Global Development Group
+#
+# Check that all C code is formatted with pgindent
+#
+
+use strict;
+use warnings FATAL => 'all';
+use Test::More;
+use PostgreSQL::Test::Utils qw(command_ok);
+
+if (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bpgindent\b/)
+{
+ plan skip_all => "test pgindent not enabled in PG_TEST_EXTRA";
+}
+
+my $pg_bsd_indent = $ARGV[0];
+command_ok(["./pgindent", "--indent=$pg_bsd_indent", "--check", "--diff"]);
+
+done_testing();
--
Tristan Partin
Neon (https://neon.tech)
On Tue, Jan 16, 2024 at 07:22:23PM -0600, Tristan Partin wrote:
I think a good solution would be to distribute pgindent and pg_bsd_indent.
At Neon, we are trying to format our extension code using pgindent. I am
sure there are other extension authors out there too that format using
pgindent. Distributing pg_bsd_indent and pgindent in the postgresql-devel
package would be a great help to those of us that pgindent out of tree code.
It would also have the added benefit of adding the tools to $PREFIX/bin,
which would make the test that I added not need a hack to get the
pg_bsd_indent executable.
So your point is that pg_bsd_indent and pgindent are in the source tree,
but not in any package distribution? Isn't that a packager decision?
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
Only you can decide what is important to you.
On Tue Jan 16, 2024 at 7:27 PM CST, Bruce Momjian wrote:
On Tue, Jan 16, 2024 at 07:22:23PM -0600, Tristan Partin wrote:
I think a good solution would be to distribute pgindent and pg_bsd_indent.
At Neon, we are trying to format our extension code using pgindent. I am
sure there are other extension authors out there too that format using
pgindent. Distributing pg_bsd_indent and pgindent in the postgresql-devel
package would be a great help to those of us that pgindent out of tree code.
It would also have the added benefit of adding the tools to $PREFIX/bin,
which would make the test that I added not need a hack to get the
pg_bsd_indent executable.So your point is that pg_bsd_indent and pgindent are in the source tree,
but not in any package distribution? Isn't that a packager decision?
It requires changes to at least the Meson build files. pg_bsd_indent is
not marked for installation currently. There is a TODO there. pgindent
has no install_data() for instance. pg_bsd_indent seemingly gets
installed somewhere in autotools given the contents of its Makefile, but
I didn't see anything in my install tree afterward.
Sure RPM/DEB packagers can solve this issue downstream, but that doesn't
help those of that run "meson install" or "make install" upstream.
Packagers are probably more likely to package the tools if they are
marked for installation by upstream too.
Hope this helps to better explain what changes would be required within
the Postgres source tree.
--
Tristan Partin
Neon (https://neon.tech)
On Tue, Jan 16, 2024 at 07:32:47PM -0600, Tristan Partin wrote:
It requires changes to at least the Meson build files. pg_bsd_indent is not
marked for installation currently. There is a TODO there. pgindent has no
install_data() for instance. pg_bsd_indent seemingly gets installed
somewhere in autotools given the contents of its Makefile, but I didn't see
anything in my install tree afterward.Sure RPM/DEB packagers can solve this issue downstream, but that doesn't
help those of that run "meson install" or "make install" upstream. Packagers
are probably more likely to package the tools if they are marked for
installation by upstream too.Hope this helps to better explain what changes would be required within the
Postgres source tree.
Yes, it does, thanks.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
Only you can decide what is important to you.
Hmm, should this also install typedefs.list and pgindent.man?
What about the tooling to reformat Perl code?
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Linux transformó mi computadora, de una `máquina para hacer cosas',
en un aparato realmente entretenido, sobre el cual cada día aprendo
algo nuevo" (Jaime Salinas)
On Wed Jan 17, 2024 at 3:50 AM CST, Alvaro Herrera wrote:
Hmm, should this also install typedefs.list and pgindent.man?
What about the tooling to reformat Perl code?
Good point about pgindent.man. It would definitely be good to install
alongside pgindent and pg_bsd_indent.
I don't know if we need to install the typedefs.list file. I think it
would just be good enough to also install the find_typedefs script. But
it needs some fixing up first[0]/messages/by-id/aaa59ef5-dce8-7369-5cae-487727664127@dunslane.net. Extension authors can then just
generate their own typedefs.list that will include the typedefs of the
extension and the typedefs of the postgres types they use. At least,
that is what we have found works at Neon.
I cannot vouch for extension authors writing Perl but I think it could
make sense to install the src/test/perl tree, so extension authors could
more easily write tests for their extensions in Perl. But we could
install the perltidy file and whatever else too. I keep my Perl writing
to a minimum, so I am not the best person to vouch for these usecases.
[0]: /messages/by-id/aaa59ef5-dce8-7369-5cae-487727664127@dunslane.net
--
Tristan Partin
Neon (https://neon.tech)