Thread: automating pg_config.h.win32 maintenance
Keeping pg_config.h.win32 up to date with pg_config.h.in is a gratuitous annoyance. This setup dates back to the minimal client-only Windows builds using win32.mak files, which has been removed in PG10. The MSVC build system has the power of Perl available, so we can do better. My proposal is that we essentially emulate what config.status does in Perl code. config.status gets a list of defines discovered by configure and processes pg_config.h.in to pg_config.h by substituting the defines. The MSVC build system basically has those defines hardcoded, but the processing we can do in just the same way. It already had code to do a bit of that anyway, so it's really not a big leap. See attached patches. (I put the remove of pg_config.h.win32 into a separate patch so that reviewers can just apply the first patch and then diff the produced pg_config.h with the existing pg_config.h.win32.) The only thing that's not quite explainable is that the existing code wrapped some parts of the pg_config.h it generated into an #ifndef IGNORE_CONFIGURED_SETTINGS block. I don't see that referenced or used anywhere else. The original commit (fb8155d0d) claimed this was "to be used by the installer", but I didn't find any reference in the current installer's Git repository either. I suspect this is obsolete. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
On 13/12/2019 14:51, Peter Eisentraut wrote:
> Keeping pg_config.h.win32 up to date with pg_config.h.in is a gratuitous
> annoyance.
Hear hear!
> My proposal is that we essentially emulate what config.status does in
> Perl code. config.status gets a list of defines discovered by configure
> and processes pg_config.h.in to pg_config.h by substituting the defines.
> The MSVC build system basically has those defines hardcoded, but the
> processing we can do in just the same way. It already had code to do a
> bit of that anyway, so it's really not a big leap. See attached
> patches. (I put the remove of pg_config.h.win32 into a separate patch
> so that reviewers can just apply the first patch and then diff the
> produced pg_config.h with the existing pg_config.h.win32.)
Sounds good. I hadn't realized we already had the infrastructure ready
for this.
A couple of minor comments:
> + print $o "/* src/include/pg_config.h. Generated from
pg_config.h.in by ", basename(__FILE__), ". */\n";
How about just hardcoding this to "Generated from pg_config.h.in by
Solution.pm". Using basename(__FILE__) seems overly cute.
> + my @simple_defines = qw(
> + HAVE_ATOMICS
> + ... long list ...
> + USE_WIN32_SHARED_MEMORY
> + );
> +
> + foreach my $k (@simple_defines)
> + {
> + $define{$k} = 1;
> + }
I don't think this @simple_defines is really any better than listing all
the options directly with "$define{HAVE_ATOMICS} = 1". And some simple
defines are already listed like that, e.g. HAVE_DECL_STRNLEN above that
list.
- Heikki
On Fri, Dec 13, 2019 at 03:14:08PM +0200, Heikki Linnakangas wrote:
> On 13/12/2019 14:51, Peter Eisentraut wrote:
>> Keeping pg_config.h.win32 up to date with pg_config.h.in is a gratuitous
>> annoyance.
>
> Hear hear!
Youpi.
> I don't think this @simple_defines is really any better than listing all the
> options directly with "$define{HAVE_ATOMICS} = 1". And some simple defines
> are already listed like that, e.g. HAVE_DECL_STRNLEN above that list.
Agreed.
It would be nice to put a comment close to FLEXIBLE_ARRAY_MEMBER,
where you use "/* */" as a way to emulate an empty value which is
still defined. Or would it be cleaner to just use an empty string?
--
Michael
Attachment
Michael Paquier <michael@paquier.xyz> writes:
> On Fri, Dec 13, 2019 at 03:14:08PM +0200, Heikki Linnakangas wrote:
>> On 13/12/2019 14:51, Peter Eisentraut wrote:
>>> Keeping pg_config.h.win32 up to date with pg_config.h.in is a gratuitous
>>> annoyance.
>> Hear hear!
> Youpi.
+1
>> I don't think this @simple_defines is really any better than listing all the
>> options directly with "$define{HAVE_ATOMICS} = 1". And some simple defines
>> are already listed like that, e.g. HAVE_DECL_STRNLEN above that list.
> Agreed.
Yeah, having one style for setting a variable is better than having two.
One thing that disturbs me slightly is that the plan seems to be to
not mention variables in this list at all if they're to be undefined
on Windows. I realize that we've frequently done that by omission in
pg_config.h.win32, but I don't think it's good practice: it encourages
failure to think about how such variables need to be set on Windows.
Would it be reasonable to require every symbol found in pg_config.h.in
to be explicitly mentioned here? We could put the ones that are to
end up undefined in a separate %undefine hash, or we could have a
convention that an empty value in %define means to #undef it (though
I suppose that might be awkward in a few cases).
Either way, though, we'd end up with a situation where adding a new
configure symbol always requires touching Solution.pm, where before
it required touching pg_config.h.win32 (at least if you were being
strict about it). So in some sense this is no improvement. But we
do have the ability with this to do some computation to select the
variable value, so that's good.
regards, tom lane
On 2019-12-13 14:44, Michael Paquier wrote: > It would be nice to put a comment close to FLEXIBLE_ARRAY_MEMBER, > where you use "/* */" as a way to emulate an empty value which is > still defined. Or would it be cleaner to just use an empty string? That's just the way Autoconf does it. I haven't pondered why it's done that way, only focusing on making the resulting files match. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019-12-13 14:56, Tom Lane wrote: > One thing that disturbs me slightly is that the plan seems to be to > not mention variables in this list at all if they're to be undefined > on Windows. I realize that we've frequently done that by omission in > pg_config.h.win32, but I don't think it's good practice: it encourages > failure to think about how such variables need to be set on Windows. OK, here is an updated patch set that has all defines in one big Perl hash, and also requires that all symbols in pg_config.h.in are accounted for. (The indentation is from pgperltidy.) -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
On Mon, Dec 16, 2019 at 01:12:27PM +0100, Peter Eisentraut wrote:
> OK, here is an updated patch set that has all defines in one big Perl hash,
> and also requires that all symbols in pg_config.h.in are accounted for.
> (The indentation is from pgperltidy.)
The patch looks pretty clean. I have a few minor comments.
- if (/^AC_INIT\(\[PostgreSQL\], \[([^\]]+)\]/)
+ if (/^AC_INIT\(\[([^\]]+)\], \[([^\]]+)\], \[([^\]]+)\]/)
{
Why did you remove the bit about "PostgreSQL"?
+ ENABLE_GSS => $self->{options}->{gss} ? 1 :
undef,
[...]
- if ($self->{options}->{gss})
- {
- print $o "#define ENABLE_GSS 1\n";
I found the part about gss and nls better with the old style. A
matter of taste, not really an objection. And your style is actually
consistent with USE_ASSERT_CHECKING as well.
+ else
+ {
+ croak "missing: $macro";
+ }
[...]
+ if (scalar(keys %define) > 0)
+ {
+ croak "unused defines: @{[%define]}";
+ }
So you have checks both ways. That's nice.
+ open(my $i, '<', "src/include/pg_config.h.in")
+ || confess "Could not open pg_config.h.in\n";
+ open(my $o, '>', "src/include/pg_config.h")
+ || confess "Could not write to pg_config.h\n";
Failure to open pg_config.h.
Wouldn't it be better to remove pg_config_ext.h.win32 as well?
--
Michael
Attachment
On 2019-12-17 07:30, Michael Paquier wrote:
> The patch looks pretty clean. I have a few minor comments.
>
> - if (/^AC_INIT\(\[PostgreSQL\], \[([^\]]+)\]/)
> + if (/^AC_INIT\(\[([^\]]+)\], \[([^\]]+)\], \[([^\]]+)\]/)
> {
> Why did you remove the bit about "PostgreSQL"?
Just to make it more general. If we're going to parse the arguments,
why not parse all of them the same way.
> + open(my $i, '<', "src/include/pg_config.h.in")
> + || confess "Could not open pg_config.h.in\n";
> + open(my $o, '>', "src/include/pg_config.h")
> + || confess "Could not write to pg_config.h\n";
> Failure to open pg_config.h.
>
> Wouldn't it be better to remove pg_config_ext.h.win32 as well?
Yeah, good idea. Attached patch is refactored so all three header files
managed by AC_CONFIG_HEADERS are processed the same way.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
On Tue, Dec 17, 2019 at 11:56:17AM +0100, Peter Eisentraut wrote: > Yeah, good idea. Attached patch is refactored so all three header files > managed by AC_CONFIG_HEADERS are processed the same way. Looks good. I just have one comment. + # XXX + open(my $f, '>>', 'src/include/pg_config.h') + || confess "Could not write to src/include/pg_config.h\n"; + print $f "\n"; + print $f "#define VAL_CONFIGURE \"" + . $self->GetFakeConfigure() . "\"\n"; + close($f); This part needs a comment. Like it is the equivalent of what src/common/'s Makefile does or something like that? -- Michael
Attachment
On 2019-12-19 04:59, Michael Paquier wrote: > On Tue, Dec 17, 2019 at 11:56:17AM +0100, Peter Eisentraut wrote: >> Yeah, good idea. Attached patch is refactored so all three header files >> managed by AC_CONFIG_HEADERS are processed the same way. > > Looks good. I just have one comment. > > + # XXX > + open(my $f, '>>', 'src/include/pg_config.h') > + || confess "Could not write to src/include/pg_config.h\n"; > + print $f "\n"; > + print $f "#define VAL_CONFIGURE \"" > + . $self->GetFakeConfigure() . "\"\n"; > + close($f); > > This part needs a comment. Like it is the equivalent of what > src/common/'s Makefile does or something like that? This was meant to be addressed by <https://www.postgresql.org/message-id/flat/6e457870-cef5-5f1d-b57c-fc89cfb8a788%402ndquadrant.com>, but that discussion has not concluded yet. Perhaps it makes more sense in this context. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Dec 19, 2019 at 08:31:05AM +0100, Peter Eisentraut wrote: > On 2019-12-19 04:59, Michael Paquier wrote: >> This part needs a comment. Like it is the equivalent of what >> src/common/'s Makefile does or something like that? > > This was meant to be addressed by <https://www.postgresql.org/message-id/flat/6e457870-cef5-5f1d-b57c-fc89cfb8a788%402ndquadrant.com>, > but that discussion has not concluded yet. Perhaps it makes more sense in > this context. Hmm. Your patch does not really change the generation of VAL_CONFIGURE in pg_config.h, so I am not sure that this other thread is an actual barrier for the improvement discussed here. I would be actually fine to just remove the XXX and still use GetFakeConfigure() with VAL_CONFIGURE for now. It would be a good thing to get rid of pg_config.h.win32 definitely. -- Michael
Attachment
On 2019-12-19 08:49, Michael Paquier wrote: > On Thu, Dec 19, 2019 at 08:31:05AM +0100, Peter Eisentraut wrote: >> On 2019-12-19 04:59, Michael Paquier wrote: >>> This part needs a comment. Like it is the equivalent of what >>> src/common/'s Makefile does or something like that? >> >> This was meant to be addressed by <https://www.postgresql.org/message-id/flat/6e457870-cef5-5f1d-b57c-fc89cfb8a788%402ndquadrant.com>, >> but that discussion has not concluded yet. Perhaps it makes more sense in >> this context. > > Hmm. Your patch does not really change the generation of > VAL_CONFIGURE in pg_config.h, so I am not sure that this other thread > is an actual barrier for the improvement discussed here. I would be > actually fine to just remove the XXX and still use GetFakeConfigure() > with VAL_CONFIGURE for now. It would be a good thing to get rid of > pg_config.h.win32 definitely. committed with that comment removed -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Dec 20, 2019 at 09:17:14AM +0100, Peter Eisentraut wrote: > committed with that comment removed Yeah, thanks! -- Michael