Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set pipefail for cram tests #1627

Closed
victorlin opened this issue Sep 10, 2024 · 5 comments · Fixed by #1636
Closed

Set pipefail for cram tests #1627

victorlin opened this issue Sep 10, 2024 · 5 comments · Fixed by #1636

Comments

@victorlin
Copy link
Member

victorlin commented Sep 10, 2024

From @tsibley in 3b50476:

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 Set pipefail for Cram tests #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
Copy link
Member

tsibley commented Sep 10, 2024

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
Copy link
Member Author

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

Doesn't work 😕

shell not found: /bin/bash -o pipefail

@genehack
Copy link
Contributor

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
Copy link
Member

tsibley commented Sep 24, 2024

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
Copy link
Member Author

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 #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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants