search

thesofproject / rimage

DSP firmware image creation and signing tool
Other
7 stars 62 forks source link

Make micro version optional for firmware file version string argument #95

Closed mengdonglin closed 2 years ago

mengdonglin commented 2 years ago

This is a preparation to fix https://github.com/thesofproject/sof/issues/5775. Another SOF PR https://github.com/thesofproject/sof/pull/5846 is to update xtensa-build-zephyr.py

rimage can accept firmware file version string like major.minor or major.minor.micro. The 3rd field 'micro' is optional.

For building with XOTS, SOF CMake always passes version string major.minor.micro to rimage.

But for building with Zephyr, xtensa-build-zephyr.py will extract version string from latest git tag which can be either major.minor or major.minor.micro

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

aiChaoSONG commented 2 years ago

I don't think we should make the version number flexible. If we want to use MAJOR.MINOR.HOTFIX, we should stick to it. It looks not good to me that sometimes MAJOR.MINOR.HOTFIX is used, but sometimes MAJOR.MINOR is used. For the place we use MAJOR.MINOR, we should really use MAJOR.MINOR.0. Or we should really improve the tag naming conversion.

mengdonglin commented 2 years ago

I don't think we should make the version number flexible. If we want to use MAJOR.MINOR.HOTFIX, we should stick to it. It looks not good to me that sometimes MAJOR.MINOR.HOTFIX is used, but sometimes MAJOR.MINOR is used. For the place we use MAJOR.MINOR, we should really use MAJOR.MINOR.0. Or we should really improve the tag naming conversion.

@aiChaoSONG Thanks for your review. Both major.minor and major.minor.micro are widely used in software versioning. That's why I hope to keep some flexibility here. When major.minor is enough, we needn't use major.minor.0

aiChaoSONG commented 2 years ago

When major.minor is enough, we needn't use major.minor.0

In the underlining code, all three slots for these three numbers are in the C structure, even we don't pass them from command line, they get the default value 0.0.0. We have been using MAJOR.MINOR.HOTFIX pattern always.

@lgirdwood If the hotfix(micro) number is zero, it is removed from the tag, we just simply use major.minor in the tag, which I think is not good. Since we use MAJOR.MINOR.HOTFIX versioning, we should have them all in the tags. Actually, Zephyr/CPython and many other softwares use x.y.z explicitly even z==0.

marc-hb commented 2 years ago

But for building with Zephyr, xtensa-build-zephyr.py will extract version string from latest git tag which can be either major.minor or major.minor.micro

I don't understand why building with Zephyr can make any difference to the SOF version, I don't think it should. When building from the same SOF commit then the SOF version should always be the same whether we're building with or without Zephyr. There should be other differences but the SOF version should stay the same.

aiChaoSONG commented 2 years ago

When building from the same SOF commit then the SOF version should always be the same whether we're building with or without Zephyr.

I think this is because we name tag differently. If z == 0 in x.y.z, we just drop the z field. image

lgirdwood commented 2 years ago

When building from the same SOF commit then the SOF version should always be the same whether we're building with or without Zephyr.

I think this is because we name tag differently. If z == 0 in x.y.z, we just drop the z field. image

Ack, if z is zero it is dropped from the version. Likewise if tag is not used we also append the commit ID.

SOF uses exactly the same versioning scheme as Linux, our scripts need to do the same. @mengdonglin pls confirm if your PRs follow Linux versioning.

marc-hb commented 2 years ago

the SOF version should always be the same whether we're building with or without Zephyr.

I think this is because we name tag differently. If z == 0 in x.y.z, we just drop the z field.

You seem to be referring to something else, not sure what.

What I'm saying is: sof_versions.h is the same with Zephyr and without Zephyr. Try it and see for yourself. The commit message seems to imply we should make a difference there. I think we should not. Whatever we do with the content of sof_versions.h, it should be the same with or without Zephyr.

mengdonglin commented 2 years ago

@lgirdwood I think my PRs follow Linux versioning.
I'll try to update xtensa-build-zephyr.py as @marc-hb and @aiChaoSONG: we always pass a 3-field version string major.minor.micro to rimage. If git tag doesn't include the micro version, the build script will generate a version string major.minor.0 and pass it to rimage. So we needn't change SOF versioning or rimage.