Skip to content
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

Luaver kill the whole batch process on error #29

Open
umireon opened this issue Jan 10, 2017 · 4 comments
Open

Luaver kill the whole batch process on error #29

umireon opened this issue Jan 10, 2017 · 4 comments
Labels

Comments

@umireon
Copy link
Contributor

umireon commented Jan 10, 2017

The following script will not work as expected:

. ~/.luaver/luaver
if luaver install-luarocks install 5.3.1; then
    echo ok
else
    echo Something wrong
fi

The result was:

$ sh test.sh
No lua version set

$

while printing Something wrong is the expected behavior.

This is caused by luaver#L34, which kills not only the luaver function but also the whole batch process.
This behavior makes difficult to use luaver on the CI.

Since there is no exception handling mechanism on shells (kill -INT $$ is appearently not working), I suggest each command has explicit error handling using && and if.

@DhavalKapil
Copy link
Owner

Can you elaborate on the issue. I see it is a problem but exactly how is it affecting the CI?

@umireon
Copy link
Contributor Author

umireon commented Jan 11, 2017

(I am sorry but I cannot fully understand why kill -INT $$ works; I cannot find any spec to describe what is happen when I shoot myself in the foot with shell by SIGINT)

tl;dr luaver behaves like exit on error and this is hard to understand since no one expect that.

Of course, the current luaver is working when there are no errors.
The problem is in debugging, the behavior of kill -INT $$ makes debugging very hard.

For example, travis_retry won't work as expected.
In CI sessions, retrying is essential for stable builds because download failure is one of the common problems.
The following code is a typical pattern to realize the retrying.

install: for i in 1 2 3; do luaver install 5.3.2; done
# essentially equivalent to `travis_retry luaver install 5.3.2`

However, this will never work as expected because luaver kills the ongoing shell process on error, which cause the loop to stop.
If the user is suficiently wise, they will conceive that luaver does something intrusive to handle errors.
The current error handling proccess is exitting the current shell without writing exit.
More concisely, luaver install 5.3.2 is exit 1 under certain conditions.

@umireon
Copy link
Contributor Author

umireon commented Jan 11, 2017

The behavior of kill -INT $$ in interactive sessions is different among shells and exit code cannot be controlled.
Even worse, zsh masks the error.

bash

$ kill -INT $$

$ echo $?
1

zsh

% kill -INT $$
% echo $?
0

dash

$ kill -INT $$

$ echo $?
130

ksh

$ kill -INT $$
$ echo $?
258

@DhavalKapil
Copy link
Owner

I get your point. This is indeed a minor yet important bug. An alternate way needs to be found out. I'm ignoring this for the present release though.

@DhavalKapil DhavalKapil added the bug label Mar 2, 2017
umireon added a commit to umireon/luaver that referenced this issue Apr 9, 2017
Exit code is used to handle errors instead of signal.

Fixes DhavalKapil#29
umireon added a commit to umireon/luaver that referenced this issue Apr 9, 2017
Exit code is used to handle errors instead of signal.

Fixes DhavalKapil#29
umireon added a commit to umireon/luaver that referenced this issue Apr 12, 2017
Exit code is used to handle errors instead of signal.

Fixes DhavalKapil#29
umireon added a commit to umireon/luaver that referenced this issue Apr 12, 2017
Exit code is used to handle errors instead of signal.

Fixes DhavalKapil#29
umireon added a commit to umireon/luaver that referenced this issue Apr 12, 2017
Exit code is used to handle errors instead of signal.

Fixes DhavalKapil#29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants