snap: cross-build snap images #99
Conversation
snap-build/amd64/run_qemu.sh
Outdated
| # SPDX-License-Identifier: Apache-2.0 | ||
| # | ||
|
|
||
| set -x |
There was a problem hiding this comment.
Do these scripts also need set -e?
snap-build/lib.sh
Outdated
| exit 1 | ||
| } | ||
|
|
||
| get_ip() { |
There was a problem hiding this comment.
For clarity these functions might be better named as make_random_ip_addr() and make_random_port()?
| setup_image() { | ||
| img_url=$1 | ||
| img=$2 | ||
| [ -f "${img}" ] && return |
There was a problem hiding this comment.
This is going to be fragile - if the download fails and the script is re-run, the full image will never be downloaded.
There was a problem hiding this comment.
if download fails, output file is removed
snap-build/xbuild.sh
Outdated
| (build_arch arm64 &> ${WORKDIR}/arm64/log) & | ||
| # FIXME: support PPC: https://github.com/kata-containers/packaging/issues/97 | ||
| #sleep 1 | ||
| # (build_arch ppc64 &> ${WORKDIR}/ppc64/log) & |
There was a problem hiding this comment.
Is this PR still WIP? If not, can you remove these commented-out lines.
snap-build/xbuild.sh
Outdated
| mkfs.vfat -n cidata "${seed_img}" &> /dev/null | ||
|
|
||
| if [ -n "${http_proxy}" ]; then | ||
| apt_proxy="apt:\n https_proxy: http://proxy-chain.intel.com:911\n proxy: http://proxy-chain.intel.com:911" |
There was a problem hiding this comment.
You shouldn't have these named proxies here.
snap-build/snap.sh
Outdated
|
|
||
| set -x | ||
| sudo apt-get update -y | ||
| sudo apt-get install -y snapd snapcraft golang-go gcc g++ libcap-ng-dev pkg-config make libz-dev libssl-dev openssl cpio libelf-dev libnewt-dev \ |
There was a problem hiding this comment.
Could you reformat this list so each package is on a separate line (with a backslash)? Also, you might be able to simplify the list by replacing some of these packages with the build-essential package I think.
snap-build/snap.sh
Outdated
| set -x | ||
| sudo apt-get update -y | ||
| sudo apt-get install -y snapd snapcraft golang-go gcc g++ libcap-ng-dev pkg-config make libz-dev libssl-dev openssl cpio libelf-dev libnewt-dev \ | ||
| libiberty-dev libdw-dev libpci-dev libpixman-1-dev libglib2.0-dev librbd-dev libcap-dev libattr1-dev autoconf flex bison python docker.io |
There was a problem hiding this comment.
Will docker.io clash with docker-ce which gets installed when you install a Kata system?
There was a problem hiding this comment.
yep, but I'm not installing Kata, I'm building Kata, and I use docker.io for osbuilder
.gitignore
Outdated
| @@ -9,3 +9,13 @@ prime/ | |||
| stage/ | |||
| snap/.snapcraft/ | |||
| snap/snapcraft.yaml | |||
| snap-build/amd64/log | |||
There was a problem hiding this comment.
Could you simplify to something like the following to avoid hard-coding the list of architectures?
snap-build/*/log
snap-build/*/*.img
snap-build/amd64/run_qemu.sh
Outdated
| @@ -0,0 +1,31 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
I think it might be simpler to use the osbuilder approach and create a single snap-build/run_qemu.sh script and then have a snap-build/${arch}/config.sh file that defines some variables for the particular architecture that snap-build/run_qemu.sh will use. That will minimise duplication and make it easier to add support for additional architectures.
snap-build/amd64/run_qemu.sh
Outdated
|
|
||
| ip="${1}" | ||
| port="${2}" | ||
| arch_dir="${WORKDIR}/amd64" |
There was a problem hiding this comment.
There are lots of variables here that can be shared between the different scripts - see comment above about config.sh.
|
@devimc - a reminder that this PR needs an update. |
devimc
force-pushed
the
snap/multiArch
branch
2 times, most recently
from
1d4206b to
b608b69
Compare
|
@jodh-intel changes applied, thanks |
| # SPDX-License-Identifier: Apache-2.0 | ||
| # | ||
|
|
||
| local arch_qemu="x86_64" |
There was a problem hiding this comment.
I don't think you can use local here as these are globals and local is for functions.
There was a problem hiding this comment.
you can if you source it inside a function 😄
There was a problem hiding this comment.
Ooooh - that's clever! I hadn't spotted that ;)
| @@ -0,0 +1,21 @@ | |||
| #cloud-config | |||
| @APT_PROXY@ | |||
snap-build/snap.sh
Outdated
|
|
||
| sudo apt-get update -y | ||
| sudo apt-get install -y \ | ||
| snapd \ |
There was a problem hiding this comment.
Nit: Might be clearer to sort this list.
snap-build/xbuild.sh
Outdated
|
|
||
| source lib.sh | ||
|
|
||
| readonly supported_archs=(all amd64 ppc64 arm64) |
There was a problem hiding this comment.
Should this be arm to match the change in .ci/lib.sh?
There was a problem hiding this comment.
nop, arm64 is correct, the change in .ci/lib.sh is to match with yq releases
snap-build/lib.sh
Outdated
| img=$2 | ||
| [ -f "${img}" ] && return | ||
| download "${img_url}" "$(dirname ${img})" | ||
| [ $? != 0 ] && rm -f "${img}" && return |
There was a problem hiding this comment.
Is this going to work? The files which source .ci/lib.sh call set -e so if curl fails, the script will immediately exit.
There was a problem hiding this comment.
removed set -e since I have to check exit codes and poweroff the VM
There was a problem hiding this comment.
Note that you can do this:
set -e
{ download "${img_url}" "$(dirname ${img})"; ret=$?; } || true
[ $ret != 0 ] && rm -f "${img}" && return
511f936 to
6ba2325
Compare
|
@jodh-intel changes applied, thanks |
|
Thanks @devimc. lgtm
|
|
lgtm |
Use `arch` to identify qemu architecture instead of hardcoding it. fixes kata-containers#91 Signed-off-by: Julio Montes <julio.montes@intel.com>
Add scripts to cross-build snap images for all supported architectures using virtual machines fixes kata-containers#98 Signed-off-by: Julio Montes <julio.montes@intel.com>
Build snap images for all supported architectures