Skip to content

Commit 023f93d

Browse files
committed
cmd-osbuild: don't use output capture for generate_runvm_osbuild_config
There's a massive gotcha with bash, which is that even with `set -e`, if you're capturing output from a function, e.g. `x=$(myfunc)`, `set -e` will not be turned on in that function. That made an error in that function much harder to diagnose because the command _kept going_ even though the function failed (and then the overall operation failed later on on a much less clear error). There's a bash option to enable this (`shopt inherit_errexit`), but I think actually that in general we shouldn't be capturing output from non-trivial bash functions. It's hard in bash to ensure clean stdout output and the longer a function gets, the harder it is to keep this up (notice e.g. how that function needs to be careful to `echo` everything to stderr). In this case, it's easy to rework the flow here to avoid this, so let's do that instead.
1 parent 0512d85 commit 023f93d

File tree

1 file changed

+8
-8
lines changed

1 file changed

+8
-8
lines changed

src/cmd-osbuild

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -163,12 +163,7 @@ postprocess_qemu_secex() {
163163

164164
# Here we generate the input JSON we pass to runvm_osbuild for all of our image builds
165165
generate_runvm_osbuild_config() {
166-
runvm_osbuild_config_json="${workdir}/tmp/runvm-osbuild-config-${build}.json"
167-
echo "${runvm_osbuild_config_json}" # Let the caller know where the config will be
168-
if [ -f "${runvm_osbuild_config_json}" ]; then
169-
return # only need to generate this once per build
170-
fi
171-
rm -f "${workdir}"/tmp/runvm-osbuild-config-*.json # clean up any previous configs
166+
local outfile=$1; shift
172167

173168
# reread these values from the build itself rather than rely on the ones loaded
174169
# by prepare_build since the config might've changed since then
@@ -223,7 +218,7 @@ generate_runvm_osbuild_config() {
223218
echo "Disk sizes: metal: ${metal_image_size_mb}M (estimated), cloud: ${cloud_image_size_mb}M" >&2
224219

225220
# Generate the JSON describing the disk we want to build
226-
yaml2json /dev/stdin "${runvm_osbuild_config_json}" <<EOF
221+
yaml2json /dev/stdin "${outfile}" <<EOF
227222
artifact-name-prefix: "${name}-${build}"
228223
build-version: "${build}"
229224
container-imgref: "${container_imgref}"
@@ -399,7 +394,12 @@ main() {
399394
platforms=("${tobuild[@]}")
400395

401396
# Run OSBuild now to build the platforms that were requested.
402-
runvm_osbuild_config_json="$(generate_runvm_osbuild_config)"
397+
runvm_osbuild_config_json="${workdir}/tmp/runvm-osbuild-config-${build}.json"
398+
if [ ! -f "${runvm_osbuild_config_json}" ]; then
399+
rm -f "${workdir}"/tmp/runvm-osbuild-config-*.json # clean up any previous configs
400+
generate_runvm_osbuild_config "$runvm_osbuild_config_json"
401+
fi
402+
403403
outdir=$(mktemp -p "${tmp_builddir}" -d)
404404
runvm_with_cache -- /usr/lib/coreos-assembler/runvm-osbuild \
405405
--config "${runvm_osbuild_config_json}" \

0 commit comments

Comments
 (0)