llext-edk: rework, read data from `build_info.yml` and `.config` #83705

pillo79 · 2025-01-08T17:31:15Z

This PR improves the LLEXT EDK in several ways:

an internal rework to reduce code duplication and improve extensibility
read existing data from the current build artifacts, instead of using arguments

It is effectively a subset of #78727 that does not add any new information to the LLEXT EDK.

Use a more concise way to append to a list property, as suggested in a previous PR review. Signed-off-by: Luca Burelli <l.burelli@arduino.cc>

pillo79 · 2025-01-30T16:59:58Z

Review ready. Rebased and included minor fix from this comment in related PR.

pillo79 · 2025-02-04T09:11:27Z

@tejlmand please review. This is the last piece of the rework to use YAML in the EDK, no functional changes.

tejlmand

Generally looks good, though care must be taken wrt. use of cmake_parse_arguments().
See relevant comments.

I also wonder if it is important in this case to have GENEX as a flag, or if we should simplify so that we just have:

#   build_info(<tag>... VALUE <value>... )
#   build_info(<tag>... GENEX <genex>... )
#   build_info(<tag>... PATH  <path>... )

that way the first keyword is still just separating <tag> from <values>.

There is also a possibility to detect genex'es, like this:

string(GENEX_STRIP "${value}" value_genex_free)
if(NOT value STREQUAL value_genex_free
  # Contain genex'es
endif()

which of course cost a little, but allows the caller of build_info() to just use or not-use genex'es as they like, just like they can do with other native CMake functions.

tejlmand · 2025-02-06T12:37:46Z

cmake/modules/extensions.cmake

-    list(FIND arg_list PATH index)
-    set(convert_path TRUE)
-  endif()
+  cmake_parse_arguments(ARG_BUILD_INFO "" "" "VALUE;PATH" ${ARGN})


there were two reasons why I refrained from using cmake_parse_arguments() for this function.

The first, and main reason, has to do with the keywords VALUE and PATH, and the fact that it turns into yaml content.
There is a possibility that at some point you may have a <key>: <value> pair, which could end up in something like this:

build_info(vendor-specific companyA type VALUE PATH)

in which case the use of cmake_parse_arguments() will give a wrong result.
You may say that the existing code of testing for VALUE before PATH also have a risk, like:

build_info(vendor-specific companyA key PATH VALUE)

but a path will usually be absolute, or as minimum relative to a known fix-point (such as Zephyr base), and therefore contain path separators.

The second (minor risk now) is that if a flag / single arg support is added, like:
build_info(<tag> FLAG stray_value VALUE foo)
then ARG_BUILD_INFO_UNPARSED_ARGUMENTS becomes <tag>;stray_value, and not just <tag>.
(and similar for one valye keywords when >1 value is passed).

This is one reason why the use of <prefix>_UNPARSED_ARGUMENTS should be done with great care, as it will be a collection of all unparsed args, regardless of their location.

Though for now, this is a non-issue, but if we are to later introduce flags, then we would probably need to swap back to the approach currently in use.

No functional change is intended in this commit.

🙈

tejlmand · 2025-02-06T12:39:05Z

cmake/modules/extensions.cmake

 function(build_info)
-  cmake_parse_arguments(ARG_BUILD_INFO "" "" "VALUE;PATH" ${ARGN})
+  cmake_parse_arguments(ARG_BUILD_INFO "GENEX" "" "VALUE;PATH" ${ARGN})


whoops. reviewing commit by commit seems like predicting the future 😆

tejlmand · 2025-02-06T12:46:14Z

cmake/llext-edk.cmake

 set(edk_file_MAKEFILE ${llext_edk}/Makefile.cflags)
 set(edk_file_CMAKE ${llext_edk}/cmake.cflags)

+# Escape problematic characters in a string


This is following current style in this file, but I find it problematic that function are defined between code lines, meaning we have:

<code> ... <functions> ... <more code>

Ref:
https://github.com/zephyrproject-rtos/zephyr/blob/0722d3675df66063c10b96467ed2768d9ae7010f/cmake/llext-edk.cmake#L146
https://github.com/zephyrproject-rtos/zephyr/blob/0722d3675df66063c10b96467ed2768d9ae7010f/cmake/llext-edk.cmake#L161
https://github.com/zephyrproject-rtos/zephyr/blob/0722d3675df66063c10b96467ed2768d9ae7010f/cmake/llext-edk.cmake#L205
https://github.com/zephyrproject-rtos/zephyr/blob/0722d3675df66063c10b96467ed2768d9ae7010f/cmake/llext-edk.cmake#L218

Non-blocking comment, as I won't request you to cleanup this in this PR, but still wanted to share my observation.

This patch enables support for genexes in the 'build_info()' function by identifying when arguments contain them and passing the GENEX flag to 'yaml_set()'. Genexes are not supported in tag lists; using them will immediately trigger an error by the schema checks. They are also not currently supported with PATH entries, because the actual contents of the list (whose paths are to be converted) are not known until after the CMake code has been processed. Signed-off-by: Luca Burelli <l.burelli@arduino.cc>

Prepare the current code so that functions come before actual code. Signed-off-by: Luca Burelli <l.burelli@arduino.cc>

This commit refactors the code that generates the Makefile and CMake EDK files into functions that accept the file type as an argument. No functional changes are introduced in this commit. Signed-off-by: Luca Burelli <l.burelli@arduino.cc>

This commit removes all CMake- or Makefile-specific variables from the code and instead uses a single list for each type of flag. Where a difference is needed between the two, a special marker is used. These markers are replaced later in the target-specific area of the write functions. Escaping of problematic characters is also added to the write functions, to ensure that quotes and backslashes in the flags are properly handled. Signed-off-by: Luca Burelli <l.burelli@arduino.cc>

pillo79 · 2025-02-06T18:31:03Z

Thanks @tejlmand for the review, I did not consider the pitfalls you mentioned with cmake_parse_arguments.

Here is v2, addressing all concerns:

use the current ad-hoc parser in build_info()
automatically add the GENEX flag when required
moved function definitions in llext-edk.cmake to the beginning of the file, as most were added in this PR

tejlmand

Looks good, though I suspect GENEX is not supposed to be part of the value 😉

tejlmand · 2025-02-06T20:02:33Z

CMakeLists.txt

@@ -2236,6 +2235,10 @@ list(APPEND llext_edk_cflags ${llext_filt_flags})
 list(APPEND llext_edk_cflags ${LLEXT_APPEND_FLAGS})
 list(APPEND llext_edk_cflags ${LLEXT_EDK_APPEND_FLAGS})

+build_info(llext-edk file PATH ${llext_edk_file})
+build_info(llext-edk cflags VALUE ${llext_edk_cflags} GENEX)


Is GENEX a value to store or left-over flag from previous iteration ?

Suggested change

build_info(llext-edk cflags VALUE ${llext_edk_cflags} GENEX)

build_info(llext-edk cflags VALUE ${llext_edk_cflags})

tejlmand · 2025-02-06T20:03:01Z

CMakeLists.txt

@@ -2236,6 +2235,10 @@ list(APPEND llext_edk_cflags ${llext_filt_flags})
 list(APPEND llext_edk_cflags ${LLEXT_APPEND_FLAGS})
 list(APPEND llext_edk_cflags ${LLEXT_EDK_APPEND_FLAGS})

+build_info(llext-edk file PATH ${llext_edk_file})
+build_info(llext-edk cflags VALUE ${llext_edk_cflags} GENEX)
+build_info(llext-edk include-dirs VALUE "$<TARGET_PROPERTY:zephyr_interface,INTERFACE_INCLUDE_DIRECTORIES>" GENEX)


Suggested change

build_info(llext-edk include-dirs VALUE "$<TARGET_PROPERTY:zephyr_interface,INTERFACE_INCLUDE_DIRECTORIES>" GENEX)

build_info(llext-edk include-dirs VALUE "$<TARGET_PROPERTY:zephyr_interface,INTERFACE_INCLUDE_DIRECTORIES>")

Currently, the llext-edk.cmake script requires a number of variables to be passed in from the main CMakeLists.txt file as arguments to be able to customize the generated files. To improve this rigid approach, the script is modified to read in the following files in the build directory: * 'zephyr/.config', for the final set of Kconfig options used; * 'build_info.yml', for the cmake-related variables. This is more flexible and also easier to maintain, as it doesn't require manual changes to the main CMakelists.txt file when new variables need to be referenced. Signed-off-by: Luca Burelli <l.burelli@arduino.cc>

pillo79 · 2025-02-07T08:12:12Z

Lunch coffee lost its effect by 7PM 🤦‍♂️ fixed, thanks for spotting!

cmake/modules/extensions.cmake

cmake/llext-edk.cmake

pillo79 mentioned this pull request Jan 9, 2025

cmake: support generator expression in yaml module #80007

Merged

pillo79 force-pushed the pr-llext-edk-rework branch 2 times, most recently from 922ea0d to 688384e Compare January 20, 2025 16:17

cmake: yaml: use APPEND PROPERTY to shorten code

302c7c9

Use a more concise way to append to a list property, as suggested in a previous PR review. Signed-off-by: Luca Burelli <l.burelli@arduino.cc>

pillo79 force-pushed the pr-llext-edk-rework branch from 688384e to 8bee6e0 Compare January 30, 2025 14:56

pillo79 marked this pull request as ready for review January 30, 2025 17:00

zephyrbot added area: llext Linkable Loadable Extensions area: Build System labels Jan 30, 2025

zephyrbot requested review from 57300, jeremybettis, lyakh, nashif, nordicjm, teburd and tejlmand January 30, 2025 17:00

zephyrbot assigned tejlmand Jan 30, 2025

tejlmand requested changes Feb 6, 2025

View reviewed changes

pillo79 added 4 commits February 6, 2025 16:15

llext-edk: refact: do not mix code and function definitions

99ee09c

Prepare the current code so that functions come before actual code. Signed-off-by: Luca Burelli <l.burelli@arduino.cc>

pillo79 force-pushed the pr-llext-edk-rework branch from 8bee6e0 to d25d2e1 Compare February 6, 2025 17:52

tejlmand requested changes Feb 6, 2025

View reviewed changes

pillo79 force-pushed the pr-llext-edk-rework branch from d25d2e1 to c40bded Compare February 6, 2025 21:36

pillo79 requested a review from tejlmand February 7, 2025 08:14

tejlmand approved these changes Feb 7, 2025

View reviewed changes

teburd reviewed Feb 7, 2025

View reviewed changes

cmake/modules/extensions.cmake Show resolved Hide resolved

cmake/llext-edk.cmake Show resolved Hide resolved

teburd approved these changes Feb 7, 2025

View reviewed changes

kartben merged commit 5d3fe7e into zephyrproject-rtos:main Feb 7, 2025
29 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

llext-edk: rework, read data from `build_info.yml` and `.config` #83705

llext-edk: rework, read data from `build_info.yml` and `.config` #83705

Uh oh!

pillo79 commented Jan 8, 2025 •

edited

Loading

Uh oh!

pillo79 commented Jan 30, 2025

Uh oh!

pillo79 commented Feb 4, 2025

Uh oh!

tejlmand left a comment

Uh oh!

tejlmand Feb 6, 2025

Uh oh!

tejlmand Feb 6, 2025

Uh oh!

tejlmand Feb 6, 2025

Uh oh!

pillo79 commented Feb 6, 2025

Uh oh!

tejlmand left a comment

Uh oh!

tejlmand Feb 6, 2025

Uh oh!

tejlmand Feb 6, 2025

Uh oh!

pillo79 commented Feb 7, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

	build_info(llext-edk cflags VALUE ${llext_edk_cflags} GENEX)
	build_info(llext-edk cflags VALUE ${llext_edk_cflags})

	build_info(llext-edk include-dirs VALUE "$<TARGET_PROPERTY:zephyr_interface,INTERFACE_INCLUDE_DIRECTORIES>" GENEX)
	build_info(llext-edk include-dirs VALUE "$<TARGET_PROPERTY:zephyr_interface,INTERFACE_INCLUDE_DIRECTORIES>")

llext-edk: rework, read data from build_info.yml and .config #83705

llext-edk: rework, read data from build_info.yml and .config #83705

Uh oh!

Conversation

pillo79 commented Jan 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pillo79 commented Jan 30, 2025

Uh oh!

pillo79 commented Feb 4, 2025

Uh oh!

tejlmand left a comment

Choose a reason for hiding this comment

Uh oh!

tejlmand Feb 6, 2025

Choose a reason for hiding this comment

Uh oh!

tejlmand Feb 6, 2025

Choose a reason for hiding this comment

Uh oh!

tejlmand Feb 6, 2025

Choose a reason for hiding this comment

Uh oh!

pillo79 commented Feb 6, 2025

Uh oh!

tejlmand left a comment

Choose a reason for hiding this comment

Uh oh!

tejlmand Feb 6, 2025

Choose a reason for hiding this comment

Uh oh!

tejlmand Feb 6, 2025

Choose a reason for hiding this comment

Uh oh!

pillo79 commented Feb 7, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

llext-edk: rework, read data from `build_info.yml` and `.config` #83705

llext-edk: rework, read data from `build_info.yml` and `.config` #83705

pillo79 commented Jan 8, 2025 •

edited

Loading