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: Support multiple overlay files #1184

Merged
merged 1 commit into from
Feb 6, 2024

Conversation

kent-mcleod
Copy link
Member

Allow multiple overlay files to be specified. This supports custom tooling support to add additional memory reserve regions to a platform.

EG:

cmake -DKernelCustomDTSOverlay=../src/plat/imx6/overlay-nitrogen6sx.dts .
-- Found GCC with prefix arm-linux-gnueabi-
-- Appending /tmp/tmp.5Kv410Y6vE/kernel/src/plat/imx6/overlay-nitrogen6sx.dts overlay
-- /tmp/tmp.5Kv410Y6vE/kernel/build/gen_headers/plat/machine/devices_gen.h is out of date. Regenerating from DTB...


cmake -DKernelCustomDTSOverlay="../src/plat/imx6/overlay-nitrogen6sx.dts;../src/plat/imx6/overlay-wandq.dts" .
-- Found GCC with prefix arm-linux-gnueabi-
-- Appending /tmp/tmp.5Kv410Y6vE/kernel/src/plat/imx6/overlay-nitrogen6sx.dts overlay
-- Appending /tmp/tmp.5Kv410Y6vE/kernel/src/plat/imx6/overlay-wandq.dts overlay
-- /tmp/tmp.5Kv410Y6vE/kernel/build/gen_headers/plat/machine/devices_gen.h is out of date. Regenerating from DTB...


Copy link
Contributor

@Indanz Indanz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea.

@lsf37 lsf37 added the build-system related to the build system label Feb 2, 2024
Copy link
Member

@lsf37 lsf37 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. Still to fix up the style failure, though.

@Indanz
Copy link
Contributor

Indanz commented Feb 2, 2024

The style failure is nonsensical though, it suggests:

diff --git a/config.cmake b/config.cmake
index e33b8721..3fc6ee6c 100644
--- a/config.cmake
+++ b/config.cmake
@@ -124,7 +124,9 @@ if(DEFINED KernelDTSList AND (NOT "${KernelDTSList}" STREQUAL ""))
     set(config_schema "${CMAKE_CURRENT_SOURCE_DIR}/tools/hardware_schema.yml")
     set(
         KernelCustomDTSOverlay ""
-        CACHE STRING "Provide an additional list of overlays to append to the selected KernelPlatform's \
+        CACHE
+            STRING
+            "Provide an additional list of overlays to append to the selected KernelPlatform's \
         device tree during build time"
     )
     if(NOT "${KernelCustomDTSOverlay}" STREQUAL "")

@axel-h
Copy link
Member

axel-h commented Feb 2, 2024

The style failure is nonsensical though

Unfortunately, we have to accept this for now. Otherwise this issue will pop up again for every future change of this file, that is also annoying. It's not the only case where the tool's rules seem to lack something. Maybe this gets fixed in future versions (see also seL4/ci-actions#329)

@Indanz
Copy link
Contributor

Indanz commented Feb 2, 2024

The style failure is nonsensical though

Unfortunately, we have to accept this for now. Otherwise this issue will pop up again for every future change of this file, that is also annoying.

Agreed, but it's very annoying that we have nonsensical style checks to begin with, supposedly someone put effort in adding them at one point in time.

It's not the only case where the tool's rules seem to lack something. Maybe this gets fixed in future versions (see also seL4/ci-actions#329)

As far as I can tell there are no sensible style checks done for CMake files, so I would personally just disable style checking for CMake files altogether.

For instance, it's currently impossible to make a config string description that doesn't look horrible in ccmake because the style checker enforces a lot of whitespace before new lines.

endif()
list(APPEND KernelDTSList "${KernelCustomDTSOverlay}")
message(STATUS "Using ${KernelCustomDTSOverlay} overlay")
foreach(dts_entry IN ITEMS ${KernelCustomDTSOverlay})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, can't we drop the if(NOT "${KernelCustomDTSOverlay}" STREQUAL "") and just iterate? Iterating over an empty variable would just do nothing then.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The if deals with the case that the variable can be left unassigned. The inner foreach block relies on the variable having an assigned value to dereference.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where you could use foreach(dts_entry IN LISTS KernelCustomDTSOverlay), which aviods the dereferencing.

@lsf37
Copy link
Member

lsf37 commented Feb 2, 2024

The cmake style checks are made by cmake-format and are generally sensible. It has a few situations (like this one) where it produces output that is not ideal, but the TSC decision on that is that it is much preferred to have that than constant discussion about what is better.

Allow multiple overlay files to be specified. This supports custom
tooling support to add additional memory reserve regions to a platform.

Signed-off-by: Kent McLeod <kent@kry10.com>
@kent-mcleod kent-mcleod merged commit 76eee24 into seL4:master Feb 6, 2024
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build-system related to the build system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants