From e6ea381aba553dbd613d1d54c9844751b11963c1 Mon Sep 17 00:00:00 2001
From: Marc Durdin <marc@durdin.net>
Date: Mon, 27 Feb 2023 15:09:42 +0700
Subject: [PATCH] chore(common): fix child build error handling

Fixes #8322.

This issue was arising due to subshells masking the exit code.

set -e is not really to be trusted.
---
 resources/build/tests/builder.inc.test.sh     | 10 ++++++
 .../build/tests/dependencies/app/build.sh     | 10 +++++-
 .../build/tests/dependencies/error/build.sh   | 33 +++++++++++++++++++
 resources/builder.inc.sh                      | 14 ++++++--
 4 files changed, 63 insertions(+), 4 deletions(-)
 create mode 100755 resources/build/tests/dependencies/error/build.sh

diff --git a/resources/build/tests/builder.inc.test.sh b/resources/build/tests/builder.inc.test.sh
index 46fd7529cf..5699bb13ca 100755
--- a/resources/build/tests/builder.inc.test.sh
+++ b/resources/build/tests/builder.inc.test.sh
@@ -162,6 +162,16 @@ $THIS_SCRIPT_PATH/build-utils-traps.test.sh error-in-function
 $THIS_SCRIPT_PATH/build-utils-traps.test.sh incomplete
 echo "${COLOR_BLUE}## Running dependency tests${COLOR_RESET}"
 $THIS_SCRIPT_PATH/builder-deps.test.sh
+$THIS_SCRIPT_PATH/dependencies/app/build.sh configure build
+
+$THIS_SCRIPT_PATH/dependencies/app/build.sh error && \
+  builder_die "FAIL: error code 0 but should have failed with exit code 22 from child dep" || (
+    result=$?
+    if [[ $result != 22 ]]; then
+      builder_die "FAIL: exit code $result but should have failed with exit code 22 from child dep"
+    fi
+  ) || exit $?
+
 echo "${COLOR_BLUE}## End external tests${COLOR_RESET}"
 echo
 
diff --git a/resources/build/tests/dependencies/app/build.sh b/resources/build/tests/dependencies/app/build.sh
index c38f6204ca..5c558af55f 100755
--- a/resources/build/tests/dependencies/app/build.sh
+++ b/resources/build/tests/dependencies/app/build.sh
@@ -14,8 +14,10 @@ cd "$THIS_SCRIPT_PATH"
 
 builder_describe "app test module" \
   @../library \
+  "@../error error" \
   configure \
-  build
+  build \
+  error
 
 builder_parse "$@"
 
@@ -39,3 +41,9 @@ if builder_start_action build:project; then
   touch out.build
   builder_finish_action success build:project
 fi
+
+if builder_start_action error:project; then
+  echo " ... doing the 'error' action for 'app'; we shouldn't have gotten here"
+  echo " ... because dependencies/error should have failed"
+  exit 99
+fi
diff --git a/resources/build/tests/dependencies/error/build.sh b/resources/build/tests/dependencies/error/build.sh
new file mode 100755
index 0000000000..83934c2197
--- /dev/null
+++ b/resources/build/tests/dependencies/error/build.sh
@@ -0,0 +1,33 @@
+#!/usr/bin/env bash
+
+set -eu
+
+## START STANDARD BUILD SCRIPT INCLUDE
+# adjust relative paths as necessary
+THIS_SCRIPT="$(readlink -f "${BASH_SOURCE[0]}")"
+. "${THIS_SCRIPT%/*}/../../../build-utils.sh"
+# END STANDARD BUILD SCRIPT INCLUDE
+
+cd "$THIS_SCRIPT_PATH"
+
+# Test builder_describe_outputs and dependencies
+
+builder_describe "library test module" \
+  configure \
+  build
+
+builder_parse "$@"
+
+builder_describe_outputs \
+  configure:project out.configure \
+  build:project     out.build
+
+if builder_start_action configure:project; then
+  echo " ... doing the 'configure' action for 'library'"
+  exit 22
+fi
+
+if builder_start_action build:project; then
+  echo " ... doing the 'build' action for 'library'"
+  exit 22
+fi
diff --git a/resources/builder.inc.sh b/resources/builder.inc.sh
index 6004724da4..622a9f42f6 100755
--- a/resources/builder.inc.sh
+++ b/resources/builder.inc.sh
@@ -290,7 +290,7 @@ _builder_execute_child() {
     result=$?
     echo "${COLOR_RED}## $scope$action$target failed with exit code $result${COLOR_RESET}"
     exit $result
-  )
+  ) || exit $? # Required due to above subshell masking exit
 }
 
 _builder_run_child_action() {
@@ -1263,7 +1263,7 @@ _builder_do_build_deps() {
     # Don't attempt to build dependencies that don't match the current
     # action:target (wildcards supported for matches here)
     if ! _builder_should_build_dep "$action_target" "$dep"; then
-      echo "[$THIS_SCRIPT_IDENTIFIER] Skipping dependency build $dep for $_builder_matched_action_name"
+      echo "[$THIS_SCRIPT_IDENTIFIER] Skipping dependency $dep for $_builder_matched_action_name"
       continue
     fi
 
@@ -1279,7 +1279,15 @@ _builder_do_build_deps() {
       $builder_debug \
       $_builder_build_deps \
       --builder-deps-built "$_builder_deps_built" \
-      --builder-dep-parent "$THIS_SCRIPT_IDENTIFIER"
+      --builder-dep-parent "$THIS_SCRIPT_IDENTIFIER" && (
+      if $_builder_debug; then
+        echo "${COLOR_GREEN}## [$THIS_SCRIPT_IDENTIFIER] Dependency $dep for $_builder_matched_action_name successfully${COLOR_RESET}"
+      fi
+    ) || (
+      result=$?
+      echo "${COLOR_RED}## [$THIS_SCRIPT_IDENTIFIER] Dependency failed with exit code $result${COLOR_RESET}"
+      exit $result
+    ) || exit $? # Required due to above subshell masking exit
   done
 }