tweak 'eb' wrapper script to correctly handle errors when python command being considered fails to run#4019
Conversation
That ship has just sailed (EasyBuild v4.5.5 has just been released), and I've learned the hard way to not include potentially disruptive changes at the last minute... The changes here look fine, but I'd rather have this battle tested in |
| # (using 'command -v', since 'which' implies an extra dependency) | ||
| command -v $python_cmd &> /dev/null | ||
| if [ $? -eq 0 ]; then | ||
| if { command -v "${python_cmd}" && "${python_cmd}" -V; } &> /dev/null; then |
There was a problem hiding this comment.
I agree with your analysis that just the "${python_cmd}" -V should be sufficient, but on the other hand I don't think there's much to gain by dropping the command -v in terms of speed anyway
There was a problem hiding this comment.
No, there's no speed gain (just some microsecond). But the command could be simplified from:
if { command -v "${python_cmd}" && "${python_cmd}" -V; } &> /dev/null; then
to
if "${python_cmd}" -V &> /dev/null; then
which looks cleaner to me. But up to you. This is neither a performance issue nor a latent error.
fair point |
There are setups (like ours), where the python command may not load due to missing dynamic libraries in the path. In those cases, the run is like:
This causes an issue in the lines 73-75 of the
ebscript:This execution returns the shown message thru stderr, stderr is redirected to stdout and it is cut to take the second space-separated field: the "error" word. So the
$pyveris "error"!This ends up in a
$pyver_maj== "error" and a$pyver_min== "".The next line, the one that compares the versions with the minimum ones, fails with:
because "error" is not an integer.
The solution goes to test if the command can be run, not just if the command exists. So I added a quick execution to test for its validity.
BTW, I think we can get rid of the original test
command -v, but I let it, just in case.This new version checks if the command exist and if the command can be run and return a version number. If the command does not exist, both tests will fail, if it works, both tests will work, and if the command exist but it does not work, only second test will fail. So I would only leave the second test. Up to you in a further commit :-)