Hi,
On 2023-10-18 12:15:51 -0700, Andres Freund wrote:
> I still think that one of the more important things we ought to do is to make
> it trivial to check if code is correctly indented and reindent it for the
> user. I've posted a preliminary patch to add a 'indent-tree' target a few
> months back, at
> https://postgr.es/m/20230527184201.2zdorrijg2inqt6v%40alap3.anarazel.de
>
> I've updated that patch, now it has
> - indent-tree, reindents the entire tree
> - indent-head, which pgindent --commit=HEAD
> - indent-check, fails if the tree isn't correctly indented
> - indent-diff, like indent-check, but also shows the diff
>
> If we tought pgindent to emit the list of files it processes to a dependency
> file, we could make it cheap to call indent-check repeatedly, by teaching
> meson/ninja to not reinvoke it if the input files haven't changed. Personally
> that'd make it more bearable to script indentation checks to happen
> frequently.
>
>
> I'll look into writing a command to update typedefs.list with all the local
> changes.
It turns out that updating the in-tree typedefs.list would be very noisy. On
my local linux system I get
1 file changed, 422 insertions(+), 1 deletion(-)
On a mac mini I get
1 file changed, 351 insertions(+), 1 deletion(-)
We could possibly address that by updating the in-tree typedefs.list a bit
more aggressively. Sure looks like the source systems are on the older side.
But in the attached patch I've implemented this slightly differently. If the
tooling to do so is available, the indent-* targets explained above,
use/depend on src/tools/pgindent/typedefs.list.merged (in the build dir),
which is the combination of a src/tools/pgindent/typedefs.list.local generated
for the local binaries/libraries and the source tree
src/tools/pgindent/typedefs.list.
src/tools/pgindent/typedefs.list.local is generated in fragments, with one
fragment for each build target. That way the whole file doesn't have to be
regenerated all the time, which can save a good bit of time (althoug obviously
less when hacking on the backend).
This makes it quite quick to locally indent, without needing to manually
fiddle around with manually modifying typedefs.list or using a separate
typedefs.list.
In a third commit I added a 'nitpick' configure time option, defaulting to
off, which runs an indentation check. The failure mode of that currently isn't
very helpful though, as it just uses --silent-diff.
All of this currently is meson only, largely because I don't feel like
spending the time messing with the configure build, particularly before there
is any agreement on this being the thing to do.
Greetings,
Andres Freund
>From f7520fd87764cddbde9d6c5d25fdfe5314ec5cbc Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Sat, 27 May 2023 11:38:27 -0700
Subject: [PATCH v3 1/3] Add meson targets to run pgindent
Author:
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
meson.build | 38 +++++++++++++++++++++++++++++++++++++
src/tools/pgindent/pgindent | 12 ++++++++++--
2 files changed, 48 insertions(+), 2 deletions(-)
diff --git a/meson.build b/meson.build
index 862c955453f..2cfeb60eb07 100644
--- a/meson.build
+++ b/meson.build
@@ -3013,6 +3013,44 @@ run_target('install-test-files',
+###############################################################
+# Indentation and similar targets
+###############################################################
+
+indent_base_cmd = [perl, files('src/tools/pgindent/pgindent'),
+ '--indent', pg_bsd_indent.full_path(),
+ '--sourcetree=@SOURCE_ROOT@']
+indent_depend = [pg_bsd_indent]
+
+# reindent the entire tree
+# Reindent the entire tree
+run_target('indent-tree',
+ command: indent_base_cmd + ['.'],
+ depends: indent_depend
+)
+
+# Reindent the last commit
+run_target('indent-head',
+ command: indent_base_cmd + ['--commit=HEAD'],
+ depends: indent_depend
+)
+
+# Silently check if any file is not corectly indented (fails the build if
+# there are differences)
+run_target('indent-check',
+ command: indent_base_cmd + ['--silent-diff', '.'],
+ depends: indent_depend
+)
+
+# Show a diff of all the unindented files (doesn't fail the build if there are
+# differences)
+run_target('indent-diff',
+ command: indent_base_cmd + ['--show-diff', '.'],
+ depends: indent_depend
+)
+
+
+
###############################################################
# Test prep
###############################################################
diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent
index bce63d95daf..08b0c95814f 100755
--- a/src/tools/pgindent/pgindent
+++ b/src/tools/pgindent/pgindent
@@ -23,7 +23,7 @@ my $devnull = File::Spec->devnull;
my ($typedefs_file, $typedef_str, @excludes,
$indent, $build, $show_diff,
- $silent_diff, $help, @commits,);
+ $silent_diff, $sourcetree, $help, @commits,);
$help = 0;
@@ -35,7 +35,8 @@ my %options = (
"excludes=s" => \@excludes,
"indent=s" => \$indent,
"show-diff" => \$show_diff,
- "silent-diff" => \$silent_diff,);
+ "silent-diff" => \$silent_diff,
+ "sourcetree=s" => \$sourcetree);
GetOptions(%options) || usage("bad command line argument");
usage() if $help;
@@ -53,6 +54,13 @@ $typedefs_file ||= $ENV{PGTYPEDEFS};
# get indent location for environment or default
$indent ||= $ENV{PGINDENT} || $ENV{INDENT} || "pg_bsd_indent";
+# When invoked from the build directory, change into source tree, otherwise
+# the heuristics in locate_sourcedir() don't work.
+if (defined $sourcetree)
+{
+ chdir $sourcetree;
+}
+
my $sourcedir = locate_sourcedir();
# if it's the base of a postgres tree, we will exclude the files
--
2.38.0
>From b8d31613acfc9d554640b4e292f366d79084bb8e Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Wed, 18 Oct 2023 16:18:58 -0700
Subject: [PATCH v3 2/3] heavily wip: update typedefs
---
meson.build | 14 +++++-
src/backend/jit/llvm/meson.build | 2 +-
src/backend/snowball/meson.build | 2 +-
src/timezone/meson.build | 2 +-
src/tools/pgindent/merge_typedefs | 24 +++++++++
src/tools/pgindent/meson.build | 70 +++++++++++++++++++++++++++
src/tools/pgindent/typedefs_from_objs | 42 ++++++++++++++++
7 files changed, 152 insertions(+), 4 deletions(-)
create mode 100755 src/tools/pgindent/merge_typedefs
create mode 100644 src/tools/pgindent/meson.build
create mode 100755 src/tools/pgindent/typedefs_from_objs
diff --git a/meson.build b/meson.build
index 2cfeb60eb07..70f6b680c2c 100644
--- a/meson.build
+++ b/meson.build
@@ -2579,6 +2579,7 @@ add_project_link_arguments(ldflags, language: ['c', 'cpp'])
# list of targets for various alias targets
backend_targets = []
+data_targets = []
bin_targets = []
pl_targets = []
contrib_targets = []
@@ -2894,6 +2895,8 @@ subdir('src/interfaces/ecpg/test')
subdir('doc/src/sgml')
+subdir('src/tools/pgindent')
+
generated_sources_ac += {'': ['GNUmakefile']}
# After processing src/test, add test_install_libs to the testprep_targets
@@ -2982,6 +2985,7 @@ endif
all_built = [
backend_targets,
bin_targets,
+ data_targets,
libpq_st,
pl_targets,
contrib_targets,
@@ -3017,12 +3021,20 @@ run_target('install-test-files',
# Indentation and similar targets
###############################################################
+# If the dependencies for generating a local typedefs.list are fulfilled, we
+# use a combination of a locally built and the source tree's typededefs.list
+# file (see src/tools/pgindent/meson.build) for reindenting. That ensures
+# newly added typedefs are indented correctly.
indent_base_cmd = [perl, files('src/tools/pgindent/pgindent'),
'--indent', pg_bsd_indent.full_path(),
'--sourcetree=@SOURCE_ROOT@']
indent_depend = [pg_bsd_indent]
-# reindent the entire tree
+if typedefs_supported
+ indent_base_cmd += ['--typedefs', typedefs_merged.full_path()]
+ indent_depend += typedefs_merged
+endif
+
# Reindent the entire tree
run_target('indent-tree',
command: indent_base_cmd + ['.'],
diff --git a/src/backend/jit/llvm/meson.build b/src/backend/jit/llvm/meson.build
index 8ffaf414609..7e2f6d5bd5c 100644
--- a/src/backend/jit/llvm/meson.build
+++ b/src/backend/jit/llvm/meson.build
@@ -82,4 +82,4 @@ llvmjit_types = custom_target('llvmjit_types.bc',
install_dir: dir_lib_pkg,
depfile: '@basen...@.c.bc.d',
)
-backend_targets += llvmjit_types
+data_targets += llvmjit_types
diff --git a/src/backend/snowball/meson.build b/src/backend/snowball/meson.build
index 0f669c0bf3c..69e777399fd 100644
--- a/src/backend/snowball/meson.build
+++ b/src/backend/snowball/meson.build
@@ -94,4 +94,4 @@ install_subdir('stopwords',
)
backend_targets += dict_snowball
-backend_targets += snowball_create
+data_targets += snowball_create
diff --git a/src/timezone/meson.build b/src/timezone/meson.build
index 7b85a01c6bd..779a51d85a4 100644
--- a/src/timezone/meson.build
+++ b/src/timezone/meson.build
@@ -50,7 +50,7 @@ if get_option('system_tzdata') == ''
install_dir: dir_data,
)
- bin_targets += tzdata
+ data_targets += tzdata
endif
subdir('tznames')
diff --git a/src/tools/pgindent/merge_typedefs b/src/tools/pgindent/merge_typedefs
new file mode 100755
index 00000000000..c2e1f446c15
--- /dev/null
+++ b/src/tools/pgindent/merge_typedefs
@@ -0,0 +1,24 @@
+#!/usr/bin/env python3
+
+# Tool to merge multiple typedefs files into one
+
+import argparse
+import os
+import subprocess
+import sys
+
+parser = argparse.ArgumentParser(
+ description='merge multiple typedef files')
+
+parser.add_argument('--output', type=argparse.FileType('w'), required=True)
+parser.add_argument('input', type=argparse.FileType('r'), nargs='*')
+
+args = parser.parse_args()
+
+typedefs = set()
+
+for input in args.input:
+ for typedef in input.readlines():
+ typedefs.add(typedef.strip())
+
+print('\n'.join(sorted(typedefs)), file=args.output)
diff --git a/src/tools/pgindent/meson.build b/src/tools/pgindent/meson.build
new file mode 100644
index 00000000000..c0801a6c86c
--- /dev/null
+++ b/src/tools/pgindent/meson.build
@@ -0,0 +1,70 @@
+# Currently the code requires meson 0.63 or upwards. This could likely be
+# lifted (in fact, some older versions actually work, but only some), but for
+# now it's not crucial.
+typedefs_supported = meson.version().version_compare('>=0.63')
+if not typedefs_supported
+ subdir_done()
+endif
+
+typedefs_from_objs = files('typedefs_from_objs')
+typedefs_from_objs_cmd = [typedefs_from_objs, '--host', host_system, '--output', '@OUTPUT@', '@INPUT@']
+merge_typedefs = files('merge_typedefs')
+merge_typedefs_cmd = [merge_typedefs, '--output', '@OUTPUT@', '@INPUT@']
+
+# XXX: This list of targets should likely not be maintained here
+typedef_src_tgts = [
+ backend_targets,
+ bin_targets,
+ [libpq_st],
+ pl_targets,
+ contrib_targets,
+ ecpg_targets,
+]
+
+# We generate partial typedefs files for each binary/library. That makes using
+# this during incremental development much faster.
+#
+# The reason we process the object files instead of executables is that that
+# doesn't work on macos. There doesn't seem to be a reason to target
+# executables directly on other platforms.
+typedef_tgts = []
+foreach tgts : typedef_src_tgts
+ foreach tgt : tgts
+ # can't use tgt.name(), as we have have targets that differ just in suffix
+ name = fs.name(tgt.full_path())
+ tdname = 'typedefs.list.local-@0@'.format(name)
+ objs = tgt.extract_all_objects(recursive: true)
+ typedef_tgts += custom_target(tdname,
+ output: tdname,
+ command: typedefs_from_objs_cmd,
+ input: objs,
+ build_by_default: false,
+ )
+
+ endforeach
+endforeach
+
+# A typedefs.list file for the local build tree. This may not contain typedefs
+# referenced e.g. by platform specific code for other platforms.
+typedefs_local = custom_target('typedefs.list.local', output: 'typedefs.list.local',
+ command: merge_typedefs_cmd,
+ input: typedef_tgts,
+ build_by_default: false,
+)
+
+# The locally generated typedef.list, merge with the source tree version. This
+# is useful for reindenting code, particularly when the local tree has new
+# types added.
+typedefs_merged = custom_target('typedefs.list.merged', output: 'typedefs.list.merged',
+ command: merge_typedefs_cmd,
+ input: [typedefs_local, files('typedefs.list')],
+ build_by_default: false,
+)
+
+# FIXME: As-is this is rarely useful, as the local typedefs file will likely
+# contain too many changes.
+if cp.found()
+ run_target('update-typedefs',
+ command: [cp, typedefs_merged, '@SOURCE_ROOT@/src/tools/pgindent/typedefs.list'],
+ )
+endif
diff --git a/src/tools/pgindent/typedefs_from_objs b/src/tools/pgindent/typedefs_from_objs
new file mode 100755
index 00000000000..821c84cb3d8
--- /dev/null
+++ b/src/tools/pgindent/typedefs_from_objs
@@ -0,0 +1,42 @@
+#!/usr/bin/env python3
+
+# Tool to extract typedefs from object files.
+
+import argparse
+import re
+import shutil
+import subprocess
+import sys
+
+parser = argparse.ArgumentParser(
+ description='generate typedefs file for a set of object files')
+
+parser.add_argument('--output', type=argparse.FileType('w'), required=True)
+parser.add_argument('--host', type=str, required=True)
+parser.add_argument('input', nargs='*')
+
+args = parser.parse_args()
+
+if args.host == 'linux':
+ find_td_re = re.compile(r'^[^\n]+TAG_typedef\)\n[^\n]+DW_AT_name\s*:\s*\([^\)]+\): ([^\n]+)$', re.MULTILINE)
+ # FIXME: should probably be set by the caller? Except that the same binary
+ # name behaves very differently on different platforms :/
+ cmd = [shutil.which('objdump'), '-Wi']
+elif args.host == 'darwin':
+ find_td_re = re.compile(r'^[^\n]+TAG_typedef\n\s*DW_AT_type[^\n]+\n\s+DW_AT_name\s*\(\"([^\n]+)\"\)$', re.MULTILINE)
+ cmd = [shutil.which('dwarfdump')]
+else:
+ raise f'unsupported platform: {args.host}'
+
+lcmd = cmd + args.input
+sp = subprocess.run(lcmd, stdout=subprocess.PIPE, universal_newlines=True)
+if sp.returncode != 0:
+ print(f'{lcmd} failed with return code {sp.returncode}', file=sys.stderr)
+ sys.exit(sp.returncode)
+
+fa = find_td_re.findall(sp.stdout)
+
+for typedef in fa:
+ print(typedef, file=args.output)
+
+sys.exit(0)
--
2.38.0
>From 6688a89700af997b5adaa55f349c8db53a86290b Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Wed, 18 Oct 2023 17:51:21 -0700
Subject: [PATCH v3 3/3] WIP: meson: nitpick mode + test checking indentation
Author:
Reviewed-by:
Discussion: https://postgr.es/m/
Backpatch:
---
meson.build | 17 +++++++++++++++--
meson_options.txt | 3 +++
.cirrus.tasks.yml | 1 +
3 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/meson.build b/meson.build
index 70f6b680c2c..86a5d0995a1 100644
--- a/meson.build
+++ b/meson.build
@@ -44,6 +44,7 @@ cc = meson.get_compiler('c')
not_found_dep = dependency('', required: false)
thread_dep = dependency('threads')
auto_features = get_option('auto_features')
+nitpick_mode = get_option('nitpick')
@@ -3025,9 +3026,10 @@ run_target('install-test-files',
# use a combination of a locally built and the source tree's typededefs.list
# file (see src/tools/pgindent/meson.build) for reindenting. That ensures
# newly added typedefs are indented correctly.
-indent_base_cmd = [perl, files('src/tools/pgindent/pgindent'),
+indent_base_args = [files('src/tools/pgindent/pgindent'),
'--indent', pg_bsd_indent.full_path(),
- '--sourcetree=@SOURCE_ROOT@']
+ '--sourcetree=@0@'.format(meson.current_source_dir())]
+indent_base_cmd = [perl, indent_base_args]
indent_depend = [pg_bsd_indent]
if typedefs_supported
@@ -3364,6 +3366,17 @@ if meson.version().version_compare('>=0.57')
endif
+# If we were asked to be nitpicky, add a test that fails if sources aren't
+# properly indented
+if nitpick_mode
+ test('indent-check',
+ perl, args: [indent_base_args, '--silent-diff', '.'],
+ depends: indent_depend,
+ suite: 'source',
+ priority: 10)
+endif
+
+
###############################################################
# Pseudo targets
diff --git a/meson_options.txt b/meson_options.txt
index d2f95cfec36..90cb2177a44 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -52,6 +52,9 @@ option('atomics', type: 'boolean', value: true,
option('spinlocks', type: 'boolean', value: true,
description: 'Use spinlocks')
+option('nitpick', type: 'boolean', value: false,
+ description: 'annoy the hell out of you, committers ought to enable this')
+
# Compilation options
diff --git a/.cirrus.tasks.yml b/.cirrus.tasks.yml
index e137769850d..5dd29bed4cb 100644
--- a/.cirrus.tasks.yml
+++ b/.cirrus.tasks.yml
@@ -358,6 +358,7 @@ task:
meson setup \
--buildtype=debug \
-Dcassert=true \
+ -Dnitpick=true \
${LINUX_MESON_FEATURES} \
-DPG_TEST_EXTRA="$PG_TEST_EXTRA" \
build
--
2.38.0
