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.

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

west: sign.py: add "REM" support to pass comments through cpp #67019

Merged
merged 3 commits into from
Jan 8, 2024

Conversation

marc-hb
Copy link
Collaborator

@marc-hb marc-hb commented Dec 26, 2023

3 sign.py commits. Main commit:

west: sign.py: add "REM" support to pass comments through cpp

Generated outputs can be difficult to read, preserving comments helps a
lot and they often provide good git grep search keywords.

marc-hb added a commit to marc-hb/sof that referenced this pull request Dec 26, 2023
Finding original files is not convenient.

Use unnecessary, double 'REM #' for better syntax highlighting.

This depends on zephyrproject-rtos/zephyr#67019

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
@marc-hb
Copy link
Collaborator Author

marc-hb commented Dec 27, 2023

Successfully tested in commit https://github.com/marc-hb/sof/commits/toml-REM/

andyross
andyross previously approved these changes Dec 29, 2023
Copy link
Contributor

@andyross andyross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No complaints from me, seems like a useful feature. I will say though that this smells a little like a workaround for a design flaw: SOF, heh, sort of has a habit[1] of picking weak languages, trying to do complicated things with them, and then having to bolt on some kind of preprocessing to make up for the lack of programmability in the original.

So we have (here) rimage writing its config file in TOML, but then having to suck in non-trivial macrobatics to do what needs to be done, which requires debugging, which requires comments that survive the translation.

Maybe it might be time to think about generating the rimage config in a first class programming language (which you can simplify with a DSL layer as needed) rather than trying to build up toward a first class language with simpler tools?

[1] The true case study here was the attempt to bolt M4 around ALSA config files to generate topologies. The tplg2 stuff walked that back, but now that too is being passed through cpp!

@marc-hb
Copy link
Collaborator Author

marc-hb commented Dec 29, 2023

Thanks for the review!

but then having to suck in non-trivial macrobatics to do what needs to be done, which requires debugging, which requires comments that survive the translation.

"requires" sounds too strong; this PR is a "quality of life" enhancement.

sort of has a habit[1] of picking weak languages,

Thanks for preaching to the choir (again) :-)

In theory, practice and theory are the same. In practice, people choose the languages they know and (think they) are the most productive with.

To be fair, this cpp-based solution seems very convenient to access config.h and similar. Zephyr is still 100% written in an ancient and weak language after all and it's not short of macrobatics either! "In Rome, do as the Romans do"

https://www.youtube.com/watch?v=w8GgP3h0M8M

Screenshot 2023-12-29 at 14 47 27

The tplg2 stuff walked that back, but now that too is being passed through cpp!

I'm not familiar with topologies, do you have a link?

@marc-hb marc-hb added area: Audio platform: Intel ADSP Intel Audio platforms manifest-rimage Enhancement Changes/Updates/Additions to existing features area: Toolchains Toolchains labels Jan 2, 2024
kv2019i
kv2019i previously approved these changes Jan 3, 2024
scripts/west_commands/sign.py Outdated Show resolved Hide resolved
@nashif
Copy link
Member

nashif commented Jan 3, 2024

Each platform has and always had its own build-platform/ build
directory. Keep all such build directories consistent with each other to
simplify installation, checksumming and post-processing. We don't need
any build-mtl/**/mtl.toml path to tell us the platform name twice,
once is enough.

"Fixes" recent commit marc-hb@1533604 ("west: sign.py: generate platf.toml
from platf.toml.h with cc -E")

That commit message is confusing because it talks about how SOF builds things and not upstream Zephyr.

Copy link
Collaborator Author

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That commit message is confusing because it talks about how SOF builds things and not upstream Zephyr.

Thanks, I'll rephrase to be more neutral. Note the .toml pre-processing seems quite SOF-specific for now though.

scripts/west_commands/sign.py Outdated Show resolved Hide resolved
@marc-hb marc-hb marked this pull request as draft January 3, 2024 16:38
CMake-based build systems like Zephyr's use separate build directories;
one for each build configuration. Even Zephyr's multi-build system
"sysbuild" (which is not relevant here) uses separate subdirectories.

So there is only one pre-processed, .toml file generated by build
directory and no need to vary its filename based on the platform name or
any other configuration parameter. It can and should keep the same
filename across build directories as zephyr.elf and all other build
artefacts do.

Moreover, when building a collection of configurations (as for instance
`sof/scripts/xtensa-build-zephyr.py` does), keeping all build
directories consistent with each other simplifies installation,
checksumming and any other post-processing.

"Fixes" recent commit 1533604 ("west: sign.py: generate platf.toml
from platf.toml.h with cc -E")

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
Zero-functional change.

Also move it to a separate line so it's more convenient to temporarily
comment it out.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
Generated outputs can be difficult to read, preserving comments helps a
lot and they often provide good `git grep` search keywords.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
@marc-hb marc-hb marked this pull request as ready for review January 4, 2024 05:23
@marc-hb
Copy link
Collaborator Author

marc-hb commented Jan 4, 2024

v2; very small changes:

  • Commit message fix (@nashif)
  • Replace REM words by nothing which makes REM # mandatory (@lyakh)

Approvals lost please review.

Copy link
Contributor

@andyross andyross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refresh +1

@nashif nashif merged commit 4aa0e7a into zephyrproject-rtos:main Jan 8, 2024
32 checks passed
@marc-hb marc-hb deleted the sign-cpp-REM branch January 8, 2024 18:24
marc-hb added a commit to marc-hb/sof that referenced this pull request Mar 15, 2024
Having to find source files to read comments is not convenient.

This depends on zephyrproject-rtos/zephyr#67019
which was merged a while ago.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
kv2019i pushed a commit to thesofproject/sof that referenced this pull request Apr 10, 2024
Having to find source files to read comments is not convenient.

This depends on zephyrproject-rtos/zephyr#67019
which was merged a while ago.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
eddy1021 pushed a commit to eddy1021/sof that referenced this pull request Jul 15, 2024
Having to find source files to read comments is not convenient.

This depends on zephyrproject-rtos/zephyr#67019
which was merged a while ago.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Audio area: Toolchains Toolchains area: West West utility Enhancement Changes/Updates/Additions to existing features manifest-rimage platform: Intel ADSP Intel Audio platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants