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

xtensa-build-zephyr.py: create /lib/firmware tarball #9363

Merged

Conversation

kv2019i
Copy link
Collaborator

@kv2019i kv2019i commented Aug 13, 2024

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.

@kv2019i kv2019i requested a review from lyakh August 13, 2024 10:44
@kv2019i kv2019i force-pushed the 202408-xtensa-build-create-tarball branch 2 times, most recently from 2bb5b01 to 4b93dc4 Compare August 13, 2024 13:20
@kv2019i
Copy link
Collaborator Author

kv2019i commented Aug 13, 2024

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).

Copy link
Collaborator

@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.

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)

Copy link
Collaborator

@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.

How about sof-info.tgz too? We have internal requests to store that too.

scripts/xtensa-build-zephyr.py Outdated Show resolved Hide resolved
scripts/xtensa-build-zephyr.py Outdated Show resolved Hide resolved

build_top = pathlib.Path(west_top, 'build-sof-staging', 'sof')
# special case when build script is called for a single
# target only
Copy link
Collaborator

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?

Copy link
Collaborator Author

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 Show resolved Hide resolved
@@ -1190,7 +1217,9 @@ def main():

build_rimage()
build_platforms()
create_firmware_bundle()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
create_firmware_bundle()
tar_lib_firmware()

Copy link
Collaborator Author

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.

scripts/xtensa-build-zephyr.py Outdated Show resolved Hide resolved
@kv2019i kv2019i force-pushed the 202408-xtensa-build-create-tarball branch from 4b93dc4 to 8c1b8a2 Compare August 14, 2024 11:31
@kv2019i
Copy link
Collaborator Author

kv2019i commented Aug 14, 2024

@marc-hb wrote:

How about sof-info.tgz too? We have internal requests to store that too.

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.

@kv2019i
Copy link
Collaborator Author

kv2019i commented Aug 14, 2024

New version pushed:

  • added cmdline option to disable tarball creation
  • use tarfile python module (so in theory works on Windows as well, although see inline comments)

@kv2019i
Copy link
Collaborator Author

kv2019i commented Aug 14, 2024

Windows build seems to work and create a tarball, but the win-linux comparison fails it seems:
https://github.com/thesofproject/sof/actions/runs/10386740848/job/28759174383?pr=9363
The quest for reproducible tarballs begins...

@lgirdwood
Copy link
Member

Windows build seems to work and create a tarball, but the win-linux comparison fails it seems: https://github.com/thesofproject/sof/actions/runs/10386740848/job/28759174383?pr=9363 The quest for reproducible tarballs begins...

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.

@kv2019i
Copy link
Collaborator Author

kv2019i commented Aug 14, 2024

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 :

$ tar ztvf t-win/build-sof-staging/sof/sof-mtl.tgz 
drwxrwxrwx 0/0               0 2024-08-14 14:38 intel/
drwxrwxrwx 0/0               0 2024-08-14 14:38 intel/sof-ipc4/
drwxrwxrwx 0/0               0 2024-08-14 14:38 intel/sof-ipc4/arl/
drwxrwxrwx 0/0               0 2024-08-14 14:38 intel/sof-ipc4/arl/community/
-rw-rw-rw- 0/0          641592 2024-08-14 14:38 intel/sof-ipc4/arl/community/sof-arl.ri
drwxrwxrwx 0/0               0 2024-08-14 14:38 intel/sof-ipc4/arl-s/
drwxrwxrwx 0/0               0 2024-08-14 14:38 intel/sof-ipc4/arl-s/community/
-rw-rw-rw- 0/0          641592 2024-08-14 14:38 intel/sof-ipc4/arl-s/community/sof-arl-s.ri
drwxrwxrwx 0/0               0 2024-08-14 14:38 intel/sof-ipc4/mtl/
drwxrwxrwx 0/0               0 2024-08-14 14:38 intel/sof-ipc4/mtl/community/
-rw-rw-rw- 0/0          641592 2024-08-14 14:38 intel/sof-ipc4/mtl/community/sof-mtl.ri
drwxrwxrwx 0/0               0 2024-08-14 14:38 intel/sof-ipc4-lib/
drwxrwxrwx 0/0               0 2024-08-14 14:38 intel/sof-ipc4-lib/mtl/
drwxrwxrwx 0/0               0 2024-08-14 14:38 intel/sof-ipc4-lib/mtl/community/
$ tar ztvf t-linux/build-sof-staging/sof/sof-mtl.tgz 
drwxr-xr-x cwd_user/cwd_group 0 2024-08-14 14:37 intel/
drwxr-xr-x cwd_user/cwd_group 0 2024-08-14 14:37 intel/sof-ipc4/
drwxr-xr-x cwd_user/cwd_group 0 2024-08-14 14:37 intel/sof-ipc4/arl/
drwxr-xr-x cwd_user/cwd_group 0 2024-08-14 14:37 intel/sof-ipc4/arl/community/
lrwxrwxrwx cwd_user/cwd_group 0 2024-08-14 14:37 intel/sof-ipc4/arl/community/sof-arl.ri -> ../../mtl/community/sof-mtl.ri
drwxr-xr-x cwd_user/cwd_group 0 2024-08-14 14:37 intel/sof-ipc4/arl-s/
drwxr-xr-x cwd_user/cwd_group 0 2024-08-14 14:37 intel/sof-ipc4/arl-s/community/
lrwxrwxrwx cwd_user/cwd_group 0 2024-08-14 14:37 intel/sof-ipc4/arl-s/community/sof-arl-s.ri -> ../../mtl/community/sof-mtl.ri
drwxr-xr-x cwd_user/cwd_group 0 2024-08-14 14:37 intel/sof-ipc4/mtl/
drwxr-xr-x cwd_user/cwd_group 0 2024-08-14 14:37 intel/sof-ipc4/mtl/community/
-rw-r--r-- cwd_user/cwd_group 641592 2024-08-14 14:37 intel/sof-ipc4/mtl/community/sof-mtl.ri
drwxr-xr-x cwd_user/cwd_group      0 2024-08-14 14:37 intel/sof-ipc4-lib/
drwxr-xr-x cwd_user/cwd_group      0 2024-08-14 14:37 intel/sof-ipc4-lib/mtl/
drwxr-xr-x cwd_user/cwd_group      0 2024-08-14 14:37 intel/sof-ipc4-lib/mtl/community/

@lgirdwood
Copy link
Member

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.

@kv2019i
Copy link
Collaborator Author

kv2019i commented Aug 14, 2024

New version pushed:

  • try to disable tarball creation in github flows (not needed, saves spaces, and avoids the win-linux comparison fail)

@kv2019i kv2019i marked this pull request as ready for review August 14, 2024 14:57
Copy link
Collaborator

@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.

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.

# 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.
Copy link
Collaborator

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.

Copy link
Collaborator Author

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).

scripts/xtensa-build-zephyr.py Show resolved Hide resolved
@marc-hb
Copy link
Collaborator

marc-hb commented Aug 16, 2024

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.

There is a lot more than this needed on the CI side. The deployment script also needs to change.

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.

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.

With current version of this script, "sof-FOO.tgz" is created when bulding for FOO, and uploaded at each iteration.

Yes - then nothing happens with this file.

After each build, files are copied out to storage and the work area is cleaned before next platform is built.

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.

@kv2019i
Copy link
Collaborator Author

kv2019i commented Aug 16, 2024

@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.

There is a lot more than this needed on the CI side. The deployment script also needs to change.

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.

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.

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.

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.

After each build, files are copied out to storage and the work area is cleaned before next platform is built.
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.

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.

@kv2019i kv2019i marked this pull request as draft August 26, 2024 10:26
@marc-hb
Copy link
Collaborator

marc-hb commented Sep 4, 2024

We don't really have to merge this now. I can leave this open.

I can't really test Jenkins changes before this is merged.

@kv2019i kv2019i force-pushed the 202408-xtensa-build-create-tarball branch from ae84fb1 to e3da356 Compare September 5, 2024 10:36
@kv2019i kv2019i marked this pull request as ready for review September 5, 2024 10:36
@kv2019i
Copy link
Collaborator Author

kv2019i commented Sep 5, 2024

Ack @marc-hb -- removed the the single/multi targets logic and rebase on top of latest main.

scripts/xtensa-build-zephyr.py Outdated Show resolved Hide resolved
scripts/xtensa-build-zephyr.py Outdated Show resolved Hide resolved
scripts/xtensa-build-zephyr.py Show resolved Hide resolved
scripts/xtensa-build-zephyr.py Show resolved Hide resolved
scripts/xtensa-build-zephyr.py Show resolved Hide resolved
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>
@kv2019i kv2019i force-pushed the 202408-xtensa-build-create-tarball branch from e3da356 to 7f876c6 Compare September 6, 2024 15:36
@kv2019i
Copy link
Collaborator Author

kv2019i commented Sep 6, 2024

New version pushed:

  • dropped close() (let with handle)
  • dropped --tarball

@cujomalainey cujomalainey merged commit 7ec73c7 into thesofproject:main Sep 6, 2024
42 of 45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants