zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.61k stars 6.5k forks source link

sysbuild/twister/cmake/kconfig: Issue with setting remote board in twister using sysbuild #69552

Open nordicjm opened 7 months ago

nordicjm commented 7 months ago

Describe the bug There appears to be an issue with sysbuild/twister/cmake/kconfig whereby if there is a sysbuild project which has a remote image which sets a board from a value provided to it and twister provides this value through extra_args it seems to have a malformed string, and fails to configure.

Expected behavior String should be passed successfully

Impact Showstopper

Environment (please complete the following information):

57300 commented 7 months ago

This bug applies to any namespaced config of string type, such as:

extra_args:
  SB_CONFIG_BOOT_SIGNATURE_KEY_FILE="foo.pem"
  mcuboot_CONFIG_KERNEL_BIN_NAME="mcuboot"

I don't think this needs to be supported by extra_args though, with its quirky handling of quotes. There is special treatment for CMake variables starting with CONFIG_: (which didn't need to be merged imho)

https://github.com/zephyrproject-rtos/zephyr/blob/4f34a410fa2c96bd98f4eb2db296bf0c3e8b0d0e/scripts/pylib/twister/twisterlib/runner.py#L1036-L1040

But trying to extend this to <namespace>_CONFIG_ would risk matching false positives, like APPLICATION_CONFIG_DIR.

I'd focus on supporting namespaced configs in extra_configs, which already works with west build -T. Twister doesn't allow it because it generates a testsuite_extra.conf with extra_configs contents verbatim*, but it could be updated to generate multiple Kconfig fragments, one per namespace. For example:

*except for arch/platform filters, which west doesn't handle

gmarull commented 7 months ago

more details https://github.com/zephyrproject-rtos/zephyr/issues/69669

tejlmand commented 7 months ago

Proper quoting of Kconfig settings through twister is already supported today, but one must use extra_configs, not extra_args.

The problem with extra_configs is that it generates a conf file and pass this conf file as -DOVERLAY_CONFIG=<twister-name>.conf, where it ought to be -DSB_OVERLAY_CONFIG when using sysbuild.

This can be seen by updating https://github.com/zephyrproject-rtos/zephyr/blob/5046229bb94cd2683ec1022848991d387152a0d2/scripts/pylib/twister/twisterlib/runner.py#L1061 to SB_OVERLAY_CONFIG and run the sample.yml in https://github.com/zephyrproject-rtos/zephyr/pull/69652. That PR will now build correctly if setting REMOTE_BOARD as extra_configs value.

Will post a fix so that sysbuild is supported in extra_configs.

Note: x_OVERLAY_CONFIG is deprecated in favor of x_EXTRA_CONF_FILE.

tejlmand commented 7 months ago

But trying to extend this to CONFIG would risk matching false positives, like APPLICATION_CONFIG_DIR.

imo that is really not a risk, because APPLICATION_CONFIG_DIR this is not a Kconfig but a CMake setting, and thus it should be passed through extra_args not extra_configs.

github-actions[bot] commented 5 months ago

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.

nashif commented 4 months ago

@tejlmand can you please take a look?

github-actions[bot] commented 2 months ago

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.