Thread: meson oddities
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
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
Attachment
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.
Attachment
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.
Attachment
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] then.
[0]:
https://www.postgresql.org/message-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.