run pgindent in CI

On 2026-09-22 Tu 3:10 AM, Peter Eisentraut wrote:

Previously discussed in [0].  People seemed to generally like the idea
of running pgindent in CI but didn't want the job to fail outright if
there were violations.  My proposal here is to use a combination of
GitHub Actions features [1] to create job annotations and step
summaries to create some kind of warning output.  I have attached an
example screenshot.

I also added a git whitespace check in the same way.  Additional
formatting checks like pgperltidy, Python stuff, etc. could be added
similarly.

(I imagine there could be some cfbot integration that represents these
annotations in a non-threatening way, but I have not looked into that
at all.)

One surprise from this is that this generates a massive pgindent diff
against the current tree, because there are a bunch of entries missing
or wrong in src/tools/pgindent/exclude_file_patterns.  I guess most
people don't see this because these are mostly generated files, which
would get reindented but then ignored via gitignore, but if you run
pgindent --check --diff, then this will show up.

I don't exactly know how the buildfarm member koel gets away with
this. The notes say "Runs pgindent --show-diff", but that is not an
actual existing option.  One evident difference is that koel runs
meson, but I have added this to the existing CompilerWarnings job and
reused the tree from the headerscheck step, which uses autoconf. 
Anyway, maybe this could be improved sometime to reduce variability,
but for now I have just included a patch to update
exclude_file_patterns as needed.

The notes are out of date re --show-diff.

You can see what koel does - it's a special buildfarm module at
https://github.com/PGBuildFarm/client-code/blob/main/PGBuild/Modules/CheckIndent.pm

It just runs:

   src/tools/pgindent/pgindent --diff --check .

Of course, that's against a pure source tree. The build tree is
elsewhere and the module never points pgindent at it. Even if koel were
using configure/make, I would have set it up to do vpath builds.

Note that you can specify extra exclude files on pgindent's command line
as well, so if your builddir is inside your source tree it would make
sense to specify one pointing at the builddir. That's not going to help
you if you build in-tree using configure/make. I guess I would say don't
do that and then run pgindent.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Andrew Dunstan <andrew@dunslane.net> writes:

On 2026-09-22 Tu 3:10 AM, Peter Eisentraut wrote:

One surprise from this is that this generates a massive pgindent diff
against the current tree, because there are a bunch of entries missing
or wrong in src/tools/pgindent/exclude_file_patterns.  I guess most
people don't see this because these are mostly generated files, which
would get reindented but then ignored via gitignore, but if you run
pgindent --check --diff, then this will show up.

[koel] just runs:
   src/tools/pgindent/pgindent --diff --check .
Of course, that's against a pure source tree.

Yeah, I generally run pgindent against a clean tree as well. We have
made some efforts to make generated files be pgindent-clean, but
it's been pretty haphazard, and I doubt we want to make a project
policy requiring that. So really the right thing is to make this
CI module do likewise.

regards, tom lane

On Tue, Sep 22, 2026 at 09:10:23AM +0200, Peter Eisentraut wrote:

Previously discussed in [0]. People seemed to generally like the idea of
running pgindent in CI but didn't want the job to fail outright if there
were violations. My proposal here is to use a combination of GitHub Actions
features [1] to create job annotations and step summaries to create some
kind of warning output. I have attached an example screenshot.

+1. I think it'd be nice if the commitfest app showed some sort of
indicator, too.

FWIW I've been using my own custom pgindent for a couple of months that is
much faster, which might be nice if we're going to start running it in CI.
I just haven't gotten to posting the patches. The main ideas are 1)
running in parallel and 2) parsing typedefs.list once per run. That pretty
dramatically decreases the duration of a whole-tree indent.

--
nathan

On 22.09.26 16:04, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

On 2026-09-22 Tu 3:10 AM, Peter Eisentraut wrote:

One surprise from this is that this generates a massive pgindent diff
against the current tree, because there are a bunch of entries missing
or wrong in src/tools/pgindent/exclude_file_patterns.  I guess most
people don't see this because these are mostly generated files, which
would get reindented but then ignored via gitignore, but if you run
pgindent --check --diff, then this will show up.

[koel] just runs:
   src/tools/pgindent/pgindent --diff --check .
Of course, that's against a pure source tree.

Yeah, I generally run pgindent against a clean tree as well. We have
made some efforts to make generated files be pgindent-clean, but
it's been pretty haphazard, and I doubt we want to make a project
policy requiring that. So really the right thing is to make this
CI module do likewise.

For clarification: We already have a exclude_file_patterns that
excludes many generated files. It just didn't exclude enough of them.
Things were just forgotten. So this proposal is not introducing a new
policy or requirement, it just fixes things so that the implementation
of the current apparent policy is complete.

We could also desupport running pgindent in a tree with both sources and
built files. But then we should do that explicitly and remove the
leftover support for that. However, I wouldn't be in favor of that
unless we desupported in-tree (non-vpath) builds altogether. In any
case, a patch to complete the exclude_file_patterns list for the time
being does exist, so this does not have to be decided urgently.

Hi,

On Tue, 22 Sept 2026 at 10:11, Peter Eisentraut <peter@eisentraut.org> wrote:

Previously discussed in [0]. People seemed to generally like the idea
of running pgindent in CI but didn't want the job to fail outright if
there were violations. My proposal here is to use a combination of
GitHub Actions features [1] to create job annotations and step summaries
to create some kind of warning output. I have attached an example
screenshot.

I also added a git whitespace check in the same way. Additional
formatting checks like pgperltidy, Python stuff, etc. could be added
similarly.

Thoughts?

For the 0003, what do you think about just mentioning that 'the
style-check steps in the CompilerWarnings have failed' instead of
printing the diff? That would make sense since we won't actually need
to understand the diff (AFAIK), we just need to run some tools.

--
Regards,
Nazir Bilal Yavuz
Microsoft

On 22.09.26 18:33, Nathan Bossart wrote:

FWIW I've been using my own custom pgindent for a couple of months that is
much faster, which might be nice if we're going to start running it in CI.
I just haven't gotten to posting the patches. The main ideas are 1)
running in parallel and 2) parsing typedefs.list once per run. That pretty
dramatically decreases the duration of a whole-tree indent.

The timing of pgindent runs in this CI job aren't terribly critical in
the overall context (8-10s). Faster is better of course. But I would
be very interested in this improvement for local interactive use.

Peter Eisentraut <peter@eisentraut.org> writes:

For clarification: We already have a exclude_file_patterns that
excludes many generated files. It just didn't exclude enough of them.
Things were just forgotten. So this proposal is not introducing a new
policy or requirement, it just fixes things so that the implementation
of the current apparent policy is complete.

My point is exactly that that list hasn't been maintained, because
there was nothing to remind developers to do so. So I'm doubtful
that there can meaningfully be claimed to be a policy requiring it.

If we want to change that, then making CI bitch about non-ignored
files would be a good step, but it's incomplete unless the buildfarm
implementation enforces indent cleanliness the same way. IOW, what
I'm complaining about here is that what you want to do doesn't match
what koel does. They need to match.

regards, tom lane

On Tue, 22 Sept 2026 at 18:33, Nathan Bossart <nathandbossart@gmail.com> wrote:

FWIW I've been using my own custom pgindent for a couple of months that is
much faster, which might be nice if we're going to start running it in CI.
I just haven't gotten to posting the patches. The main ideas are 1)
running in parallel and 2) parsing typedefs.list once per run. That pretty
dramatically decreases the duration of a whole-tree indent.

See also[1]/messages/by-id/DHJCUQA4HF9H.10EN25MHG0YRW@jeltef.nl which adds parallel support and a bunch of other things.
Only parsing typedefs.list once sounds nice though. Would be good to
combine these efforts I think.

[1]: /messages/by-id/DHJCUQA4HF9H.10EN25MHG0YRW@jeltef.nl

On 23.09.26 16:35, Tom Lane wrote:

If we want to change that, then making CI bitch about non-ignored
files would be a good step, but it's incomplete unless the buildfarm
implementation enforces indent cleanliness the same way. IOW, what
I'm complaining about here is that what you want to do doesn't match
what koel does. They need to match.

I have launched two new threads to prepare for this:

pgindent to ignore build directories
(/messages/by-id/fc77b35a-34c4-4229-82f1-8cf0ce67f276@eisentraut.org)

convert CI CompilerWarnings job to meson
(/messages/by-id/e52e2c29-4834-4c64-a200-eddcba38fb4d@eisentraut.org)

When those are done, it will be possible to reproduce the koel
environment on CI, and we can come back to this.