-
Notifications
You must be signed in to change notification settings - Fork 8k
sysbuild: support application CMakePresets.json files with sysbuild #96609
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
sysbuild: support application CMakePresets.json files with sysbuild #96609
Conversation
Needs a rebase |
004d112
to
d8f180a
Compare
@benedekkupper ptal. |
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.
As a minor (not blocking) note then this PR only makes it possible to use presets together with
west
, and thus does not support pure CMake / projects which are not using west, but have custom tools.
Actually, it already works with pure CMake if you define APP_DIR
in your environment beforehand.
In fact, I would suggest updating sysbuild itself so it could pick up the same environment variable:
zephyr/share/sysbuild/CMakeLists.txt
Lines 7 to 9 in 3232354
if(NOT DEFINED APP_DIR) | |
message(FATAL_ERROR "No main application specified") | |
endif() |
The standard, non-west sysbuild invocation could then change from:
cmake -Bbuild ~/ncs/zephyr/share/sysbuild -DAPP_DIR=$PWD
to:
APP_DIR=$PWD cmake -Bbuild ~/ncs/zephyr/share/sysbuild
and let the --preset
argument be added without hassle.
Not blocking this PR, but that would be another benefit to this approach.
This PR is a lot cleaner, and I'd prefer to do it this way than #95047, however it doesn't solve the relative path translation issue: the application's presets file is full of such variables (e.g. |
@57300 That was a comment to #95047.
because #95047 relies on But PR#96609 supports both
Yes, I did consider that as well, but wanted some feedback from @benedekkupper first regarding the alternative approach before doing wider changes. |
I see, I didn't realize that "this PR" was referring to #95047. I'm sorry for misreading your original comment.
Good to know. I guess this is essentially a work in progress and I jumped in too early. |
I could make this patch work in my environment, by relying on |
@benedekkupper Thanks for testing it out and provide feedback. Will cleanup the PR and add some doc notes on regarding this. |
d8f180a
to
5f5e1a6
Compare
@57300 and @benedekkupper ptal, it's fully ready now 🙂 |
doc/build/sysbuild/index.rst
Outdated
|
||
.. group-tab:: ``west build`` | ||
|
||
Here is an example where preser ``release`` should be used. |
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.
typo "preser" -> preset
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.
Fixed
doc/build/sysbuild/index.rst
Outdated
|
||
.. note:: | ||
|
||
Because sysbuild consumes and process ``CMakePresets.json`` then preset macros referring to |
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.
I would rephrase this a bit, e.g.: As sysbuild changes the top-level cmake project to its own directory, the cmake presets are parsed from there, the application's presets are included from this file verbatim. Therefore relative paths, and macros resolving relative to the source directory will not work as expected, but as relative to share/sysbuild
. The ${fileDir}
macro can be used to create portable paths relative to the application's directory.
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 the suggestion.
Applied.
This commit provides a CMakePresets.json which includes the sample's CMakePresets.json file. `west build` is extended to set `APP_DIR` in environment when sysbuild is used. This allows sysbuild's CMakePresets.json to include the sample's presets file. Signed-off-by: Torsten Rasmussen <[email protected]>
5f5e1a6
to
522eb86
Compare
|
LGTM :) |
This commit provides a CMakePresets.json which includes the sample's CMakePresets.json file.
west build
is extended to setAPP_DIR
in environment when sysbuild is used. This allows sysbuild's CMakePresets.json to include the sample's presets file.Alternative to: #95047