Skip to content

make "yes;no" cmake options boolean instead of string#7380

Merged
dgarske merged 1 commit intowolfSSL:masterfrom
oltolm:yesno
May 15, 2024
Merged

make "yes;no" cmake options boolean instead of string#7380
dgarske merged 1 commit intowolfSSL:masterfrom
oltolm:yesno

Conversation

@oltolm
Copy link
Copy Markdown
Contributor

@oltolm oltolm commented Apr 1, 2024

Description

Please describe the scope of the fix or feature addition.

Fixes #5438

add_option in functions.cmake always produces variables of type string even for boolean options. I fixed it to produce boolean variables for options that only allow "yes;no".

@wolfSSL-Bot
Copy link
Copy Markdown

Can one of the admins verify this patch?

@dgarske
Copy link
Copy Markdown
Member

dgarske commented Apr 1, 2024

Okay to test. Contributor agreement on file. Thank you @oltolm . Changes look great!

@dgarske dgarske self-requested a review April 1, 2024 15:14
@dgarske dgarske self-assigned this Apr 1, 2024
@dgarske
Copy link
Copy Markdown
Member

dgarske commented Apr 1, 2024

Retest this please

@dgarske dgarske assigned douzzer and unassigned dgarske Apr 1, 2024
Copy link
Copy Markdown
Member

@dgarske dgarske left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this change, but I think it requires some additional changes in the CMakeList.txt. For example after this change many of the options change to OFF/ON, but some are still yes/no.

Current master:

WOLFSSL_16BIT                    no
WOLFSSL_32BIT                    no
WOLFSSL_AES                      yes
WOLFSSL_AESCBC                   yes
WOLFSSL_AESCCM                   no
WOLFSSL_AESCFB                   no
WOLFSSL_AESCTR                   no
WOLFSSL_AESGCM                   yes
WOLFSSL_AESKEYWRAP               no
WOLFSSL_AESOFB                   no
WOLFSSL_AESSIV                   no
WOLFSSL_ALIGN_DATA               yes
WOLFSSL_ALPN                     no
WOLFSSL_ALT_CERT_CHAINS          no
WOLFSSL_ARC4                     no
WOLFSSL_ARIA                     no
WOLFSSL_ASIO                     no
WOLFSSL_ASM                      yes
WOLFSSL_ASN                      yes
WOLFSSL_ASYNC_THREADS            no
WOLFSSL_BASE64_ENCODE            yes
WOLFSSL_BUILD_OUT_OF_TREE        yes
WOLFSSL_CAAM                     no
WOLFSSL_CERTEXT                  no
WOLFSSL_CERTGEN                  no
WOLFSSL_CERTGENCACHE             no
WOLFSSL_CERTREQ                  no
WOLFSSL_CHACHA                   yes
...

Your PR 7380:

WOLFSSL_16BIT                    OFF
WOLFSSL_32BIT                    OFF
WOLFSSL_AES                      ON
WOLFSSL_AESCBC                   ON
WOLFSSL_AESCCM                   OFF
WOLFSSL_AESCFB                   OFF
WOLFSSL_AESCTR                   OFF
WOLFSSL_AESGCM                   yes
WOLFSSL_AESKEYWRAP               OFF
WOLFSSL_AESOFB                   OFF
WOLFSSL_AESSIV                   OFF
WOLFSSL_ALIGN_DATA               ON
WOLFSSL_ALPN                     OFF
WOLFSSL_ALT_CERT_CHAINS          OFF
WOLFSSL_ARC4                     OFF
WOLFSSL_ARIA                     OFF
WOLFSSL_ASIO                     OFF
WOLFSSL_ASM                      ON
WOLFSSL_ASN                      ON
WOLFSSL_ASYNC_THREADS            OFF
WOLFSSL_BASE64_ENCODE            ON
WOLFSSL_BUILD_OUT_OF_TREE        ON
WOLFSSL_CAAM                     OFF
WOLFSSL_CERTEXT                  OFF
WOLFSSL_CERTGEN                  OFF
WOLFSSL_CERTGENCACHE             OFF
WOLFSSL_CERTREQ                  OFF
WOLFSSL_CHACHA                   yes
...

@dgarske dgarske assigned oltolm and unassigned douzzer May 15, 2024
@oltolm
Copy link
Copy Markdown
Contributor Author

oltolm commented May 15, 2024

Those options that are still "yes" or "no" are not boolean. For example WOLFSSL_AESGCM is defined like this

add_option("WOLFSSL_AESGCM"
    "Enable wolfSSL AES-GCM support (default: enabled)"
    "yes" "yes;no;table;small;word32;4bit")

As you can see it's not a boolean option.

@dgarske
Copy link
Copy Markdown
Member

dgarske commented May 15, 2024

Those options that are still "yes" or "no" are not boolean. For example WOLFSSL_AESGCM is defined like this

add_option("WOLFSSL_AESGCM"
    "Enable wolfSSL AES-GCM support (default: enabled)"
    "yes" "yes;no;table;small;word32;4bit")

As you can see it's not a boolean option.

That makes sense. I like the change. Does this change to use boolean fix an issue or is it more to improve your experience when building with CMake?

@oltolm
Copy link
Copy Markdown
Contributor Author

oltolm commented May 15, 2024

Those options that are still "yes" or "no" are not boolean. For example WOLFSSL_AESGCM is defined like this

add_option("WOLFSSL_AESGCM"
    "Enable wolfSSL AES-GCM support (default: enabled)"
    "yes" "yes;no;table;small;word32;4bit")

As you can see it's not a boolean option.

That makes sense. I like the change. Does this change to use boolean fix an issue or is it more to improve your experience when building with CMake?

I remember having some kind of problem, but I couldn't reproduce it. It's just a general improvement. The CMake GUI now shows a checkbox instead of a dropdown for boolean options.

@dgarske dgarske merged commit 1d1800a into wolfSSL:master May 15, 2024
@oltolm oltolm deleted the yesno branch May 16, 2024 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Autoconf'ism in CMakeLists.txt

5 participants