Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmake: new add_option_defs helper, add KEYLOG_EXPORT build param #8174

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

redbaron
Copy link

@redbaron redbaron commented Nov 11, 2024

Description

Add add_option_defs helper which cover simple case of setting compile definitions when build option is selected. New helper uses named arguments and adds following QoL improvements:

  • optionally infer default values from list of possible values. First possible value becomes default.
  • optionally append default value to helper string. Helper string and default values can't go out of sync
  • support adding defines if build option is set. It covers many simple cases like new WOLFSSL_KEYLOG_EXPORT

Fixes #8165

Testing

  1. Build before and after patch, diffed CMakeCache.txt files to find only new WOLFSSL_KEYLOG_EXPORT function is different.

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • Updated manual and documentation

@wolfSSL-Bot
Copy link

Can one of the admins verify this patch?

endif()

if(DEFINED ${NAME})
list(FIND VALUES ${${NAME}} IDX)
if(DEFINED ${arg_NAME})
Copy link
Author

@redbaron redbaron Nov 11, 2024

Choose a reason for hiding this comment

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

I don't understand purpose this if() in original function. I transformed it mechanically to new args so it should work as before.

when building there is no difference in CMakeCache.txt before and after, so this branch either never triggered or mechanical transformation works as before.

Copy link
Author

Choose a reason for hiding this comment

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

OK, I think I get it. It triggers when build options is passed via command line with value which is not among the list of supported values, like we pass "ON" for values like "yes|no" This works with booleans, but doesn't it break build options where string input is expected?

Copy link
Contributor

Choose a reason for hiding this comment

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

@redbaron sorry accidentally deleted my previous response.

You are correct, this looks like it squashes non-boolean values inappropriately. Could you add a check for this case, to ensure this doesn't happen? Something like this to fall back to defaults maybe?

if(DEFINED ${NAME})
    list(FIND VALUES ${${NAME}} IDX)

    # Apply override only if VALUES is yes/no
    if (VALUES STREQUAL "yes;no" OR VALUES STREQUAL "no;yes")
        if (${IDX} EQUAL -1)
            # Interpret CMake truthiness for boolean-like values
            if(${${NAME}})
                override_cache(${NAME} "yes")
            else()
                override_cache(${NAME} "no")
            endif()
        endif()
    else()
        # For non-boolean VALUES, reset to default or error on invalid input
        if (${IDX} EQUAL -1)
            message(WARNING "Invalid value for ${NAME}. Resetting to default: ${DEFAULT}")
            set(${NAME} ${DEFAULT} CACHE STRING "${HELP_STRING}")
        endif()
    endif()
endif()

@embhorn
Copy link
Member

embhorn commented Nov 11, 2024

Hi @redbaron

Thanks for opening this PR. We require contributors to be approved by the wolfSSL team. Could you please send an email to [email protected] to request an application form?

Thanks,
@embhorn - wolfSSL Support

Add add_option_defs helper which cover simple case of setting
compile definitions when build option is selected. New helper
uses named arguments and adds following QoL improvements:

- optionally infer default values from list of possible values.
  First value of allowed values list becomes default.
- optionally append default value to helper string. Helper string
  always reflects accurate default value.
- support adding defines if build option is set. It covers many
  simple cases like new WOLFSSL_KEYLOG_EXPORT.
@embhorn
Copy link
Member

embhorn commented Nov 12, 2024

@redbaron is approved as a wolfSSL contributor

endif()

if(DEFINED ${NAME})
list(FIND VALUES ${${NAME}} IDX)
if(DEFINED ${arg_NAME})
Copy link
Contributor

Choose a reason for hiding this comment

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

@redbaron sorry accidentally deleted my previous response.

You are correct, this looks like it squashes non-boolean values inappropriately. Could you add a check for this case, to ensure this doesn't happen? Something like this to fall back to defaults maybe?

if(DEFINED ${NAME})
    list(FIND VALUES ${${NAME}} IDX)

    # Apply override only if VALUES is yes/no
    if (VALUES STREQUAL "yes;no" OR VALUES STREQUAL "no;yes")
        if (${IDX} EQUAL -1)
            # Interpret CMake truthiness for boolean-like values
            if(${${NAME}})
                override_cache(${NAME} "yes")
            else()
                override_cache(${NAME} "no")
            endif()
        endif()
    else()
        # For non-boolean VALUES, reset to default or error on invalid input
        if (${IDX} EQUAL -1)
            message(WARNING "Invalid value for ${NAME}. Resetting to default: ${DEFAULT}")
            set(${NAME} ${DEFAULT} CACHE STRING "${HELP_STRING}")
        endif()
    endif()
endif()

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.

CMake buid scripts don't offer --enable-keylog-export like option
4 participants