Add err_trap function

[Neepawa]

Signed-off-by: Neepawa github@groupbcl.ca

By submitting this pull request, I confirm the following

• I have read and understood the
  contributors guide,
  as well as this entire template.
• I have made only one major change in my proposed changes.
• I have commented my proposed changes within the code.
• I have tested my proposed changes, and have included unit tests where possible.
• I am willing to help maintain this change if there are issues with it later.
• I give this submission freely and claim no ownership.
• It is compatible with the EUPL 1.2 license
• I have squashed any insignificant commits.
  (git rebase)

---

What does this PR aim to accomplish?

Fix issue #2252 by providing useful messages to the user if a command run as part of the base-install.sh script faile.

How does this PR accomplish the above?

The PR implements a new function called err_trap, which run whenever an error occurs in a stand-alone command (that is, a commmand that is not part of a pipeline.) Output is similar to the following:

Error: a command failed

The command was:
  halt_and_catch_fire --flame_colour=blue --now

Output from the command:
| halt_and_catch_fire: missing /dev file of device to destroy

Call stack:
  The failed command was called in function 'foo' at line 1560
  Called from function 'bar' at line 2095
  Called from function 'install_main' at line 2681
  Called from function 'main' at line 2787

Experienced users can use this information to jump-start troubleshooting on problems. Inexperienced users can copy and paste the information to a help forum.

Implementation notes

• In order for the command's output to be displayed to the user, it must be redirected to file descriptor 4:
    tag_output; command --parameters >&4 2>&1

• File descriptor 4 is set up shortly after the installer starts.

• The tag_ouput call is needed to add __TAG__ to file descriptor 4. This is required because writes to file descriptors are appending, and the __TAG__ lines separate output between commands. Without it, when err_trap runs it would display the output of all prior commands in addition to the one it's reporting on.

• I added code to terminate the script if installPihole fails.

Minor cosmetic changes

• I corrected a couple of "its/it's" errors in the comments, and replaced a couple of instances of "setup" (a noun) with "set up" (the verb.) I resisted the urge to reduce very long comment lines to multiple shorter lines, given that's already being addressed in the "Cleanup of basic-install.sh" pull request.

What documentation changes (if any) are needed to support this PR?

None.

[stickler-ci]

rc3 appears unused. Verify it or export it.

[stickler-ci]

rc2 appears unused. Verify it or export it.

[Neepawa]

DCO issue is due to a misconfiguration on my end. Is there any way to fix it aside from killing my repo and reinstating it?

[dschaper]

Yes, have a look at https://github.com/pi-hole/pi-hole/wiki/Contributing-to-the-project for the directions on how to fix the signature.

[dschaper]

We're scheduled for a release coming up rather quickly. We can look again at this after the release and see about solving the merge conflict.

[DL6ER]

@dschaper Do you know the current status of this PR? The last comment is almost one year old.

[dschaper]

I don't have any other information about it.

[Neepawa]

Hi! I got distracted by some work on Python Markdown, then being released from my employment, then porting a 1980s-era DOS Point of Sale system to Linux.

If there's continued interest in this, I can close this pull request, fix up the conflicts against the most recent branch, and submit a new request. Or can I do that work on this PR?

[AzureMarker]

@Neepawa I really like the output and the level of debugging info this provides.

However, I'm a little concerned in how much work has to be done to trigger the code (lots of file redirects and manually calling tag_output). Maybe this could be minimized further?

We are also currently trying to figure out how best to evolve the installation process, and I don't want this work to be lost in that process. It might be best to wait until we know for certain the direction we want to go in before making too many changes to the installer.

@pi-hole/core-approvers should also chime in if they have any other thoughts.

[PromoFaux]

@pi-hole/core-approvers should also chime in if they have any other thoughts.

Sorry, this fell by the wayside again. I think the overall consensus was that it is a good addition to the code, there was just uncertainty on which direction we were headed with the install process. To be honest, I don't see any major changes happening for a while, any ideas we have floated internally are the "ideal" situation, and I personally don't think any of us really have the time to put into such a major overhaul of the install script at this time.

With that said:

@Neepawa Would you mind fixing up some of the merge conflicts so that we can test this against current 5.0 code?

[MichaIng]

@PromoFaux

I personally don't think any of us really have the time to put into such a major overhaul of the install script at this time.

I was reminded of this PR by some threads in the forum.

Currently, especially due to set -e, the installer can exit at various stages without printing any error message. The result is that in many cases users didn't even recognise that the installer/updater had an error and post their issues like "the web UI dashboard shows empty bars after update", which makes us usually thinks into the direction of missing PHP module or failed gravity.sh. The only way then to find out what exactly failed is to call the script with set -x which then revealed things like #3353. Debugging such cases can be very time consuming.

Of course one can try to handle all cases, check for the existence of every command before executing it, else print a reasonable error message, like suggested in #3209, but this would cause a very large coding effort (with every change again) and huge amount of code lines. An exit trap is actually the most common and "simple" solution for this.

Having that in mind, actually this PR is not the "major overhaul" it looks like. It just forwards commands output to a file and prints the content + call stack back when the command fails. It just looks like a large change because of the many changed code lines.

What I suggest is indeed implementing such an ERR trap, probably even an EXIT trap, which includes regular exit and interrupts (user hitting ctrl+c) but make any related code change (other then the trap definition) optional to avoid the need to implement it (correctly) for every single command and when doing any changes to the script.

The issue currently is that the trap prints the last entry of the implemented file descriptor without knowing if it was actually generated by the failed command or not. Probably it would be less intrusive to create an error handler function to use like err_handle command, which calls the given arguments as command, forwards output to a (new/local) tmp file or variable, and make the error trap only print a commands output if the tmp file or variable is actually set. This as well renders tag_output obsolete and the awk call to loop through the whole descriptor file + the need to keep an increasing file descriptor stored throughout the whole script. The call stack stays valid, within the trap it always matches the last called function name and related line number and the command error output is, when not explicitly redirected, anyway printed to screen. Often the exact command + position in code is pretty much enough to quickly debug things.

There are some code changes when a failed command should only lead to a function return code. Since those cases are already handled, with meaningful output or related actions, IMO calling err_trap is not required there, but could be by times thought through in individual cases. However removing those cases for now reduces the impression of a "major overhaul".

So final result would be:

1. If a command fails, and it is not masked by pipe or handled otherwise by if-then-else or || constructions, instead of exiting the script silently due to set -e, it will print the failed command name and call stack. The commands output is printed to console regularly, respectively as it was before based on stream redirection, BEFORE the output of the error trap.
   • Most importantly the user will get a clear info that something went wrong!
2. If a command is called via error handler function, it's output is omitted and instead, if the command fails, is instead printed as part of the error trap. This makes any change throughout the code optional, as the error handler does not more than merging error/command output and call stack into a cleaner formatted output unit. It also provides a starting point for adding more informational, debugging or solution steps into the error handler function in the future.
3. Optionally the ERR trap could be changed to an EXIT trap, which in bash includes INT (interruption i.e. ctrl+c) and every "regular" exit or script end. Might or might not be wanted to provide info about where the script stopped, even when it is done gracefully, but of course before the script ends completely successful, the EXIT trap would need to be unset 😉.

[PromoFaux]

Having that in mind, actually this PR is not the "major overhaul" it looks like.

I'll read this Wall Of Text™ (😉) properly when I have some time later, but just to clarify, I was not talking about this PR when I was mentioning major overhaul. I agree in principle with the PR and think it is a good idea.

The major overhaul would be some ideas that have been floated about internally for the direction the install script should head in, but like I say, I don't see it coming to fruition in the near future. So IMHO this PR can land (provided merge conflicts are tidied up)

[MichaIng]

Ah okay, sorry for misunderstanding and sorry for the wall of text 😅. The implementation changes above should be reviewed by @Neepawa anyway, probably I've overseen something. Making the actual redirect and print of commands output optional for the error trap to function, is generally a good thing IMO, so there is no strict need to wrap any code around every command call.

[DL6ER]

Closing due to inactivity. Feel free to reopen if you want to revive this.