Thread: meson: Non-feature feature options

meson: Non-feature feature options

From
Peter Eisentraut
Date:
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?



Re: meson: Non-feature feature options

From
Nazir Bilal Yavuz
Date:
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




Re: meson: Non-feature feature options

From
Andres Freund
Date:
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



Re: meson: Non-feature feature options

From
Nazir Bilal Yavuz
Date:
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





Re: meson: Non-feature feature options

From
Nazir Bilal Yavuz
Date:
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

Re: meson: Non-feature feature options

From
Peter Eisentraut
Date:
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.




Re: meson: Non-feature feature options

From
Nazir Bilal Yavuz
Date:
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



Re: meson: Non-feature feature options

From
Daniel Gustafsson
Date:
> 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




Re: meson: Non-feature feature options

From
Andres Freund
Date:
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



Re: meson: Non-feature feature options

From
Nazir Bilal Yavuz
Date:
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



Re: meson: Non-feature feature options

From
Peter Eisentraut
Date:
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.




Re: meson: Non-feature feature options

From
Nazir Bilal Yavuz
Date:
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

Re: meson: Non-feature feature options

From
Peter Eisentraut
Date:
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.



Re: meson: Non-feature feature options

From
Nazir Bilal Yavuz
Date:
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



Re: meson: Non-feature feature options

From
Peter Eisentraut
Date:
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.




Re: meson: Non-feature feature options

From
Nazir Bilal Yavuz
Date:
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

Re: meson: Non-feature feature options

From
Peter Eisentraut
Date:
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?




Re: meson: Non-feature feature options

From
Daniel Gustafsson
Date:
> 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




Re: meson: Non-feature feature options

From
Nazir Bilal Yavuz
Date:
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



Re: meson: Non-feature feature options

From
Daniel Gustafsson
Date:
> 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




Re: meson: Non-feature feature options

From
Peter Eisentraut
Date:
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,
    ...
}




Re: meson: Non-feature feature options

From
Nazir Bilal Yavuz
Date:
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



Re: meson: Non-feature feature options

From
Peter Eisentraut
Date:
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.




Re: meson: Non-feature feature options

From
Nathan Bossart
Date:
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



Re: meson: Non-feature feature options

From
Nazir Bilal Yavuz
Date:
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



Re: meson: Non-feature feature options

From
Nathan Bossart
Date:
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



Re: meson: Non-feature feature options

From
Andres Freund
Date:
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

Re: meson: Non-feature feature options

From
Andres Freund
Date:
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



Re: meson: Non-feature feature options

From
Nathan Bossart
Date:
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



Re: meson: Non-feature feature options

From
Nazir Bilal Yavuz
Date:
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



Re: meson: Non-feature feature options

From
Andres Freund
Date:
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



Re: meson: Non-feature feature options

From
Nathan Bossart
Date:
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



Re: meson: Non-feature feature options

From
Nazir Bilal Yavuz
Date:
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

Re: meson: Non-feature feature options

From
Peter Eisentraut
Date:
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?



Re: meson: Non-feature feature options

From
Nazir Bilal Yavuz
Date:
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



Re: meson: Non-feature feature options

From
Peter Eisentraut
Date:
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.




Re: meson: Non-feature feature options

From
Nazir Bilal Yavuz
Date:
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

Attachment