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

Provide a third state to strict which omits set -e #352

Closed
halostatue opened this issue Feb 28, 2023 · 10 comments
Closed

Provide a third state to strict which omits set -e #352

halostatue opened this issue Feb 28, 2023 · 10 comments
Labels
enhancement New feature or request

Comments

@halostatue
Copy link

Description

I usually write my shell scripts without set -e, because exits too quietly. Instead, I have a pair of standard functions (fail-unless and halt) which let me control flow much better and provide better messaging. I find set -e dangerous precisely because it turns harmless failures like the example below into silent halts.

[[ "${args["--force"]} ]] || return

I was debugging a script yesterday that was halting for unexplained reasons that became clear when I saw the generated script included set -e. I was able to resolve this with set +e, but when I use these, I prefer to use set +e -o pipefail (I might include -u, but I generally declare every variable that I use).

IMO, the states could be:

  • strict: true: set -euo pipefail
  • strict: false: set -eo pipefail
  • strict: disabled: set -o pipefail
  • strict: manual: omitted entirely; could be none, but manual declares that I, as the script writer, will take the responsibility for those settings myself.

I do think that set -o pipefail should be part of the written defaults all the time; there’s almost never a time when you want to hide individual pipe command failures, but I’m less bothered about that.

@halostatue halostatue added the enhancement New feature or request label Feb 28, 2023
@DannyBen
Copy link
Owner

DannyBen commented Feb 28, 2023

Well, thanks, but I have several issues with this proposal.

Issue 1: set -e is needed by bashly.

The code generated by bashly itself (i.e. not user code) relies on set -e to be set. In my experience, when you need it to NOT be set, it is a code smell that has multiple solutions other than calling set +e.

In your example, the problem is not with the variable test [[ "${args["--force"]}" ]], but rather with the fact that you just use return instead of return 0.

This minor tweak makes it work without disabling set -e:

#!/usr/bin/env bash
declare -A args
set -e

func() {
  [[ "${args["--force"]}" ]] || return 0
  echo "--force is set, continuing to do something"
}

# args["--force"]='1'
func
echo "end of script"

Also additional tip: Personally, I try to avoid shortcuts that look cool and compact. Be explicit:

func() {
  # instead of this...
  # [[ "${args["--force"]}" ]] || return 0

  # ...do this
  if [[ -z "${args["--force"]}" ]]; then
    return 0
  fi
  
  echo "--force is set, continuing to do something"
}

Issue 2: Confusing options.

Having a setting that can accept true, false, disabled, or none is confusing, as three of these words are synonyms to false. I do not like manual.

Issue 3: This is overridable in user-land.

Finally, if someone for some reason does not care about my first argument, they can always override it as they see fit in the initialize.sh. Of course this will result in set -e followed immediately by set +e, which - for someone who reads the script - might seem awkward, but it should serve as a sign that this should not be done.

It is ok to do this for debugging purposes of course, but I would not recommend leaving the final resulting script with it.


Bashly itself tries to stay out of your way, but still nudge you towards good standards and practices.
In my opinion, set -e is a bare minimum safety measure - disabling it globally may cause your generated script to behave unexpectedly.

@halostatue
Copy link
Author

I disagree about the benefits of set -e; as I noted, I had silent early termination which was substantially harder to debug than if the script had continued.

The BashFAQ also indicates that set -e provides a tissue instead of a safety blanket: https://mywiki.wooledge.org/BashFAQ/105 because there’s a lot of corner cases. This is one of the reasons why I have explicitly not used set -e on all but the simplest of shell scripts but instead explicitly check for errors on lines where failure or success matters.

If set -e cannot be removed from Bashly safely, then I’ll probably have to make a prefix/suffix for all of my functions to disable/enable it for anything that is in my code, because I find set -e unusable and untrustworthy. (And yes, I use a lot of subshell calls, which is one of the places where set -e fails fairly hard.)

I agree with you about the confusing options; I do think that some other configuration option for this would be better.

@DannyBen
Copy link
Owner

Even in the link you posted, the very last Conclusion paragraph states two opposite opinions. On says use it, the other don't.

The set -e option in bash is the closest thing to how all other languages operate - when there is an error, exit. Non zero return code in bash is error.

That said - bashly should not fight you if you feel otherwise.
I will execute all specs without set -e first, see if things pass. If they do, then the door may be open to allowing users to set whatever they want.

If you can take a look anywhere in the code (either the bashly source code, or your generated code) and see if there are things that might fail with set +e, it would help expedite resolving this.

@DannyBen
Copy link
Owner

Specs pass with set +e. So I suggest you set this in your initialize.sh, and see if you have any issues.
If you do, please report them.

@halostatue
Copy link
Author

OK. Thanks for your thoughts on this.

@DannyBen
Copy link
Owner

Lets keep this open. I need to sleep on it, perhaps allow a string value in strict so you can set an empty string or whatever else you want.

@DannyBen
Copy link
Owner

DannyBen commented Mar 1, 2023

I have changed the strict setting to allow any string. This does not change how the true or false values work.

Can you take a look and see if it covers your use case in a satisfactory manner?

@halostatue
Copy link
Author

Looks great. Thanks again for such a useful project.

@DannyBen
Copy link
Owner

DannyBen commented Mar 1, 2023

Excellent. I have built an edge docker, assuming you can use it (or the GitHub version) until next release (instructions).

Release will be coming soon.

@DannyBen
Copy link
Owner

DannyBen commented Mar 3, 2023

Released in 1.0.1

@DannyBen DannyBen closed this as completed Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants