gemini-code-assist[bot] commented on code in PR #18854:
URL: https://github.com/apache/tvm/pull/18854#discussion_r2867622531
##########
docker/bash.sh:
##########
@@ -126,47 +168,66 @@ COMMAND
contains dash-prefixed arguments, the command should be preceded
by -- to indicate arguments that are not intended for bash.sh.
-EOF
-}
+Environment Variables:
+TVM_DEV_MOUNTS
-#################################
-### Start of argument parsing ###
-#################################
-
-SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd -P)"
-REPO_DIR="$(dirname "${SCRIPT_DIR}")"
-
-DRY_RUN=false
-INTERACTIVE=false
-TTY=false
-USE_NET_HOST=false
-USE_GPU=true
-DOCKER_IMAGE_NAME=
-COMMAND=bash
-MOUNT_DIRS=( )
-CONTAINER_NAME=
-
-# TODO(Lunderberg): 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.
-# TODO(https://github.com/apache/tvm/issues/11952):
-# Figure out a better way to keep the same path
-# between build and testing stages.
-if [[ -n "${JENKINS_HOME:-}" ]]; then
- REPO_MOUNT_POINT=/workspace
-else
- REPO_MOUNT_POINT="${REPO_DIR}"
-fi
+ Space-separated list of additional mount specifications.
+ Each entry is either:
+ /path/to/dir - bind mount at same path inside container
+ /src:/dst - bind mount src to dst inside container
+ Source paths are validated; missing sources are skipped with a warning.
+EOF
+}
function parse_error() {
echo "$@" >&2
show_usage >&2
exit 1
}
+# Detect if Docker daemon is running in rootless mode.
+# Uses DOCKER_IS_ROOTLESS env var as override (for CI), otherwise auto-detects.
+# Result is cached after first call.
+_ROOTLESS_CACHED=
+_ROOTLESS_RESULT=1
+function detect_rootless() {
+ if [ -n "${_ROOTLESS_CACHED}" ]; then
+ return $_ROOTLESS_RESULT
+ fi
+ _ROOTLESS_CACHED=1
+ if [ -n "${DOCKER_IS_ROOTLESS:-}" ]; then
+ # env var override for CI
+ _ROOTLESS_RESULT=0
+ return 0
+ fi
+ if docker info -f '{{.SecurityOptions}}' 2>/dev/null | grep -q "rootless";
then
+ _ROOTLESS_RESULT=0
+ return 0
+ fi
+ _ROOTLESS_RESULT=1
+ return 1
+}
Review Comment:
The `detect_rootless` function can be simplified to be more concise and
improve readability by reducing redundant code. The multiple `if` statements
with `return` can be combined into a single `if/elif/else` block that sets the
result, with a single `return` at the end.
```suggestion
function detect_rootless() {
if [ -n "${_ROOTLESS_CACHED}" ]; then
return $_ROOTLESS_RESULT
fi
_ROOTLESS_CACHED=1
if [ -n "${DOCKER_IS_ROOTLESS:-}" ]; then
# env var override for CI
_ROOTLESS_RESULT=0
elif docker info -f '{{.SecurityOptions}}' 2>/dev/null | grep -q
"rootless"; then
_ROOTLESS_RESULT=0
else
_ROOTLESS_RESULT=1
fi
return $_ROOTLESS_RESULT
}
```
##########
docker/bash.sh:
##########
@@ -349,14 +439,55 @@ if [ -n "${EXPANDED_SHORTCUT}" ]; then
fi
fi
-# Set up working directories
+##############################################
+### Section 5: Mount construction ###
+##############################################
+
+# 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
+# Expose external directories from --mount flags
+for MOUNT_DIR in ${MOUNT_DIRS[@]+"${MOUNT_DIRS[@]}"}; do
+ DOCKER_MOUNT+=( --volume "${MOUNT_DIR}:${MOUNT_DIR}" )
+done
+
+# Parse TVM_DEV_MOUNTS env var: space-separated mount specs
+# Each entry is either /path (same path) or /src:/dst
+if [[ -n "${TVM_DEV_MOUNTS:-}" ]]; then
+ for mount_spec in ${TVM_DEV_MOUNTS}; do
+ if [[ "$mount_spec" == *:* ]]; then
+ # src:dst format
+ mount_src="${mount_spec%%:*}"
+ if validate_mount "$mount_src"; then
+ DOCKER_MOUNT+=( --volume "$mount_spec" )
+ fi
+ else
+ # Same path format
+ if validate_mount "$mount_spec"; then
+ DOCKER_MOUNT+=( --volume "$mount_spec:$mount_spec" )
+ fi
+ fi
+ done
Review Comment:
The loop `for mount_spec in ${TVM_DEV_MOUNTS}` is subject to word splitting
and glob expansion, which can lead to unexpected behavior if mount paths
contain spaces or glob characters (like `*`).
To make this more robust, consider using `read -r -a` to parse the
space-separated list into an array. This avoids unintended globbing and
provides better support for complex path names in the future.
```suggestion
read -r -a mount_specs <<< "${TVM_DEV_MOUNTS}"
for mount_spec in "${mount_specs[@]}"; do
if [[ "$mount_spec" == *:* ]]; then
# src:dst format
mount_src="${mount_spec%%:*}"
if validate_mount "$mount_src"; then
DOCKER_MOUNT+=( --volume "$mount_spec" )
fi
else
# Same path format
if validate_mount "$mount_spec"; then
DOCKER_MOUNT+=( --volume "$mount_spec:$mount_spec" )
fi
fi
done
```
##########
docker/bash.sh:
##########
@@ -191,7 +252,21 @@ while (( $# )); do
-t*|--tty)
TTY=true
- eval $break_joined_flag
+ if [[ "$1" == "--tty" ]]; then
+ shift
+ else
+ # Short form: -t or combined like -th
+ eval $break_joined_flag
Review Comment:
The comment on line 258 indicates that combined flags like `-th` are
supported, but `break_joined_flag` is hardcoded to handle flags combined with
`-i` (e.g., `-ih`). Using it for `-t` will not work as intended. For example,
`-th` would not be split into `-t` and `-h`.
To correctly support combined flags for `-t`, you should use logic specific
to `-t` instead of reusing `break_joined_flag`.
```suggestion
if (( ${#1} > 2 )); then set -- -"${1#-t}" "${@:2}"; else
shift; fi
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
