build: Add commandline versioning #7930
Conversation
There was no option to add version from command line this commit adds that functionality leaving all other versioning options viable. Versioning from commandline takes precedence over every other way of versioning and will not activate unless proper command line args are sent. Version and build number can be overwritten this way and can be done so separatley. Signed-off-by: Jakub Dabek <jakub.dabek@intel.com>
lgirdwood
left a comment
lgirdwood
left a comment
There was a problem hiding this comment.
@dabekjakub I assume this will be used by production builld environments ?
|
Not all of theme but yes that is the intent. |
marc-hb
left a comment
marc-hb
left a comment
There was a problem hiding this comment.
The build_version is duplicated, there is one (hard-coded) copy in the (Zephyr) binary and another copy in the rimage header.
In fact I believe this is what test_00_01_load_fw_and_check_version checks: that both copies are identical.
Correct me but this new feature will allow the user to make these two copies different because the zephyr copy will stay hardcoded while the rimage header can now be changed with these new options. I never understood why this build_version is duplicated in two places but surely the --help should warn that using these new options affects only rimage and creates a mismatch with the binary. Neither the --help nor the commit message mention rimage right now!
Also, what is this build_version used for? It's became hardcoded during the transition to Zephyr and has stayed hardcoded for years since the beginning of the Zephyr history. So why do you need it now? The commit message does not say.
Similar comment about the other, sof_fw_version: it should warn that this creates a mismatch and the --help, commit message and source code comments should mention rimage and the mismatch with the binary.
| args.platforms = platform_names | ||
|
|
||
| global sof_fw_version | ||
| global sof_build_version |
There was a problem hiding this comment.
Please don't define new globals, just use args.sof_fw_version directly in get_sof_version() and keep all the sof_fw_version related code in a single place.
We already use args as a global all over the place, as long as it's keep immutable it's fine.
There was a problem hiding this comment.
There is a difference between sof_fw_version and sof_build_version when passing it to rimage so i would have to do string operations to get out the build number. I think global is less bothersome.
Or did I misunderstand what is your intent here ?
| # test_00_01_load_fw_and_check_version | ||
| opts.append(("-b", "1")) | ||
|
|
||
| global sof_build_version |
There was a problem hiding this comment.
Same thing as above, just use args.sof_build_version directly for consistency with the rest of the code, no need for a new global. One args global is enough.
| opts.append(("-b", "1")) | ||
|
|
||
| global sof_build_version | ||
| if sof_build_version is not None: |
There was a problem hiding this comment.
Avoid double negations
if sof_build_version is None:
opts.append(("-b", "1"))
else:
...
|
These are the versions that are not changed by this PR: That's because it still comes from the small I'm not sure why changing the
|
|
Maybe this is a little too flexible? Totally agree that there's a need for unifying the version tracking in the build artifacts. But almost always, it's best if the source for the version info be part of the source tree (c.f. Linux's root of version info is at the top of the Makefile), so that someone who gets a blob can find an answer to "where was this built from"? Passing it as a command line argument to the (technically optional) top level build script, which then passes it as an argument to rimage, seems like, heh, just passing the buck. Can we make this a cmake variable or something similar and force everything to use it in the same way? |
Actually you can already do this right now: This has the same effect as this PR:
=> mismatch |
After second thought, this would not work because CMake runs AFTER rimage configuration. So the pre-requisite required for this CLI feature (if I'm guessing the requirement correctly) is to complete the rimage migration job that I started in #6917 etc.: move all things rimage completely AWAY from this This is the full migration that @aborisovich requested in #6917 (comment) and he was right! But it's probably not a small zephyr/cmake task. |
|
First thing first - moving this back to draft since @marc-hb pointed out the mismatch and it can't go in like that. I will move it to review as soon as I find a good way to do it without mismatch. Secondly - main reasoning for this pr is to satisfy some release requirements. Right now there is no way to change build number which is needed by some windows releases. I wanted to introduce the way that would not interfere with current versioning. Tercio - ultimately @aborisovich way is the best however is there any problem with using the input of version (form cmd as in the pr) and add it as cmake variable already ? Light change to the cmake would make that work. And ultimately both cmake and rimage would be configured by one arg. |
Possibly but what cmake variable, how would that work? Also, we need a vision and some TODOs in the code explaining how we eventually move all that out of this script and entirely into the zephyr build system / cmake. |
|
Also, to keep the implementation much simpler everywhere and more flexible and more future proof, would you be OK with implementing all this as some kind of |
4 participants
There was no option to add version from command line this commit adds that functionality leaving all other versioning options viable. Versioning from commandline takes precedence over every other way of versioning and will not activate unless proper command line args are sent. Version and build number can be overwritten this way and can be done so separatley.