-
Notifications
You must be signed in to change notification settings - Fork 27
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
error not caught #224
Comments
I am pretty sure we need I think REPORT_TRAP_ERR='yes' and FAIL_TRAP_ERR='yes' should be enabled by default anyhow. Even without any options needed. That's a good style. More reliable, secure. What do you think? Otherwise conditionally enabling Could you advice please so I prepare a pull request? |
Pretty sure the |
That helps.
Error was caught. (Created #225 for the underlying error.) |
I'm aware that there are two kind of folks: one that always prefer Nowadays I personally tend to write scripts with But adding / enabling
AFAICS this is what you implemented in #226 |
Enable `set -e` if environment variables `REPORT_TRAP_ERR` or `FAIL_TRAP_ERR` are set to `yes`. related to grml#224
Interesting.
Lucky we're on the same page here. I also remorse that I haven't written all my scripts with pipefail and nounset from the beginning.
I think strict error handling and failing early is more important and doubt there will be many failures. And the only way to fix these issues that I can see is to let it fail and fix it. The uses of For the Kicksecure, Whonix build script we've been using grml-debootstrap for raw VM image builds with However, my blind spot here is that I am not using grml-debootstrap for non-VM builds and not in interactive mode. Related topic: So what I could suggest is that I attempt to rework the error handling, make sure that VMs build. Would it be feasible for you to test the "other" builds and fix these new issues? Any failing command would probably be easily fixed with appending |
[...]
ACK, I try to use
Fine for me, as long it's not only myself having to do the heavy lifting :)
ACK
Ah, that's very interesting feedback, which certainly removes much of my reluctant feelings towards it and makes me feel positive towards enabling
I personally don't use the interactive mode neither that often, though care very much about non-VM builds. Related topic:
nod
Yes, that sounds a like a good plan to me! Count me in :) |
I am a bit unsure about the If is in two cases, either:
In case of A), one would probably want "strict error handling". I suppose, if some command unexpectedly exits non-zero, one would want function to abort and the In case of B), I suppose one probably wants "relaxed error handling". That is, if some command exits non-zero, one would want to see that command failing, however the Need some feedback on this one. Happy to do something simpler. It will might become more clear once I push the pull request. As for execution of the stages... The following would be a bug...
For deminstration:
output:
If we want the error handler to be triggered and abort as quickly as possible, we need to avoid |
Why this is happening is I need to investigate. Could be a localized issue. However, I can see REPORT_TRAP_ERR and FAIL_TRAP_ERR being
yes
. Yet, the error handler does not fire. The error is only caught later but not thorugh theerror_handler
.The text was updated successfully, but these errors were encountered: