builder icon indicating copy to clipboard operation
builder copied to clipboard

Potential Typo in builder

Open ydaiming opened this issue 3 years ago • 1 comments

Should this be MANY_LINUX_VERSION instead of MANY_LINUX_VERSON? https://github.com/pytorch/builder/blob/3e6551ca3b205d17fa9570e747c87fdf8df1449c/manywheel/build_docker.sh#L56

ydaiming avatar Feb 22 '22 19:02 ydaiming

Noticed this last night, MANY_LINUX_VERSON appears only once in this entire repo, almost certainly a typo and they meant MANY_LINUX_VERSION.

Further, given the set -eou pipefail settings, it should fail, per the following snippet derived from the original file

$ cat ./variable_test.sh
#!/usr/bin/env bash

set -eou pipefail

MANY_LINUX_VERSION=${MANY_LINUX_VERSION:-}
if [[ -n ${MANY_LINUX_VERSION} ]]; then
    DOCKERFILE_SUFFIX=_${MANY_LINUX_VERSON}
else
    DOCKERFILE_SUFFIX=''
fi

echo "DOCKERFILE_SUFFIX: '${DOCKERFILE_SUFFIX}'"

$ ./variable_test.sh 
DOCKERFILE_SUFFIX: ''
$ MANY_LINUX_VERSION=FOO ./variable_test.sh 
./variable_test.sh: line 7: MANY_LINUX_VERSON: unbound variable

Given how any script calling such with MANY_LINUX_VERSION would surely fail, I looked into the wrapper script, and discovered a 2nd possible issue. Note how in the wrapper script it uses MANYLINUX_VERSION instead. So, the reason it is not failing is because MANY_LINUX_VERSION is never actually set due to a 2nd typo?

Investigating a fix in this fork / branch: https://github.com/ORNL/builder/tree/issue_969

Update 1: Fix is now in pull request #979 , my company's IP attorneys are awaiting a response from meta on the agreement phraseology before they can sign.

Update 2: Looks like one typo was fixed by another user and merged into main.

But, MANYLINUX_VERSION vs MANY_LINUX_VERSION still exists.

qhaas avatar Feb 25 '22 13:02 qhaas