Horton

Thread

Commits

  1. meson: Fix installation path computation

  1. meson oddities

    Andrew Dunstan <andrew@dunslane.net> — 2022-11-14T22:41:54Z

    Here's a couple of things I've noticed.


andrew@ub22:HEAD $ inst.meson/bin/pg_config --libdir --ldflags
/home/andrew/pgl/pg_head/root/HEAD/inst.meson/lib/x86_64-linux-gnu
-fuse-ld=lld -DCOPY_PARSE_PLAN_TREES -DRAW_EXPRESSION_COVERAGE_TEST
-DWRITE_READ_PARSE_PLAN_TREES


Are we really intending to add a new subdirectory to the default layout?
Why is that x84_64-linux-gnu there?

Also, why have the CPPFLAGS made their way into the LDFLAGS? That seems
wrong.


cheers


andrew

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

  2. Re: meson oddities

    Michael Paquier <michael@paquier.xyz> — 2022-11-14T23:22:59Z

    On Mon, Nov 14, 2022 at 05:41:54PM -0500, Andrew Dunstan wrote:
> Also, why have the CPPFLAGS made their way into the LDFLAGS? That seems
> wrong.

Not only CPPFLAGS.  I pass down some custom CFLAGS to the meson
command as well, and these find their way to LDFLAGS on top of
CFLAGS for the user-defined entries.  I would not have expected that,
either.
--
Michael

  3. Re: meson oddities

    Andres Freund <andres@anarazel.de> — 2022-11-14T23:24:07Z

    Hi,

On 2022-11-14 17:41:54 -0500, Andrew Dunstan wrote:
> Here's a couple of things I've noticed.
> 
> 
> andrew@ub22:HEAD $ inst.meson/bin/pg_config --libdir --ldflags
> /home/andrew/pgl/pg_head/root/HEAD/inst.meson/lib/x86_64-linux-gnu
> -fuse-ld=lld -DCOPY_PARSE_PLAN_TREES -DRAW_EXPRESSION_COVERAGE_TEST
> -DWRITE_READ_PARSE_PLAN_TREES
> 
> 
> Are we really intending to add a new subdirectory to the default layout?
> Why is that x84_64-linux-gnu there?

It's the platform default on, at least, debian derived distros - that's how
you can install 32bit/64bit libraries and libraries with different ABIs
(e.g. linking against glibc vs linking with musl) in parallel.

We could override meson inferring that from the system if we want to, but it
doesn't seem like a good idea?


> Also, why have the CPPFLAGS made their way into the LDFLAGS? That seems
> wrong.

Because these days meson treats CPPFLAGS as part of CFLAGS as it apparently
repeatedly confused build system writers and users when e.g. header-presence
checks would only use CPPFLAGS. Some compiler options aren't entirely clearly
delineated, consider e.g. -isystem (influencing warning behaviour as well as
preprocessor paths).  Not sure if that's the best choice, but it's imo
defensible.

Greetings,

Andres Freund

  4. Re: meson oddities

    Andres Freund <andres@anarazel.de> — 2022-11-14T23:48:12Z

    Hi,

On 2022-11-15 08:22:59 +0900, Michael Paquier wrote:
> I pass down some custom CFLAGS to the meson command as well, and these find
> their way to LDFLAGS on top of CFLAGS for the user-defined entries.  I would
> not have expected that, either.

We effectively do that with autoconf as well, except that we don't mention
that in pg_config --ldflags. Our linking rules include CFLAGS, see e.g.:

%: %.o
	$(CC) $(CFLAGS) $^ $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)

postgres: $(OBJS)
	$(CC) $(CFLAGS) $(call expand_subsys,$^) $(LDFLAGS) $(LDFLAGS_EX) $(export_dynamic) $(LIBS) -o $@

ifdef PROGRAM
$(PROGRAM): $(OBJS)
	$(CC) $(CFLAGS) $(OBJS) $(PG_LIBS_INTERNAL) $(LDFLAGS) $(LDFLAGS_EX) $(PG_LIBS) $(LIBS) -o $@$(X)
endif

# Rule for building a shared library from a single .o file
%.so: %.o
	$(CC) $(CFLAGS) $< $(LDFLAGS) $(LDFLAGS_SL) -shared -o $@


Should we try that fact in pg_configin the meson build as well?


Meson automatically includes compiler flags during linking because a)
apparently many dependencies (.pc files etc) specify linker flags in CFLAGS b)
at least some kinds of LTO requires compiler flags being present during
"linking".

Greetings,

Andres Freund

  5. Re: meson oddities

    Andrew Dunstan <andrew@dunslane.net> — 2022-11-15T13:04:29Z

    On 2022-11-14 Mo 18:24, Andres Freund wrote:
> Hi,
>
> On 2022-11-14 17:41:54 -0500, Andrew Dunstan wrote:
>> Here's a couple of things I've noticed.
>>
>>
>> andrew@ub22:HEAD $ inst.meson/bin/pg_config --libdir --ldflags
>> /home/andrew/pgl/pg_head/root/HEAD/inst.meson/lib/x86_64-linux-gnu
>> -fuse-ld=lld -DCOPY_PARSE_PLAN_TREES -DRAW_EXPRESSION_COVERAGE_TEST
>> -DWRITE_READ_PARSE_PLAN_TREES
>>
>>
>> Are we really intending to add a new subdirectory to the default layout?
>> Why is that x84_64-linux-gnu there?
> It's the platform default on, at least, debian derived distros - that's how
> you can install 32bit/64bit libraries and libraries with different ABIs
> (e.g. linking against glibc vs linking with musl) in parallel.
>
> We could override meson inferring that from the system if we want to, but it
> doesn't seem like a good idea?
>

That's a decision that packagers make. e.g. on my Ubuntu system
configure has been run with:

--libdir=${prefix}/lib/x86_64-linux-gnu


Incidentally, Redhat flavored systems don't use this layout. they have
/lib and /lib64, so it's far from universal.


But ISTM we shouldn't be presuming what packagers will do, and that
there is some virtue in having a default layout under ${prefix} that is
consistent across platforms, as is now the case with autoconf/configure.


>> Also, why have the CPPFLAGS made their way into the LDFLAGS? That seems
>> wrong.
> Because these days meson treats CPPFLAGS as part of CFLAGS as it apparently
> repeatedly confused build system writers and users when e.g. header-presence
> checks would only use CPPFLAGS. Some compiler options aren't entirely clearly
> delineated, consider e.g. -isystem (influencing warning behaviour as well as
> preprocessor paths).  Not sure if that's the best choice, but it's imo
> defensible.
>

Yes, I get that there is confusion around CPPFLAGS. One of my otherwise
extremely knowledgeable colleagues told me a year or two back that he
had thought the CPP in CPPFLAGS referred to C++ rather that C
preprocessor. And the authors of meson seem to have labored under a
similar misapprehension, so they use 'cpp' instead of 'cxx' like just
about everyone else.

But it's less clear to me that a bunch of defines belong in LDFLAGS.
Shouldn't that be only things that ld itself will recognize?


cheers


andrew

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

  6. Re: meson oddities

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-11-15T16:07:05Z

    On 15.11.22 00:48, Andres Freund wrote:
> We effectively do that with autoconf as well, except that we don't mention
> that in pg_config --ldflags. Our linking rules include CFLAGS, see e.g.:
> 
> %: %.o
> 	$(CC) $(CFLAGS) $^ $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)
> 
> postgres: $(OBJS)
> 	$(CC) $(CFLAGS) $(call expand_subsys,$^) $(LDFLAGS) $(LDFLAGS_EX) $(export_dynamic) $(LIBS) -o $@
> 
> ifdef PROGRAM
> $(PROGRAM): $(OBJS)
> 	$(CC) $(CFLAGS) $(OBJS) $(PG_LIBS_INTERNAL) $(LDFLAGS) $(LDFLAGS_EX) $(PG_LIBS) $(LIBS) -o $@$(X)
> endif
> 
> # Rule for building a shared library from a single .o file
> %.so: %.o
> 	$(CC) $(CFLAGS) $< $(LDFLAGS) $(LDFLAGS_SL) -shared -o $@
> 
> 
> Should we try that fact in pg_configin the meson build as well?

It's up to the consumer of pg_config to apply CFLAGS and LDFLAGS as they 
need.  But pg_config and pkg-config etc. should report them separately.

  7. Re: meson oddities

    Andres Freund <andres@anarazel.de> — 2022-11-15T19:04:25Z

    Hi,

On 2022-11-15 08:04:29 -0500, Andrew Dunstan wrote:
> On 2022-11-14 Mo 18:24, Andres Freund wrote:
> > Hi,
> >
> > On 2022-11-14 17:41:54 -0500, Andrew Dunstan wrote:
> >> Here's a couple of things I've noticed.
> >>
> >>
> >> andrew@ub22:HEAD $ inst.meson/bin/pg_config --libdir --ldflags
> >> /home/andrew/pgl/pg_head/root/HEAD/inst.meson/lib/x86_64-linux-gnu
> >> -fuse-ld=lld -DCOPY_PARSE_PLAN_TREES -DRAW_EXPRESSION_COVERAGE_TEST
> >> -DWRITE_READ_PARSE_PLAN_TREES
> >>
> >>
> >> Are we really intending to add a new subdirectory to the default layout?
> >> Why is that x84_64-linux-gnu there?
> > It's the platform default on, at least, debian derived distros - that's how
> > you can install 32bit/64bit libraries and libraries with different ABIs
> > (e.g. linking against glibc vs linking with musl) in parallel.
> >
> > We could override meson inferring that from the system if we want to, but it
> > doesn't seem like a good idea?
> >
> 
> That's a decision that packagers make. e.g. on my Ubuntu system
> configure has been run with:
> 
> --libdir=${prefix}/lib/x86_64-linux-gnu

Sure - but that doesn't mean that it's a good idea to break the distribution's
layout when you install from source.


> Incidentally, Redhat flavored systems don't use this layout. they have
> /lib and /lib64, so it's far from universal.

Meson infers that and uses lib64 as the default libdir.


> But ISTM we shouldn't be presuming what packagers will do, and that
> there is some virtue in having a default layout under ${prefix} that is
> consistent across platforms, as is now the case with autoconf/configure.

I don't think it's a virtue to break the layout of the platform by
e.g. installing 64bit libs into the directory containing 32bit libs.


> And the authors of meson seem to have labored under a similar
> misapprehension, so they use 'cpp' instead of 'cxx' like just about everyone
> else.

Yea, not a fan of that either. I don't think it was a misapprehension, but a
decision I disagree with...


> But it's less clear to me that a bunch of defines belong in LDFLAGS.
> Shouldn't that be only things that ld itself will recognize?

I don't think there's a clear cut line what is for ld and what
isn't. Including stuff that influences both preprocessor and
linker. -ffreestanding will e.g. change preprocessor, compiler (I think), and
linker behaviour.

Greetings,

Andres Freund

  8. Re: meson oddities

    Andrew Dunstan <andrew@dunslane.net> — 2022-11-15T20:47:39Z

    On 2022-11-15 Tu 14:04, Andres Freund wrote:
>> But ISTM we shouldn't be presuming what packagers will do, and that
>> there is some virtue in having a default layout under ${prefix} that is
>> consistent across platforms, as is now the case with autoconf/configure.
> I don't think it's a virtue to break the layout of the platform by
> e.g. installing 64bit libs into the directory containing 32bit libs.


You might end up surprising people who have installed from source for
years and will have the layout suddenly changed, especially on RedHat
flavored systems.

I can work around it in the buildfarm, which does make some assumptions
about the layout (e.g. in the cross version pg_upgrade stuff), by
explicitly using --libdir.


>> But it's less clear to me that a bunch of defines belong in LDFLAGS.
>> Shouldn't that be only things that ld itself will recognize?
> I don't think there's a clear cut line what is for ld and what
> isn't. Including stuff that influences both preprocessor and
> linker. -ffreestanding will e.g. change preprocessor, compiler (I think), and
> linker behaviour.
>

Well it sure looks odd.


cheers


andrew


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

  9. Re: meson oddities

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-11-15T21:08:35Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> On 2022-11-15 Tu 14:04, Andres Freund wrote:
>> I don't think it's a virtue to break the layout of the platform by
>> e.g. installing 64bit libs into the directory containing 32bit libs.

> You might end up surprising people who have installed from source for
> years and will have the layout suddenly changed, especially on RedHat
> flavored systems.

Yeah, I'm not too pleased with this idea either.  The people who want
to install according to some platform-specific plan have already figured
out how to do that.  People who are accustomed to the way PG has done
it in the past are not likely to think this is an improvement.	 

Also, unless you intend to drop the special cases involving whether
the install path string contains "postgres" or "pgsql", it's already
not platform-standard.

			regards, tom lane

  10. Re: meson oddities

    Andres Freund <andres@anarazel.de> — 2022-11-15T23:40:42Z

    Hi,

On 2022-11-15 16:08:35 -0500, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
> > On 2022-11-15 Tu 14:04, Andres Freund wrote:
> >> I don't think it's a virtue to break the layout of the platform by
> >> e.g. installing 64bit libs into the directory containing 32bit libs.
>
> > You might end up surprising people who have installed from source for
> > years and will have the layout suddenly changed, especially on RedHat
> > flavored systems.

Just to make sure that's clear: meson defaults to lib/ or lib64/ (depending on
bitness obviously) on RedHat systems, not lib/i386-linux-gnu/ or
lib/x86_64-linux-gnu.


> Yeah, I'm not too pleased with this idea either.  The people who want
> to install according to some platform-specific plan have already figured
> out how to do that.  People who are accustomed to the way PG has done
> it in the past are not likely to think this is an improvement.

I think that's a good argument to not change the default for configure, but
imo not a good argument for forcing 'lib' rather than the appropriate platform
default in the meson build, given that that already requires changing existing
recipes.

Small note: I didn't intentionally make that change during the meson porting
work, it's just meson's default.

I can live with forcing lib/, but I don't think it's the better solution long
term. And this seems like the best point for switching we're going to get.


We'd just have to add 'libdir=lib' to the default_options array in the
toplevel meson.build.


> Also, unless you intend to drop the special cases involving whether
> the install path string contains "postgres" or "pgsql", it's already
> not platform-standard.

For me that's the best argument for forcing 'lib'. Still not quite enough to
swing me around, because it's imo a pretty reasonable thing to want to install
a 32bit and 64bit libpq, and I don't think we should make that harder.

Somewhat relatedly, I wonder if we should have a better way to enable/disable
the 'pgsql' path logic. It's pretty annoying that prefix basically doesn't
work if it doesn't contain 'pgsql' or 'postgres'.

Greetings,

Andres Freund

  11. Re: meson oddities

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-11-16T09:53:59Z

    On 16.11.22 00:40, Andres Freund wrote:
> Somewhat relatedly, I wonder if we should have a better way to enable/disable
> the 'pgsql' path logic. It's pretty annoying that prefix basically doesn't
> work if it doesn't contain 'pgsql' or 'postgres'.

Could you explain this in more detail?

  12. Re: meson oddities

    Andres Freund <andres@anarazel.de> — 2022-11-16T16:40:10Z

    Hi,

On 2022-11-16 10:53:59 +0100, Peter Eisentraut wrote:
> On 16.11.22 00:40, Andres Freund wrote:
> > Somewhat relatedly, I wonder if we should have a better way to enable/disable
> > the 'pgsql' path logic. It's pretty annoying that prefix basically doesn't
> > work if it doesn't contain 'pgsql' or 'postgres'.
> 
> Could you explain this in more detail?

If I just want to install postgres into a prefix without 'postgresql' added in
a bunch of directories, e.g. because I already have pg-$version to be in the
prefix, there's really no good way to do so - you can't even specify
--sysconfdir or such, because we just override that path.

And because many of our binaries are major version specific you pretty much
need to include the major version in the prefix, making the 'postgresql' we
add redundant.

I think the easiest way today is to use a temporary prefix and then just
rename the installation path. But that obviously doesn't deal well with
rpaths, at least as long as we don't use relative rpaths.

Greetings,

Andres Freund

  13. Re: meson oddities

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-11-16T16:54:10Z

    Andres Freund <andres@anarazel.de> writes:
> On 2022-11-16 10:53:59 +0100, Peter Eisentraut wrote:
>> Could you explain this in more detail?

> If I just want to install postgres into a prefix without 'postgresql' added in
> a bunch of directories, e.g. because I already have pg-$version to be in the
> prefix, there's really no good way to do so - you can't even specify
> --sysconfdir or such, because we just override that path.

At least for the libraries, the point of the 'postgresql' subdir IMO
is to keep backend-loadable extensions separate from random libraries.
It's not great that we may fail to do that depending on what the
initial part of the library path is.

I could get behind allowing the user to specify that path explicitly
and then not modifying it; but when we're left to our own devices
I think we should preserve that separation.

			regards, tom lane

  14. Re: meson oddities

    Andres Freund <andres@anarazel.de> — 2022-11-16T17:07:47Z

    Hi,

On 2022-11-16 11:54:10 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2022-11-16 10:53:59 +0100, Peter Eisentraut wrote:
> >> Could you explain this in more detail?
> 
> > If I just want to install postgres into a prefix without 'postgresql' added in
> > a bunch of directories, e.g. because I already have pg-$version to be in the
> > prefix, there's really no good way to do so - you can't even specify
> > --sysconfdir or such, because we just override that path.
> 
> At least for the libraries, the point of the 'postgresql' subdir IMO
> is to keep backend-loadable extensions separate from random libraries.
> It's not great that we may fail to do that depending on what the
> initial part of the library path is.

Agreed, extensions really should never be in a path searched by the dynamic
linker, even if the prefix contains 'postgres'.

To me that's a separate thing from adding postgresql to datadir, sysconfdir,
includedir, docdir... On a green field I'd say the 'extension library'
directory should just always be extensions/ or such.

Greetings,

Andres Freund

  15. Re: meson oddities

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-01-04T11:35:35Z

    On 16.11.22 18:07, Andres Freund wrote:
>>> If I just want to install postgres into a prefix without 'postgresql' added in
>>> a bunch of directories, e.g. because I already have pg-$version to be in the
>>> prefix, there's really no good way to do so - you can't even specify
>>> --sysconfdir or such, because we just override that path.
>>
>> At least for the libraries, the point of the 'postgresql' subdir IMO
>> is to keep backend-loadable extensions separate from random libraries.
>> It's not great that we may fail to do that depending on what the
>> initial part of the library path is.
> 
> Agreed, extensions really should never be in a path searched by the dynamic
> linker, even if the prefix contains 'postgres'.
> 
> To me that's a separate thing from adding postgresql to datadir, sysconfdir,
> includedir, docdir... On a green field I'd say the 'extension library'
> directory should just always be extensions/ or such.

I think we should get the two build systems to produce the same 
installation layout when given equivalent options.

Unless someone comes up with a proposal to address the above broader 
issues, also taking into account current packaging practices etc., then 
I think we should do a short-term solution to either port the 
subdir-appending to the meson scripts or remove it from the makefiles 
(or maybe a bit of both).

  16. Re: meson oddities

    Andres Freund <andres@anarazel.de> — 2023-01-04T19:35:02Z

    Hi,

On 2023-01-04 12:35:35 +0100, Peter Eisentraut wrote:
> On 16.11.22 18:07, Andres Freund wrote:
> > > > If I just want to install postgres into a prefix without 'postgresql' added in
> > > > a bunch of directories, e.g. because I already have pg-$version to be in the
> > > > prefix, there's really no good way to do so - you can't even specify
> > > > --sysconfdir or such, because we just override that path.
> > >
> > > At least for the libraries, the point of the 'postgresql' subdir IMO
> > > is to keep backend-loadable extensions separate from random libraries.
> > > It's not great that we may fail to do that depending on what the
> > > initial part of the library path is.
> >
> > Agreed, extensions really should never be in a path searched by the dynamic
> > linker, even if the prefix contains 'postgres'.
> >
> > To me that's a separate thing from adding postgresql to datadir, sysconfdir,
> > includedir, docdir... On a green field I'd say the 'extension library'
> > directory should just always be extensions/ or such.
>
> I think we should get the two build systems to produce the same installation
> layout when given equivalent options.

I'm not convinced that that's the right thing to do. Distributions have
helper infrastructure for buildsystems - why should we make it harder for them
by deviating further from the buildsystem defaults?

I have yet to hear an argument why installing libraries below
/usr/[local]/lib/{x86_64,i386,...}-linux-{gnu,musl,...}/ is the wrong thing to
do on Debian based systems (or similar, choosing lib64 over lib on RH based
systems).  But at the same time I haven't heard of an argument why we should
break existing scripts building with autoconf for this. To me a different
buildsystem is a convenient point to adapt to build path from the last decade.


> Unless someone comes up with a proposal to address the above broader issues,
> also taking into account current packaging practices etc., then I think we
> should do a short-term solution to either port the subdir-appending to the
> meson scripts or remove it from the makefiles (or maybe a bit of both).

Just to be clear, with 'subdir-appending' you mean libdir defaulting to
'lib/x86_64-linux-gnu' (or similar)? Or do you mean adding 'postgresql' into
various dirs when the path doesn't already contain postgres?

I did try to mirror the 'postgresql' adding bit in the meson build.

Greetings,

Andres Freund

  17. Re: meson oddities

    Robert Haas <robertmhaas@gmail.com> — 2023-01-04T21:06:15Z

    On Wed, Jan 4, 2023 at 2:35 PM Andres Freund <andres@anarazel.de> wrote:
> > I think we should get the two build systems to produce the same installation
> > layout when given equivalent options.
>
> I'm not convinced that that's the right thing to do. Distributions have
> helper infrastructure for buildsystems - why should we make it harder for them
> by deviating further from the buildsystem defaults?

If we don't do as Peter suggests, then any difference between the
results of one build system and the other could either be a bug or an
intentional deviation. There will be no easy way to know which it is.
And if or when people switch build systems, stuff will be randomly
different, and they won't understand why.

I hear your point too. It's unpleasant for you to spend a lot of
effort overriding meson's behavior if the result is arguably worse
than the default, and it has the effect of carrying forward in
perpetuity hacks that may not have been a good idea in the first
place, or may not be a good idea any more. Those seem like valid
concerns, too.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  18. Re: meson oddities

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-04T21:18:38Z

    Robert Haas <robertmhaas@gmail.com> writes:
> If we don't do as Peter suggests, then any difference between the
> results of one build system and the other could either be a bug or an
> intentional deviation. There will be no easy way to know which it is.
> And if or when people switch build systems, stuff will be randomly
> different, and they won't understand why.

> I hear your point too. It's unpleasant for you to spend a lot of
> effort overriding meson's behavior if the result is arguably worse
> than the default, and it has the effect of carrying forward in
> perpetuity hacks that may not have been a good idea in the first
> place, or may not be a good idea any more. Those seem like valid
> concerns, too.

Yeah.  I think the way forward probably needs to be to decide that
we are (or are not) going to make changes to the installation tree
layout, and then make both build systems conform to that.  I don't
really buy the argument that it's okay to let them install different
layouts.  I *am* prepared to listen to arguments that "this is dumb
and we shouldn't do it anymore".

			regards, tom lane

  19. Re: meson oddities

    Andres Freund <andres@anarazel.de> — 2023-01-04T21:29:10Z

    Hi,

On 2023-01-04 16:18:38 -0500, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > If we don't do as Peter suggests, then any difference between the
> > results of one build system and the other could either be a bug or an
> > intentional deviation. There will be no easy way to know which it is.
> > And if or when people switch build systems, stuff will be randomly
> > different, and they won't understand why.

Given the difference is "localized", I think calling this out in the docs
would contain confusion.


> > I hear your point too. It's unpleasant for you to spend a lot of
> > effort overriding meson's behavior if the result is arguably worse
> > than the default, and it has the effect of carrying forward in
> > perpetuity hacks that may not have been a good idea in the first
> > place, or may not be a good idea any more. Those seem like valid
> > concerns, too.

This specific instance luckily is trivial to change from code POV.


> Yeah.  I think the way forward probably needs to be to decide that
> we are (or are not) going to make changes to the installation tree
> layout, and then make both build systems conform to that.  I don't
> really buy the argument that it's okay to let them install different
> layouts.  I *am* prepared to listen to arguments that "this is dumb
> and we shouldn't do it anymore".

What exactly shouldn't we do anymore?

I just want to re-iterate that, in my understanding, what we're talking about
here is just whether libdir defaults to just "lib" or whether it adapts to the
platform default (so we end up with libdir as 'lib64' or
'lib/x86_64-linux-gnu' etc). And *not* whether we should continue to force
"postgresql" into the paths.

Greetings,

Andres Freund

  20. Re: meson oddities

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-01-04T22:17:30Z

    On 04.01.23 20:35, Andres Freund wrote:
>> Unless someone comes up with a proposal to address the above broader issues,
>> also taking into account current packaging practices etc., then I think we
>> should do a short-term solution to either port the subdir-appending to the
>> meson scripts or remove it from the makefiles (or maybe a bit of both).
> Just to be clear, with 'subdir-appending' you mean libdir defaulting to
> 'lib/x86_64-linux-gnu' (or similar)? Or do you mean adding 'postgresql' into
> various dirs when the path doesn't already contain postgres?
> 
> I did try to mirror the 'postgresql' adding bit in the meson build.

I meant the latter, which I see is already in there, but it doesn't 
actually fully work.  It only looks at the subdirectory (like "lib"), 
not the whole path (like "/usr/local/pgsql/lib").  With the attached 
patch I have it working and I get the same installation layout from both 
build systems.

  21. Re: meson oddities

    Andres Freund <andres@anarazel.de> — 2023-01-04T22:53:54Z

    Hi,

On 2023-01-04 23:17:30 +0100, Peter Eisentraut wrote:
> I meant the latter, which I see is already in there, but it doesn't actually
> fully work.  It only looks at the subdirectory (like "lib"), not the whole
> path (like "/usr/local/pgsql/lib").  With the attached patch I have it
> working and I get the same installation layout from both build systems.

Oh, oops. I tested this at some point, but I guess I over-simplified it at
some point.

Then I have zero objections to this. One question below though.



>  dir_data = get_option('datadir')
> -if not (dir_data.contains('pgsql') or dir_data.contains('postgres'))
> +if not ((dir_prefix/dir_data).contains('pgsql') or (dir_prefix/dir_data).contains('postgres'))
>    dir_data = dir_data / pkg
>  endif

Hm. Perhaps we should just test once whether prefix contains pgsql/postgres,
and then just otherwise leave the test as is? There afaict can't be a
dir_prefix/dir_* that matches postgres/pgsql that won't also match either of
the components.

Greetings,

Andres Freund

  22. Re: meson oddities

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-01-11T11:05:34Z

    On 04.01.23 23:53, Andres Freund wrote:
>>   dir_data = get_option('datadir')
>> -if not (dir_data.contains('pgsql') or dir_data.contains('postgres'))
>> +if not ((dir_prefix/dir_data).contains('pgsql') or (dir_prefix/dir_data).contains('postgres'))
>>     dir_data = dir_data / pkg
>>   endif
> Hm. Perhaps we should just test once whether prefix contains pgsql/postgres,
> and then just otherwise leave the test as is? There afaict can't be a
> dir_prefix/dir_* that matches postgres/pgsql that won't also match either of
> the components.

You mean something like

     dir_prefix_contains_pg =
       (dir_prefix.contains('pgsql') or dir_prefix.contains('postgres'))

and

     if not (dir_prefix_contains_pg or
            (dir_data.contains('pgsql') or dir_data.contains('postgres'))

Seems more complicated to me.

I think there is also an adjacent issue:  The subdir options may be 
absolute or relative.  So if you specify --prefix=/usr/local and 
--sysconfdir=/etc/postgresql, then

     config_paths_data.set_quoted('SYSCONFDIR', dir_prefix / dir_sysconf)

would produce something like /usr/local/etc/postgresql.

I think maybe we should make all the dir_* variables absolute right at 
the beginning, like

     dir_lib = get_option('libdir')
     if not fs.is_absolute(dir_lib)
         dir_lib = dir_prefix / dir_lib
     endif

And then the appending stuff could be done after that, keeping the 
current code.

  23. Re: meson oddities

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-01-19T20:37:15Z

    On 11.01.23 12:05, Peter Eisentraut wrote:
> I think there is also an adjacent issue:  The subdir options may be 
> absolute or relative.  So if you specify --prefix=/usr/local and 
> --sysconfdir=/etc/postgresql, then
> 
>      config_paths_data.set_quoted('SYSCONFDIR', dir_prefix / dir_sysconf)
> 
> would produce something like /usr/local/etc/postgresql.
> 
> I think maybe we should make all the dir_* variables absolute right at 
> the beginning, like
> 
>      dir_lib = get_option('libdir')
>      if not fs.is_absolute(dir_lib)
>          dir_lib = dir_prefix / dir_lib
>      endif
> 
> And then the appending stuff could be done after that, keeping the 
> current code.

Here is a proposed patch.  This should fix all these issues.

  24. Re: meson oddities

    Andres Freund <andres@anarazel.de> — 2023-01-19T20:45:49Z

    Hi,

On 2023-01-19 21:37:15 +0100, Peter Eisentraut wrote:
> On 11.01.23 12:05, Peter Eisentraut wrote:
> > I think there is also an adjacent issue:  The subdir options may be
> > absolute or relative.  So if you specify --prefix=/usr/local and
> > --sysconfdir=/etc/postgresql, then
> > 
> >      config_paths_data.set_quoted('SYSCONFDIR', dir_prefix / dir_sysconf)
> > 
> > would produce something like /usr/local/etc/postgresql.

I don't think it would. The / operator understands absolute paths and doesn't
add the "first component" if the second component is absolute.


>  
>  dir_bin = get_option('bindir')
> +if not fs.is_absolute(dir_bin)
> +  dir_bin = dir_prefix / dir_bin
> +endif

Hm, I'm not sure this works entirely right on windows. A path like /blub isn't
absolute on windows, but it's not really relative either. It's a "drive local"
path. I.e. relative to the current drive (c:/), but not the subdirectory
therein.

Greetings,

Andres Freund

  25. Re: meson oddities

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-01-26T09:20:58Z

    On 19.01.23 21:45, Andres Freund wrote:
> Hi,
> 
> On 2023-01-19 21:37:15 +0100, Peter Eisentraut wrote:
>> On 11.01.23 12:05, Peter Eisentraut wrote:
>>> I think there is also an adjacent issue:  The subdir options may be
>>> absolute or relative.  So if you specify --prefix=/usr/local and
>>> --sysconfdir=/etc/postgresql, then
>>>
>>>       config_paths_data.set_quoted('SYSCONFDIR', dir_prefix / dir_sysconf)
>>>
>>> would produce something like /usr/local/etc/postgresql.
> 
> I don't think it would. The / operator understands absolute paths and doesn't
> add the "first component" if the second component is absolute.

Oh, that is interesting.  In that case, this is not the right patch.  We 
should proceed with my previous patch in [0] then.

[0]: 
https://www.postgresql.org/message-id/a6a6de12-f705-2b33-2fd9-9743277deb08@enterprisedb.com

  26. Re: meson oddities

    Andres Freund <andres@anarazel.de> — 2023-01-26T18:05:04Z

    On 2023-01-26 10:20:58 +0100, Peter Eisentraut wrote:
> On 19.01.23 21:45, Andres Freund wrote:
> > Hi,
> > 
> > On 2023-01-19 21:37:15 +0100, Peter Eisentraut wrote:
> > > On 11.01.23 12:05, Peter Eisentraut wrote:
> > > > I think there is also an adjacent issue:  The subdir options may be
> > > > absolute or relative.  So if you specify --prefix=/usr/local and
> > > > --sysconfdir=/etc/postgresql, then
> > > > 
> > > >       config_paths_data.set_quoted('SYSCONFDIR', dir_prefix / dir_sysconf)
> > > > 
> > > > would produce something like /usr/local/etc/postgresql.
> > 
> > I don't think it would. The / operator understands absolute paths and doesn't
> > add the "first component" if the second component is absolute.
> 
> Oh, that is interesting.  In that case, this is not the right patch.  We
> should proceed with my previous patch in [0] then.

WFM.

I still think it'd be slightly more legible if we tested the prefix for
postgres|pgsql once, rather than do the per-variable .contains() checks on the
"combined" path. But it's a pretty minor difference, and I'd have no problem
with you comitting your version.

Basically:
is_pg_prefix = dir_prefix.contains('pgsql) or dir_prefix.contains('postgres')
...
if not (is_pg_prefix or dir_data.contains('pgsql') or dir_data.contains('postgres'))

instead of "your":

if not ((dir_prefix/dir_data).contains('pgsql') or (dir_prefix/dir_data).contains('postgres'))

Greetings,

Andres Freund

  27. Re: meson oddities

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-01-27T10:58:51Z

    On 26.01.23 19:05, Andres Freund wrote:
>> Oh, that is interesting.  In that case, this is not the right patch.  We
>> should proceed with my previous patch in [0] then.
> WFM.
> 
> I still think it'd be slightly more legible if we tested the prefix for
> postgres|pgsql once, rather than do the per-variable .contains() checks on the
> "combined" path.

Ok, committed with that change.