xtensa-build-zephyr.py: pass SOF firmware file version string to rimage by mengdonglin · Pull Request #5846 · thesofproject/sof

mengdonglin · 2022-05-23T10:10:19Z

This is to fix #5775. It depends on rimage PR thesofproject/rimage#95

Extract SOF firmware file version string (x.y or x.y.z) from latst git tag
by 'git describe --tags' and python regex, and pass it to rimage in
signing phase. So rimage can write the version info to the firmware
manifest headers for cAVS platforms.

Signed-off-by: mengdonglin mengdong.lin@intel.com

aiChaoSONG · 2022-05-23T10:23:25Z

This is really an independent feature, let's make it a function, for example, extract_sof_version_from_git() or simplely extract_sof_version, let's hear more from @marc-hb

@aiChaoSONG Okay. Thank you! I updated the PR to define a function get_sof_version() to get SOF FW file version string.

marc-hb · 2022-05-23T19:23:07Z

Please don't copy/paste/diverge the code already in scripts/cmake/version.cmake. It's more complex than this and already different from this. Also, git describe can take time.

Instead parse the output of scripts/cmake/version.cmake like this (tested):

versions = {} with open("zephyrproject/build-$platform/zephyr/include/generated/sof_versions.h") as hfile: for hline in hfile: words = hline.split() if words[0] == '#define': versions[words[1]] = words[2] print(versions) {'SOF_MAJOR': '3', 'SOF_MINOR': '2', 'SOF_MICRO': '1', 'SOF_TAG': ...

@marc-hb I updated the PR to get version from sof_version.h as you suggested, and we can always pass a 3-field version string to rimage.
@aiChaoSONG @lgirdwood We no longer need the rimage PR thesofproject/rimage#95 so I closed it.

marc-hb · 2022-05-26T16:49:55Z

Can you move this test inside get_sof_version() ? Making the cache a private implementation detail.

def get_sof_version(abs_build_dir): if sof_version: return sof_version with open(... sof_version = ... return sof_version

Okay, moving this test inside get_sof_version() is better. Thanks for your suggestions @marc-hb I updated the PR to address all your review comments.

marc-hb · 2022-05-26T16:51:07Z

Suggested change

sof_version = "0.0.0"

sof_version = None

Updated. Set sof_version is initialized to None now.

marc-hb · 2022-05-26T16:51:20Z

Suggested change

When builing multiple platforms from the same SOF commit, all platforms share the same version.

When building multiple platforms from the same SOF commit, all platforms share the same version.

Function description has been updated and typo is fixed

aiChaoSONG · 2022-05-27T03:21:18Z

The feature can be done by:

use a global sof_version, and modify it with get_sof_version. in signing, use the global one directly.

don't use a global sof_version, and in signing, use the one returned from get_sof_version, (function runs for each platform)

Seems you mixed use above two in the code. since the sof_version is already global, the return statement in get_sof_version is redundant. you can modify and use the global sof_version directly.

I agree with @aiChaoSONG that there is a problem now: the sof_version global is set twice, BOTH inside and outside the function. That's not good.

However I prefer the other option: keep all read/write accesses to the global sof_version and its "maintenance" private and isolated inside the function.

Oh wait: the second sof_version in build_platforms() is local, shadowing the global. That's not good either; shadowing == bad.

pylint scripts/xtensa-build-zephyr.py| sort -t: -k2,3 -n scripts/xtensa-build-zephyr.py:472:2: W0621: Redefining name 'sof_version' from outer scope (line 27) (redefined-outer-name)

https://pylint.pycqa.org/en/latest/user_guide/messages/warning/redefined-outer-name.html

Same solution.

marc-hb · 2022-05-27T05:30:15Z

Nit: the comment does not seem to add much. Self-describing code FTW.

Will remove

marc-hb · 2022-05-27T05:34:44Z

New line

Suggested change

with open(pathlib.Path(abs_build_dir, "zephyr/include/generated/sof_versions.h")) as hfile:

with open(pathlib.Path(abs_build_dir,

"zephyr/include/generated/sof_versions.h")) as hfile:

Try pylint scripts/xtensa-build-zephyr.py | sort -t: -k2,3 -n or black (warning: make a git commit or backup before using black)

marc-hb · 2022-05-27T05:36:12Z

Newline

Suggested change

sof_version = versions['SOF_MAJOR'] + '.' + versions['SOF_MINOR'] + '.' + versions['SOF_MICRO']

sof_version = versions['SOF_MAJOR'] + '.' + versions['SOF_MINOR'] +

'.' + versions['SOF_MICRO']

Again let pylint scripts/xtensa-build-zephyr.py | sort -t: -k2,3 -n or black tell you the "right" way to format this.

Thanks for the tips @marc-hb Will fix all the identification issues.

marc-hb · 2022-05-27T05:36:31Z

Suggested change

sign_cmd += ["-f", sof_version]

sign_cmd += ["-f", get_sof_version(abs_build_dir)]

marc-hb · 2022-05-27T05:36:36Z

I agree with @aiChaoSONG that there is a problem now: the sof_version global is set twice, BOTH inside and outside the function. That's not good.

However I prefer the other option: keep all read/write accesses to the global sof_version and its "maintenance" private and isolated inside the function.

mengdonglin · 2022-05-27T15:18:45Z

@marc-hb @aiChaoSONG Thanks for your review in the vacation. Are you good if I introduce a class sof_version? This class can has a variable member value as a private cache of the version string, and a member function get() to generate the version string from sof_version.h if value is None. Thus we needn't use a global variable sof_version as a private cache

marc-hb · 2022-05-27T17:13:06Z

Are you good if I introduce a class sof_version?

I think a class with a single variable and single method is overkill and that it will restart from scratch the review cycle that was basically completed. Otherwise I agree it's an appropriate "design pattern" for this here and Python can hopefully do that with very little code. Up to you!

mengdonglin · 2022-05-28T03:56:38Z

@marc-hb No, I don't want to restart the review cycle. I updated the PR: keep sof_version global, removed unnecessary comments for it, and separated the 2 long lines. I ran pylint to give a check. Could you check if it looks better now?

Extract SOF firmware file version string major.minor.micro from the generated sof_version.h, and pass the version string to rimage in the signing phase. So rimage can write the version info to the firmware manifest headers for cAVS platforms. Signed-off-by: mengdonglin <mengdong.lin@intel.com>

aiChaoSONG · 2022-05-30T02:39:37Z

However I prefer the other option: keep all read/write accesses to the global sof_version and its "maintenance" private and isolated inside the function.

A closure is a good fit for this.

def sof_version_getter():
    sof_version = None
    
    def get_sof_version(abs_path):
        nonlocal sof_version
        if sof_version:
            return sof_version
        read_and_parse sof_version_from_file
        return sof_version

    return get_sof_version

sign_cmd += ["-f", sof_version_getter()(abs_build_dir)]

mengdonglin · 2022-05-31T01:10:47Z

Closure can make the cache local for the outer function and looks simpler than class. What do you think @marc-hb ?

marc-hb

A closure sounds cool but I don't think the following code optimizes anything because the same sof_version is shared across all function calls (try: print(id(sof_version)). So sof_version is reset to None every time sof_version_getter() is called. It would work if the get_sof_version function returned by sof_version_getter() were remembered in a... global! But then we have more complicated code and still a global :-)

This is just a simple build script, I think a global sof_version is fine.

def sof_version_getter():
    sof_version = None
    
    def get_sof_version(abs_path):
        nonlocal sof_version
        if sof_version:
            return sof_version
        read_and_parse sof_version_from_file
        return sof_version

    return get_sof_version

sign_cmd += ["-f", sof_version_getter()(abs_build_dir)]

aiChaoSONG · 2022-05-31T02:49:58Z

So sof_version is reset to None every time sof_version_getter() is called.

@marc-hb You are right, to make the code work, the call to sof_version_getter() should be moved out of the for loop.

mengdonglin requested review from aborisovich and marc-hb as code owners May 23, 2022 10:10

mengdonglin requested review from aiChaoSONG, kv2019i and lgirdwood May 23, 2022 10:10

mengdonglin mentioned this pull request May 23, 2022

Make micro version optional for firmware file version string argument thesofproject/rimage#95

Closed

aiChaoSONG reviewed May 23, 2022

View reviewed changes

marc-hb requested changes May 23, 2022

View reviewed changes

mengdonglin requested review from aiChaoSONG and marc-hb May 26, 2022 10:32

mengdonglin changed the title ~~[RFC] xtensa-build-zephyr.py: pass SOF firmware file version string to rimage~~ xtensa-build-zephyr.py: pass SOF firmware file version string to rimage May 26, 2022

marc-hb reviewed May 26, 2022

View reviewed changes

mengdonglin requested a review from marc-hb May 27, 2022 03:05

aiChaoSONG reviewed May 27, 2022

View reviewed changes

marc-hb requested changes May 27, 2022

View reviewed changes

mengdonglin requested a review from marc-hb May 28, 2022 03:58

marc-hb approved these changes May 31, 2022

View reviewed changes

lgirdwood approved these changes May 31, 2022

View reviewed changes

lgirdwood merged commit 55d8d90 into thesofproject:main May 31, 2022

mengdonglin deleted the zephyr-build-version branch June 6, 2022 00:46

	When builing multiple platforms from the same SOF commit, all platforms share the same version.
	When building multiple platforms from the same SOF commit, all platforms share the same version.

	with open(pathlib.Path(abs_build_dir, "zephyr/include/generated/sof_versions.h")) as hfile:
	with open(pathlib.Path(abs_build_dir,
	"zephyr/include/generated/sof_versions.h")) as hfile:

	sof_version = versions['SOF_MAJOR'] + '.' + versions['SOF_MINOR'] + '.' + versions['SOF_MICRO']
	sof_version = versions['SOF_MAJOR'] + '.' + versions['SOF_MINOR'] +
	'.' + versions['SOF_MICRO']

	sign_cmd += ["-f", sof_version]
	sign_cmd += ["-f", get_sof_version(abs_build_dir)]

Uh oh!

Conversation

mengdonglin commented May 23, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marc-hb May 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aiChaoSONG May 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marc-hb May 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marc-hb May 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marc-hb May 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mengdonglin commented May 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marc-hb commented May 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mengdonglin commented May 28, 2022

Uh oh!

aiChaoSONG commented May 30, 2022

Uh oh!

mengdonglin commented May 31, 2022

Uh oh!

marc-hb left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aiChaoSONG commented May 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

marc-hb May 23, 2022 •

edited

Loading

aiChaoSONG May 27, 2022 •

edited

Loading

marc-hb May 27, 2022 •

edited

Loading

marc-hb May 27, 2022 •

edited

Loading

marc-hb May 27, 2022 •

edited

Loading

mengdonglin commented May 27, 2022 •

edited

Loading

marc-hb commented May 27, 2022 •

edited

Loading

marc-hb left a comment •

edited

Loading