Subject: Re: meson: Non-feature feature options

From: Nazir Bilal Yavuz <[email-masked]>
Date: Fri, 24 Feb 2023 16:01:29 +0300

Hi,

On Wed, 22 Feb 2023 at 12:14, Peter Eisentraut
<[email-masked]> 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

---

From: Peter Eisentraut <[email-masked]>
Date: Wed, 1 Mar 2023 16:52:18 +0100

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

---

From: Nazir Bilal Yavuz <[email-masked]>
Date: Thu, 2 Mar 2023 13:41:58 +0300

Hi,

Thanks for the review.

> I think at that point, ssl.found() is never true, so it can be removed.

I agree, ssl.found() can be removed.

>        error('SSL Library could not be found')

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.

> We don't want to issue a message like this when a non-required function

I agree, the message part can be removed.

Regards,
Nazir Bilal Yavuz
Microsoft

---

From: Peter Eisentraut <[email-masked]>
Date: Fri, 3 Mar 2023 10:16:09 +0100

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

---

From: Nazir Bilal Yavuz <[email-masked]>
Date: Fri, 3 Mar 2023 13:01:00 +0300

Hi,

> Please send an updated patch with the small changes that have been

The updated patch is attached.

Regards,
Nazir Bilal Yavuz
Microsoft
From 9cb9d50ba008e2a385a7b72219a759490a3de00e Mon Sep 17 00:00:00 2001
From: Nazir Bilal Yavuz <[email-masked]>
Date: Fri, 3 Mar 2023 12:24:46 +0300
Subject: [PATCH v3] meson: Refactor SSL option

---
 .cirrus.yml                      |   7 +-
 meson.build                      | 121 ++++++++++++++++++-------------
 meson_options.txt                |   4 +-
 src/interfaces/libpq/meson.build |   2 +-
 src/makefiles/meson.build        |   2 +-
 src/test/ssl/meson.build         |   2 +-
 6 files changed, 80 insertions(+), 58 deletions(-)

diff --git a/.cirrus.yml b/.cirrus.yml
index f2129787529..aaf4066366c 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -181,7 +181,7 @@ task:
     su postgres <<-EOF
       meson setup \
         --buildtype=debug \
-        -Dcassert=true -Dssl=openssl -Duuid=bsd -Dtcl_version=tcl86 -Ddtrace=auto \
+        -Dcassert=true -Duuid=bsd -Dtcl_version=tcl86 -Ddtrace=auto \
         -DPG_TEST_EXTRA="$PG_TEST_EXTRA" \
         -Dextra_lib_dirs=/usr/local/lib -Dextra_include_dirs=/usr/local/include/ \
         build
@@ -243,7 +243,6 @@ LINUX_CONFIGURE_FEATURES: &LINUX_CONFIGURE_FEATURES >-
 
 LINUX_MESON_FEATURES: &LINUX_MESON_FEATURES >-
   -Dllvm=enabled
-  -Dssl=openssl
   -Duuid=e2fs

@@ -497,7 +496,7 @@ task:
       -Dextra_include_dirs=${brewpath}/include \
       -Dextra_lib_dirs=${brewpath}/lib \
       -Dcassert=true \
-      -Dssl=openssl -Duuid=e2fs -Ddtrace=auto \
+      -Duuid=e2fs -Ddtrace=auto \
       -Dsegsize_blocks=6 \
       -DPG_TEST_EXTRA="$PG_TEST_EXTRA" \
       build
@@ -568,7 +567,7 @@ task:
   # Use /DEBUG:FASTLINK to avoid high memory usage during linking
   configure_script: |
     vcvarsall x64
-    meson setup --backend ninja --buildtype debug -Dc_link_args=/DEBUG:FASTLINK -Dcassert=true -Db_pch=true -Dssl=openssl -Dextra_lib_dirs=c:\openssl\1.1\lib -Dextra_include_dirs=c:\openssl\1.1\include -DTAR=%TAR% -DPG_TEST_EXTRA="%PG_TEST_EXTRA%" build
+    meson setup --backend ninja --buildtype debug -Dc_link_args=/DEBUG:FASTLINK -Dcassert=true -Db_pch=true -Dextra_lib_dirs=c:\openssl\1.1\lib -Dextra_include_dirs=c:\openssl\1.1\include -DTAR=%TAR% -DPG_TEST_EXTRA="%PG_TEST_EXTRA%" build
 
   build_script: |
     vcvarsall x64
diff --git a/meson.build b/meson.build
index 26be83afb61..1e9411eb247 100644
--- a/meson.build
+++ b/meson.build
@@ -43,6 +43,7 @@ cc = meson.get_compiler('c')
 
 not_found_dep = dependency('', required: false)
 thread_dep = dependency('threads')
+auto_features = get_option('auto_features')

@@ -1171,7 +1172,16 @@ cdata.set('USE_SYSTEMD', systemd.found() ? 1 : false)
 # Library: SSL
 ###############################################################
 
-if get_option('ssl') == 'openssl'
+ssl = not_found_dep
+ssl_library = 'none'
+sslopt = get_option('ssl')
+
+if (sslopt == 'auto' and auto_features.disabled())
+  sslopt = 'none'
+endif
+
+if sslopt in ['auto', 'openssl']
+  openssl_required = sslopt == 'openssl'
 
   # Try to find openssl via pkg-config et al, if that doesn't work
   # (e.g. because it's provided as part of the OS, like on FreeBSD), look for
@@ -1192,59 +1202,72 @@ if get_option('ssl') == 'openssl'
     ssl_int = [ssl_lib, crypto_lib]
 
     ssl = declare_dependency(dependencies: ssl_int,
-                             include_directories: postgres_inc)
-  else
-    cc.has_header('openssl/ssl.h', args: test_c_args, dependencies: ssl, required: true)
-    cc.has_header('openssl/err.h', args: test_c_args, dependencies: ssl, required: true)
-
-    ssl_int = [ssl]
+                            include_directories: postgres_inc)
+  elif cc.has_header('openssl/ssl.h', args: test_c_args, dependencies: ssl, required: openssl_required) and \
+        cc.has_header('openssl/err.h', args: test_c_args, dependencies: ssl, required: openssl_required)
+  ssl_int = [ssl]
   endif
 
-  check_funcs = [
-    ['CRYPTO_new_ex_data', {'required': true}],
-    ['SSL_new', {'required': true}],
-
-    # Function introduced in OpenSSL 1.0.2.
-    ['X509_get_signature_nid'],
-
-    # Functions introduced in OpenSSL 1.1.0. We used to check for
-    # OPENSSL_VERSION_NUMBER, but that didn't work with 1.1.0, because LibreSSL
-    # defines OPENSSL_VERSION_NUMBER to claim version 2.0.0, even though it
-    # doesn't have these OpenSSL 1.1.0 functions. So check for individual
-    # functions.
-    ['OPENSSL_init_ssl'],
-    ['BIO_get_data'],
-    ['BIO_meth_new'],
-    ['ASN1_STRING_get0_data'],
-    ['HMAC_CTX_new'],
-    ['HMAC_CTX_free'],
-
-    # OpenSSL versions before 1.1.0 required setting callback functions, for
-    # thread-safety. In 1.1.0, it's no longer required, and CRYPTO_lock()
-    # function was removed.
-    ['CRYPTO_lock'],
-
-    # Function introduced in OpenSSL 1.1.1
-    ['X509_get_signature_info'],
-  ]
+  if ssl.found()
+    check_funcs = [
+      ['CRYPTO_new_ex_data', {'required': true}],
+      ['SSL_new', {'required': true}],
+
+      # Function introduced in OpenSSL 1.0.2.
+      ['X509_get_signature_nid'],
+
+      # Functions introduced in OpenSSL 1.1.0. We used to check for
+      # OPENSSL_VERSION_NUMBER, but that didn't work with 1.1.0, because LibreSSL
+      # defines OPENSSL_VERSION_NUMBER to claim version 2.0.0, even though it
+      # doesn't have these OpenSSL 1.1.0 functions. So check for individual
+      # functions.
+      ['OPENSSL_init_ssl'],
+      ['BIO_get_data'],
+      ['BIO_meth_new'],
+      ['ASN1_STRING_get0_data'],
+      ['HMAC_CTX_new'],
+      ['HMAC_CTX_free'],
+
+      # OpenSSL versions before 1.1.0 required setting callback functions, for
+      # thread-safety. In 1.1.0, it's no longer required, and CRYPTO_lock()
+      # function was removed.
+      ['CRYPTO_lock'],
+
+      # Function introduced in OpenSSL 1.1.1
+      ['X509_get_signature_info'],
+    ]
+
+    are_openssl_funcs_complete = true
+    foreach c : check_funcs
+      func = c.get(0)
+      val = cc.has_function(func, args: test_c_args, dependencies: ssl_int)
+      required = c.get(1, {}).get('required', false)
+      if required and not val
+        are_openssl_funcs_complete = false
+        if openssl_required
+          error('openssl function @0@ is required'.format(func))
+        endif
+        break
+      elif not required
+        cdata.set('HAVE_' + func.to_upper(), val ? 1 : false)
+      endif
+    endforeach
 
-  foreach c : check_funcs
-    func = c.get(0)
-    val = cc.has_function(func, args: test_c_args, dependencies: ssl_int)
-    required = c.get(1, {}).get('required', false)
-    if required and not val
-      error('openssl function @0@ is required'.format(func))
-    elif not required
-      cdata.set('HAVE_' + func.to_upper(), val ? 1 : false)
+    if are_openssl_funcs_complete
+      cdata.set('USE_OPENSSL', 1,
+                description: 'Define to 1 to build with OpenSSL support. (-Dssl=openssl)')
+      cdata.set('OPENSSL_API_COMPAT', '0x10001000L',
+                description: '''Define to the OpenSSL API version in use. This avoids deprecation warnings from newer OpenSSL versions.''')
+      ssl_library = 'openssl'
+    else
+      ssl = not_found_dep
     endif
-  endforeach
+  endif
+endif
 
-  cdata.set('USE_OPENSSL', 1,
-            description: 'Define to 1 to build with OpenSSL support. (-Dssl=openssl)')
-  cdata.set('OPENSSL_API_COMPAT', '0x10001000L',
-            description: '''Define to the OpenSSL API version in use. This avoids deprecation warnings from newer OpenSSL versions.''')
-else
-  ssl = not_found_dep
+# At least one SSL library must be found, otherwise throw an error
+if sslopt == 'auto' and auto_features.enabled() and not ssl.found()
+  error('SSL Library could not be found')
 endif

diff --git a/meson_options.txt b/meson_options.txt
index 7d33c9f1d4b..4402dd4299d 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -130,8 +130,8 @@ option('readline', type : 'feature', value : 'auto',
 option('selinux', type : 'feature', value : 'disabled',
   description: 'build with SELinux support')
 
-option('ssl', type : 'combo', choices : ['none', 'openssl'],
-  value : 'none',
+option('ssl', type : 'combo', choices : ['auto', 'none', 'openssl'],
+  value : 'auto',
   description: 'use LIB for SSL/TLS support (openssl)')
 
 option('systemd', type : 'feature', value: 'auto',
diff --git a/src/interfaces/libpq/meson.build b/src/interfaces/libpq/meson.build
index 573fd9b6ea4..3cd0ddb4945 100644
--- a/src/interfaces/libpq/meson.build
+++ b/src/interfaces/libpq/meson.build
@@ -117,7 +117,7 @@ tests += {
       't/001_uri.pl',
       't/002_api.pl',
     ],
-    'env': {'with_ssl': get_option('ssl')},
+    'env': {'with_ssl': ssl_library},
   },
 }
 
diff --git a/src/makefiles/meson.build b/src/makefiles/meson.build
index 5a0032ab0d2..7635771c5ae 100644
--- a/src/makefiles/meson.build
+++ b/src/makefiles/meson.build
@@ -66,7 +66,7 @@ pgxs_kv = {
   'SUN_STUDIO_CC': 'no', # not supported so far
 
   # want the chosen option, rather than the library
-  'with_ssl' : get_option('ssl'),
+  'with_ssl' : ssl_library,
   'with_uuid': uuidopt,
 
   'default_port': get_option('pgport'),
diff --git a/src/test/ssl/meson.build b/src/test/ssl/meson.build
index a8d9a5424d4..4cda81f3bc2 100644
--- a/src/test/ssl/meson.build
+++ b/src/test/ssl/meson.build
@@ -6,7 +6,7 @@ tests += {
   'bd': meson.current_build_dir(),
   'tap': {
     'env': {
-      'with_ssl': get_option('ssl'),
+      'with_ssl': ssl_library,
       'OPENSSL': openssl.path(),
     },
     'tests': [
-- 
2.39.2

---

From: Peter Eisentraut <[email-masked]>
Date: Thu, 9 Mar 2023 14:45:24 +0100

> 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?

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 9 Mar 2023 14:54:53 +0100

> On 9 Mar 2023, at 14:45, Peter Eisentraut <[email-masked]> 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

---

From: Nazir Bilal Yavuz <[email-masked]>
Date: Thu, 9 Mar 2023 17:12:26 +0300

Hi,

> 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

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 9 Mar 2023 15:15:49 +0100

> On 9 Mar 2023, at 15:12, Nazir Bilal Yavuz <[email-masked]> wrote:
> 
> Hi,
> 

That seems like a good idea.

--
Daniel Gustafsson

---

From: Peter Eisentraut <[email-masked]>
Date: Thu, 9 Mar 2023 15:18:07 +0100

> 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,
    ...
}

---

From: Nazir Bilal Yavuz <[email-masked]>
Date: Thu, 9 Mar 2023 17:40:13 +0300

Hi,

> Then we might as well use ssl_library as the key, like:

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

---

From: Peter Eisentraut <[email-masked]>
Date: Mon, 13 Mar 2023 07:27:18 +0100

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

---

From: Nathan Bossart <[email-masked]>
Date: Mon, 13 Mar 2023 11:04:32 -0700

> 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

---

From: Nazir Bilal Yavuz <[email-masked]>
Date: Mon, 13 Mar 2023 21:57:22 +0300

Hi,

> Shouldn't "auto" cause Postgres to be built without OpenSSL if the required

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

---

From: Nathan Bossart <[email-masked]>
Date: Mon, 13 Mar 2023 12:04:05 -0700

> 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

---

From: Andres Freund <[email-masked]>
Date: Mon, 13 Mar 2023 13:13:31 -0700

Hi,

> Shouldn't "auto" cause Postgres to be built without OpenSSL if the required

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
>From b8bd0200667bac16674e40e769a9fb4bc4f54306 Mon Sep 17 00:00:00 2001
From: Andres Freund <[email-masked]>
Date: Mon, 13 Mar 2023 13:11:37 -0700
Subject: [PATCH v1] meson: fix openssl detection issues in 6a30027

Reported-by: Nathan Bossart <[email-masked]>
Discussion: https://postgr.es/m/20230313180432.GA246741@nathanxps13
---
 meson.build | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/meson.build b/meson.build
index 8208815c96c..2ebdf914c1b 100644
--- a/meson.build
+++ b/meson.build
@@ -1189,23 +1189,29 @@ if sslopt in ['auto', 'openssl']
 
   # via pkg-config et al
   ssl = dependency('openssl', required: false)
+  # only meson >= 0.57 supports declare_dependency() in cc.has_function(), so
+  # we pass cc.find_library() results if necessary
+  ssl_int = []
 
   # via library + headers
   if not ssl.found()
     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
   elif cc.has_header('openssl/ssl.h', args: test_c_args, dependencies: ssl, required: openssl_required) and \
        cc.has_header('openssl/err.h', args: test_c_args, dependencies: ssl, required: openssl_required)
     ssl_int = [ssl]
+  else
+    ssl = not_found_dep
   endif
 
   if ssl.found()
-- 
2.38.0

---

From: Andres Freund <[email-masked]>
Date: Mon, 13 Mar 2023 13:14:56 -0700

Hi,

> 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

---

From: Nathan Bossart <[email-masked]>
Date: Mon, 13 Mar 2023 13:25:44 -0700

> +      ssl = declare_dependency(dependencies: ssl_int, include_directories: postgres_inc)

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.

---

From: Nazir Bilal Yavuz <[email-masked]>
Date: Mon, 13 Mar 2023 23:46:41 +0300

Hi,

> It worked for the dependency() path, but not the cc.find_library() path. See

Thanks for the patch, I understand the problem now and your patch fixes this.

Regards,
Nazir Bilal Yavuz
Microsoft

---

From: Andres Freund <[email-masked]>
Date: Mon, 13 Mar 2023 14:45:29 -0700

Hi,

> Thanks for the patch, I understand the problem now and your patch fixes this.

Pushed the patch.

Greetings,

Andres Freund

---

From: Nathan Bossart <[email-masked]>
Date: Mon, 13 Mar 2023 15:32:03 -0700

> Pushed the patch.

Thanks for the prompt fix.