Skip to content

add build image script#1203

Merged
helloyongyang merged 1 commit into
mainfrom
feat-docker
Jun 30, 2026
Merged

add build image script#1203
helloyongyang merged 1 commit into
mainfrom
feat-docker

Conversation

@black-eleven

Copy link
Copy Markdown
Contributor

No description provided.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request parameterizes the base image tag in the deployment Dockerfile, adds librosa to the pip installation, and introduces a new build_deploy.sh script to automate the Docker build process. The review feedback suggests three key improvements: handling missing arguments in the script's option parser to prevent unbound variable crashes under set -u, using --no-cache-dir in the Dockerfile to reduce image size, and replacing the non-portable grep -oP command with awk to ensure compatibility on macOS.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +12 to +24
while [[ $# -gt 0 ]]; do
case "$1" in
-t|--tag) BASE_TAG="$2"; shift 2 ;;
-r|--registry) REGISTRY="$2"; shift 2 ;;
-h|--help)
echo "Usage: $0 [-t base_tag] [-r registry]"
echo " -t, --tag Base image tag (default: $AUTO_BASE_TAG)"
echo " -r, --registry Registry prefix (default: $REGISTRY)"
exit 0
;;
*) echo "Unknown option: $1"; exit 1 ;;
esac
done

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Since set -u (nounset) is enabled, referencing $2 when there is no second argument (e.g., if the script is run with -t or -r as the last argument) will cause the script to crash with an 'unbound variable' error. Checking the number of remaining arguments before accessing $2 prevents this crash.

while [[ $# -gt 0 ]]; do
    case "$1" in
        -t|--tag)
            if [[ $# -lt 2 ]]; then
                echo "Error: -t/--tag requires an argument" >&2
                exit 1
            fi
            BASE_TAG="$2"
            shift 2
            ;;
        -r|--registry)
            if [[ $# -lt 2 ]]; then
                echo "Error: -r/--registry requires an argument" >&2
                exit 1
            fi
            REGISTRY="$2"
            shift 2
            ;;
        -h|--help)
            echo "Usage: $0 [-t base_tag] [-r registry]"
            echo "  -t, --tag       Base image tag (default: $AUTO_BASE_TAG)"
            echo "  -r, --registry  Registry prefix (default: $REGISTRY)"
            exit 0
            ;;
        *) echo "Unknown option: $1"; exit 1 ;;
    esac
done

ENV PYTHONPATH=/workspace/LightX2V

RUN pip install diffusers==0.37.1
RUN pip install diffusers==0.37.1 librosa

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using pip install without --no-cache-dir caches the downloaded packages inside the Docker image, unnecessarily increasing the final image size. Adding --no-cache-dir keeps the image lightweight.

RUN pip install --no-cache-dir diffusers==0.37.1 librosa

REPO_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)"
DOCKERFILE="$SCRIPT_DIR/Dockerfile_deploy"

AUTO_BASE_TAG=$(grep -oP 'ARG BASE_TAG=\K\S+' "$DOCKERFILE" | head -1)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The grep -oP command uses Perl-compatible regular expressions (-P), which is a GNU extension and is not supported by default on macOS (BSD grep). This will cause the script to fail on macOS environments. Using awk or sed provides a portable alternative that works across both Linux and macOS.

Suggested change
AUTO_BASE_TAG=$(grep -oP 'ARG BASE_TAG=\K\S+' "$DOCKERFILE" | head -1)
AUTO_BASE_TAG=$(awk -F= '/^ARG BASE_TAG=/ {print $2; exit}' "$DOCKERFILE")

@helloyongyang helloyongyang merged commit 16d7202 into main Jun 30, 2026
2 checks passed
@helloyongyang helloyongyang deleted the feat-docker branch June 30, 2026 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants