-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
Snippets #53640
Snippets #53640
Conversation
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.
If someone configures a project using snippets, then re-configures it without (i.e. not a pristine build), would you expect the snippets to still be used or would they not be used? At present they persist
doc/build/snippets/design.rst
Outdated
- **not DRY**: for example, shields require identical board-specific | ||
devicetree overlays to be specified via multiple identical files on | ||
the file system |
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.
This is not right, shields does not require identical board-specific devicetree overlays, but one can customize it for a specific board, see Board compatibility and Board specific shield configuration.
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.
see Board compatibility and Board specific shield configuration.
This comment is referring specifically to the .overlay files documented in "Board specific shield configuration". If a DK and a customer board need the same .overlay, it must be duplicated on the file system. What am I missing?
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.
If you mean overlay like boards/shields/baz_shield/boards/my_board.overlay
, these kind of overlays are not required and are usually there to resolve conflicts if any, for example https://github.com/zephyrproject-rtos/zephyr/blob/main/boards/shields/buydisplay_2_8_tft_touch_arduino/boards/nrf52840dk_nrf52840.overlay
doc/build/snippets/design.rst
Outdated
List of Design Goals | ||
******************** | ||
|
||
As design goals, snippets are: |
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.
Can/may a snippet contain piece of code that is built?
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 wonder the same thing. If it's useful, I don't see why not. The fun part about having a YAML format is that we can put whatever we want into it. Do you have a use case?
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.
For CDC ACM snippets we could have a code-snippet which configures and enables USB device support (new device support requires more runtime confirmation). But there are also situations where only the configuration part (Kconfig + DTS) of snippet is needed without code, for example
thingy53 + CDC ACM UART snippet for logging backend and USB initialization + hello_world sampe
vs.
thingy53 + CDC ACM UART snippet for logging backend + any samples/subsys/usb
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.
For CDC ACM snippets we could have a code-snippet which configures and enables USB device support (new device support requires more runtime confirmation)
I think we should leave this to a future extension once the basics are in place. The snippet.yml file format is designed for future extension.
I would expect them to persist. This is why changing SNIPPETS requires a pristine build in the current implementation. Are you hinting that there is a problem with that, or just checking for clarification? Thanks for catching the -s issue -- fixed. |
282ce11
to
f66c3cf
Compare
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.
Open idea.
If mixins / snippets is to become a more integral part of Zephyr and where shields may be considered a special case of a mixin, then should we consider to create it a higher class citizen by allowing to specify a mixin directly as part of the board ?
For example using %
as a mixin separator, so that a board like bl654_usb
to specify that a given mixin is always required, so that -DBOARD=bl654_usb
would fail, but -DBOARD=bl654_usb%cdc_acm
would be accepted, as the bl654_usb requires the cdc_acm mixin to work correctly in this case.
Partly inspired by #40645.
Such a scheme could also allow us to target ns
board variants in future, as those are not really boards but more a setup / configuration similar to what a snippet might do.
3bbca53
to
50e7401
Compare
@tejlmand (tejlmandy?)
I'm not sure exactly what you mean, but that overall sounds interesting.
This does not sound good to me. I want to keep this feature dead simple to use, and adding more line noise to the way we specify a board isn't good UX in my view. However, since board.cmake is processed before snippets.cmake, I think a board directory can already add anything it wants to the SNIPPET_ROOT and SNIPPET variables in the current implementation to require any snippets that it desires. |
Writing '-S foo' is a convenience shorthand for adding snippets at CMake time without having to add '-- -DSNIPPET=foo' to the west build command line. It is worth adding a new one-letter command line option because snippets are anticipated to be a very commonly used feature. Signed-off-by: Marti Bolivar <marti.bolivar@nordicsemi.no>
The linter is complaining about unnecessary parens. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
No, that's a bug actually. The first warning shouldn't be there since we're automatically adding the application directory to SNIPPET_ROOT, and that often won't have snippet directories inside. So the whole warning should be removed. (The warning also has a bug in the format string.) Thanks for catching that. |
Document this new build system feature. Since its purpose is customizing application builds, the logical place for the main body of documentation is in a new snippets/ directory in doc/build/. Create that directory and add its initial documentation. Like boards and samples, however, we expect people to write documentation for each snippet within the directory that defines the snippet itself. Therefore, add a new top-level snippets/ directory and stub out the documentation needed to document individual snippets as well. Add documentation and cross-references in other required places as well. Signed-off-by: Marti Bolivar <marti.bolivar@nordicsemi.no>
This snippet is based on samples/subsys/usb/console. Since it's a snippet, it can be used in any application as long as its requirements are met. This makes it more general purpose. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
Add an initial test suite for the basic SNIPPET_ROOT, SNIPPET, and 'append' features in snippet.yml. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
e701fcb
to
fb7477a
Compare
Correct. |
I really like the feature, I think in the end there will be some nifty patterns people come up with to make good use of it. I can think of at least once or twice I'd have loved this already. |
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.
This PR is a no-go for me. One of the concerns is wrong naming that simply do not resemble what this feature does.
On the other hand, this PR also lacks feature of "snippets" mutual inclusion or exclusion.
See my comment in the #51834 (comment) .
The review part:
I do not understand the design completely yet, but few parts in docs look to me like a completely bad implementation. Like re-running cmake.
Can the author maybe create some design in the #51834 ?
# Putting the body into a function prevents us from polluting the | ||
# parent scope. We'll set our outcome variables in the parent scope of | ||
# the function to ensure the outcome of the module. |
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'm against comments that explain how cmake build system works. Developers working with cmake should know what is a function scope and understand its uses. Please replace with a description on what actually this function does (what is snipped processing).
name: cdc-acm-console | ||
append: | ||
OVERLAY_CONFIG: cdc-acm-console.conf | ||
DTC_OVERLAY_FILE: cdc-acm-console.overlay |
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.
No possibility to append (include) other snippet files (or to exclude them), what will lead to combinatory explosions just like with CMakePresets.
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 don't personally see the merit in excluding files, if you want a different set of files, create a new snippet. Likewise in a project with build files, if you want to exclude files, create a new project or create a separate configuration directory and set APP_CONFIGURATION_DIR to point to those files instead of hacking around cmake files trying to alter zephyr variables.
# Set SNIPPET_AS_LIST, removing snippets_generated.cmake if we are | ||
# running cmake again and snippets are no longer requested. | ||
if (NOT DEFINED SNIPPET) | ||
set(SNIPPET_AS_LIST "" PARENT_SCOPE) | ||
file(REMOVE ${snippets_generated}) | ||
else() | ||
string(REPLACE " " ";" SNIPPET_AS_LIST "${SNIPPET}") | ||
set(SNIPPET_AS_LIST "${SNIPPET_AS_LIST}" PARENT_SCOPE) | ||
endif() |
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.
This looks wrong. "SNIPPET_AS_LIST" variable looks wrong as variables and lists are "implicitly castable".
In Cmake variables created using set command and multiple values eg. set(MY_VAR VAL1 VAL2)
by default create a list. Selective quote from docs:
Set(...)
Signatures of this command that specify a ... placeholder expect zero or more arguments. Multiple arguments will be joined as a semicolon-separated list to form the actual variable value to be set.
Just delete this code part and use SNIPPET
variable where you used SNIPPET_AS_LIST
.
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.
This uses SNIPPET for a local function-scoped variable and SNIPPET_AS_LIST in the parent scope, the 2 variables have completely different visibilities
# Set SNIPPET_ROOT. | ||
list(APPEND SNIPPET_ROOT ${APPLICATION_SOURCE_DIR}) | ||
list(APPEND SNIPPET_ROOT ${ZEPHYR_BASE}) | ||
unset(real_snippet_root) |
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.
This is not needed as you create a new list(APPEND real_snippet_root ...) variable in the function scope.
From cmake list introduction, partial docs quote:
Similar to the set() command, the LIST command creates new variable values in the current scope, even if the list itself is actually defined in a parent scope. To propagate the results of these operations upwards, use set() with PARENT_SCOPE, set() with CACHE INTERNAL, or some other means of value propagation.
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.
It creates a new list, that means, it takes the existing one from the parent and duplicates it locally in the function scope, it does not mean it creates a new empty list
multiple times. Forces CMake to run again if given. | ||
Do not use this option with manually specified | ||
-DSNIPPET... cmake arguments: the results are | ||
undefined''') |
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.
This is very bad implementation. I'm still in the process of understanding this whole PR, but by this comment I can say straight ahead that this is wrong. Re-running cmake? You mean cmake-configure step or build step? Can't it be done better to run it only once like it should? I'll try to propose better alternative but I still need more time to uderstand this PR content fully.
And this part:
Do not use this option with manually specified -DSNIPPET... cmake arguments
This is really bad, espacially that adding -D flags is part of west build
public interface to use! (-- [cmake_opt] flag). And now you specify some "do not use" rules. What if I have snippets too in my zephyr application that are something utterly different?
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.
cmake to run means configuring, ninja or make would be the build. It would be invalid to not re-run cmake if the shield variable has changed - how would that be propagated to the application?
Then use -S multiple times? -DSNIPPET=... is fine when configuring using cmake, when configuring use west, different rules apply
Ahh, 8 minutes late with the review... |
@aborisovic as it seems that you've misunderstood some of the implementation details (see @nordicjm 's responses to your comments), I will leave it to you to file bugs if there are any remaining issues from your side. |
see: - https://docs.zephyrproject.org/latest/develop/modules.html#build-settings - zephyrproject-rtos/zephyr#53640 issues: #106 Signed-off-by: Stephan Linz <linz@li-pro.net>
see: - https://docs.zephyrproject.org/latest/build/snippets/index.html#snippets - zephyrproject-rtos/zephyr#53640 issues: #106 Signed-off-by: Stephan Linz <linz@li-pro.net>
The point of this PR is to begin implementing the snippets feature discussed in #51834.
Please refer to the documentation patch for a full description of the current feature set. The snippet.yml file format defined here is extensible, so this PR is not trying to boil the ocean by implementing all planned snippet.yml features. The goal is to establish a minimum viable implementation which can be reviewed and merged now. Future design and implementation can then proceed in parallel, since multiple people are going to want to be adding features.
In summary, this PR:
west build
for using snippetscdc-acm-console
snippet, with documentation, as a proof of concept