cmake: prototyping support for CMake presets json file. #3979
Conversation
tejlmand
requested review from
wbober,
trond-snekvik,
carlescufi and
mbolivar-nordic
|
The following west manifest projects have been modified in this Pull Request:
Note: This message is automatically posted and updated by the Manifest GitHub Action. |
|
Running twister with Running twister with |
This commit introduces a CMakePresets.json in asset tracker.
CMake presets allows for additional sample and build description in a
single file.
The CMake presets also allows vendor specific fields, and thus allows
it to contain information that normally lives in sample.yaml or
testcase.yaml.
It is also possible to introduce IDE specific data to improve user
experience.
The sample can still be build with existing commands, but the preset
file also allows for building of the application as:
`cmake -DBOARD=nrf9160dk_nrf9160ns --preset=default .`
or
`cmake -DBOARD=nrf9160dk_nrf9160ns --preset=test.`
instead of:
`cmake -DBOARD=nrf9160dk_nrf9160ns -GNinja -Bbuild .`
or
`cmake -DBOARD=nrf9160dk_nrf9160ns -GNinja -Bbuild
-DCONF_FILE=prj_test.conf .`
Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no>
tejlmand
force-pushed
the
prototyping/cmake_presets
branch
from
3ff7090 to
546462a
Compare
|
You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds. Note: This comment is automatically posted by the Documentation Publishing GitHub Action. |
|
Can these presets also be used with or does west break the preset mechanism? |
West is just a wrapper; you should be able to pass any CMake arguments you like, including |
West adds generator and directory flags unconditionally, so it's not a pure wrap. I suppose what I'm really asking is, does the preset override this, and should it? |
Not true: as expected, Ninja is used. Now, let's tell CMake (and and because So user can still overrule what |
Specifying NCS Toolchain, v1.5.0 in preset. Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no>
|
With the new Integration Test Specification process we don't need the github labels. Right now this PR runs all Integration tests because it changes sdk-zephyr in west.yml and there is no test-spec.yml in that repo yet. To get the test spec feature working correctly on this PR, you need to rebase it to the latest main to get the .github/test-spec.yml file Then run CI here to update the test spec. |
Do you have some specific examples or were you just guessing? cc: @aborisovich |
Source directory, build directory, generator, The issue is that the preset doesn't stay a fully reproducible recipe for the build when you pass it to west:
If we want preset support for |
Presets main goal is not to ensure reproducible builds but easier sharing of settings.
Ref: https://cmake.org/cmake/help/latest/manual/cmake-presets.7.html Even when using presets then user can still pass whatever setting they want in addition the the preset file by hand.
Presets doesn't give you that, even if you specify this in a preset file then user can overrule while invoking CMake directly Ref:
What you can say is that
Just because you have a very special use-case with Bluetooth Audio projects that doesn't mean What you describe are not my main concerns with presets. |
While I never tried to read the mind of the CMake designers, I can see how having fewer settings on the command line and in environment variables and more settings in git instead can HELP achieving reproducibility.
Totally agreed except I would replace "trustworthy" with "silver bullet". I do not expect CMake presets (or anything else) to provide any reproducibility "silver bullet". However it looks like CMake presets can HELP. Achieving build reproducibility is exactly like fixing other bugs: there are plenty of tools to help but the moment you stop trying then the game is over. Using different command line options or environment variables and expecting build reproducibility is like ignoring all compiler warnings and expecting bug-free software. No CMake preset or any other "silver bullet" will ever fix that but there are tools and practices that help a lot |
Dunno about this specific parameter but generally speaking precedence should always be intuitive and more importantly well documented.
This looks like a (future) bug. User input should never be silently ignored - except of course by other user input with a higher precedence. If these specific values cannot be defined in presets then attempting to define them in presets should simply fail the build until they are removed from presets. |
There was a problem hiding this comment.
In my impression, Cmake presents are competitive to west build command.
I mean west was created to be a command line wrapper to build a concrete thing ("board" - which is a set of configurations) conveniently.
CMake presets provide similar functionality. So here we are in the awkward situation where the West tool loses some of its usability (this is my personal subjective opinion). I think it is always better to use tools more native to the build system.
So I am all for this change @tejlmand . I think it might need changes to west just to make sure we do not scrumble the settings.
I mean some addition to west that would just purely pass by everything what a preset does. In other words, I agree with @trond-snekvik said:
If we want preset support for west build, it has to become a proper flag for that west command, so these overrides can be stripped from the cmake invocation. This isn't some massive blocker, but I'm still of the opinion that west build doesn't cleanly wrap cmake in a way that's compatible with cmake presets and its primary use case (shareable and reproducible builds).
We certainly should add this as a separate west flag and make sure nothing is broken in the process (as @tejlmand mentioned):
If a preset is given, then defaults in there should be preferred over defaults in west, but even such would not guarantee you a specific build folder name if the user manually specifies the build dir.
To sum up: in my opinion, we should add a new flag west --preset, and validate in the process that all preset defaults are not overridden by west python code.
| "version": 1, | ||
| "cmakeMinimumRequired": { | ||
| "major": 3, | ||
| "minor": 19, | ||
| "patch": 0 |
There was a problem hiding this comment.
What I can advise from my experience with presets is to use possibly the newest Preset manifest version.
Looks like presets for CMake configure, build and test steps were added as separate steps in CMake v3.20.0 (manifest ver 2.0). Features grow very dynamically and we will need them to group configurations using inheritance successfully. Zephyr minimal Cmake is 3.20.0 as far as I remember?
Completely agree, I was not in anyway trying to give impression that it cannot help.
Completely agree. |
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
This commit introduces a CMakePresets.json in asset tracker.
CMake presets allows for additional sample and build description in a
single file.
The CMake presets also allows vendor specific fields, and thus allows
it to contain information that normally lives in sample.yaml or
testcase.yaml.
It is also possible to introduce IDE specific data to improve user
experience.
The sample can still be build with existing commands, but the preset
file also allows for building of the application as:
cmake -DBOARD=nrf9160dk_nrf9160ns --preset=default .or
cmake -DBOARD=nrf9160dk_nrf9160ns --preset=test.instead of:
cmake -DBOARD=nrf9160dk_nrf9160ns -GNinja -Bbuild .or
cmake -DBOARD=nrf9160dk_nrf9160ns -GNinja -Bbuild -DCONF_FILE=prj_test.conf .Signed-off-by: Torsten Rasmussen Torsten.Rasmussen@nordicsemi.no