-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
soc: nordic: handle -include for IAR #81446
soc: nordic: handle -include for IAR #81446
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this cleanup, but please make the changes to the toolchain / compiler infrastructure instead.
soc/nordic/CMakeLists.txt
Outdated
@@ -17,7 +17,11 @@ zephyr_library_sources( | |||
# headers for different SoCs. | |||
set(dt_binding_includes ${DTS_INCLUDE_FILES}) | |||
list(FILTER dt_binding_includes INCLUDE REGEX "/dt-bindings/.*\.h$") | |||
list(TRANSFORM dt_binding_includes PREPEND "-include;") | |||
if(CMAKE_C_COMPILER_ID STREQUAL "IAR") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should avoid compiler / toolchain specific handling inside the code as it makes support for multiple toolchains much harder.
Instead the code itself should be cleaned up and the include flag should be defined in the compiler properties and picked up where used.
For example like this:
set(include_flag $<TARGET_PROPERTY:compiler,include-file>)
set_source_files_properties(
validate_binding_headers.c
DIRECTORY ${CMAKE_CURRENT_LIST_DIR}
PROPERTIES COMPILE_OPTIONS "${include_flag}$<JOIN:${dt_binding_includes},;${include_flag}>"
)
and then in https://github.com/zephyrproject-rtos/zephyr/blob/main/cmake/compiler/gcc/compiler_flags.cmake add a line like this:
set_compiler_property(PROPERTY include-file "-include$<SEMICOLON>")
# Note to @RobinKastberg: check if "-include;" is sufficient in above.
and in the correspong IAR compiler_flags.cmake
you can then do similar:
set_compiler_property(PROPERTY include-file "-preinclude$<SEMICOLON>")
and there will no longer be any need for doing compiler check in the CMake code itself, plus a benefit is that another toolchain can just provide its own flag and doesn't have to add an extra if-clause to this CMake code.
cae1de5
to
ea534ef
Compare
1d4d1fa
to
e3fa6b9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, a minor suggestion to keep a general safer pattern.
(devs tends to copy-paste code they find elsewhere)
e3fa6b9
to
096d51e
Compare
@RobinKastberg this needs rebasing to get past the unrelated CI failure |
096d51e
to
ce322c5
Compare
CMakeLists.txt uses the C compiler parameter -include, This is causing issues for other toolchains and needs to generalized. Signed-off-by: Robin Kastberg <[email protected]>
ce322c5
to
267a790
Compare
CMakeLists.txt uses the C compiler parameter -include, for IAR this is --preinclude.