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

Use set -o nounset -o pipefail -o errexit on top/in all scripts #160

Open
ypid opened this issue Feb 13, 2017 · 8 comments
Open

Use set -o nounset -o pipefail -o errexit on top/in all scripts #160

ypid opened this issue Feb 13, 2017 · 8 comments

Comments

@ypid
Copy link
Contributor

ypid commented Feb 13, 2017

Ref: https://news.ycombinator.com/item?id=10736584

@Stratus3D
Copy link
Member

I've been using the "Unofficial Bash Strict Mode" (http://redsymbol.net/articles/unofficial-bash-strict-mode/) in my personal Bash scripts, which is very similar to this. For an example see this: https://github.com/Stratus3D/dotfiles/blob/master/scripts/tools/epmd_port_forwarder

@HashNuke @tuvistavie @vic what do you guys think? This can have a pretty significant effect on the way we write Bash script so it's important everyone is on board with this. I'm fine either way. We've got a good test suite so this isn't critical.

@danhper
Copy link
Member

danhper commented Mar 8, 2017

I do not have any strong opinion about this, so if you want to use these options it I am fine with it too.

@vic
Copy link
Contributor

vic commented Oct 25, 2017

Now that we have #223 merged, I'm +1 on also adding the "bash script mode" to all our bash files.

set -euo pipefail
IFS=$'\t\n' # Stricter IFS settings

I'll try adding this and test everything goes as expected.

@vic vic self-assigned this Oct 25, 2017
@ypid
Copy link
Contributor Author

ypid commented Oct 25, 2017

set -euo pipefail

I used to do that but the long versions are more understandable by other people who will look at the script.

Ref: https://news.ycombinator.com/item?id=10738276

I would also prefer the long version. Why go with the short one?

@Stratus3D
Copy link
Member

@ypid that's a good question. I typically use the long version in my personal scripts, but I'm not sure there was a reason behind using the short version in asdf. We could add the details on strict mode to the document contributing rather than switch to the long version of strict mode. One nice this with the short version is that it's only two lines, so it hardly takes up any space at the top of the file.

@ypid
Copy link
Contributor Author

ypid commented Oct 25, 2017

One nice this with the short version is that it's only two lines, so it hardly takes up any space at the top of the file.

My previous quote was incomplete. Both versions fit into two lines:

set -o nounset -o pipefail -o errexit
IFS=$'\t\n' # Stricter IFS settings

vs.

set -euo pipefail
IFS=$'\t\n' # Stricter IFS settings

Note that I have not yet checked out the IFS setting in depth nor used it, but I have used set -o nounset -o pipefail -o errexit since about a year in Bash scripts and find it useful.

@vic
Copy link
Contributor

vic commented Oct 25, 2017

Yes, using the long version is better, thanks for pointing that.

@peterwwillis
Copy link

peterwwillis commented Mar 14, 2024

Fwiw, adding pipefail by default leads to a lot more problems than it solves. If a pipe fails, the script will exit without any error message about why (or where) the script exited. It's then up to a developer to find the script that failed, add debugging lines to identify the pipe that failed, and discover why it failed. (Using set -o xtrace does not emit information about what command failed or why; the output simply stops before the failed pipe, but without telling you which part of a pipe returned a failure or why)

OTOH, if you don't enable pipefail, you can write scripts that check output and explicitly validate the result of a command. This enforces validation of a command or state, and is a more graceful way to handle errors (makes debugging less annoying).


The 'strict' IFS shown above will break scripts that are designed for the default IFS, which includes a space first. This first character is used in parameter expansion.

In the linked article ("unofficial strict mode") the author is improperly using parameter expansion. They have this example block:

for name in ${names[@]}; do
  echo "$name"
done

Without double-quotes around the array-expanding variable, it will be expanded using IFS. If double-quotes enclose the variable, then each variable name expands to a separate word (spaces are preserved).

Shellcheck explains this:

$ shellcheck foo.sh

In foo.sh line 2:
for name in ${names[@]}; do
            ^---------^ SC2068 (error): Double quote array expansions to avoid re-splitting elements.
            ^---------^ SC2154 (warning): names is referenced but not assigned (did you mean 'name'?).

For more information:
  https://www.shellcheck.net/wiki/SC2068 -- Double quote array expansions to ...
  https://www.shellcheck.net/wiki/SC2154 -- names is referenced but not assig...

Leave IFS alone and do this instead:

for name in "${names[@]}" ; do
  echo "$name"
done

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

Successfully merging a pull request may close this issue.

5 participants