Importing pg_bsd_indent into our source tree

Hi,

On 2023-02-09 13:30:30 -0500, Tom Lane wrote:

0003 lacks meson support (anyone want to help with that?)

I'll give it a go, unless somebody else wants to.

Do we expect pg_bsd_indent to build / work on windows, right now? If it
doesn't, do we want to make that a hard requirement?

I'll have CI test that, once I added meson support.

I'd anticipate pushing 0001-0003 shortly but holding 0004 until we are ready
to do the post-March-CF pgindent run. (Come to think of it, 0004 had
probably better include a pg_bsd_indent version bump too.)

How large is the diff that creates? If it's not super-widespread, it might be
ok to do that earlier. I wouldn't mind not seeing that uglyness every time I
run pgindent on a patch... Although I guess post-March-CF isn't that far away
at this point :)

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2023-02-09 13:30:30 -0500, Tom Lane wrote:

0003 lacks meson support (anyone want to help with that?)

I'll give it a go, unless somebody else wants to.

Thanks.

Do we expect pg_bsd_indent to build / work on windows, right now?

It would be desirable, for sure. I've not noticed anything remarkably
unportable in the code, so probably it's just a matter of getting the
build infrastructure to build it. I suppose that we aren't going to
update the src/tools/msvc scripts anymore, so getting meson to handle
it should be enough??

I'd anticipate pushing 0001-0003 shortly but holding 0004 until we are ready
to do the post-March-CF pgindent run. (Come to think of it, 0004 had
probably better include a pg_bsd_indent version bump too.)

How large is the diff that creates? If it's not super-widespread, it might be
ok to do that earlier.

It wasn't that big; I posted it in the thread discussing that change.

I think the real issue might just be that, assuming we bump the
pg_bsd_indent version number, that in itself would force interested
committers to update their copy Right Now. I'd rather give a little
notice.

regards, tom lane

Hi,

On 2023-02-09 13:02:02 -0800, Andres Freund wrote:

On 2023-02-09 13:30:30 -0500, Tom Lane wrote:

0003 lacks meson support (anyone want to help with that?)

I'll give it a go, unless somebody else wants to.

Did that in the attached.

I didn't convert the test though, due to the duplicating it'd create. Perhaps
we should just move it to a shell script? Or maybe it just doesn't matter
enough to bother with?

Do we expect pg_bsd_indent to build / work on windows, right now? If it
doesn't, do we want to make that a hard requirement?

I'll have CI test that, once I added meson support.

It doesn't build as-is with msvc, but does build with mingw. Failure:
https://cirrus-ci.com/task/6290206869946368?logs=build#L1573

"cl" "-Isrc\tools/pg_bsd_indent\pg_bsd_indent.exe.p" "-Isrc\tools/pg_bsd_indent" "-I..\src\tools\pg_bsd_indent" "-Isrc\include" "-I..\src\include" "-Ic:\openssl\1.1\include" "-I..\src\include\port\win32" "-I..\src\include\port\win32_msvc" "/MDd" "/nologo" "/showIncludes" "/utf-8" "/W2" "/Od" "/Zi" "/DWIN32" "/DWINDOWS" "/D__WINDOWS__" "/D__WIN32__" "/D_CRT_SECURE_NO_DEPRECATE" "/D_CRT_NONSTDC_NO_DEPRECATE" "/wd4018" "/wd4244" "/wd4273" "/wd4101" "/wd4102" "/wd4090" "/wd4267" "/Fdsrc\tools/pg_bsd_indent\pg_bsd_indent.exe.p\args.c.pdb" /Fosrc/tools/pg_bsd_indent/pg_bsd_indent.exe.p/args.c.obj "/c" ../src/tools/pg_bsd_indent/args.c
../src/tools/pg_bsd_indent/args.c(179): error C2065: 'PATH_MAX': undeclared identifier
../src/tools/pg_bsd_indent/args.c(179): error C2057: expected constant expression
../src/tools/pg_bsd_indent/args.c(179): error C2466: cannot allocate an array of constant size 0
../src/tools/pg_bsd_indent/args.c(179): error C2133: 'fname': unknown size
../src/tools/pg_bsd_indent/args.c(183): warning C4034: sizeof returns 0
../src/tools/pg_bsd_indent/args.c(185): warning C4034: sizeof returns 0
[1557/2161] Compiling C object src/tools/pg_bsd_indent/pg_bsd_indent.exe.p/err.c.obj
[1558/2161] Precompiling header src/interfaces/ecpg/ecpglib/libecpg.dll.p/meson_pch-c.c
[1559/2161] Compiling C object src/tools/pg_bsd_indent/pg_bsd_indent.exe.p/indent.c.obj
FAILED: src/tools/pg_bsd_indent/pg_bsd_indent.exe.p/indent.c.obj
"cl" "-Isrc\tools/pg_bsd_indent\pg_bsd_indent.exe.p" "-Isrc\tools/pg_bsd_indent" "-I..\src\tools\pg_bsd_indent" "-Isrc\include" "-I..\src\include" "-Ic:\openssl\1.1\include" "-I..\src\include\port\win32" "-I..\src\include\port\win32_msvc" "/MDd" "/nologo" "/showIncludes" "/utf-8" "/W2" "/Od" "/Zi" "/DWIN32" "/DWINDOWS" "/D__WINDOWS__" "/D__WIN32__" "/D_CRT_SECURE_NO_DEPRECATE" "/D_CRT_NONSTDC_NO_DEPRECATE" "/wd4018" "/wd4244" "/wd4273" "/wd4101" "/wd4102" "/wd4090" "/wd4267" "/Fdsrc\tools/pg_bsd_indent\pg_bsd_indent.exe.p\indent.c.pdb" /Fosrc/tools/pg_bsd_indent/pg_bsd_indent.exe.p/indent.c.obj "/c" ../src/tools/pg_bsd_indent/indent.c
../src/tools/pg_bsd_indent/indent.c(63): error C2065: 'MAXPATHLEN': undeclared identifier
../src/tools/pg_bsd_indent/indent.c(63): error C2057: expected constant expression
../src/tools/pg_bsd_indent/indent.c(63): error C2466: cannot allocate an array of constant size 0

This specific issue at least should be easily fixable.

Freebsd emits a compiler warning:

[21:37:50.909] In file included from ../src/tools/pg_bsd_indent/indent.c:54:
[21:37:50.909] ../src/tools/pg_bsd_indent/indent.h:31:9: warning: 'nitems' macro redefined [-Wmacro-redefined]
[21:37:50.909] #define nitems(array) (sizeof (array) / sizeof (array[0]))
[21:37:50.909] ^
[21:37:50.909] /usr/include/sys/param.h:306:9: note: previous definition is here
[21:37:50.909] #define nitems(x) (sizeof((x)) / sizeof((x)[0]))
[21:37:50.909] ^
[21:37:50.911] 1 warning generated.

To we really want to require users to install pg_bsd_indent into PATH? Seems
like we ought to have a build target to invoke pgindent with a path to
pg_bsd_indent or such? But I guess we can address that later.

Independent of this specific patch: You seem to be generating your patch
series by invoking git show and redirecting that to a file? How do you apply a
series of such patches, while maintaining the commit messages? When git
format-patch is used, I can just use git am, but that doesn't work with your
patch series.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

Did that in the attached.

Thanks.

I didn't convert the test though, due to the duplicating it'd create. Perhaps
we should just move it to a shell script? Or maybe it just doesn't matter
enough to bother with?

We could move it to a shell script perhaps, but that seems pretty
low-priority.

It doesn't build as-is with msvc, but does build with mingw. Failure:
https://cirrus-ci.com/task/6290206869946368?logs=build#L1573

Thanks, I'll take a look at these things.

To we really want to require users to install pg_bsd_indent into PATH? Seems
like we ought to have a build target to invoke pgindent with a path to
pg_bsd_indent or such? But I guess we can address that later.

For the moment I was just interested in maintaining the current workflow.
I know people muttered about having some sort of build target that'd
indent the whole tree from scratch after building pg_bsd_indent, but it's
not very clear to me how that'd work with e.g. VPATH configurations.

(I think you can already tell pgindent to use a specific pg_bsd_indent,
if your gripe is just about wanting to use a prebuilt copy that you
don't want to keep in PATH for some reason.)

Independent of this specific patch: You seem to be generating your patch
series by invoking git show and redirecting that to a file?

Yeah, it's pretty low-tech. I'm not in the habit of posting multi-patch
series very often, so I haven't really bothered to use format-patch.
(I gave up on "git am" long ago as being too fragile, and always
use good ol' "patch" to apply patches, so I don't think about things
like whether it'd automatically absorb commit messages. I pretty much
never use anyone else's commit message verbatim anyway ...)

regards, tom lane

Hi,

On 2023-02-09 13:55:32 -0800, Andres Freund wrote:

../src/tools/pg_bsd_indent/args.c(179): error C2065: 'PATH_MAX': undeclared identifier
../src/tools/pg_bsd_indent/args.c(179): error C2057: expected constant expression
../src/tools/pg_bsd_indent/args.c(179): error C2466: cannot allocate an array of constant size 0
../src/tools/pg_bsd_indent/args.c(179): error C2133: 'fname': unknown size
../src/tools/pg_bsd_indent/args.c(183): warning C4034: sizeof returns 0
../src/tools/pg_bsd_indent/args.c(185): warning C4034: sizeof returns 0
[1557/2161] Compiling C object src/tools/pg_bsd_indent/pg_bsd_indent.exe.p/err.c.obj
[1558/2161] Precompiling header src/interfaces/ecpg/ecpglib/libecpg.dll.p/meson_pch-c.c
[1559/2161] Compiling C object src/tools/pg_bsd_indent/pg_bsd_indent.exe.p/indent.c.obj
FAILED: src/tools/pg_bsd_indent/pg_bsd_indent.exe.p/indent.c.obj
"cl" "-Isrc\tools/pg_bsd_indent\pg_bsd_indent.exe.p" "-Isrc\tools/pg_bsd_indent" "-I..\src\tools\pg_bsd_indent" "-Isrc\include" "-I..\src\include" "-Ic:\openssl\1.1\include" "-I..\src\include\port\win32" "-I..\src\include\port\win32_msvc" "/MDd" "/nologo" "/showIncludes" "/utf-8" "/W2" "/Od" "/Zi" "/DWIN32" "/DWINDOWS" "/D__WINDOWS__" "/D__WIN32__" "/D_CRT_SECURE_NO_DEPRECATE" "/D_CRT_NONSTDC_NO_DEPRECATE" "/wd4018" "/wd4244" "/wd4273" "/wd4101" "/wd4102" "/wd4090" "/wd4267" "/Fdsrc\tools/pg_bsd_indent\pg_bsd_indent.exe.p\indent.c.pdb" /Fosrc/tools/pg_bsd_indent/pg_bsd_indent.exe.p/indent.c.obj "/c" ../src/tools/pg_bsd_indent/indent.c
../src/tools/pg_bsd_indent/indent.c(63): error C2065: 'MAXPATHLEN': undeclared identifier
../src/tools/pg_bsd_indent/indent.c(63): error C2057: expected constant expression
../src/tools/pg_bsd_indent/indent.c(63): error C2466: cannot allocate an array of constant size 0

This specific issue at least should be easily fixable.

The trivial fix of using MAXPGPATH made it build, without warnings. That
doesn't say anything about actually working. So I guess porting the test would
make sense.

Opinions on whether it would make sense as a shell script?

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

The trivial fix of using MAXPGPATH made it build, without warnings. That
doesn't say anything about actually working. So I guess porting the test would
make sense.

Opinions on whether it would make sense as a shell script?

Hmmm .. a shell script would be fine by me, but it won't help in
testing a Windows build. Maybe we need to make it a Perl script?

BTW, the attachments to your previous message are identical to what
I previously posted --- did you attach the wrong set of diffs?

regards, tom lane

Hi,

On 2023-02-09 17:19:22 -0500, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

The trivial fix of using MAXPGPATH made it build, without warnings. That
doesn't say anything about actually working. So I guess porting the test would
make sense.

Opinions on whether it would make sense as a shell script?

Hmmm .. a shell script would be fine by me, but it won't help in
testing a Windows build. Maybe we need to make it a Perl script?

At least for casual testing a shell script actually mostly works, due to git
it's easy enough to have a sh.exe around... Not something I'd necessarily want
to make a hard dependency, but for something like this it might suffice. Of
course perl would be more dependable...

BTW, the attachments to your previous message are identical to what
I previously posted --- did you attach the wrong set of diffs?

I attached an extra patch, in addition to yours. I also attached yours so that
cfbot could continue to work, if you registered this.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2023-02-09 17:19:22 -0500, Tom Lane wrote:

Hmmm .. a shell script would be fine by me, but it won't help in
testing a Windows build. Maybe we need to make it a Perl script?

At least for casual testing a shell script actually mostly works, due to git
it's easy enough to have a sh.exe around... Not something I'd necessarily want
to make a hard dependency, but for something like this it might suffice. Of
course perl would be more dependable...

Yeah, also less question about whether it works on Windows.
I'll see about moving that into Perl. It's short enough.

BTW, the attachments to your previous message are identical to what
I previously posted --- did you attach the wrong set of diffs?

I attached an extra patch, in addition to yours.

D'oh, I didn't notice that :-(

I also attached yours so that
cfbot could continue to work, if you registered this.

I thought about registering it, but that won't teach us anything unless
we make it built-by-default, which was not my intention. I guess we
could temporarily include it in the build.

regards, tom lane

Hi,

On 2023-02-09 18:19:06 -0500, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2023-02-09 17:19:22 -0500, Tom Lane wrote:

Hmmm .. a shell script would be fine by me, but it won't help in
testing a Windows build. Maybe we need to make it a Perl script?

At least for casual testing a shell script actually mostly works, due to git
it's easy enough to have a sh.exe around... Not something I'd necessarily want
to make a hard dependency, but for something like this it might suffice. Of
course perl would be more dependable...

Yeah, also less question about whether it works on Windows.
I'll see about moving that into Perl. It's short enough.

Cool.

I also attached yours so that
cfbot could continue to work, if you registered this.

I thought about registering it, but that won't teach us anything unless
we make it built-by-default, which was not my intention. I guess we
could temporarily include it in the build.

The meson patch I sent did build it by default, that's why I saw the windows
failure and the freebsd warnings. If we don't want that, we'd need to add
build_by_default: false

I'm fine either way. It's barely noticeable compared to the rest of postgres.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2023-02-09 18:19:06 -0500, Tom Lane wrote:

I thought about registering it, but that won't teach us anything unless
we make it built-by-default, which was not my intention. I guess we
could temporarily include it in the build.

The meson patch I sent did build it by default, that's why I saw the windows
failure and the freebsd warnings. If we don't want that, we'd need to add
build_by_default: false

I'm fine either way. It's barely noticeable compared to the rest of postgres.

Yeah, build-by-default isn't really a big deal. Install-by-default
is more of a problem...

regards, tom lane

On 2023-02-09 19:20:37 -0500, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

I'm fine either way. It's barely noticeable compared to the rest of postgres.

Yeah, build-by-default isn't really a big deal. Install-by-default
is more of a problem...

Perhaps we should install it, just not in bin/, but alongside pgxs/, similar
to pg_regress et al?

Andres Freund <andres@anarazel.de> writes:

Perhaps we should install it, just not in bin/, but alongside pgxs/, similar
to pg_regress et al?

For my own purposes, I really don't want it anywhere in the --prefix
tree. That's not necessarily present when I'm using the program.

(Hmm, clarify: it wouldn't matter if install sticks it under pgxs,
but I don't want to be forced to use it from there.)

regards, tom lane

Here's a v3 of this patchset, incorporating your meson fixes as
well as patches for the portability problems you noted.

I ended up converting the test infrastructure into a TAP test,
which kind of feels like overkill; but the Meson system doesn't
seem to provide any lower-overhead way to run a test.

I've not touched the issue of whether and where to install
pg_bsd_indent; for now, neither build system will do so.

Also, for now both build systems *will* run tests on it,
although I'm not sure if plugging it into "make check-world"
is enough to cause the cfbot to do so, and I'm pretty sure
that the buildfarm won't notice that.

I'll let the cfbot loose on this, and if it runs the tests
successfully I plan to go ahead and push. We can resolve
the installation question later. We might want to back off
testing too once we're satisfied about portability.

(I left out the 0004 declaration-formatting patch for now, btw.)

regards, tom lane

I wrote:

I'll let the cfbot loose on this, and if it runs the tests
successfully I plan to go ahead and push.

cfbot didn't like that ...

regards, tom lane

Hi,

On 2023-02-11 18:54:00 -0500, Tom Lane wrote:

I ended up converting the test infrastructure into a TAP test,
which kind of feels like overkill; but the Meson system doesn't
seem to provide any lower-overhead way to run a test.

FWIW, The default way to indicate failures in a test is the exit
code. Obviously that allows less detailed reporting, but other than that, it
works (that's how we test pg_regress today).

Also, for now both build systems *will* run tests on it,
although I'm not sure if plugging it into "make check-world"
is enough to cause the cfbot to do so, and I'm pretty sure
that the buildfarm won't notice that.

That's sufficient for cfbot, on the CI task still using autoconf. And for
meson it'll also suffice.

It actually already ran:
https://cirrus-ci.com/build/5984572702195712

The windows test failure is a transient issue independent of the patch
(something went wrong with image permissions). However the linux autoconf one
isn't:
https://api.cirrus-ci.com/v1/artifact/task/5482952532951040/log/src/tools/pg_bsd_indent/tmp_check/log/regress_log_001_pg_bsd_indent

# Running: pg_bsd_indent --version
Command 'pg_bsd_indent' not found in /tmp/cirrus-ci-build/tmp_install/usr/local/pgsql/bin, /tmp/cirrus-ci-build/src/tools/pg_bsd_indent, /usr/local/sbin, /usr/local/bin, /usr/sbin, /usr/bin, /sbin, /bin at /tmp/cirrus-ci-build/src/tools/pg_bsd_indent/../../../src/test/perl/PostgreSQL/Test/Utils.pm line 832.

I guess there might be a missing dependency? PATH looks sufficient.

I'll let the cfbot loose on this, and if it runs the tests
successfully I plan to go ahead and push. We can resolve
the installation question later. We might want to back off
testing too once we're satisfied about portability.

(I left out the 0004 declaration-formatting patch for now, btw.)

Makes sense.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

# Running: pg_bsd_indent --version
Command 'pg_bsd_indent' not found in /tmp/cirrus-ci-build/tmp_install/usr/local/pgsql/bin, /tmp/cirrus-ci-build/src/tools/pg_bsd_indent, /usr/local/sbin, /usr/local/bin, /usr/sbin, /usr/bin, /sbin, /bin at /tmp/cirrus-ci-build/src/tools/pg_bsd_indent/../../../src/test/perl/PostgreSQL/Test/Utils.pm line 832.

I guess there might be a missing dependency? PATH looks sufficient.

Yeah, I expected that "check" would have a dependency on "all",
but apparently it doesn't (and I'd missed this because I had
pg_bsd_indent installed elsewhere in my PATH :-(). New build
running now.

regards, tom lane

Hmmm ... ci autoconf build is now happy, but the Windows run complains
that none of the output files match. I'm betting that this is a
Windows-newline problem, since I now see that indent.c opens both the
input and output files in default (text) mode. I'm inclined to
change it to open the output file in binary mode while leaving the
input in text, which should have the effect of stripping \r if it's
present.

regards, tom lane

I wrote:

Hmmm ... ci autoconf build is now happy, but the Windows run complains
that none of the output files match. I'm betting that this is a
Windows-newline problem, since I now see that indent.c opens both the
input and output files in default (text) mode. I'm inclined to
change it to open the output file in binary mode while leaving the
input in text, which should have the effect of stripping \r if it's
present.

So let's see if that theory is correct at all ...

regards, tom lane

On Sun, Feb 12, 2023 at 2:44 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I wrote:

Hmmm ... ci autoconf build is now happy, but the Windows run complains
that none of the output files match. I'm betting that this is a
Windows-newline problem, since I now see that indent.c opens both the
input and output files in default (text) mode. I'm inclined to
change it to open the output file in binary mode while leaving the
input in text, which should have the effect of stripping \r if it's
present.

So let's see if that theory is correct at all ...

(Since I happened to be tinkering on cfbot while you posted these, I
noticed that cfbot took over 50 minutes to start processing the v4.
The problem was upstream: the time in the second-last column of
https://commitfest.postgresql.org/42/ didn't change for that whole
time, even though the archives had your new email. Cf castles, sand;
I should probably get a better trigger mechanism :-) I like that page
because it lets me poll one single end point once per minute to learn
about changes across all threads, but I am not sure what sort of
technology connects the archives to the CF app, and how it can fail.)