install-deps is far too brittle to use set -e

[mcsaucy]

64430c3 introduced a neat regression. If a user doesn't have a file/directory named /tmp/OpenBLAS, we will reliably fail to build OpenBLAS due to a failure to rm at https://github.com/torch/ezinstall/blob/master/install-deps#L12.

7fd56ce is just an awful hack around this behavior to keep the whole script from bailing. Basically, if anyone new tries to use install-deps, they'll never build OpenBLAS.

As a more philosophical statement, silent failure is soooo user-unfriendly. The script is still kinda janky and silent failure makes it really hard to troubleshoot. As an alternative, you could make a function that runs a command, checks the exit code, and fails with a useful error message (if you use BASH_LINENO, you can even get line numbers) if non-zero.

... As a side note, are there any tests performed for these changes?

Let me know if you have any questions,

-J

[lukeyeager]

64430c3 introduced a neat regression. If a user doesn't have a file/directory named /tmp/OpenBLAS, we will reliably fail to build OpenBLAS due to a failure to rm at https://github.com/torch/ezinstall/blob/master/install-deps#L12.

On Ubuntu, the -f flag makes rm ignore the error. What platform are you using?

$ ls OpenBLAS/
ls: cannot access OpenBLAS/: No such file or directory
$ rm -rf OpenBLAS
$ echo $?
0

[mcsaucy]

You're totally right and the behavior is backed by POSIX. Not sure what happened when I tested this last night and I can't repro now. Sorry for the knee-jerk finger-pointing. :(

Regarding my philosophical section, I still feel those concerns have merit. set -e gives you a rough, silent failure that doesn't even reliably cause failures (see [1] for deets on that), but most of my concerns stem from the opacity of failure scenarios [2].

Going back to tests, I think it'd be good if there were some semi-consistent set of tests to be run. For example, we can run one-off tests on Ubuntu, but we have more platforms than that. For example, I'm pretty sure the script will fail on OS X machines that don't have homebrew gnuplot pre-installed [2,3].

[1] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_25
[2] https://groups.google.com/forum/#!topic/torch7/qyr_bHyf3hE
[3] https://github.com/torch/ezinstall/blob/master/install-deps#L81

[soumith]

@lukeyeager @mcsaucy you're right. i'm still a bit too uneasy about set -e tbh. should i just revert it?

[lukeyeager]

i'm still a bit too uneasy about set -e tbh. should i just revert it?

Your call, but I'd vote to leave the set -e, remove the || true's and address the failures as they come up to make the script less brittle. I'm probably not the only one using this script in automated build environments, and I think we'd all rather see install errors happen in this script rather than having to track down potentially cryptic build errors because of missing dependencies.

As a side note, are there any tests performed for these changes?

I like that idea. It'd be pretty easy to throw this repo up on TravisCI. I'm already doing it here:

https://github.com/NVIDIA/DIGITS/blob/v3.1.0/scripts/travis/install-torch.sh

Seems like we might be able to run it on multiple OS's too?

https://docs.travis-ci.com/user/multi-os/

# something like this?
matrix:
  include:
    - os: linux
      dist: precise
    - os: linux
      dist: trusty
    - os: osx