meson oddities

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

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

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

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

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.

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

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

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

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

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?

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

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

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

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).

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

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

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

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

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.

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

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.

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.

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

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]/messages/by-id/a6a6de12-f705-2b33-2fd9-9743277deb08@enterprisedb.com then.

[0]: /messages/by-id/a6a6de12-f705-2b33-2fd9-9743277deb08@enterprisedb.com
/messages/by-id/a6a6de12-f705-2b33-2fd9-9743277deb08@enterprisedb.com

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

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.