Thread: meson: Non-feature feature options
Most meson options (meson_options.txt) that enable an external
dependency (e.g., icu, ldap) are of type 'feature'. Most of these have
a default value of 'auto', which means they are pulled in automatically
if found. Some have a default value of 'disabled' for specific reasons
(e.g., selinux). This is all good.
Two options deviate from this in annoying ways:
option('ssl', type : 'combo', choices : ['none', 'openssl'],
value : 'none',
description: 'use LIB for SSL/TLS support (openssl)')
option('uuid', type : 'combo', choices : ['none', 'bsd', 'e2fs', 'ossp'],
value : 'none',
description: 'build contrib/uuid-ossp using LIB')
These were moved over from configure like that.
The problem is that these features now cannot be automatically enabled
and behave annoyingly different from other feature options.
For the 'ssl' option, we have deprecated the --with-openssl option in
configure and replaced it with --with-ssl, in anticipation of other SSL
implementations. None of that ever happened or is currently planned
AFAICT. So I suggest that we semi-revert this, so that we can make
'openssl' an auto option in meson.
For the 'uuid' option, I'm not sure what the best way to address this
would. We could establish a search order of libraries that is used if
no specific one is set (similar to libreadline, libedit, in a way). So
we'd have one option 'uuid' that is of type feature with default 'auto'
and another option, say, 'uuid-library' of type 'combo'.
Thoughts?
Hi,
On 2/8/23 13:45, Peter Eisentraut wrote:
>
> The problem is that these features now cannot be automatically enabled
> and behave annoyingly different from other feature options.
Agreed.
> For the 'ssl' option, we have deprecated the --with-openssl option in
> configure and replaced it with --with-ssl, in anticipation of other
> SSL implementations. None of that ever happened or is currently
> planned AFAICT. So I suggest that we semi-revert this, so that we can
> make 'openssl' an auto option in meson.
+1
> For the 'uuid' option, I'm not sure what the best way to address this
> would. We could establish a search order of libraries that is used if
> no specific one is set (similar to libreadline, libedit, in a way).
> So we'd have one option 'uuid' that is of type feature with default
> 'auto' and another option, say, 'uuid-library' of type 'combo'.
>
Your suggestion looks good and TCL already has a similar implementation
with what you suggested:
option('pltcl', type : 'feature', value: 'auto',
description: 'build with TCL support')
option('tcl_version', type : 'string', value : 'tcl',
description: 'specify TCL version')
Regards,
Nazir Bilal Yavuz
Microsoft
Hi,
On 2023-02-08 11:45:05 +0100, Peter Eisentraut wrote:
> Most meson options (meson_options.txt) that enable an external dependency
> (e.g., icu, ldap) are of type 'feature'. Most of these have a default value
> of 'auto', which means they are pulled in automatically if found. Some have
> a default value of 'disabled' for specific reasons (e.g., selinux). This is
> all good.
>
> Two options deviate from this in annoying ways:
>
> option('ssl', type : 'combo', choices : ['none', 'openssl'],
> value : 'none',
> description: 'use LIB for SSL/TLS support (openssl)')
>
> option('uuid', type : 'combo', choices : ['none', 'bsd', 'e2fs', 'ossp'],
> value : 'none',
> description: 'build contrib/uuid-ossp using LIB')
>
> These were moved over from configure like that.
>
> The problem is that these features now cannot be automatically enabled and
> behave annoyingly different from other feature options.
Oh, yes, this has been bothering me too.
> For the 'ssl' option, we have deprecated the --with-openssl option in
> configure and replaced it with --with-ssl, in anticipation of other SSL
> implementations. None of that ever happened or is currently planned AFAICT.
> So I suggest that we semi-revert this, so that we can make 'openssl' an auto
> option in meson.
Hm. I'm inclined to leave it there - I do think it's somewhat likely that
we'll eventually end up with some platform native library. I think it's likely
the NSS patch isn't going anywhere, but I'm not sure that's true for
e.g. using the windows encryption library. IIRC Heikki had a patch at some
point.
I'd probably just add a 'auto' option, and manually make it behave like a
feature option.
> For the 'uuid' option, I'm not sure what the best way to address this would.
> We could establish a search order of libraries that is used if no specific
> one is set (similar to libreadline, libedit, in a way). So we'd have one
> option 'uuid' that is of type feature with default 'auto' and another
> option, say, 'uuid-library' of type 'combo'.
Or add 'auto' as a combo option, and handle the value of the auto_features
option ourselves?
Greetings,
Andres Freund
Hi, On 2/8/23 19:23, Andres Freund wrote: >> For the 'uuid' option, I'm not sure what the best way to address this would. >> We could establish a search order of libraries that is used if no specific >> one is set (similar to libreadline, libedit, in a way). So we'd have one >> option 'uuid' that is of type feature with default 'auto' and another >> option, say, 'uuid-library' of type 'combo'. > Or add 'auto' as a combo option, and handle the value of the auto_features > option ourselves? If we do it like this, meson's --auto-features option won't work for uuid. Is this something we want to consider? Regards, Nazir Bilal Yavuz Microsoft
Hi, I added SSL and UUID patches. UUID patch has two different fixes: 1 - v1-0002-meson-Refactor-UUID-option.patch: Adding 'auto' choice to 'uuid' combo option. 2 - v1-0002-meson-Refactor-UUID-option-with-uuid_library.patch: Making 'uuid' feature option and adding new 'uuid_library' combo option with ['auto', 'bsd', 'e2fs', 'ossp'] choices. If 'uuid_library' is set other than 'auto' and it can't be found, build throws an error. What do you think? Regards, Nazir Bilal Yavuz Microsoft
Attachment
On 20.02.23 13:33, Nazir Bilal Yavuz wrote: > I added SSL and UUID patches. UUID patch has two different fixes: > > 1 - v1-0002-meson-Refactor-UUID-option.patch: Adding 'auto' choice to > 'uuid' combo option. > > 2 - v1-0002-meson-Refactor-UUID-option-with-uuid_library.patch: Making > 'uuid' feature option and adding new 'uuid_library' combo option with > ['auto', 'bsd', 'e2fs', 'ossp'] choices. If 'uuid_library' is set other > than 'auto' and it can't be found, build throws an error. > > What do you think? I like the second approach, with a 'uuid' feature option. As you wrote earlier, adding an 'auto' choice to a combo option doesn't work fully like a real feature option. But what does uuid_library=auto do? Which one does it pick? This is not a behavior we currently have, is it? I would rename the ssl_type variable to ssl_library, so that if we ever expose that as an option, it would be consistent with uuid_library.
Hi, On Mon, 20 Feb 2023 at 21:53, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote: > > But what does uuid_library=auto do? Which one does it pick? This is > not a behavior we currently have, is it? Yes, we didn't have that behavior before. It checks uuid libs by the order of 'e2fs', 'bsd' and 'ossp'. It uses the first one it finds and doesn't try to find the rest but the build doesn't fail if it can't find any library. Regards, Nazir Bilal Yavuz Microsoft
> On 20 Feb 2023, at 19:53, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote: > I would rename the ssl_type variable to ssl_library, so that if we ever expose that as an option, it would be consistentwith uuid_library. +1, ssl_library is a better name. -- Daniel Gustafsson
Hi, On 2023-02-20 19:53:53 +0100, Peter Eisentraut wrote: > On 20.02.23 13:33, Nazir Bilal Yavuz wrote: > > I added SSL and UUID patches. UUID patch has two different fixes: > > > > 1 - v1-0002-meson-Refactor-UUID-option.patch: Adding 'auto' choice to > > 'uuid' combo option. > > > > 2 - v1-0002-meson-Refactor-UUID-option-with-uuid_library.patch: Making > > 'uuid' feature option and adding new 'uuid_library' combo option with > > ['auto', 'bsd', 'e2fs', 'ossp'] choices. If 'uuid_library' is set other > > than 'auto' and it can't be found, build throws an error. > > > > What do you think? > > I like the second approach, with a 'uuid' feature option. As you wrote > earlier, adding an 'auto' choice to a combo option doesn't work fully like a > real feature option. But we can make it behave exactly like one, by checking the auto_features option. Greetings, Andres Freund
Hi,
On Mon, 20 Feb 2023 at 22:42, Andres Freund <andres@anarazel.de> wrote:
> On 2023-02-20 19:53:53 +0100, Peter Eisentraut wrote:
> > On 20.02.23 13:33, Nazir Bilal Yavuz wrote:
> > > I added SSL and UUID patches. UUID patch has two different fixes:
> > >
> > > 1 - v1-0002-meson-Refactor-UUID-option.patch: Adding 'auto' choice to
> > > 'uuid' combo option.
> > >
> > > 2 - v1-0002-meson-Refactor-UUID-option-with-uuid_library.patch: Making
> > > 'uuid' feature option and adding new 'uuid_library' combo option with
> > > ['auto', 'bsd', 'e2fs', 'ossp'] choices. If 'uuid_library' is set other
> > > than 'auto' and it can't be found, build throws an error.
> > >
> > > What do you think?
> >
> > I like the second approach, with a 'uuid' feature option. As you wrote
> > earlier, adding an 'auto' choice to a combo option doesn't work fully like a
> > real feature option.
>
> But we can make it behave exactly like one, by checking the auto_features
> option.
Yes, we can set it like `uuidopt = get_option('auto_features')`.
However, if someone wants to set 'auto_features' to 'disabled' but
'uuid' to 'enabled'(to find at least one working uuid library); this
won't be possible. We can add 'enabled', 'disabled and 'auto' choices
to 'uuid' combo option to make all behaviours possible but adding
'uuid' feature option and 'uuid_library' combo option seems better to
me.
Regards,
Nazir Bilal Yavuz
Microsoft
On 21.02.23 17:32, Nazir Bilal Yavuz wrote:
>>> I like the second approach, with a 'uuid' feature option. As you wrote
>>> earlier, adding an 'auto' choice to a combo option doesn't work fully like a
>>> real feature option.
>> But we can make it behave exactly like one, by checking the auto_features
>> option.
> Yes, we can set it like `uuidopt = get_option('auto_features')`.
> However, if someone wants to set 'auto_features' to 'disabled' but
> 'uuid' to 'enabled'(to find at least one working uuid library); this
> won't be possible. We can add 'enabled', 'disabled and 'auto' choices
> to 'uuid' combo option to make all behaviours possible but adding
> 'uuid' feature option and 'uuid_library' combo option seems better to
> me.
I think the uuid side of this is making this way too complicated. I'm
content leaving this as a manual option for now.
There is much more value in making the ssl option work automatically.
So I would welcome a patch that just makes -Dssl=auto work smoothly,
perhaps using the "trick" that Andres described.
Hi,
On Wed, 22 Feb 2023 at 12:14, Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
>
> On 21.02.23 17:32, Nazir Bilal Yavuz wrote:
> >>> I like the second approach, with a 'uuid' feature option. As you wrote
> >>> earlier, adding an 'auto' choice to a combo option doesn't work fully like a
> >>> real feature option.
> >> But we can make it behave exactly like one, by checking the auto_features
> >> option.
> > Yes, we can set it like `uuidopt = get_option('auto_features')`.
> > However, if someone wants to set 'auto_features' to 'disabled' but
> > 'uuid' to 'enabled'(to find at least one working uuid library); this
> > won't be possible. We can add 'enabled', 'disabled and 'auto' choices
> > to 'uuid' combo option to make all behaviours possible but adding
> > 'uuid' feature option and 'uuid_library' combo option seems better to
> > me.
>
> I think the uuid side of this is making this way too complicated. I'm
> content leaving this as a manual option for now.
>
> There is much more value in making the ssl option work automatically.
> So I would welcome a patch that just makes -Dssl=auto work smoothly,
> perhaps using the "trick" that Andres described.
>
Thanks for the feedback. I updated the ssl patch and if you like
changes, I can apply the same logic to uuid.
Regards,
Nazir Bilal Yavuz
Microsoft
Attachment
On 24.02.23 14:01, Nazir Bilal Yavuz wrote:
> Thanks for the feedback. I updated the ssl patch and if you like
> changes, I can apply the same logic to uuid.
Maybe we can make some of the logic less nested. Right now there is
if sslopt != 'none'
if not ssl.found() and sslopt in ['auto', 'openssl']
I think at that point, ssl.found() is never true, so it can be removed.
And the two checks for sslopt are nearly redundant.
At the end of the block, there is
# At least one SSL library must be found, otherwise throw an error
if sslopt == 'auto' and auto_features.enabled()
error('SSL Library could not be found')
endif
endif
which also implies sslopt != 'none'. So I think the whole thing could be
if sslopt in ['auto', 'openssl']
...
endif
if sslopt == 'auto' and auto_features.enabled()
error('SSL Library could not be found')
endif
both at the top level.
Another issue, I think this is incorrect:
+ openssl_required ? error('openssl function @0@ is
required'.format(func)) : \
+ message('openssl function @0@ is
required'.format(func))
We don't want to issue a message like this when a non-required function
is missing.
Hi,
Thanks for the review.
On Wed, 1 Mar 2023 at 18:52, Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
>
> Maybe we can make some of the logic less nested. Right now there is
>
> if sslopt != 'none'
>
> if not ssl.found() and sslopt in ['auto', 'openssl']
>
> I think at that point, ssl.found() is never true, so it can be removed.
I agree, ssl.found() can be removed.
> And the two checks for sslopt are nearly redundant.
>
> At the end of the block, there is
>
> # At least one SSL library must be found, otherwise throw an error
> if sslopt == 'auto' and auto_features.enabled()
> error('SSL Library could not be found')
> endif
> endif
>
> which also implies sslopt != 'none'. So I think the whole thing could be
>
> if sslopt in ['auto', 'openssl']
>
> ...
>
> endif
>
> if sslopt == 'auto' and auto_features.enabled()
> error('SSL Library could not be found')
> endif
>
> both at the top level.
>
I am kind of confused. I added these checks for considering other SSL
implementations in the future, for this reason I have two nested if
checks. The top one is for checking if we need to search an SSL
library and the nested one is for checking if we need to search this
specific SSL library. What do you think?
The other thing is(which I forgot before) I need to add "and not
ssl.found()" condition to the "if sslopt == 'auto' and
auto_features.enabled()" check.
> Another issue, I think this is incorrect:
>
> + openssl_required ? error('openssl function @0@ is
> required'.format(func)) : \
> + message('openssl function @0@ is
> required'.format(func))
>
> We don't want to issue a message like this when a non-required function
> is missing.
I agree, the message part can be removed.
Regards,
Nazir Bilal Yavuz
Microsoft
On 02.03.23 11:41, Nazir Bilal Yavuz wrote: > I am kind of confused. I added these checks for considering other SSL > implementations in the future, for this reason I have two nested if > checks. The top one is for checking if we need to search an SSL > library and the nested one is for checking if we need to search this > specific SSL library. What do you think? I suppose that depends on how you envision integrating other SSL libraries into this logic. It's not that important right now; if the structure makes sense to you, that's fine. Please send an updated patch with the small changes that have been mentioned.
Hi, On Fri, 3 Mar 2023 at 12:16, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote: > > On 02.03.23 11:41, Nazir Bilal Yavuz wrote: > > I am kind of confused. I added these checks for considering other SSL > > implementations in the future, for this reason I have two nested if > > checks. The top one is for checking if we need to search an SSL > > library and the nested one is for checking if we need to search this > > specific SSL library. What do you think? > > I suppose that depends on how you envision integrating other SSL > libraries into this logic. It's not that important right now; if the > structure makes sense to you, that's fine. > > Please send an updated patch with the small changes that have been > mentioned. > The updated patch is attached. Regards, Nazir Bilal Yavuz Microsoft
Attachment
On 03.03.23 11:01, Nazir Bilal Yavuz wrote: > On Fri, 3 Mar 2023 at 12:16, Peter Eisentraut > <peter.eisentraut@enterprisedb.com> wrote: >> >> On 02.03.23 11:41, Nazir Bilal Yavuz wrote: >>> I am kind of confused. I added these checks for considering other SSL >>> implementations in the future, for this reason I have two nested if >>> checks. The top one is for checking if we need to search an SSL >>> library and the nested one is for checking if we need to search this >>> specific SSL library. What do you think? >> >> I suppose that depends on how you envision integrating other SSL >> libraries into this logic. It's not that important right now; if the >> structure makes sense to you, that's fine. >> >> Please send an updated patch with the small changes that have been >> mentioned. >> > > The updated patch is attached. This seems to work well. One flaw, the "External libraries" summary shows something like ssl : YES 3.0.7 It would be nice if it showed "openssl". How about we just hardcode "openssl" here instead? We could build that array dynamically, of course, but maybe we leave that until we actually have a need?
> On 9 Mar 2023, at 14:45, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote: > How about we just hardcode "openssl" here instead? We could build that array dynamically, of course, but maybe we leavethat until we actually have a need? At least for 16 keeping it hardcoded is an entirely safe bet so +1 for leaving additional complexity for when needed. -- Daniel Gustafsson
Hi,
On Thu, 9 Mar 2023 at 16:54, Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > On 9 Mar 2023, at 14:45, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
>
> > How about we just hardcode "openssl" here instead? We could build that array dynamically, of course, but maybe we
leavethat until we actually have a need?
>
> At least for 16 keeping it hardcoded is an entirely safe bet so +1 for leaving
> additional complexity for when needed.
We already have the 'ssl_library' variable. Can't we use that instead
of hardcoding 'openssl'? e.g:
summary(
{
'ssl': ssl.found() ? [ssl, '(@0@)'.format(ssl_library)] : ssl,
},
section: 'External libraries',
list_sep: ', ',
)
And it will output:
ssl : YES 3.0.8, (openssl)
I don't think that using 'ssl_library' will increase the complexity.
Regards,
Nazir Bilal Yavuz
Microsoft
> On 9 Mar 2023, at 15:12, Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
>
> Hi,
>
> On Thu, 9 Mar 2023 at 16:54, Daniel Gustafsson <daniel@yesql.se> wrote:
>>
>>> On 9 Mar 2023, at 14:45, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
>>
>>> How about we just hardcode "openssl" here instead? We could build that array dynamically, of course, but maybe we
leavethat until we actually have a need?
>>
>> At least for 16 keeping it hardcoded is an entirely safe bet so +1 for leaving
>> additional complexity for when needed.
>
> We already have the 'ssl_library' variable. Can't we use that instead
> of hardcoding 'openssl'? e.g:
>
> summary(
> {
> 'ssl': ssl.found() ? [ssl, '(@0@)'.format(ssl_library)] : ssl,
> },
> section: 'External libraries',
> list_sep: ', ',
> )
>
> And it will output:
> ssl : YES 3.0.8, (openssl)
>
> I don't think that using 'ssl_library' will increase the complexity.
That seems like a good idea.
--
Daniel Gustafsson
On 09.03.23 15:12, Nazir Bilal Yavuz wrote:
> Hi,
>
> On Thu, 9 Mar 2023 at 16:54, Daniel Gustafsson <daniel@yesql.se> wrote:
>>
>>> On 9 Mar 2023, at 14:45, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
>>
>>> How about we just hardcode "openssl" here instead? We could build that array dynamically, of course, but maybe we
leavethat until we actually have a need?
>>
>> At least for 16 keeping it hardcoded is an entirely safe bet so +1 for leaving
>> additional complexity for when needed.
>
> We already have the 'ssl_library' variable. Can't we use that instead
> of hardcoding 'openssl'? e.g:
>
> summary(
> {
> 'ssl': ssl.found() ? [ssl, '(@0@)'.format(ssl_library)] : ssl,
> },
> section: 'External libraries',
> list_sep: ', ',
> )
>
> And it will output:
> ssl : YES 3.0.8, (openssl)
>
> I don't think that using 'ssl_library' will increase the complexity.
Then we might as well use ssl_library as the key, like:
{
...
'selinux': selinux,
ssl_library: ssl,
'systemd': systemd,
...
}
Hi,
On Thu, 9 Mar 2023 at 17:18, Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
>
> On 09.03.23 15:12, Nazir Bilal Yavuz wrote:
> > Hi,
> >
> > On Thu, 9 Mar 2023 at 16:54, Daniel Gustafsson <daniel@yesql.se> wrote:
> >>
> >>> On 9 Mar 2023, at 14:45, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
> >>
> >>> How about we just hardcode "openssl" here instead? We could build that array dynamically, of course, but maybe
weleave that until we actually have a need?
> >>
> >> At least for 16 keeping it hardcoded is an entirely safe bet so +1 for leaving
> >> additional complexity for when needed.
> >
> > We already have the 'ssl_library' variable. Can't we use that instead
> > of hardcoding 'openssl'? e.g:
> >
> > summary(
> > {
> > 'ssl': ssl.found() ? [ssl, '(@0@)'.format(ssl_library)] : ssl,
> > },
> > section: 'External libraries',
> > list_sep: ', ',
> > )
> >
> > And it will output:
> > ssl : YES 3.0.8, (openssl)
> >
> > I don't think that using 'ssl_library' will increase the complexity.
>
> Then we might as well use ssl_library as the key, like:
>
> {
> ...
> 'selinux': selinux,
> ssl_library: ssl,
> 'systemd': systemd,
> ...
> }
>
There will be a problem if ssl is not found. It will output 'none: NO'
because 'ssl_library' is initialized as 'none' for now. We can
initialize 'ssl_library' as 'ssl' but I am not sure that is a good
idea.
Regards,
Nazir Bilal Yavuz
Microsoft
On 09.03.23 14:54, Daniel Gustafsson wrote: >> On 9 Mar 2023, at 14:45, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote: > >> How about we just hardcode "openssl" here instead? We could build that array dynamically, of course, but maybe we leavethat until we actually have a need? > > At least for 16 keeping it hardcoded is an entirely safe bet so +1 for leaving > additional complexity for when needed. I have committed it like this. I didn't like the other variants, because they would cause the openssl line to stick out for purely implementation reasons (e.g., we don't have a line "compression: YES (lz4)". If we get support for another ssl library, we can easily reconsider this.
On Mon, Mar 13, 2023 at 07:27:18AM +0100, Peter Eisentraut wrote:
> I have committed it like this.
I noticed that after 6a30027, if you don't have the OpenSSL headers
installed, 'meson setup' will fail:
meson.build:1195:4: ERROR: C header 'openssl/ssl.h' not found
Shouldn't "auto" cause Postgres to be built without OpenSSL if the required
headers are not present?
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Hi, On Mon, 13 Mar 2023 at 21:04, Nathan Bossart <nathandbossart@gmail.com> wrote: > > On Mon, Mar 13, 2023 at 07:27:18AM +0100, Peter Eisentraut wrote: > > I have committed it like this. > > I noticed that after 6a30027, if you don't have the OpenSSL headers > installed, 'meson setup' will fail: > > meson.build:1195:4: ERROR: C header 'openssl/ssl.h' not found > > Shouldn't "auto" cause Postgres to be built without OpenSSL if the required > headers are not present? Yes, I tested again and it is working as expected on my end. It shouldn't fail like that unless the 'ssl' option is set to 'openssl'. Is it possible that it has been set to 'openssl' without you noticing? Regards, Nazir Bilal Yavuz Microsoft
On Mon, Mar 13, 2023 at 09:57:22PM +0300, Nazir Bilal Yavuz wrote:
> On Mon, 13 Mar 2023 at 21:04, Nathan Bossart <nathandbossart@gmail.com> wrote:
>> Shouldn't "auto" cause Postgres to be built without OpenSSL if the required
>> headers are not present?
>
> Yes, I tested again and it is working as expected on my end. It
> shouldn't fail like that unless the 'ssl' option is set to 'openssl'.
> Is it possible that it has been set to 'openssl' without you noticing?
I do not believe so. For the test in question, here are the build options
reported in meson-log.txt:
Build Options: -Dlz4=enabled -Dplperl=enabled -Dplpython=enabled -Dpltcl=enabled -Dlibxml=enabled -Duuid=ossp
-Dlibxslt=enabled-Ddebug=true -Dcassert=true -Dtap_tests=enabled -Dwerror=True
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Hi, On 2023-03-13 11:04:32 -0700, Nathan Bossart wrote: > On Mon, Mar 13, 2023 at 07:27:18AM +0100, Peter Eisentraut wrote: > > I have committed it like this. > > I noticed that after 6a30027, if you don't have the OpenSSL headers > installed, 'meson setup' will fail: > > meson.build:1195:4: ERROR: C header 'openssl/ssl.h' not found > > Shouldn't "auto" cause Postgres to be built without OpenSSL if the required > headers are not present? Yea. I found another thing: When dependency() found something, but the headers weren't present, ssl_int wouldn't exist. Maybe something like the attached? Greetings, Andres Freund
Attachment
Hi, On 2023-03-13 21:57:22 +0300, Nazir Bilal Yavuz wrote: > On Mon, 13 Mar 2023 at 21:04, Nathan Bossart <nathandbossart@gmail.com> wrote: > > > > On Mon, Mar 13, 2023 at 07:27:18AM +0100, Peter Eisentraut wrote: > > > I have committed it like this. > > > > I noticed that after 6a30027, if you don't have the OpenSSL headers > > installed, 'meson setup' will fail: > > > > meson.build:1195:4: ERROR: C header 'openssl/ssl.h' not found > > > > Shouldn't "auto" cause Postgres to be built without OpenSSL if the required > > headers are not present? > > Yes, I tested again and it is working as expected on my end. It > shouldn't fail like that unless the 'ssl' option is set to 'openssl'. > Is it possible that it has been set to 'openssl' without you noticing? It worked for the dependency() path, but not the cc.find_library() path. See the patch I just sent. Greetings, Andres Freund
On Mon, Mar 13, 2023 at 01:13:31PM -0700, Andres Freund wrote:
> On 2023-03-13 11:04:32 -0700, Nathan Bossart wrote:
>> I noticed that after 6a30027, if you don't have the OpenSSL headers
>> installed, 'meson setup' will fail:
>>
>> meson.build:1195:4: ERROR: C header 'openssl/ssl.h' not found
>>
>> Shouldn't "auto" cause Postgres to be built without OpenSSL if the required
>> headers are not present?
>
> Yea. I found another thing: When dependency() found something, but the headers
> weren't present, ssl_int wouldn't exist.
>
> Maybe something like the attached?
> ssl_lib = cc.find_library('ssl',
> dirs: test_lib_d,
> header_include_directories: postgres_inc,
> - has_headers: ['openssl/ssl.h', 'openssl/err.h'])
> + has_headers: ['openssl/ssl.h', 'openssl/err.h'],
> + required: openssl_required)
> crypto_lib = cc.find_library('crypto',
> dirs: test_lib_d,
> - header_include_directories: postgres_inc)
> - ssl_int = [ssl_lib, crypto_lib]
> -
> - ssl = declare_dependency(dependencies: ssl_int,
> - include_directories: postgres_inc)
> + required: openssl_required)
> + if ssl_lib.found() and crypto_lib.found()
> + ssl_int = [ssl_lib, crypto_lib]
> + ssl = declare_dependency(dependencies: ssl_int, include_directories: postgres_inc)
> + endif
I was just about to post a patch to set "required" like you have for
ssl_lib and crypto_lib. It seemed to work alright without the 'if
ssl_lib.found() and crypto_lib.found()' line, but I haven't worked with
these meson files very much, so what you have is probably better form.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Hi, On Mon, 13 Mar 2023 at 23:14, Andres Freund <andres@anarazel.de> wrote: > > Hi, > > On 2023-03-13 21:57:22 +0300, Nazir Bilal Yavuz wrote: > > On Mon, 13 Mar 2023 at 21:04, Nathan Bossart <nathandbossart@gmail.com> wrote: > > > > > > On Mon, Mar 13, 2023 at 07:27:18AM +0100, Peter Eisentraut wrote: > > > > I have committed it like this. > > > > > > I noticed that after 6a30027, if you don't have the OpenSSL headers > > > installed, 'meson setup' will fail: > > > > > > meson.build:1195:4: ERROR: C header 'openssl/ssl.h' not found > > > > > > Shouldn't "auto" cause Postgres to be built without OpenSSL if the required > > > headers are not present? > > > > Yes, I tested again and it is working as expected on my end. It > > shouldn't fail like that unless the 'ssl' option is set to 'openssl'. > > Is it possible that it has been set to 'openssl' without you noticing? > > It worked for the dependency() path, but not the cc.find_library() path. See > the patch I just sent. Thanks for the patch, I understand the problem now and your patch fixes this. Regards, Nazir Bilal Yavuz Microsoft
Hi, On 2023-03-13 23:46:41 +0300, Nazir Bilal Yavuz wrote: > Thanks for the patch, I understand the problem now and your patch fixes this. Pushed the patch. Greetings, Andres Freund
On Mon, Mar 13, 2023 at 02:45:29PM -0700, Andres Freund wrote: > Pushed the patch. Thanks for the prompt fix. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Hi,
On Wed, 22 Feb 2023 at 12:14, Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
>
> On 21.02.23 17:32, Nazir Bilal Yavuz wrote:
> >>> I like the second approach, with a 'uuid' feature option. As you wrote
> >>> earlier, adding an 'auto' choice to a combo option doesn't work fully like a
> >>> real feature option.
> >> But we can make it behave exactly like one, by checking the auto_features
> >> option.
> > Yes, we can set it like `uuidopt = get_option('auto_features')`.
> > However, if someone wants to set 'auto_features' to 'disabled' but
> > 'uuid' to 'enabled'(to find at least one working uuid library); this
> > won't be possible. We can add 'enabled', 'disabled and 'auto' choices
> > to 'uuid' combo option to make all behaviours possible but adding
> > 'uuid' feature option and 'uuid_library' combo option seems better to
> > me.
>
> I think the uuid side of this is making this way too complicated. I'm
> content leaving this as a manual option for now.
>
> There is much more value in making the ssl option work automatically.
> So I would welcome a patch that just makes -Dssl=auto work smoothly,
> perhaps using the "trick" that Andres described.
I tried to implement what we did for ssl to uuid as well, do you have
any comments?
Regards,
Nazir Bilal Yavuz
Microsoft
Attachment
On 14.03.23 15:07, Nazir Bilal Yavuz wrote: >> I think the uuid side of this is making this way too complicated. I'm >> content leaving this as a manual option for now. >> >> There is much more value in making the ssl option work automatically. >> So I would welcome a patch that just makes -Dssl=auto work smoothly, >> perhaps using the "trick" that Andres described. > I tried to implement what we did for ssl to uuid as well, do you have > any comments? For the uuid option, we have three different choices. What should be the search order and why?
Hi, On Wed, 15 Mar 2023 at 11:12, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote: > > On 14.03.23 15:07, Nazir Bilal Yavuz wrote: > >> I think the uuid side of this is making this way too complicated. I'm > >> content leaving this as a manual option for now. > >> > >> There is much more value in making the ssl option work automatically. > >> So I would welcome a patch that just makes -Dssl=auto work smoothly, > >> perhaps using the "trick" that Andres described. > > I tried to implement what we did for ssl to uuid as well, do you have > > any comments? > > For the uuid option, we have three different choices. What should be > the search order and why? Docs [1] say that: OSSP uuid library is not well maintained, and is becoming increasingly difficult to port to newer platforms; so we can put 'uuid-ossp' to the end. Between 'uuid-e2fs' and 'uuid-bsd', I believe 'uuid-e2fs' is used more often than 'uuid-bsd'. Hence, they can be ordered as 'uuid-e2fs', 'uuid-bsd', 'uuid-ossp'. Does that make sense? Regards, Nazir Bilal Yavuz Microsoft [1] https://www.postgresql.org/docs/current/uuid-ossp.html
On 22.03.23 11:16, Nazir Bilal Yavuz wrote: > Hi, > > On Wed, 15 Mar 2023 at 11:12, Peter Eisentraut > <peter.eisentraut@enterprisedb.com> wrote: >> >> On 14.03.23 15:07, Nazir Bilal Yavuz wrote: >>>> I think the uuid side of this is making this way too complicated. I'm >>>> content leaving this as a manual option for now. >>>> >>>> There is much more value in making the ssl option work automatically. >>>> So I would welcome a patch that just makes -Dssl=auto work smoothly, >>>> perhaps using the "trick" that Andres described. >>> I tried to implement what we did for ssl to uuid as well, do you have >>> any comments? >> >> For the uuid option, we have three different choices. What should be >> the search order and why? > > Docs [1] say that: OSSP uuid library is not well maintained, and is > becoming increasingly difficult to port to newer platforms; so we can > put 'uuid-ossp' to the end. Between 'uuid-e2fs' and 'uuid-bsd', I > believe 'uuid-e2fs' is used more often than 'uuid-bsd'. > Hence, they can be ordered as 'uuid-e2fs', 'uuid-bsd', 'uuid-ossp'. > > Does that make sense? I think that's fair.
Hi, I was looking at older threads and found that. There were failures when rebasing v1-uuid patch to master so I updated it and also added documentation. I attached the v2 of the patch. I am sending v2 patch to this thread but should I create a new thread for uuid patch? Regards, Nazir Bilal Yavuz Microsoft