#1970 PR merged: Let Source function return exit code of source call (related to issue 1965)

Labels: enhancement, cleanup, fixed / solved / done

jsmeix opened issue at 2018-11-19 14:12:

First and foremost in general
no piece of code that does not finish successfully
should lie to its caller and return a success code
regardless if a caller of a piece of code
cares about the return code.

So now the Source function in lib/framework-functions.sh
returns the the return value of its actual work which is the
return value of its source $source_file call so that the caller
of the Source function can now decide what to do.

Now there is a log entry when source $source_file fails.
For usr/sbin/rear -D mkrescue I get those new messages in my log:

Source function: 'source /root/rear.github.master/usr/share/rear/init/default/950_check_missing_programs.sh' returns 1
Source function: 'source /root/rear.github.master/usr/share/rear/prep/NETFS/default/040_inspect_configuration_files.sh' returns 1
Source function: 'source /root/rear.github.master/usr/share/rear/prep/default/050_check_keep_old_output_copy_var.sh' returns 1
Source function: 'source /root/rear.github.master/usr/share/rear/prep/GNU/Linux/200_include_getty.sh' returns 1
Source function: 'source /root/rear.github.master/usr/share/rear/build/default/950_check_missing_programs.sh' returns 1
Source function: 'source /root/rear.github.master/usr/share/rear/output/default/970_remove_lock.sh' returns 1

Additionally I cleaned up an unrelated and minor annoyance
that I have on my personal todo list since a very long time:
In debugscript mode one gets much useless 'set -x' debug output
in the log for the apply_bash_flags_and_options_commands call
in the Source function where basically only set +x is done like

2018-11-19 14:38:07.839205310 Including init/default/005_verify_os_conf.sh
2018-11-19 14:38:07.839968307 Entering debugscripts mode via 'set -x'.
+ source /root/rear.github.master/usr/share/rear/init/default/005_verify_os_conf.sh
++ [[ ! -f /root/rear.github.master/etc/rear/os.conf ]]
++ echo OS_VENDOR=SUSE_LINUX
++ echo OS_VERSION=15.0
++ Log 'Created the /root/rear.github.master/etc/rear/os.conf file with content:'
+++ date '+%Y-%m-%d %H:%M:%S.%N '
++ local 'timestamp=2018-11-19 14:38:07.843115791 '
++ test 1 -gt 0
++ echo '2018-11-19 14:38:07.843115791 Created the /root/rear.github.master/etc/rear/os.conf file with content:'
2018-11-19 14:38:07.843115791 Created the /root/rear.github.master/etc/rear/os.conf file with content:
++ cat /root/rear.github.master/etc/rear/os.conf
OS_VENDOR=SUSE_LINUX
OS_VERSION=15.0
+ test 1
+ Debug 'Leaving debugscripts mode (back to previous bash flags and options settings).'
+ test 1
+ Log 'Leaving debugscripts mode (back to previous bash flags and options settings).'
++ date '+%Y-%m-%d %H:%M:%S.%N '
+ local 'timestamp=2018-11-19 14:38:07.844718486 '
+ test 1 -gt 0
+ echo '2018-11-19 14:38:07.844718486 Leaving debugscripts mode (back to previous bash flags and options settings).'
2018-11-19 14:38:07.844718486 Leaving debugscripts mode (back to previous bash flags and options settings).
+ apply_bash_flags_and_options_commands 'set +o xtrace;set +o vi;set +o verbose;set +o privileged;set +o posix;set +o 
pipefail;set +o physical;set +o onecmd;set +o nounset;set +o notify;set +o nolog;set +o noglob;set +o noexec;set +o 
noclobber;set +o monitor;set +o keyword;set -o interactive-comments;set +o ignoreeof;set +o history;set +o 
histexpand;set -o hashall;set +o functrace;set +o errtrace;set +o errexit;set +o emacs;set -o braceexpand;set +o 
allexport;shopt -u autocd;shopt -u cdable_vars;shopt -u cdspell;shopt -u checkhash;shopt -u checkjobs;shopt -s 
checkwinsize;shopt -s cmdhist;shopt -u compat31;shopt -u compat32;shopt -u compat40;shopt -u compat41;shopt -u 
compat42;shopt -u compat43;shopt -s complete_fullquote;shopt -u direxpand;shopt -u dirspell;shopt -u dotglob;shopt -u 
execfail;shopt -u expand_aliases;shopt -u extdebug;shopt -s extglob;shopt -s extquote;shopt -u failglob;shopt -s 
force_fignore;shopt -u globasciiranges;shopt -u globstar;shopt -u gnu_errfmt;shopt -u histappend;shopt -u 
histreedit;shopt -u histverify;shopt -s hostcomplete;shopt -u huponexit;shopt -u inherit_errexit;shopt -s 
interactive_comments;shopt -u lastpipe;shopt -u lithist;shopt -u login_shell;shopt -u mailwarn;shopt -u 
no_empty_cmd_completion;shopt -u nocaseglob;shopt -u nocasematch;shopt -s nullglob;shopt -s progcomp;shopt -s 
promptvars;shopt -u restricted_shell;shopt -u shift_verbose;shopt -s sourcepath;shopt -u xpg_echo;'
+ eval 'set +o xtrace;set +o vi;set +o verbose;set +o privileged;set +o posix;set +o pipefail;set +o physical;set +o 
onecmd;set +o nounset;set +o notify;set +o nolog;set +o noglob;set +o noexec;set +o noclobber;set +o monitor;set +o 
keyword;set -o interactive-comments;set +o ignoreeof;set +o history;set +o histexpand;set -o hashall;set +o 
functrace;set +o errtrace;set +o errexit;set +o emacs;set -o braceexpand;set +o allexport;shopt -u autocd;shopt -u 
cdable_vars;shopt -u cdspell;shopt -u checkhash;shopt -u checkjobs;shopt -s checkwinsize;shopt -s cmdhist;shopt -u 
compat31;shopt -u compat32;shopt -u compat40;shopt -u compat41;shopt -u compat42;shopt -u compat43;shopt -s 
complete_fullquote;shopt -u direxpand;shopt -u dirspell;shopt -u dotglob;shopt -u execfail;shopt -u 
expand_aliases;shopt -u extdebug;shopt -s extglob;shopt -s extquote;shopt -u failglob;shopt -s force_fignore;shopt -u 
globasciiranges;shopt -u globstar;shopt -u gnu_errfmt;shopt -u histappend;shopt -u histreedit;shopt -u histverify;shopt 
-s hostcomplete;shopt -u huponexit;shopt -u inherit_errexit;shopt -s interactive_comments;shopt -u lastpipe;shopt -u 
lithist;shopt -u login_shell;shopt -u mailwarn;shopt -u no_empty_cmd_completion;shopt -u nocaseglob;shopt -u 
nocasematch;shopt -s nullglob;shopt -s progcomp;shopt -s promptvars;shopt -u restricted_shell;shopt -u 
shift_verbose;shopt -s sourcepath;shopt -u xpg_echo;'
++ set +o xtrace
2018-11-19 14:38:07.847582948 Including init/default/010_set_drlm_env.sh

Now the apply_bash_flags_and_options_commands call is silenced
so that the same looks now in the log like

2018-11-19 14:39:51.917100357 Including init/default/005_verify_os_conf.sh
2018-11-19 14:39:51.918023455 Entering debugscripts mode via 'set -x'.
+ source /root/rear.github.master/usr/share/rear/init/default/005_verify_os_conf.sh
++ [[ ! -f /root/rear.github.master/etc/rear/os.conf ]]
+ source_return_code=0
+ test 0 -eq 0
+ test 1
+ Debug 'Leaving debugscripts mode (back to previous bash flags and options settings).'
+ test 1
+ Log 'Leaving debugscripts mode (back to previous bash flags and options settings).'
++ date '+%Y-%m-%d %H:%M:%S.%N '
+ local 'timestamp=2018-11-19 14:39:51.932436117 '
+ test 1 -gt 0
+ echo '2018-11-19 14:39:51.932436117 Leaving debugscripts mode (back to previous bash flags and options settings).'
2018-11-19 14:39:51.932436117 Leaving debugscripts mode (back to previous bash flags and options settings).
2018-11-19 14:39:51.935978274 Including init/default/010_set_drlm_env.sh

schlomo commented at 2018-11-19 14:21:

What would you propose us to do with the information, that a sourced script returned an error? Is this an intermediate step towards failing on such errors or this this an intermediate step towards ignoring them?

Maybe better turn the Log into a Debug so that it won't bother during regular usage.

Long term, do you see it as an error or a bug if a script returns an error? In the meaning of "unhandled exceptions"?

jsmeix commented at 2018-11-19 14:21:

@gdha @schlomo
I would like to know what you think about my proposed change here.

Currently nothing errors out because of my proposed change here.
We only get a log message and a useful return code from the Source function
that we may or may not use as needed at each particular place where
the Source function is called.

jsmeix commented at 2018-11-19 14:25:

@schlomo
thanks for your input - I agree Debug is better than Log here.

jsmeix commented at 2018-11-19 14:41:

I fear in the foreseeable future it can neither be an error nor a bug
if a ReaR script finishes with a non-zero return code because
such cases "just happen" too often by "usual bash scripting".

If at all I think this would be a bug because I think the code of a script
in ReaR should never finish with a failed last command - if it did
I would consider it as an unhandled case (i.e. missing code for that case).

The basic severe problem (severe for our users) with any kind of
automated error exit in ReaR if something failed is the same as in
https://github.com/rear/rear/issues/700

This means my new useful return code of the Source function
can in practice not be used often - only in some specific cases,
e.g. https://github.com/rear/rear/pull/1969#issuecomment-439907344
But this does not mean it would be bad or wrong when the Source function
now returns a return code that could be even useful in some specific cases ;-)

gdha commented at 2018-11-20 07:26:

@jsmeix I'm fine with it. We will see how it turns out after a while using it (in good and bad cases).

[Export of Github issue for rear/rear.]