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