meson: Non-feature feature options

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

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 consistent with 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

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

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

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

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

That seems like a good idea.

--
Daniel Gustafsson