Lunderberg commented on a change in pull request #8670:
URL: https://github.com/apache/tvm/pull/8670#discussion_r686299795
##########
File path: docker/bash.sh
##########
@@ -30,152 +33,356 @@
# With -i, execute interactively.
#
-set -e
+set -euo pipefail
-source "$(dirname $0)/dev_common.sh" || exit 2
+function show_usage() {
+ cat <<EOF
+Usage: docker/bash.sh [-i|--interactive] [--net=host]
+ [--mount MOUNT_DIR] [--repo-mount-point REPO_MOUNT_POINT]
+ [--dry-run]
+ <DOCKER_IMAGE_NAME> [--] [COMMAND]
+
+-h, --help
+
+ Display this help message.
+
+-i, --interactive
+
+ Start the docker session in interactive mode.
+
+--net=host
+
+ Expose servers run into the container to the host, passing the
+ "--net=host" argument through to docker. On MacOS, this is
+ instead passed as "-p 8888:8888" since the host networking driver
+ isn't supported.
+
+--mount MOUNT_DIR
+
+ Expose MOUNT_DIR as an additional mount point inside the docker
+ container. The mount point inside the container is the same as
+ the folder location outside the container. This option can be
+ specified multiple times.
+
+--repo-mount-point REPO_MOUNT_POINT
+
+ The directory inside the docker container at which the TVM
+ repository should be mounted, and is used as the workspace inside
+ the docker container.
+
+ If unspecified, the mount location depends on the environment. If
+ running inside Jenkins, the mount location will be /workspace.
+ Otherwise, the mount location of the repository will be the same
+ as the external location of the repository, to maintain
+ compatibility with git-worktree.
+
+--dry-run
+
+ Print the docker command to be run, but do not execute it.
+
+DOCKER_IMAGE_NAME
+
+ The name of the docker container to be run. This can be an
+ explicit name of a docker image (e.g. "tlcpack/ci-gpu:v0.76") or
+ can be a shortcut as defined in the TVM Jenkinsfile
+ (e.g. "ci_gpu").
+
+COMMAND
+
+ The command to be run inside the docker container. If this is set
+ to "bash", both the --interactive and --net=host flags are set.
+ If no command is specified, defaults to "bash". If the command
+ contains dash-prefixed arguments, the command should be preceded
+ by -- to indicate arguments that are not intended for bash.sh.
+
+EOF
+}
-interactive=0
-if [ "$1" == "-i" ]; then
- interactive=1
- shift
+
+#################################
+### Start of argument parsing ###
+#################################
+
+SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd -P)"
+REPO_DIR="$(dirname "${SCRIPT_DIR}")"
+
+DRY_RUN=false
+INTERACTIVE=false
+USE_NET_HOST=false
+DOCKER_IMAGE_NAME=
+COMMAND=bash
+MOUNT_DIRS=( )
+
+# TODO(elunderberg): Remove this if statement and always set to
+# "${REPO_DIR}". The consistent directory for Jenkins is currently
+# necessary to allow cmake build commands to run in CI after the build
+# steps.
+if [[ -n "${JENKINS_HOME:-}" ]]; then
+ REPO_MOUNT_POINT=/workspace
+else
+ REPO_MOUNT_POINT="${REPO_DIR}"
fi
-CI_DOCKER_EXTRA_PARAMS=( )
-if [[ "$1" == "--net=host" ]]; then
- CI_DOCKER_EXTRA_PARAMS+=('--net=host')
- shift 1
+
+function parse_error() {
+ echo "$@" >&2
+ show_usage >&2
+ exit 1
+}
+
+# Handle joined flags, such as interpreting -ih as -i -h. Either rewrites
+# the current argument if it is a joined argument, or shifts all arguments
+# otherwise. Should be called as "eval $break_joined_flag" where joined
+# flags are possible. Can't use a function definition, because it needs
+# to overwrite the parent scope's behavior.
+break_joined_flag='if (( ${#1} == 2 )); then shift; else set -- -"${1#-i}"
"${@:2}"; fi'
+
+
+while (( $# )); do
+ case "$1" in
+ -h|--help)
+ show_usage
+ exit 0
+ ;;
+
+ -i*|--interactive)
+ INTERACTIVE=true
+ eval $break_joined_flag
+ ;;
+
+ --net=host)
+ USE_NET_HOST=true
+ shift
+ ;;
+
+ --mount)
+ if [[ -n "$2" ]]; then
+ MOUNT_DIRS+=("$2")
+ shift 2
+ else
+ parse_error 'ERROR: --mount requires a non-empty argument'
+ fi
+ ;;
+
+ --mount=?*)
+ MOUNT_DIRS+=("${1#*=}")
+ shift
+ ;;
+
+ --dry-run)
+ DRY_RUN=true
+ shift
+ ;;
+
+ --repo-mount-point)
+ if [[ -n "$2" ]]; then
+ REPO_MOUNT_POINT="$2"
+ shift 2
+ else
+ parse_error 'ERROR: --repo-mount-point requires a non-empty
argument'
+ fi
+ ;;
+
+ --repo-mount-point=?*)
+ REPO_MOUNT_POINT="${1#*=}"
+ shift
+ ;;
+
+ --)
+ shift
+ COMMAND=( "$@" )
+ break
+ ;;
+
+ -*|--*)
+ echo "Error: Unknown flag: $1" >&2
+ echo " If this flag is intended to be passed to the" >&2
+ echo " docker command, please add -- before the docker" >&2
+ echo " command (e.g. docker/bash.sh ci_gpu -- build -j2)" >&2
+ show_usage >&2
+ exit 1
+ ;;
+
+ *)
+ # First positional argument is the image name, all
+ # remaining below to the COMMAND.
+ if [[ -z "${DOCKER_IMAGE_NAME}" ]]; then
+ DOCKER_IMAGE_NAME=$1
+ shift
+ else
+ COMMAND=( "$@" )
+ break
+ fi
+ ;;
+ esac
+done
+
+if [[ -z "${DOCKER_IMAGE_NAME}" ]]; then
+ echo "Error: Missing DOCKER_IMAGE_NAME" >&2
+ show_usage >&2
fi
-# Mount external directory to the docker
-CI_DOCKER_MOUNT_CMD=( )
-if [ "$1" == "--mount" ]; then
- shift 1
- CI_DOCKER_MOUNT_CMD=( -v "$1:$1" )
- shift 1
+if [[ ${COMMAND[@]+"${COMMAND[@]}"} = bash ]]; then
+ INTERACTIVE=true
+ USE_NET_HOST=true
fi
-if [ "$#" -lt 1 ]; then
- echo "Usage: docker/bash.sh [-i] [--net=host] <CONTAINER_NAME> [COMMAND]"
- exit -1
+
+
+###############################
+### End of argument parsing ###
+###############################
+
+source "$(dirname $0)/dev_common.sh" || exit 2
+
+DOCKER_FLAGS=( )
+DOCKER_ENV=( )
+DOCKER_MOUNT=( )
+DOCKER_DEVICES=( )
+
+
+# If the user gave a shortcut defined in the Jenkinsfile, use it.
+EXPANDED_SHORTCUT=$(lookup_image_spec "${DOCKER_IMAGE_NAME}")
+if [ -n "${EXPANDED_SHORTCUT}" ]; then
+ DOCKER_IMAGE_NAME="${EXPANDED_SHORTCUT}"
fi
-DOCKER_IMAGE_NAME=$(lookup_image_spec "$1")
-if [ -z "${DOCKER_IMAGE_NAME}" ]; then
- DOCKER_IMAGE_NAME=("$1")
+# Set up working directories
+
+DOCKER_FLAGS+=( --workdir "${REPO_MOUNT_POINT}" )
+DOCKER_MOUNT+=( --volume "${REPO_DIR}":"${REPO_MOUNT_POINT}"
+ --volume "${SCRIPT_DIR}":/docker
+ )
+
+# Set up CI-specific environment variables
+DOCKER_ENV+=( --env CI_BUILD_HOME="${REPO_MOUNT_POINT}"
+ --env CI_BUILD_USER="$(id -u -n)"
+ --env CI_BUILD_UID="$(id -u)"
+ --env CI_BUILD_GROUP="$(id -g -n)"
+ --env CI_BUILD_GID="$(id -g)"
+ --env CI_PYTEST_ADD_OPTIONS="${CI_PYTEST_ADD_OPTIONS:-}"
+ --env CI_IMAGE_NAME="${DOCKER_IMAGE_NAME}"
+ )
+
+
+# Pass tvm test data folder through to the docker container, to avoid
+# repeated downloads. Check if we have permissions to write to the
+# directory first, since the CI may not.
+TEST_DATA_PATH="${TVM_DATA_ROOT_PATH:-${HOME}/.tvm_test_data}"
+if [[ -d "${TEST_DATA_PATH}" && -w "${TEST_DATA_PATH}" ]]; then
+ DOCKER_MOUNT+=( --volume
"${TEST_DATA_PATH}":"${REPO_MOUNT_POINT}"/.tvm_test_data )
fi
-if [ "$#" -eq 1 ]; then
- COMMAND="bash"
- interactive=1
+
+# Remove the container once it finishes running (--rm) and share the
+# PID namespace (--pid=host). The process inside does not have pid 1
+# and SIGKILL is propagated to the process inside, allowing jenkins to
+# kill it if needed.
+DOCKER_FLAGS+=( --rm --pid=host)
+
+# Expose services running in container to the host.
+if $USE_NET_HOST; then
if [[ $(uname) == "Darwin" ]]; then
# Docker's host networking driver isn't supported on macOS.
# Use default bridge network and expose port for jupyter notebook.
- CI_DOCKER_EXTRA_PARAMS+=( "${CI_DOCKER_EXTRA_PARAMS[@]}" "-p
8888:8888" )
+ DOCKER_FLAGS+=( -p 8888:8888 )
else
- CI_DOCKER_EXTRA_PARAMS+=( "${CI_DOCKER_EXTRA_PARAMS[@]}" "--net=host" )
+ DOCKER_FLAGS+=(--net=host)
fi
-else
- shift 1
- COMMAND=("$@")
fi
-if [ $interactive -eq 1 ]; then
- CI_DOCKER_EXTRA_PARAMS=( "${CI_DOCKER_EXTRA_PARAMS[@]}" -it )
+# Set up interactive sessions
+if ${INTERACTIVE}; then
+ DOCKER_FLAGS+=( --interactive --tty )
fi
-SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
-WORKSPACE="$(pwd)"
-
-# Use nvidia-docker if the container is GPU.
-if [[ ! -z $CUDA_VISIBLE_DEVICES ]]; then
- CUDA_ENV="-e CUDA_VISIBLE_DEVICES=${CUDA_VISIBLE_DEVICES}"
-else
- CUDA_ENV=""
-fi
+# Expose external directories to the docker container
+for MOUNT_DIR in ${MOUNT_DIRS[@]+"${MOUNT_DIRS[@]}"}; do
Review comment:
I think the current implementation is the pedantically correct pattern,
following [this stackoverflow
post](https://stackoverflow.com/questions/7577052/bash-empty-array-expansion-with-set-u/61551944#61551944).
The main problem with the `"${MOUNT_DIRS[@]-}"` pattern is that it is
vulnerable to typos, and `"${MOUNTT_DIRS[@]-}"` would silently be expanded to
the empty string as well. I tested the current version to verify that the
array elements aren't shlexed apart, and the recommended variant does keep
separate elements separate.
That said, I was going back and forth on it while writing it, because
avoiding typos at the expense of making it nigh unreadable isn't really worth
it. Especially if we want to migrate this to a python script at some point,
I'd go for version that's easier to read, and will change them to
`"${MOUNT_DIRS[@]-}"`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
