Skip to content
This repository has been archived by the owner on Dec 9, 2018. It is now read-only.

pass through exit status from install cmds #156

Merged
merged 4 commits into from
Feb 15, 2016

Conversation

jonlawlor
Copy link
Contributor

By checking the exit status from the various install commands (apt-get,
git, curl, etc.) we can set the travis exit status to failure in the
right spot, so that it is obvious when there is a network failure or
other temporary problem.

This should help with #155 - the only shortcoming is that it doesn't pass through the exit code and instead assigns 1 to all failures, but it should be OK for travis, and in any case apt-get, curl, git, and go get do not have equivalent exit codes.

PTAL

By checking the exit status from the various install commands (apt-get,
git, curl, etc.) we can set the travis exit status to failure in the
right spot, so that it is obvious when there is a network failure or
other temporary problem.
@vladimir-ch
Copy link
Member

Could you please also add a similar modification to the ATLAS install script? Then LGTM. Thanks!

@jonlawlor
Copy link
Contributor Author

I've added it to the ATLAS install script, but currently there is no travis build environment for it so I can't test.

@jonlawlor
Copy link
Contributor Author

Well, it looks like travis has enabled multi-os (and OSX builds) by default, and we just need to change the .travis.yml script. There will probably be issues because ATLAS comes with xtools if I recall correctly. I can enable it but then have it ignore errors, I'll give it a shot this Saturday.

@vladimir-ch
Copy link
Member

I've added it to the ATLAS install script, but currently there is no travis build environment for it so I can't test.

I know but I thought it would be good to have it ready for what if one day ...

Well, it looks like travis has enabled multi-os (and OSX builds) by default, and we just need to change the .travis.yml script.

Having the tests run also on OSX would be indeed very nice.

There will probably be issues because ATLAS comes with xtools if I recall correctly.

Do you mean the Accelerate framework?

@btracey
Copy link
Member

btracey commented Feb 3, 2016

Just know that I've had ATLAS installation problems on OSX in the past, and that ATLAS also fails at least part of our test suite at the moment.

@sbinet
Copy link
Member

sbinet commented Feb 3, 2016

wouldn't the same thing be achieved with set -e put at the top?
see:
http://www.davidpashley.com/articles/writing-robust-shell-scripts/#id2382181

@jonlawlor
Copy link
Contributor Author

Looks like it! I wasn't aware of that command (not surprising because I am not a bash expert). I'll research it a little and then modify as appropriate.

@jonlawlor
Copy link
Contributor Author

Also, @vladimir-ch - yes, I meant the Accelerate framework and I guess I got them mixed up in my head for a bit. I'll revise my statement earlier and just say that I'll be fiddling with travis, ATLAS, Accelerate, and OSX over the weekend (in a fork).

@vladimir-ch
Copy link
Member

Will you use the set -e that @sbinet suggested?

@jonlawlor
Copy link
Contributor Author

From reading http://mywiki.wooledge.org/BashFAQ/105 it seems like there is some disagreement regarding if it is a good idea to use, and the alternative is to check error status as I am doing here. set -e was an attempt to avoid the repetition, but I'd rather be careful than DRY.

set -ex is simpler, so let’s use that method instead of copy pasting
conditionals.  Also added some comments to the code stanzas.
@jonlawlor
Copy link
Contributor Author

PTAL

@jonlawlor
Copy link
Contributor Author

ping @sbinet @vladimir-ch

@vladimir-ch
Copy link
Member

PTAL

@jonlawlor
Copy link
Contributor Author

Uh, at what in particular?

@sbinet
Copy link
Member

sbinet commented Feb 14, 2016

LGTM

jonlawlor added a commit that referenced this pull request Feb 15, 2016
pass through exit status from install cmds
@jonlawlor jonlawlor merged commit 354f67e into master Feb 15, 2016
@jonlawlor jonlawlor deleted the better-openblas-install-errors branch July 25, 2016 15:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants