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:bugfix generate config.h contain ; characters will be handled incorrectly #12237

Merged
merged 1 commit into from
Apr 29, 2024

Conversation

xuxin930
Copy link
Contributor

Summary

Issue

; is treated as a list separator in CMake.
when setting a configuration similar to the following,
an error will occur when generating config.h

CONFIG_LV_TXT_BREAK_CHARS " ,.;:-_)]}"

Caution

This header file is completely wrong from line 237 onwards
img_v2_cfa97d61-86f3-43ff-b771-3be50c2602dl

Reason

This is because the file(STRING) function will read the wrong .config value line.
and string(REGEX REPLACE) will also incorrectly handle lines containing ; .

Solution

I changed the reading process to parse the entire .config as a big string.
parse each line character by character.
when there is a special character ; , it is treated as a list and finally spliced.

Impact

Testing

CI build

@xuxin930
Copy link
Contributor Author

Hi @anchao @XuNeo
please help review whether this is correct 🙏

@XuNeo
Copy link
Contributor

XuNeo commented Apr 26, 2024

I have tested with lvgl v9 and it works!

@acassis
Copy link
Contributor

acassis commented Apr 26, 2024

@xuxin930 nice description of a problem and its solution! Kudos!!!

@anchao
Copy link
Contributor

anchao commented Apr 26, 2024

Is there any other way? This implementation seems complex and ugly,have you tried this config in zephyr build system?

@anchao
Copy link
Contributor

anchao commented Apr 28, 2024

This issue not caused by ';' character, which should be foreach() treated close bracket ']' as a regular expression:
https://stackoverflow.com/questions/26057678/escape-characters-in-a-semicolon-separated-list-in-cmake
https://cmake.org/pipermail/cmake/2016-March/063030.html

@xuxin930 try this patch to check if there is any improvement.

diff --git a/cmake/nuttx_mkconfig.cmake b/cmake/nuttx_mkconfig.cmake
index fc040eb840..88353e2042 100644
--- a/cmake/nuttx_mkconfig.cmake
+++ b/cmake/nuttx_mkconfig.cmake
@@ -92,7 +92,11 @@ file(APPEND ${CONFIG_H}
 file(APPEND ${CONFIG_H} "#define CONFIG_BASE_DEFCONFIG \"${BASE_DEFCONFIG}\"\n")
 
 file(STRINGS ${CMAKE_BINARY_DIR}/.config ConfigContents)
+STRING(REGEX REPLACE "\\[" "__OPEN_BRACKET__" ConfigContents "${ConfigContents}")
+STRING(REGEX REPLACE "\\]" "__CLOSE_BRACKET__" ConfigContents "${ConfigContents}")
 foreach(NameAndValue ${ConfigContents})
+  STRING(REGEX REPLACE "__OPEN_BRACKET__" "[" NameAndValue "${NameAndValue}")
+  STRING(REGEX REPLACE "__CLOSE_BRACKET__" "]" NameAndValue "${NameAndValue}")
   string(REGEX REPLACE "^[ ]+" "" NameAndValue ${NameAndValue})
   string(REGEX MATCH "^CONFIG[^=]+" NAME ${NameAndValue})
   # skip BASE_DEFCONFIG here as it is handled above

@xuxin930 xuxin930 marked this pull request as draft April 28, 2024 11:35
@xuxin930 xuxin930 marked this pull request as ready for review April 29, 2024 11:20
…ncorrectly

`;` is treated as a list separator in CMake.
the file(STRING) function will read the wrong .config value
string(REGEX REPLACE) will also incorrectly handle lines containing `;`
so here we can only parse character by character.

Signed-off-by: xuxin19 <xuxin19@xiaomi.com>
@xuxin930
Copy link
Contributor Author

xuxin930 commented Apr 29, 2024

This issue not caused by ';' character, which should be foreach() treated close bracket ']' as a regular expression: https://stackoverflow.com/questions/26057678/escape-characters-in-a-semicolon-separated-list-in-cmake https://cmake.org/pipermail/cmake/2016-March/063030.html

@xuxin930 try this patch to check if there is any improvement.

@anchao Yes! 👍
I changed the solution and modified other methods of reading config to ensure consistency.
Unfortunately🙁, CMake does NOT provide calls like proxy functions or wrapper functions.
I can only use marco to replace them.

@xuxin930
Copy link
Contributor Author

@xuxin930 nice description of a problem and its solution! Kudos!!!

@acassis thank you very much for your encouragement, I will continue to be as clear as possible when dealing with issues

@acassis acassis merged commit 94d8fba into apache:master Apr 29, 2024
23 of 26 checks passed
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.

6 participants