various: Enclose string if checks in quotes #11617

nordicjm · 2023-06-26T14:25:35Z

Fixes possible issues whereby string variables are checked without being properly encased in quote marks.

This is needed because unescaped variables will be treated as variable names, then errors will be emitted when they are dereferenced and those variables do not exist.

An example of what happens with bad code when configuring a build:

   *************************
   * Running CMake for ncp *
   *************************

CMake Error at CMakeLists.txt:10 (if):
  if given arguments:

    "STREQUAL" "nrf52840dongle_nrf52840"

  Unknown arguments specified


-- Configuring incomplete, errors occurred!
CMake Error at cmake/modules/sysbuild_extensions.cmake:405 (message):
  CMake configure failed for Zephyr project: ncp

NordicBuilder · 2023-06-26T14:27:18Z

Test specification

CI/Jenkins/NRF

Integration Platforms

CI/Jenkins/integration

Test Module	File based changes	Manually selected	West overwrite
desktop52_verification	X
test-ci-nrfconnect-boot-fw-update	X
test-fw-nrfconnect-ble	X
test-fw-nrfconnect-ble_samples	X
test-fw-nrfconnect-chip	X
test-fw-nrfconnect-nrf-iot_zephyr_lwm2m	X
test-fw-nrfconnect-nrf_crypto	X
test-fw-nrfconnect-tfm	X
test-fw-nrfconnect-zigbee	X
test-sdk-find-my	X
test-sdk-homekit	X

Detailed information of selected test modules

Note: This message is automatically posted and updated by the CI

Damian-Nordic · 2023-06-27T06:59:10Z

Could you improve the PR description? That is, what kind of issues this might cause? At least I wasn't aware this might be wrong so would like to learn something :)

SebastianBoe · 2023-06-28T09:20:14Z

applications/matter_weather_station/CMakeLists.txt

@@ -14,7 +14,7 @@ set(APPLICATION_CONFIG_DIR "${CMAKE_CURRENT_SOURCE_DIR}/configuration/\${BOARD}"
 set(multiprotocol_rpmsg_KCONFIG_ROOT "\\\${ZEPHYR_CONNECTEDHOMEIP_MODULE_DIR}/config/nrfconnect/chip-module/Kconfig.multiprotocol_rpmsg.root")
 set(hci_rpmsg_KCONFIG_ROOT "\\\${ZEPHYR_CONNECTEDHOMEIP_MODULE_DIR}/config/nrfconnect/chip-module/Kconfig.hci_rpmsg.root")

-if(OVERLAY_CONFIG STREQUAL "overlay-factory_data.conf")
+if("${OVERLAY_CONFIG}" STREQUAL "overlay-factory_data.conf")


How is this a possible issue? :)

https://cmake.org/cmake/help/latest/command/if.html#strequal

Not all of them are issues, it is the ones using variables outside of quotes that are issues, I've updated the rest so that the style throughout the code remains uniform.

nordicjm · 2023-06-29T07:52:39Z

@Damian-Nordic I have added a description and example to the original message.

carlescufi

The change itself looks good to me, but could you explain in the commit message why this is necessary? Right now it is not clear to me, and I'd like to understand it. Thanks!

KAGA164

Looks good. It even fixes some CI toolchain issues

samples/bluetooth/direct_test_mode/CMakeLists.txt

samples/peripheral/radio_test/CMakeLists.txt

frkv · 2023-07-21T10:13:23Z

will the IN_LIST => IN LISTS issue be resolved? it looks like an actual error

nordicjm · 2023-07-25T08:45:37Z

will the IN_LIST => IN LISTS issue be resolved? it looks like an actual error

IN_LIST is the correct command, IN_LISTS does not exist, as per https://cmake.org/cmake/help/v3.3/command/if.html

KAGA164 · 2023-07-25T09:34:51Z

@nordicjm @frkv

will the IN_LIST => IN LISTS issue be resolved? it looks like an actual error

IN_LIST is the correct command, IN_LISTS does not exist, as per https://cmake.org/cmake/help/v3.3/command/if.html

We have foreach statement here not if. https://cmake.org/cmake/help/v3.3/command/foreach.html

Fixes possible issues whereby string variables are checked without being properly encased in quote marks. This is needed because unescaped variables will be treated as variable names, then errors will be emitted when they are dereferenced and those variables do not exist. This fixes issue like the following: ************************* * Running CMake for ncp * ************************* CMake Error at CMakeLists.txt:10 (if): if given arguments: "STREQUAL" "nrf52840dongle_nrf52840" Unknown arguments specified Signed-off-by: Jamie McCrae <jamie.mccrae@nordicsemi.no>

frkv · 2023-07-25T10:17:26Z

will the IN_LIST => IN LISTS issue be resolved? it looks like an actual error

IN_LIST is the correct command, IN_LISTS does not exist, as per https://cmake.org/cmake/help/v3.3/command/if.html

IN LISTS is not with an underscore, and this is not an if-statement.

frkv

LGTM

nordicjm requested review from frkv, Vge0rge, vili-nordic, joerchan, SebastianBoe, mswarowsky, hakonfam, sigvartmh, tomchy, sebastiandraus, KAGA164, maje-emb, Damian-Nordic, kkasperczyk-no, alwa-nordic and carlescufi as code owners June 26, 2023 14:25

github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jun 26, 2023

nordicjm requested review from tejlmand and removed request for maje-emb and vili-nordic June 26, 2023 14:25

nordicjm force-pushed the zigfix branch from bf3175d to e67fc53 Compare June 26, 2023 14:43

nordicjm requested a review from nordic-krch as a code owner June 26, 2023 14:43

nordicjm force-pushed the zigfix branch from e67fc53 to 42ca757 Compare June 27, 2023 06:47

tomchy approved these changes Jun 27, 2023

View reviewed changes

SebastianBoe reviewed Jun 28, 2023

View reviewed changes

nordicjm force-pushed the zigfix branch from 42ca757 to 881ee56 Compare June 29, 2023 08:02

SebastianBoe approved these changes Jun 29, 2023

View reviewed changes

nordicjm force-pushed the zigfix branch from 881ee56 to 0720565 Compare July 10, 2023 07:26

carlescufi requested changes Jul 14, 2023

View reviewed changes

nordicjm requested a review from carlescufi July 17, 2023 08:22

nordicjm force-pushed the zigfix branch 2 times, most recently from 79ede0e to 0ccd0ac Compare July 17, 2023 08:28

KAGA164 approved these changes Jul 19, 2023

View reviewed changes

samples/bluetooth/direct_test_mode/CMakeLists.txt Outdated Show resolved Hide resolved

samples/peripheral/radio_test/CMakeLists.txt Outdated Show resolved Hide resolved

joerchan approved these changes Jul 21, 2023

View reviewed changes

nordicjm force-pushed the zigfix branch from 0ccd0ac to 8b75c62 Compare July 25, 2023 10:13

frkv approved these changes Jul 25, 2023

View reviewed changes

carlescufi approved these changes Jul 26, 2023

View reviewed changes

carlescufi merged commit 6f4c69d into nrfconnect:main Jul 26, 2023

nordicjm deleted the zigfix branch September 25, 2023 13:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

various: Enclose string if checks in quotes #11617

various: Enclose string if checks in quotes #11617

nordicjm commented Jun 26, 2023 •

edited

Loading

NordicBuilder commented Jun 26, 2023 •

edited

Loading

Damian-Nordic commented Jun 27, 2023

SebastianBoe Jun 28, 2023

nordicjm Jun 29, 2023

nordicjm commented Jun 29, 2023

carlescufi left a comment

KAGA164 left a comment •

edited

Loading

frkv commented Jul 21, 2023

nordicjm commented Jul 25, 2023

KAGA164 commented Jul 25, 2023

frkv commented Jul 25, 2023

frkv left a comment

various: Enclose string if checks in quotes #11617

various: Enclose string if checks in quotes #11617

Conversation

nordicjm commented Jun 26, 2023 • edited Loading

NordicBuilder commented Jun 26, 2023 • edited Loading

Test specification

CI/Jenkins/NRF

CI/Jenkins/integration

Damian-Nordic commented Jun 27, 2023

SebastianBoe Jun 28, 2023

Choose a reason for hiding this comment

nordicjm Jun 29, 2023

Choose a reason for hiding this comment

nordicjm commented Jun 29, 2023

carlescufi left a comment

Choose a reason for hiding this comment

KAGA164 left a comment • edited Loading

Choose a reason for hiding this comment

frkv commented Jul 21, 2023

nordicjm commented Jul 25, 2023

KAGA164 commented Jul 25, 2023

frkv commented Jul 25, 2023

frkv left a comment

Choose a reason for hiding this comment

nordicjm commented Jun 26, 2023 •

edited

Loading

NordicBuilder commented Jun 26, 2023 •

edited

Loading

KAGA164 left a comment •

edited

Loading