Run each component in a subshell and handle errors more robustly

This commit completely rewrites keep-going mode. Instead of relying
solely on "set -e", which has some subtle limitations (such as being
off anywhere inside a conditional), use an ERR trap to record errors.

Run each component in a subshell. This way a component can set
environment variables, change the current directory, etc., without
affecting other components.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

tests/scripts/all.sh

@@ -59,6 +59,15 @@
 # This script must be invoked from the toplevel directory of a git
 # working copy of Mbed TLS.
 #
+# The behavior on an error depends on whether --keep-going (alias -k)
+# is in effect.
+#  * Without --keep-going: the script stops on the first error without
+#    cleaning up. This lets you work in the configuration of the failing
+#    component.
+#  * With --keep-going: the script runs all requested components and
+#    reports failures at the end. In particular the script always cleans
+#    up on exit.
+#
 # Note that the output is not saved. You may want to run
 #   script -c tests/scripts/all.sh
 # or
@@ -81,6 +90,9 @@
 #
 # Each component must start by invoking `msg` with a short informative message.
 #
+# Each component is executed in a separate shell process. The component
+# fails if any command in it returns a non-zero status.
+#
 # The framework performs some cleanup tasks after each component. This
 # means that components can assume that the working directory is in a
 # cleaned-up state, and don't need to perform the cleanup themselves.
@@ -91,19 +103,6 @@
 #   `tests/Makefile` and `programs/fuzz/Makefile` from git.
 #   This cleans up after an in-tree use of CMake.
 #
-# Any command that is expected to fail must be protected so that the
-# script keeps running in --keep-going mode despite `set -e`. In keep-going
-# mode, if a protected command fails, this is logged as a failure and the
-# script will exit with a failure status once it has run all components.
-# Commands can be protected in any of the following ways:
-# * `make` is a function which runs the `make` command with protection.
-#   Note that you must write `make VAR=value`, not `VAR=value make`,
-#   because the `VAR=value make` syntax doesn't work with functions.
-# * Put `report_status` before the command to protect it.
-# * Put `if_build_successful` before a command. This protects it, and
-#   additionally skips it if a prior invocation of `make` in the same
-#   component failed.
-#
 # The tests are roughly in order from fastest to slowest. This doesn't
 # have to be exact, but in general you should add slower tests towards
 # the end and fast checks near the beginning.
@@ -477,8 +476,9 @@ pre_check_git () {
 }
 
 pre_setup_keep_going () {
-    failure_summary=
+    failure_count=0 # Number of failed components
-    failure_count=0
+    last_failure_status=0 # Last failure status in this component
+
     start_red=
     end_color=
     if [ -t 1 ]; then
@@ -489,57 +489,73 @@ pre_setup_keep_going () {
                 ;;
         esac
     fi
-    record_status () {
+
-        if "$@"; then
+    # Keep a summary of failures in a file. We'll print it out at the end.
-            last_status=0
+    failure_summary_file=$PWD/all-sh-failures-$$.log
-        else
+    : >"$failure_summary_file"
-            last_status=$?
+
-            text="$current_section: $* -> $last_status"
+    # Whether it makes sense to keep a component going after the specified
-            failure_summary="$failure_summary
+    # command fails (test command) or not (configure or build).
-$text"
+    # This doesn't have to be 100% accurate: all failures are recorded anyway.
-            failure_count=$((failure_count + 1))
+    can_keep_going_after_failure () {
-            echo "${start_red}^^^^$text^^^^${end_color}" >&2
+        case "$1" in
-        fi
+            "msg "*) false;;
-    }
+            *[!A-Za-z]"test"|*[!A-Za-z]"test"[!A-Za-z]*) true;;
-    make () {
+            "tests/"*) true;;
-        case "$*" in
+            "grep "*|"not grep "*) true;;
-            *test|*check)
+            *) false;;
-                if [ $build_status -eq 0 ]; then
-                    record_status command make "$@"
-                else
-                    echo "(skipped because the build failed)"
-                fi
-                ;;
-            *)
-                record_status command make "$@"
-                build_status=$last_status
-                ;;
         esac
     }
+
+    # This function runs if there is any error in a component.
+    # It must either exit with a nonzero status, or set
+    # last_failure_status to a nonzero value.
+    err_trap () {
+        # Save $? (status of the failing command). This must be the very
+        # first thing, before $? is overridden.
+        last_failure_status=$?
+        failed_command=$BASH_COMMAND
+
+        text="$current_section: $failed_command -> $last_failure_status"
+        echo "${start_red}^^^^$text^^^^${end_color}" >&2
+        echo "$text" >>"$failure_summary_file"
+
+        # If the command is fatal (configure or build command), stop this
+        # component. Otherwise (test command) keep the component running
+        # (run more tests from the same build).
+        if ! can_keep_going_after_failure "$failed_command"; then
+            exit $last_failure_status
+        fi
+    }
+
     final_report () {
         if [ $failure_count -gt 0 ]; then
             echo
             echo "!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"
-            echo "${start_red}FAILED: $failure_count${end_color}$failure_summary"
+            echo "${start_red}FAILED: $failure_count components${end_color}"
+            cat "$failure_summary_file"
             echo "!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"
-            exit 1
         elif [ -z "${1-}" ]; then
             echo "SUCCESS :)"
         fi
         if [ -n "${1-}" ]; then
             echo "Killed by SIG$1."
         fi
+        rm -f "$failure_summary_file"
+        if [ $failure_count -gt 0 ]; then
+            exit 1
+        fi
     }
 }
 
-if_build_succeeded () {
+# These functions are kept temporarily for backward compatibility.
-    if [ $build_status -eq 0 ]; then
+# Don't use them in new components.
-        record_status "$@"
+record_status () {
-    fi
+    "$@"
+}
+if_build_succeeded () {
+    "$@"
 }
-
-# to be used instead of ! for commands run with
-# record_status or if_build_succeeded
 not() {
     ! "$@"
 }
@@ -2667,12 +2683,35 @@ run_component () {
     # have messed it up or shortened it.
     redirect_err dd if=/dev/urandom of=./tests/seedfile bs=64 count=1
 
-    # Run the component code.
+    # Run the component in a subshell
-    if [ $QUIET -eq 1 ]; then
+    if [ $KEEP_GOING -eq 1 ]; then
-        # msg() is silenced, so just print the component name here
+        set +e
-        echo "${current_component#component_}"
+    fi
+    (
+        if [ $QUIET -eq 1 ]; then
+            # msg() will be silenced, so just print the component name here.
+            echo "${current_component#component_}"
+            exec >/dev/null
+        fi
+        if [ $KEEP_GOING -eq 1 ]; then
+            # Keep "set -e" off, and run an ERR trap instead to record failures.
+            set -E
+            trap err_trap ERR
+        fi
+        # The next line is what runs the component
+        "$@"
+        if [ $KEEP_GOING -eq 1 ]; then
+            trap - ERR
+            exit $last_failure_status
+        fi
+    )
+    component_status=$?
+    if [ $KEEP_GOING -eq 1 ]; then
+        set -e
+        if [ $component_status -ne 0 ]; then
+            failure_count=$((failure_count + 1))
+        fi
     fi
-    redirect_out "$@"
 
     # Restore the build tree to a clean state.
     cleanup
@@ -2689,10 +2728,6 @@ pre_check_git
 build_status=0
 if [ $KEEP_GOING -eq 1 ]; then
     pre_setup_keep_going
-else
-    record_status () {
-        "$@"
-    }
 fi
 pre_setup_quiet_redirect
 pre_prepare_outcome_file