didactic-drunk / sodium.cr

Crystal wrapper for the libsodium crypto API
MIT License
49 stars 13 forks source link

Auto-install of libsodium fails silenty when `wget` is missing #3

Closed m-o-e closed 4 years ago

m-o-e commented 4 years ago

Rather minor thing but can hit by surprise e.g. when building in a minimal docker image.

Sorry no time for PR at this time, but I imagine can likely be a 1-line fix (which wget || exit 1 or such)

didactic-drunk commented 4 years ago

set -e should handle this condition without || exit

Running on MacOS confirms this:

./build/libsodium_install.sh: line 33: wget: command not found

Which shell and version(s) are used?

m-o-e commented 4 years ago

Sorry, I should have clarified what exactly happens.

You can reproduce with this one-liner:

docker run -ti crystallang/crystal:latest /bin/bash -c -- "echo >shard.yml -e 'name: test\nversion: 0.0.1\ndependencies:\n  sodium:\n    github: didactic-drunk/sodium.cr'; shards; echo >test.cr 'require \"sodium\"'; crystal test.cr"

Whereas this one works:

docker run -ti crystallang/crystal:latest /bin/bash -c -- "apt update; apt install wget; echo >shard.yml -e 'name: test\nversion: 0.0.1\ndependencies:\n  sodium:\n    github: didactic-drunk/sodium.cr'; shards; echo >test.cr 'require \"sodium\"'; crystal test.cr"
kingsleyh commented 4 years ago

maybe curl is more widely available than wget these days

m-o-e commented 4 years ago

Yep that might do as a workaround. However there could still be other errors (e.g. network unreachable), so it would be good to have a clear error message when sth goes wrong.

I'm at a loss as to why that currently doesn't happen.

It seems like the exit code of the build script is ignored somewhere but I'm not sure where/why.

didactic-drunk commented 4 years ago

Here's the problem:

# Sometimes I use `set -x` to debug libsodium_install.sh.  Output would mess up compiling.
./build/libsodium_install.sh > libsodium_install.out 2>&1

Moving this discussion to #6. See if the branch works for you.