#700 Issue closed: make rear working with "set -ue -o pipefail"¶

Labels: enhancement, cleanup, won't fix / can't fix / obsolete

jsmeix opened issue at 2015-11-18 16:15:¶

In the future rear should work with ''set -ue -o pipefail".

See
https://github.com/rear/rear/wiki/Coding-Style
that reads (excerpt):

TODO Use set -eu to die from unset variables and unhandled errors

See also
https://github.com/rear/rear/pull/699
and
https://github.com/rear/rear/issues/688

gdha commented at 2015-11-19 13:35:¶

@jsmeix just merged the pull request. OK - it is testing time...

jsmeix commented at 2015-11-20 09:14:¶

To avoid exaggerated expectations:
Currently only the 'help' workflow should work with 'set -ue'
all other workflows do not yet work with 'set -ue'
(cf. https://github.com/rear/rear/pull/699).

gdha commented at 2015-11-20 09:42:¶

@jsmeix do not forget to update the man page as well with the updated arguments. Thank you for the hard work. Gratien

jsmeix commented at 2015-11-20 13:53:¶

Updating the man page (and other documentation as needed) regarding '--debugscripts' belongs to https://github.com/rear/rear/issues/688 that I reopened because of that missing documentation.

jsmeix commented at 2015-11-27 14:46:¶

I have an idea how I could make rear working with ''set -ue -o pipefail"
step by step:

I can do it script by script.

For each script I can add at the beginning a line

set -ue -o pipefail

and at the end a line

set +ue +o pipefail

to make that one working with 'set -ue -o pipefail'.

The advantage of having 'set -ue -o pipefail' hardcoded in the
scripts is that it ensures that subsequent changes are also
implemented so that it still works with 'set -ue -o pipefail'.

And scripts where I cannot test them (e.g. scripts for using
third-party backup tools that I do not have) can be left as is.

I will start with "my" scripts for btrfs support to make them
working with 'set -ue -o pipefail'.

schlomo commented at 2015-11-28 17:18:¶

@jsmeix great idea! That way it is also visible in that script that those settings are active. Maybe less of a surprise for the next developer to work on a script.

schlomo commented at 2015-11-28 17:21:¶

One other thought. By disabling those settings at the end of a script you will also disable them if they are enabled globally. Maybe you write a function to remember the settings and to return them to the remembered state?

In general it might be useful to have functions for those settings so that we can change them centrally or extend them with tracking code and other stuff.

jsmeix commented at 2015-11-30 09:38:¶

I agree.

I found
http://unix.stackexchange.com/questions/210158/how-can-i-list-bashs-options-for-the-current-shell

Accordingly it seems the right way to save and restore bash flags and options is

# Save current flags and option settings as bash commands:
bash_flags_and_option_settings=$( set +o | tr '\n' ';' ; shopt -p | tr '\n' ';' )
# Change flags and options as one likes:
...
# Restore bash flags and option settings:
eval $bash_flags_and_option_settings

I have tested it a little bit and it seems to work (at least for me).

To provide that via generic functions

function save_bash_flags_and_option_settings () { ... }
function restore_bash_flags_and_option_settings () { ... }

it must be save against nested function calls which means
I need to implement a stack of bash_flags_and_option_settings
variables where the states are stored.

jsmeix commented at 2015-11-30 09:57:¶

It seems Rear does not yet have a stack implemented.

Via
http://brizzled.clapper.org/blog/2011/10/28/a-bash-stack/
I found
https://github.com/bmc/lib-sh
where
https://github.com/bmc/lib-sh/blob/master/LICENSE.md
reads that it is "under a BSD license".

@schlomo
could you check if their license is o.k. so that it could be used in rear?

jsmeix commented at 2015-11-30 10:09:¶

Or avoid the stack and use local variables in place like

# Save current flags and option settings:
flags_and_options=$( get_bash_flags_and options )
# Change flags and options as one likes:
...
# Restore bash flags and option settings:
set_bash_flags_and options "$flags_and_options"

that call complementary global stateless functions

function get_bash_flags_and_options () { set +o | tr '\n' ';' ; shopt -p | tr '\n' ';' ; }
function set_bash_flags_and_options () { eval $1 ; }

jsmeix commented at 2015-11-30 10:52:¶

Or don't use a generic stack but a simple array that is sufficient
only for this particular use case?

I mean a global array like bash_flags_and_options_array
that contains bash_flags_and_options values
plus complementary global stateful functions
save_bash_flags_and_option_settings and
restore_bash_flags_and_option_settings
that operate on that global array.

I.e. like currently EXIT_TASKS, AddExitTask, RemoveExitTask.

jsmeix commented at 2015-12-02 10:26:¶

I experimented with using implicitly a stack (i.e. an array) of saved_flags_options_cmds via save_flags_and_options and restore_flags_and_options functions but its usage was not sufficient for my needs.

In particular I like to be able to restore an explicity named state.

In the end in https://github.com/rear/rear/pull/726 I implemented simple functions get_bash_flags_and_options_commands and apply_bash_flags_and_options_commands that are used via an explicit variable that stores the current bash flags and options commands and using that explicit variable to re-apply them, i.e. like

      saved_flags_options_cmds="$( get_bash_flags_and_options_commands )"
      ... [change bash flags and options] ...
      ... [do something] ...
      apply_bash_flags_and_options_commands "$saved_flags_options_cmds"

jsmeix commented at 2015-12-03 10:02:¶

With https://github.com/rear/rear/pull/726 the precondition for https://github.com/rear/rear/issues/700#issuecomment-160153205 is implemented so that now one can make a particular piece of code (e.g a script) working with ''set -ue -o pipefail" as follows:

local saved_bash_flags_and_options_commands="$( get_bash_flags_and_options_commands )"
set -ue -o pipefail
... [code that works with "set -ue -o pipefail"] ...
apply_bash_flags_and_options_commands "$saved_bash_flags_and_options_commands"

gdha commented at 2016-02-09 13:55:¶

@jsmeix can this issue be closed?

jsmeix commented at 2016-02-09 14:10:¶

Current rear does not at all work with ''set -ue -o pipefail".

There are zillions of places where the code
is not yet safe against "set -ue -o pipefail".

I assume this issue will stay open for a very long time.
But on the other hand there is no time pressure.

I set the "Milestone" to "Rear future".

jsmeix commented at 2017-04-28 08:49:¶

Only FYI
see https://github.com/rear/rear/pull/1334
which is an interesting example how complicated things
could get if one likes to use bash arrays with 'set -ue'.

Because of this I meanwhile wonder if 'set -ue' would be really
a good thing for the future because it seems bash with 'set -ue'
works very much against what is "usually expected to work" in bash.
In other words:
I wonder if bash code with 'set -ue' may abort too often
not because of real issues but because of "false alarm"
that result from nitpicking stuff in bash when 'set -ue' is set?

gdha commented at 2017-04-28 08:54:¶

@jsmeix I fully agree with you Johannes - less is sometimes better

jsmeix commented at 2017-04-28 09:38:¶

At least for now (unless others object)
I set the "won't fix" label to this issue
but I will keep it open for some time
so that others could provide more feedback.

Personally I am unsure:

Obviously 'set -ue' enforces programmers to write code
that works more stable because they must care about errors.

On the other hand 'set -ue' may fail more often for users
because bash (as any interpreted programming language)
fails basically only during runtime ('bash -n script.sh' is
insufficient, cf. https://github.com/rear/rear/issues/1040)
and all cases where 'set -ue' did not fail for programmers
causes additional failures for users.

On the third ;-) hand those additional failures for users
with 'set -ue' would be reported to ReaR upstream
so that in the end with 'set -ue' ReaR could work much
better (mainly much more reliably) than it works now.

schlomo commented at 2017-04-28 10:03:¶

IMHO ReaR reached a size and complexity where such strict coding styles will really help us to maintain and maybe even increase our quality.

If we can't enable it globally we might add it as a development mode where we maintainers run our tests with strict mode to harden ReaR.

jsmeix commented at 2017-04-28 10:17:¶

My proposal was to use 'set -ue' in new scripts,
cf. "Try to care about possible errors" in
https://github.com/rear/rear/wiki/Coding-Style

Probably also when scripts are so much overhauled
that they got basically rewritten.

Regarding "maybe even increase quality":

I got used to use 'set -eu -o pipefail' when I made
my own generic system installation scrits, see the section
"Generic usage of the plain SUSE installation system ..."
in https://en.opensuse.org/SDB:Disaster_Recovery

My own experience when I use 'set -eu' from the beginning
is that I think my scripts become very much better
because I am enforced to care all the time about
any kind of possible errors (up to nitpicking stuff in bash)
but - at least from my personal point of view - the result
works so much better compared to "usual bash scripting".

jsmeix commented at 2017-04-28 10:33:¶

I think
https://github.com/rear/rear/issues/1327#issuecomment-297964501
is perhaps a good example how in practice 'set -ue' may work
too much against what is "usually expected to work" in bash
which may let ReaR fail later in perhaps very unexpected ways.

jsmeix commented at 2017-09-07 10:07:¶

Because some scripts already use "set -e -u -o pipefail"
we need to implement some verbose error exit trap
for cases where bash exits because of "set -e -u"
so that the user is sufficiently informed what happened
because by default bash exits silently in case of "set -e -u"
and when the user does not actively check the exit code
it would look like a regular finishing program.

jsmeix commented at 2017-09-20 16:15:¶

With https://github.com/rear/rear/pull/1507 merged
a failure message of the form "rear $WORKFLOW failed ..."
is shown when it exits in any unintended way, in particular
when it exits at an arbitrary command because of 'set -e'.

jsmeix commented at 2018-03-07 10:36:¶

Probably https://github.com/rear/rear/issues/1747 is another good example
why "set -e -u -o pipefail" can result more problems in practice than it solves.

Meanwhile I am thinking about a generic wrapper function
for "set -e -u -o pipefail" that only sets it when in debugging mode
cf. https://github.com/rear/rear/issues/700#issuecomment-297960017
so that in practice for the users out there who do not run ReaR
in debugging mode there are no such unexpected "sudden death"
bash error exits because of any minor (perhaps even irrelevant) stuff
cf. https://github.com/rear/rear/issues/700#issuecomment-297944196
and https://github.com/rear/rear/issues/700#issuecomment-297965594

jsmeix commented at 2018-03-08 10:37:¶

https://github.com/rear/rear/commit/32eafc491f793e5b8a510ff8f4219ff9be2a7edf
is another case where an unconditioned "set -e -u -o pipefail"
caused more problems in practice (i.e. problems for ReaR users)
than it may solve in theory.

jsmeix commented at 2022-10-05 13:23:¶

https://github.com/rear/rear/issues/2871
is a nice example where set -eu -o pipefail
(therein in particular the set -e part)
would hinder the user to do some "dirty hacks"
to make ReaR behave as he needs it in his specific case.

---

[Export of Github issue for rear/rear.]