More fixes for error handling #103
Conversation
mathias-luedtke
force-pushed
the
error-handling
branch
from
af884e7
to
0aaac72
Compare
|
now it was easy to implement an exit code check (supersedes #80) |
mathias-luedtke
force-pushed
the
error-handling
branch
4 times, most recently
from
3daabfa
to
a1276d1
Compare
|
Perhaps |
mathias-luedtke
force-pushed
the
error-handling
branch
from
69d6893
to
07180a2
Compare
|
rebased |
mathias-luedtke
force-pushed
the
error-handling
branch
4 times, most recently
from
192f538
to
cdb717c
Compare
| if [ "$DEBUG_BASH" ] && [ "$DEBUG_BASH" == true ]; then set -x; fi | ||
| if [ $exit_code -ne "-1" ]; then exit $exit_code; fi | ||
| if [ $exit_code -ne "-1" ]; then ici_exit $exit_code; fi |
There was a problem hiding this comment.
Depending on situation, calling this function can fail where ici_exit is not yet loaded.
There was a problem hiding this comment.
I cannot think of a situation like this, do you have an example?
I can move ici_exitto the top if you prefer it..
There was a problem hiding this comment.
I don't have a usecase for now, but only because this file is "util" we better address all the possibility we can think of.
Moving ici_exit sounds good to me.
There was a problem hiding this comment.
I guess ici_exit will be refactored to become the exit handler
mathias-luedtke
changed the title
|
Do not merge yet, I'd like to add error tracing and the error function needs to get modified for exit code propagation |
| @@ -86,15 +86,14 @@ function _end_fold_script { | |||
| color_wrap=${2:-32} | |||
|
|
|||
| if [ $exit_code -eq "1" ]; then color_wrap=31; fi # Red color | |||
| if [ -z $TRAVIS_FOLD_NAME ]; then | |||
| if ! [ -z $TRAVIS_FOLD_NAME ]; then | |||
There was a problem hiding this comment.
If this change is intended, the following else block may no longer make sense.
There was a problem hiding this comment.
It is intended, but the else block still makes sense.
if ! [ -z $TRAVIS_FOLD_NAME ] -> if $TRAVIS_FOLD_NAME is not empty -> if fold is open
This is the bug I did not find in #100
There was a problem hiding this comment.
Ah, I see. Turned out I've been consistently misunderstanding if -z option.
There was a problem hiding this comment.
if [ -n $TRAVIS_FOLD_NAME ] ("not empty") might be easier to understand ^^
| @@ -52,7 +52,7 @@ export HIT_ENDOFSCRIPT=false | |||
|
|
|||
| source ${ICI_PKG_PATH}/util.sh | |||
|
|
|||
| trap error ERR | |||
| trap error EXIT | |||
There was a problem hiding this comment.
Do we want to create a new function exit? Isn't trapping EXIT with error function misleading?
There was a problem hiding this comment.
Good idea, will add this later
There was a problem hiding this comment.
Thanks for tracking this down further! Seems like a tough one (reviewing this was too)!
Does this change require to change .travis.yml in users' repos, from like source .ci_config/travis.sh to .ci_config/travis.sh
(regardless, I agree in readme we no longer need to maintain the backward compatibility with the source usage)?
Perhaps
AFTER_SCRIPTshould only be executed insource_tests.sh(likeBEFORE_SCRIPT).
Sorry but I can't remember why BEFORE_SCRIPT is run from source_tests.sh. Being called from ci_main.sh is more intuitive isn't it?
No,
This way it is called after ROS and workspace set-up.
I guess this will change with #98 anyway: It works for now, we should not touch it for now. |
|
I see. +1 from me. |
mathias-luedtke
force-pushed
the
error-handling
branch
2 times, most recently
from
907db28
to
9c39d51
Compare
mathias-luedtke
force-pushed
the
error-handling
branch
5 times, most recently
from
f33622a
to
e793265
Compare
mathias-luedtke
changed the title
mathias-luedtke
force-pushed
the
error-handling
branch
2 times, most recently
from
963ff9e
to
285b3ee
Compare
|
Error-code related refactoring completed :) |
mathias-luedtke
force-pushed
the
error-handling
branch
from
285b3ee
to
9076329
Compare
| if [ -n $1 ]; then | ||
| echo "\e[31m$1\e0m" # print error in red | ||
| fi | ||
| if [ "$exit_code" == "0" ]; then # 0 is not error |
There was a problem hiding this comment.
I assume this if block is added to handle mis-use? If so printing so would be friendly to the users?
There was a problem hiding this comment.
exit_code is read from $?, which might be 0 in some cases, like if ! false ; then echo "$?"; fi.
So the following code works now as expected:
if ! [ -e "$my_path" ]; then
error "path does not exist: '$my_path'"
fiI think, the documentation needs to be updated..
There was a problem hiding this comment.
Alternative implementation, with exit_code passing:
test -e "$my_path" || error "path does not exist: '$my_path'"
| if [ $exit_code -ne "-1" ]; then exit $exit_code; fi | ||
| if [ "$exit_code" == "${EXPECT_EXIT_CODE:-0}" ]; then | ||
| exit 0 | ||
| elif [ "$exit_code" == "0" ]; then # 0 was not expected |
There was a problem hiding this comment.
There was a problem hiding this comment.
No, this is another use case.
If exit_code != EXPECT_EXIT_CODE then the exit_code will be used.
However, If exit_code is 0, but EXPECT_EXIT_CODE is not 0, the exit_code can not be returned as 0 would mean success.
|
Left a minor comment.
That looks convenient! |
|
If you do not object, I will merge this PR tomorrow morning (= in 10 hours) :) |
It turned out that #100 did not fix #99 completely.
error()did exit the travis-generated script as well.Lesson learned: never
exitfrom asourced script.So
ci_main.shdoes not get sourced anymore; environment is no longer exposed.That's why I have added
AFTER_SCRIPT.Please note: Additional scripts added in .travis.yml do not get executed in docker container!
Travis executes all
script:lines regardless of the exit code, but marks the build as failed if one line failed.However, this might change: travis-ci/travis-ci#1066 (comment)