-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
rimage: add TOML preprocessing #65411
Conversation
4'. Generated files must be in the build directory. [*] I'm sorry because who wants to write CMake code. |
@marc-hb as you can see in thesofproject/sof#8490 it isn't just about combining files. We had a simple PoC earlier for that that just concatenated files. We need preprocessing because (1) we want conditional module inclusion, based on Kconfig options, (2) we want to define new and to re-use existing macros from header files, (3) we want to remove the hard-coded module counter and to count modules dynamically instead.
See above - we're programming in a language from the 70s. I don't see why adding more tooling instead of continuing to use what we already use is better.
Well, rimage is called directly from .py. If rimage were called from cmake, we could add preprocessing there too. AFAICS
Correct, that's where we put the result of toml preprocessing.
|
TOML configuration files for rimage are composed of multiple logical blocks, and those blocks are repeated between multiple configuration files. We use the C preprocessor to merge those blocks, to substitude any parameter values and to perform condition checking. Signed-off-by: Guennadi Liakhovetski <[email protected]>
That additional layer of indirection/abstraction is unfortunate but it serves a specific purpose: rimage is not the only signing tool.
I hope not but
All these look like very simple jobs for Python or any vaguely modern and popular scripting language. Our Kconfig implementation is entirely in Python, as are
Python is not from the 70s, neither is CMake and TOML is 10 years old.
This PR is adding new tooling; it's adding the C preprocessor in a place where it wasn't at all. Invoking the C preprocessor directly is very rare, using it to preprocess anything else than C/C++ or assembly is even more rare. This PR is an unusual and strange experiment. I suspect it's down to "let's use the only tool we know" |
@marc-hb have I answered your questions? Also see thesofproject/sof#8490 for SOF side motivation |
@marc-hb SOF has been using TOML for the last 4 years and there are no plans to change. The pre-processor is the right tool today for this use case as all the SOF developers understand pre-processing directives to the level required. There is a tight coupling between the SOF C code/Kconfig contents and the toml data at build time (i.e. we need to pre-process both with the same rules and macros for all C/toml files to make sure everything is aligned). |
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.
Most of the code looks ok but there are some big UI and backwards compatibility issues with the cmake_toml = target + '.toml.h'
logic.
First, this introduces a "flag day" where both SOF and Zephyr have to be changed at the same time. It will block git bisect and similar things. That's not necessary.
The new, hard dependency on .toml.h
will also make it harder to test and debug pure sign.py
issues in isolation.
Most importantly, this PR missed the fact that the -c
option can come from multiple places: see lines 559. So this PR demands .toml.h
and pre-processing when -c comes from some place but demands the older and plain .toml
when -c comes from other places. That inconsistency is not OK
All these issues are hopefully easy to address:
- move the logic later, AFTER all
-c
origin have been considered. - something like:
if exists(.toml.h):
pre-process
use build/zephyr/.toml
else if exists(.toml)
use src/.toml
else
raise exception(missing .toml)
subprocess.check_call(preproc_cmd) | ||
conf_path_cmd = ['-c', output_toml] | ||
else: | ||
conf_path_cmd = [] |
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.
Move this at the top to avoid the "lost" and barely visible else
clause.
conf_path_cmd = []
if conf_dir:
...
conf_path_cmd = ['-c', output_toml]
After more thought, it may not be that easy. BTW this PR seems currently incompatible with |
Right now the combination of these two PRs builds neither TGL nor MTL.
TL;DR: rimage configuration is just quite complicated already even before this split. There are some options, I'm looking at them and experimenting. |
`west sign` has been invoked by `west build` (through CMake) since commit fad2da3, almost one year ago. During that time, this new workflow has been refined and successfully used by at least two vendors, multiple CIs across both SOF and Zephyr and many developers. At the time, the ability to sign from `west flash` was preserved for backwards compatibility. This means rimage parameters can come from many different places at once and that rimage can be invoked twice during a single `west flash` invocation! Now that Zephyr 3.5 has been released, we need to reduce the number of rimage use cases and the corresponding validation complexity and maintenance workload to simplify and accelerate new features like splitting rimage configuration files (zephyrproject-rtos#65411) Signed-off-by: Marc Herbert <[email protected]>
rimage is very verbose by default and has no -q(uiet) option, so saving one line out of more than 100 lines is pointless. RimageSigner.sign() was already very complex and suffering from combinatorial explosion of parameters. With .toml pre-processing (zephyrproject-rtos#65411) it's getting worse, so we really need all build logs to show the complete rimage command. Signed-off-by: Marc Herbert <[email protected]>
Candidate replacement:
Both are required for this to work but they can be merged in any order. |
`west sign` has been invoked by `west build` (through CMake) since commit fad2da3, almost one year ago. During that time, this new workflow has been refined and successfully used by at least two vendors, multiple CIs across both SOF and Zephyr and many developers. At the time, the ability to sign from `west flash` was preserved for backwards compatibility. This means rimage parameters can come from many different places at once and that rimage can be invoked twice during a single `west flash` invocation! Now that Zephyr 3.5 has been released, we need to reduce the number of rimage use cases and the corresponding validation complexity and maintenance workload to simplify and accelerate new features like splitting rimage configuration files (#65411) Signed-off-by: Marc Herbert <[email protected]>
rimage is very verbose by default and has no -q(uiet) option, so saving one line out of more than 100 lines is pointless. RimageSigner.sign() was already very complex and suffering from combinatorial explosion of parameters. With .toml pre-processing (#65411) it's getting worse, so we really need all build logs to show the complete rimage command. Signed-off-by: Marc Herbert <[email protected]>
@lyakh close? |
The `--rimage-tool=...` parameter was added in March 2022 by the very first map.yml commit (rebased f6d6f1e15022) But `west sign` has been invoked by `west build` (through CMake) since commit fad2da3, almost one year ago. At the time, the ability to sign from west flash was preserved for backwards compatibility. Today, `west flash` does not invoke `west sign` any more which means this `--rimage-tool=...` parameter is now ignored. The CI for more recent, ACE platforms (MTL, LNL, etc.) does not pass any `--rimage-tool=...` at all, see evidence in CI runs of zephyrproject-rtos#860 Removing `--rimage-tool=...` from `.github/data/map.yml` will allow blocking that option in `west flash` (zephyrproject-rtos#860) Now that Zephyr 3.5 has been released, we need to reduce the number of rimage use cases and the corresponding validation complexity and maintenance workload to simplify and accelerate new features like splitting rimage configuration files (zephyrproject-rtos#65411) Signed-off-by: Marc Herbert <[email protected]>
TOML configuration files for rimage are composed of multiple logical blocks, and those blocks are repeated between multiple configuration files. We use the C preprocessor to merge those blocks, to substitude any parameter values and to perform condition checking.
Note, my Python is mostly copy-pasting, so please advise any improvements.