Skip to content
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.

Sign up for GitHub

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

Jump to bottom

rimage: pre-process .toml files with cc -E #66420

Merged
merged 5 commits into from
Dec 15, 2023
Merged

rimage: pre-process .toml files with cc -E #66420

merged 5 commits into from
Dec 15, 2023

Conversation

marc-hb
Copy link
Collaborator

@marc-hb marc-hb commented Dec 12, 2023 •
edited
Loading

5 commits, main ones:

@marc-hb
intel_adsp: remove rimage sign() from west flash
0b1acd4 
`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 <marc.herbert@intel.com>
@marc-hb
west: sign.py: extract new method RimageSigner.rimage_config_dir()
5f855af 
Zero functional change, preparation for the .toml modularization.

RimageSigner.sign() is also way too long and too complex and this helps.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
@marc-hb
west: sign.py: recommend separator -- tool_args always
0781927 
In the following command, the first argument `for_rimage` is passed to
`rimage` whereas `--for west` goes to west.

```
west sign  for_rimage --for west
```

This is somehow valid but we really don't want anyone to do that.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
@marc-hb
west: sign.py: always log the rimage command
2dfbdbc 
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 <marc.herbert@intel.com>
@marc-hb
west: sign.py: generate platf.toml from platf.toml.h with cc -E
0b8848a 
Allow using the C pre-processor to generate a
`rimage/config/platform.toml` file from a "source"
`rimage/config/platform.toml.h` file.

This is optional and fully backwards compatible.

To use, do not use `-c` and point west sign at a configuration directory
instead or let it use the default `rimage/config/` directory and change
the files there.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
@marc-hb marc-hb marked this pull request as ready for review December 12, 2023 08:40
@zephyrbot zephyrbot added area: West West utility platform: Intel ADSP Intel Audio platforms labels Dec 12, 2023
@marc-hb marc-hb mentioned this pull request Dec 12, 2023
Closed
@marc-hb marc-hb changed the title rimage: pre-process .toml files with gcc -E rimage: pre-process .toml files with cc -E Dec 12, 2023
@marc-hb marc-hb mentioned this pull request Dec 12, 2023
Merged
@lyakh
Copy link
Collaborator

lyakh commented Dec 14, 2023

Mostly works. Tested it with thesofproject/sof#8490 (only modified for a recent renaming) and west sign worked without the -D option, but with it and with a relative path to the configuration files, it couldn't find the file:

cc1: fatal error: modules/audio/sof/tools/rimage/config/tgl.toml.h: No such file or directory

@marc-hb
Copy link
Collaborator Author

marc-hb commented Dec 14, 2023

Thanks @lyakh for the testing!

the -D option, but with it and with a relative path to the configuration files, it couldn't find the file:

I admit I did not test the -D option, I will do today. Relative paths can be trickier, can you please confirm that -D works with an absolute path? Also, does a relative path work BEFORE this PR?

If the answers are respectively "yes" and "no" then it is a separate issue that should not hold back this entire PR.

@marc-hb
Copy link
Collaborator Author

marc-hb commented Dec 14, 2023 •
edited
Loading

I admit I did not test the -D option, I will do today.

So that went much faster than expected. I tried various relative west sign -D parameters run from various current directories and they all worked for me. Please provide detailed reproduction steps.

@marc-hb
Copy link
Collaborator Author

marc-hb commented Dec 15, 2023 •
edited
Loading

cc1: fatal error: modules/audio/sof/tools/rimage/config/tgl.toml.h: No such file or directory

Depending on how old and "hybrid" is your west workspace, you have may two copies of the sof repo:

workspace/modules/
└── audio
    └── sof
workspace/sof

Make sure you get rid of the one that is not in west list | grep sof

Also, make sure you have no symbolic link.

@carlescufi carlescufi merged commit 1533604 into zephyrproject-rtos:main Dec 15, 2023
42 checks passed
@marc-hb marc-hb deleted the sign-cpp branch December 15, 2023 16:23
@marc-hb marc-hb mentioned this pull request Dec 18, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbolivar-ampere mbolivar-ampere mbolivar-ampere approved these changes

@carlescufi carlescufi carlescufi approved these changes

@aborisovich aborisovich Awaiting requested review from aborisovich

@andyross andyross Awaiting requested review from andyross

@ceolin ceolin Awaiting requested review from ceolin

@dcpleung dcpleung Awaiting requested review from dcpleung

@jxstelter jxstelter Awaiting requested review from jxstelter

@kv2019i kv2019i Awaiting requested review from kv2019i

@lgirdwood lgirdwood Awaiting requested review from lgirdwood

@lyakh lyakh Awaiting requested review from lyakh

@marcinszkudlinski marcinszkudlinski Awaiting requested review from marcinszkudlinski

@nashif nashif Awaiting requested review from nashif

@softwarecki softwarecki Awaiting requested review from softwarecki

@swinslow swinslow Awaiting requested review from swinslow

@tmleman tmleman Awaiting requested review from tmleman

Assignees

@mbolivar-ampere mbolivar-ampere

Labels
area: West West utility platform: Intel ADSP Intel Audio platforms
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@marc-hb @lyakh @carlescufi @mbolivar-ampere @zephyrbot