tlc-pack / tlcpack

https://tlcpack.ai/
Apache License 2.0
23 stars 30 forks source link

Fixed LLVM major version from "10.0" to "10". #26

Closed leandron closed 3 years ago

leandron commented 3 years ago

Using MAJOR.MINOR string in Dockerfile.package-cpu crashes due to a numeric comparison made by scripts/centos_install_llvm.sh.

This simply set LLVM version from 10.0 to 10.

~Note: I couldn't find a LLVM released under 10.1.x (in https://releases.llvm.org/), so I assume the current returned value 10.0.1 is correct.~

Edit: This is moving from 10.0 to 10.

leandron commented 3 years ago

cc @icemelon9 @tqchen

icemelon commented 3 years ago

Currently, we are using "10.0". I don't see there's any problem

leandron commented 3 years ago

Currently, we are using "10.0". I don't see there's any problem

I tried rebuilding the Docker image and it seems it crashes because the -lt operator only accepts integer values (match with docs at https://pubs.opengroup.org/onlinepubs/9699919799.2016edition/utilities/test.html).

https://github.com/tlc-pack/tlcpack/blob/57b59372698b360c6871addcfdffd79e4188e946/docker/install/centos_build_llvm.sh#L50

Am I missing something? Please correct me of I’m wrong.

leandron commented 3 years ago

This is a reproducer of the issue I see:

$ cat test.sh 
set -e

LLVM_VERSION_MAJOR=10.0

if [ ${LLVM_VERSION_MAJOR} -lt 9 ]; then
  echo "${LLVM_VERSION_MAJOR} is lt 9"
else
  echo "${LLVM_VERSION_MAJOR} is NOT lt 9"
fi

When running it:

$ bash test.sh 
test.sh: line 5: [: 10.0: integer expression expected
10.0 is NOT lt 9

$ 

Edit: I was wrong about 10.1, but still I think we can fix th bash warning.

leandron commented 3 years ago

I agree this is a minor problem - we can live with it for now.