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

soc: nordic: handle -include for IAR #81446

Merged
merged 1 commit into from
Dec 16, 2024

Conversation

RobinKastberg
Copy link
Contributor

CMakeLists.txt uses the C compiler parameter -include, for IAR this is --preinclude.

masz-nordic
masz-nordic previously approved these changes Nov 15, 2024
@thedjnK thedjnK assigned tejlmand and unassigned anangl and masz-nordic Nov 15, 2024
@thedjnK thedjnK requested a review from tejlmand November 15, 2024 18:34
Copy link
Collaborator

@tejlmand tejlmand left a 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.

@@ -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")
Copy link
Collaborator

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.

Copy link
Collaborator

@tejlmand tejlmand left a 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)

soc/nordic/CMakeLists.txt Outdated Show resolved Hide resolved
@kartben
Copy link
Collaborator

kartben commented Dec 13, 2024

@RobinKastberg this needs rebasing to get past the unrelated CI failure

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]>
@kartben kartben merged commit 742679a into zephyrproject-rtos:main Dec 16, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants