-
Notifications
You must be signed in to change notification settings - Fork 314
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
xtensa-build-zephyr.py: create /lib/firmware tarball #9363
xtensa-build-zephyr.py: create /lib/firmware tarball #9363
Conversation
2bb5b01
to
4b93dc4
Compare
This is overlapping with sof/installer/GNUmakefile tarball target, but I wanted to now focs on the tooling we actually use to create binaries for recent sof-bin releases (and that's this script). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is likely to break on some Windows systems: those which don't have a "tar" command.
"Batteries included" Python has of course https://docs.python.org/3/library/tarfile.html
This is overlapping with sof/installer/GNUmakefile tarball target
It shouldn't: that GNUmakefile is for XTOS (and topologies if needed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about sof-info.tgz too? We have internal requests to store that too.
scripts/xtensa-build-zephyr.py
Outdated
|
||
build_top = pathlib.Path(west_top, 'build-sof-staging', 'sof') | ||
# special case when build script is called for a single | ||
# target only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Special cases suck. Is this really needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is currently needed to work with the build tools we use to prepare sof-bin releases. I added more commentary on how the script is used in this case, and what we are doing to facilitate. I think this is a fairly reasonable compromise.
scripts/xtensa-build-zephyr.py
Outdated
@@ -1190,7 +1217,9 @@ def main(): | |||
|
|||
build_rimage() | |||
build_platforms() | |||
create_firmware_bundle() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create_firmware_bundle() | |
tar_lib_firmware() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename with yet another way to name... it's a tarball.
4b93dc4
to
8c1b8a2
Compare
@marc-hb wrote:
There's no symlinks, so not sure we need to tarball this. What's needed is to add publishing step for the already created sof-info. The current Jenkins script only uploads build-sof-staging/sof to storage. |
New version pushed:
|
Windows build seems to work and create a tarball, but the win-linux comparison fails it seems: |
Builds dont need to be reproducible between Windows and Linux given that the 3rd party toolchain binaries are out of scope. Reproducible builds within Linux and within Windows must remain true though. i.e. if build reproduction failure is due to toolchain, then we need to leave it as is, otherwise its fixable if build delta is within our script. |
Ok, the tarballs are not comparable across Linux/Windows as symbolic links behave differently. Looking at binaries created in https://github.com/thesofproject/sof/actions/runs/10386740848/job/28758676270?pr=9363 :
|
ok, thanks for checking - Good for me. |
New version pushed:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, the tarballs are not comparable across Linux/Windows as symbolic links behave differently.
Yes I stopped using symbolic links on Windows because they require extra permissions and hassle. I replaced them by copies.
Even if that weren't the case, comparing tarballs would still fail because tarballs include timestamps in file metadata.
There is already a test script to scrub "non-reproducible" (and uninteresting) bits so the --no-tarball
option was not strictly necessary but I like it.
scripts/xtensa-build-zephyr.py
Outdated
# Some CI/build usages incrementally call this script to create | ||
# a staging area with multiple targets. Support this mode | ||
# of use by naming the tarball with target name in case only a single | ||
# target is built. In other cases, use a generic tarball name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't get this sorry. If you build targets "incrementally" without cleaning then recreating the tarball from scratch every time will "incrementally" add previously built targets: no need for this special case.
Also, why would building targets 2 by 2 or 3 by 3 differ from 1 by 1?
This really look like unnecessary, extra complexity to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marc-hb The build scripts we use to prepare Intel binaries for release, build a full set of platforms calling this script one platform at a time. After each build, files are copied out to storage and the work area is cleaned before next platform is built. Thus if a single "sof.tgz" filename is used in this script as output, at the end of this release build creation, there's a sof.tgz with only one platform (the last one built). We cannot incrementally update a tarball that was already uploaded to strorage (with the same name).
With current version of this script, "sof-FOO.tgz" is created when bulding for FOO, and uploaded at each iteration.
For deployment, this is not ideal, so I'd like to keep the option to run this script to build a full set of platforms, and create a singular tarball that contains all files for the release. So if/when we change the Intel release scripts, and/or somebody else takes this script to prepare non-Intel binaries, they can build full set of platforms in one go and get a nice single tarball as a result.
But yeah, it's a bit ugly but not sure how to change. Enumerating all platforms and building a tarball for each one in this function is not easy as if this script is called with e.g. "tgl tgl-h", we don't really know which files need to be in which tarball (without hardcoding Intel-specific paths to search for).
There is a lot more than this needed on the CI side. The deployment script also needs to change.
Which release preparation scripts are you referring to? I'm not aware of any. AFAIK we only had so far "CI secrets" for storage and deployment. These must all change too.
Yes - then nothing happens with this file.
I'll fix this. In the meantime, let's get a version of this merged without any hack. The current hack won't be used yet anyway. |
@marc-hb wrote:
Yup, certainly if you change the layout/contents of the storage area, then that requires changes to deployment scripts as well. This PR is assuming minimal (=no) changes to to downstream scripts.
sof-ci/jenkins scripts (e.g. sof_config_build.sh) that are used to call this xtensa-build-zephyr.py from Jenkins jobs (to build binaries for Intel targets built with xt toolchain). These really don't have any additional logic than steps to publish the artifacts. I've used output from these Jenkins jobs to create PRs to sof-bin, as there are the binaries the on-DUT CI tests. I don't see anything fundamentally wrong here.
OK great. We don't really have to merge this now. I can leave this open. If we have a better solution in time for SOF2.11, we can go with that instead, and if we run out of time, we can merge this temporarily. If the sof-ci/jenkins/pr-build job can tar the staging area and publish that, I'm happy. |
I can't really test Jenkins changes before this is merged. |
ae84fb1
to
e3da356
Compare
Ack @marc-hb -- removed the the single/multi targets logic and rebase on top of latest main. |
With addition of loadable modules, SOF firmware installation depends much more on symbolic links. Add a step to build script to create a tarball of the installed firmware binaries in the build-staging-sof tree. The created tarball (e.g. build-sof-staging/sof/sof.tar.gz) provides a ready structure that can be unpacked to e.g. /lib/firmware on Linux target machines. Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Github artifact store allows to download packages already, so no need to create tarballs in these builds. Also this avoids problems comparing Windows and Linux builds as due to handling of symbolic links, the resulting tarball are not comparable. Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
e3da356
to
7f876c6
Compare
New version pushed:
|
With addition of loadable modules, SOF firmware installation depends much more on symbolic links. Add a step to build script to create a tarball of the installed firmware binaries in the build-staging-sof tree. The created tarball (build-sof-staging/sof/sof.tar.gz) provides a ready structure that can be unpacked to e.g. /lib/firmware on Linux target machines.