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: Escape literal '"' chars in image_preload variables #7997

Conversation

mike-scofield
Copy link

The Zephyr build system supports passing CONFIG_ variables on the west command line, like this:

    west build -- -D"CONFIG_SOME_STRING=\"some string\""

The logic in zephyr/cmake/modules/kconfig.cmake which implements this feature takes each CONFIG_ variable defined in the cmake cache, and writes the value of that variable directly into extra_kconfig_options.conf. Therefore, if the user wants to add a string value to kconfig using this mechanism, the extra_kconfig_options.conf entry would need to look like:

    CONFIG_BOOT_SIGNATURE_KEY_FILE="/foo/bar/private.pem"

and the image_preload.cmake definition that generates this result would therefore be:

    set(CONFIG_BOOT_SIGNATURE_KEY_FILE "\"/foo/bar/private.pem\"" CACHE INTERNAL "NCS child image controlled")

However, as of NCS v2.0.0, this doesn't work correctly. If I build with this command line:

    west build --pristine --cmake-only -b nrf52840dk_nrf52840 -- \
        -D"mcuboot_CONFIG_BOOT_SIGNATURE_KEY_FILE=\"/foo/bar/private.pem\""

I see that the multi_image.cmake script from NCS generates invalid syntax in both image_preload files:

    set(CONFIG_BOOT_SIGNATURE_KEY_FILE ""/foo/bar/private.pem"" CACHE INTERNAL "NCS child image controlled")

To fix this, NCS must add explicit escaping of '"' characters if they are present in the user-supplied string.

@CLAassistant
Copy link

CLAassistant commented Jun 17, 2022

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jun 17, 2022
@NordicBuilder
Copy link
Contributor

Thank you for your contribution!
It seems you are not a member of the nrfconnect GitHub organization. External contributions are handled as follows:
Large contributions, affecting multiple subsystems for example, may be rejected if they are complex, may introduce regressions due to lack of test coverage, or if they are not consistent with the architecture of nRF Connect SDK.
PRs will be run in our continuous integration (CI) test system.
If CI passes, PRs will be tagged for review and merged on successful completion of review. You may be asked to make some modifications to your contribution during review.
If CI fails, PRs may be rejected or may be tagged for review and rework.
PRs that become outdated due to other changes in the repository may be rejected or rework requested.
External contributions will be prioritized for review based on the relevance to current development efforts in nRF Connect SDK. Bug fix PRs will be prioritized.
You may raise issues or ask for help from our Technical Support team by visiting https://devzone.nordicsemi.com/.

Note: This comment is automatically posted and updated by the Contribs GitHub Action.

@NordicBuilder NordicBuilder added the external External contribution label Jun 17, 2022
@lemrey
Copy link
Contributor

lemrey commented Jun 17, 2022

Thank you for contributing to NCS!
I've let CI run on your PR, you'll probably have to sign-off the commit and shorten some lines to please gitlint.

@NordicBuilder
Copy link
Contributor

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publishing GitHub Action.

The Zephyr build system supports passing CONFIG_ variables on the
west command line, like this:

    west build -- -D"CONFIG_SOME_STRING=\"some string\""

The logic in zephyr/cmake/modules/kconfig.cmake which implements this
feature takes each CONFIG_ variable defined in the cmake cache, and
writes the value of that variable directly into extra_kconfig_options.conf.
Therefore, if the user wants to add a *string* value to kconfig using this
mechanism, the extra_kconfig_options.conf entry would need to look like:

    CONFIG_BOOT_SIGNATURE_KEY_FILE="/foo/bar/private.pem"

and the image_preload.cmake definition that generates this result would
therefore be:

    set(CONFIG_BOOT_SIGNATURE_KEY_FILE "\"/foo/bar/private.pem\"" CACHE
        INTERNAL "NCS child image controlled")

However, as of NCS v2.0.0, this doesn't work correctly.  If I build with
this command line:

    west build --pristine --cmake-only -b nrf52840dk_nrf52840 -- \
        -D"mcuboot_CONFIG_BOOT_SIGNATURE_KEY_FILE=\"/foo/bar/private.pem\""

I see that the multi_image.cmake script from NCS generates invalid syntax
in both image_preload files:

    set(CONFIG_BOOT_SIGNATURE_KEY_FILE ""/foo/bar/private.pem"" CACHE
        INTERNAL "NCS child image controlled")

To fix this, NCS must add explicit escaping of '"' characters if they
are present in the user-supplied string.

Signed-off-by: Michael Scofield <mscof4378@gmail.com>
@mike-scofield
Copy link
Author

Further testing indicated that the string(REPLACE line needed additional quotes to avoid breaking overlays. Added

@github-actions
Copy link

This pull request 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 pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. external External contribution Stale
Projects
None yet
4 participants