Set pipefail for cram tests

[victorlin]

From @tsibley in 3b50476e7e1f6002751bf6234fadf4e7216263f5:

This should probably be a default for our Cram tests?

Possible solutions

Roughly sorted by least to most work involved.

1. Set in .cramrc
   1. ⛔️ Use shell (ref)
      • This doesn't work; shell requires a single executable.
   2. ⛔️ Use shell-opts (ref)
      • Requires patching/forking cram; its config file parsing has a small bug that makes this unworkable as-is.
2. Set in every test environment (done with #1636)
   • Options:
     1. ✅ Use set -o pipefail
     2. Use SHELLOPTS=pipefail (ref)
     3. Use CRAM=--shell-opts='-o pipefail' (ref)
   • Approaches:
     1. ✅ Set in all shared setup scripts
     2. Create a global setup script tests/functional/setup which can hold setup commands such as this one that doesn't require relative paths, and call it from all the individual setup scripts. This may be overkill if it's just a single command but would make it easier to add other similar commands.
     3. Set in a wrapper (e.g. run_tests.sh) and enforce usage of the wrapper instead of directly using cram

[tsibley]

For pipefail, an alternative to shared setup scripts is this diff to .cramrc:

diff --git a/.cramrc b/.cramrc
index 153d20f5..04b65851 100644
--- a/.cramrc
+++ b/.cramrc
@@ -1,3 +1,3 @@
 [cram]
-shell = /bin/bash
+shell = /bin/bash -o pipefail
 indent = 2

I think.

[victorlin]

For pipefail, an alternative to shared setup scripts is this diff to .cramrc

Doesn't work 😕

shell not found: /bin/bash -o pipefail

[genehack]

For pipefail, an alternative to shared setup scripts is this diff to .cramrc

Doesn't work 😕

shell not found: /bin/bash -o pipefail

I wonder if having export SHELLOPTS=pipefail in run_tests.sh would work? From the bash man page:

SHELLOPTS A colon-separated list of enabled shell options. Each word in the list is a valid argument for the -o option to the set builtin command (see SHELL BUILTIN COMMANDS below). The options appearing in SHELLOPTS are those reported as on by set -o. If this variable is in the environment when bash starts up, each shell option in the list will be enabled before reading any startup files. This variable is read-only.

[tsibley]

shell not found: /bin/bash -o pipefail

Ah, so cram wants a single executable for shell. That makes sense. I'd say try this instead:

diff --git a/.cramrc b/.cramrc
index 153d20f5..04b65851 100644
--- a/.cramrc
+++ b/.cramrc
@@ -1,3 +1,4 @@
 [cram]
 shell = /bin/bash
+shell-opts = -o pipefail
 indent = 2

since cram supports --shell-opts, but I tried it myself first this time and found that its config file parsing has a small bug that makes this unworkable without patching/forking. (We could easily do that though…)

Other approaches like setting shell = ./devel/bash and then handling shell options ourselves in there doesn't work because cram requires any relative shell path be relative to an entry in PATH (i.e. and not the config file).

I wonder if having export SHELLOPTS=pipefail in run_tests.sh would work?

SHELLOPTS certainly works in general (though I dunno if cram does anything special with it), but I wouldn't want to put it only in run_tests.sh as that's not the only way we run tests (and I personally nearly never run tests that way). CRAM=--shell-opts='-o pipefail' is also an alternative that definitely works, but again, we have to set CRAM in the right places.

[victorlin]

Thanks for the alternative suggestions – I hadn't thought of those. I just updated the issue description with all the different approaches that have been proposed.

I'll stick with what I've done in https://github.com/nextstrain/augur/pull/1636 since it seems to be the most direct way of addressing this without getting into more sophisticated changes (e.g. patching/forking cram, enforcing run_tests.sh), but open to other opinions.