[ci] removed need to explicitly exit from failed commands in CI

[jameslamb]

Per this comment, in this PR I'd like to propose that we change .ci/test.sh in a way that should reduce the maintenance burden.

Currently, we explicitly add an exit to many stages to ensure builds fail, e.g. do_something() || exit -1. I propose that, instead, we just use set -e to say "exit immediately with a non-zero exit code when any process called from this script fails". I think this will be less error-prone and easier to maintain.

I also propose we use set -e in .ci/setup.sh just so that jobs stop running and fail immediately if anything in setup fails.

Opening the PR for discussion.

[StrikerRUS]

@jameslamb I guess that this is unacceptable for setup.sh because sometimes some software may be already installed. See current Travis failures.

[jameslamb]

@jameslamb I guess that this is unacceptable for setup.sh because sometimes some software may be already installed. See current Travis failures.

Oh interesting. In my opinion it's a poor design choice by those package managers to make "will not re-install because this is already installed" return a non-0 exit code.

But either way:

1. I learned something today 😀
2. I'll revert the change to setup.sh

[StrikerRUS]

@jameslamb As you might see they even hang forever and reach 60-minutes limit for no activity from the Travis side. Really quite weird behavior.

Thanks for removing set -e from setup.sh. I think we may want to have more complicated decision for it (or leave everything as-is 🙂 ) . I'll leave my review a little bit later, sorry.

[jameslamb]

@jameslamb As you might see they even hang forever and reach 60-minutes limit for no activity from the Travis side. Really quite weird behavior.

Thanks for removing set -e from setup.sh. I think we may want to have more complicated decision for it (or leave everything as-is 🙂 ) . I'll leave my review a little bit later, sorry.

@StrikerRUS I don't think the "hang forever for no activity" is necessarily related to this PR. I believe Travis puts some limits on the total number of jobs that can run for an account at a given time, and remember that I pushed this PR alongside two others all within about a minute.

The most recent build had this build time, which doesn't seem notably different from our usual:

I just restarted that build to see if I can reproduce the hanging stages.

But ultimately...if you are not comfortable with these changes after another review we can close the PR.

[StrikerRUS]

@jameslamb

But ultimately...if you are not comfortable with these changes after another review we can close the PR.

I'm absolutely comfortable with the proposed changes! I'm sorry if I confused you by any my previous comment.

However, I think we need +1 reviewer for this PR.
Just some refs to cover the opposite point of view and dispel the impression of silver bullet for set -e 🙂 .
https://stackoverflow.com/a/19622569
http://mywiki.wooledge.org/BashFAQ/105
https://serverfault.com/questions/143445/what-does-set-e-do-and-why-might-it-be-considered-dangerous

[jameslamb]

@jameslamb

But ultimately...if you are not comfortable with these changes after another review we can close the PR.

I'm absolutely comfortable with the proposed changes! I'm sorry if I confused you by any my previous comment.

However, I think we need +1 reviewer for this PR.
Just some refs to cover the opposite point of view and dispel the impression of silver bullet for set -e 🙂 .
https://stackoverflow.com/a/19622569
http://mywiki.wooledge.org/BashFAQ/105
https://serverfault.com/questions/143445/what-does-set-e-do-and-why-might-it-be-considered-dangerous

WOW this was a good read (from the second link)

So the implementors decided to make a bunch of special rules, like "commands that are part of an if test are immune", or "commands in a pipeline, other than the last one, are immune".

These rules are extremely convoluted, and they still fail to catch even some remarkably simple cases. Even worse, the rules change from one Bash version to another,

ok now I'm scared of set -e 😬

I think agree we need another reviewer to help decide. @Laurae2 do you have a strong opinion?

[StrikerRUS]

ping @guolinke and @chivee for the discussion

[guolinke]

sorry for the late response.
If set -e indeed is dangerous, I think we should keep the original one.

[jameslamb]

sorry for the late response.
If set -e indeed is dangerous, I think we should keep the original one.

you've convinced me too @StrikerRUS . With the complexity of our setup + the links you shared, I think set -e is not appropriate and that we should close this PR.