Skip to content

[docker] add dockerfile for qt-av-deps image - #99

Open
fabiencastan wants to merge 3 commits into
developfrom
add-docker
Open

[docker] add dockerfile for qt-av-deps image#99
fabiencastan wants to merge 3 commits into
developfrom
add-docker

Conversation

@fabiencastan

Copy link
Copy Markdown
Member

Dockerfile for building an image containing Qt and avdeps (AliceVision dependencies only). This image is required to set up the CI pipeline for QtAliceVision.

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

Copy link
Copy Markdown

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 adds a Dockerfile and a helper script to build a Rocky Linux image with Qt via aqtinstall, as well as minor updates to the .gitignore file. The review feedback highlights several improvement opportunities: passing all defined version variables as build arguments to avoid mismatches, making the build script directory-independent, explicitly installing dnf-plugins-core, using the --no-cache-dir flag with pip to reduce image size, and combining sequential RUN commands to minimize Docker layers.

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 thread docker/build-rocky.sh
Comment on lines +11 to +16
docker build \
--rm \
--progress=plain \
--build-arg "QT_VERSION=${QT_VERSION}" \
--tag "${DOCKER_TAG}" \
-f Dockerfile_rocky .

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The script defines AVDEPS_VERSION, ROCKY_VERSION, and CUDA_VERSION and uses them to construct the DOCKER_TAG. However, these variables are not passed as --build-arg to the docker build command. As a result, the build will use the default values hardcoded in the Dockerfile_rocky, which can lead to a mismatch between the actual image contents and the generated tag if these variables are modified in this script.

Suggested change
docker build \
--rm \
--progress=plain \
--build-arg "QT_VERSION=${QT_VERSION}" \
--tag "${DOCKER_TAG}" \
-f Dockerfile_rocky .
docker build \
--rm \
--progress=plain \
--build-arg "AVDEPS_VERSION=${AVDEPS_VERSION}" \
--build-arg "ROCKY_VERSION=${ROCKY_VERSION}" \
--build-arg "CUDA_VERSION=${CUDA_VERSION}" \
--build-arg "QT_VERSION=${QT_VERSION}" \
--tag "${DOCKER_TAG}" \
-f Dockerfile_rocky .

Comment thread docker/build-rocky.sh
Comment on lines +1 to +3
#!/bin/bash
set -ex

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

To make the script robust and executable from any directory (e.g., the repository root), it is recommended to change the working directory to the script's directory at the beginning of the script.

Suggested change
#!/bin/bash
set -ex
#!/bin/bash
set -ex
# Ensure the script runs from its own directory
cd "$(dirname "$0")"

Comment thread docker/Dockerfile_rocky
Comment on lines +21 to +22
RUN dnf -y install epel-release && \
dnf config-manager --set-enabled crb && \

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The dnf config-manager command requires dnf-plugins-core to be installed. If it is not pre-installed in the base image, this step will fail. It is safer to explicitly install dnf-plugins-core before running dnf config-manager.

RUN dnf -y install epel-release dnf-plugins-core && \
    dnf config-manager --set-enabled crb && \

Comment thread docker/Dockerfile_rocky
Comment on lines +58 to +59
RUN pip3 install --upgrade pip && \
pip3 install aqtinstall

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

When installing Python packages via pip in a Dockerfile, it is best practice to use the --no-cache-dir flag to prevent caching package files, which unnecessarily increases the Docker image size.

RUN pip3 install --no-cache-dir --upgrade pip && \
    pip3 install --no-cache-dir aqtinstall

Comment thread docker/Dockerfile_rocky
Comment on lines +97 to +102
RUN ls -l ${QT_ROOT}/lib
RUN find ${QT_DIR} -name "libQt6*.so*"
RUN echo "LD_LIBRARY_PATH: ${LD_LIBRARY_PATH}"
RUN ldd ${QT_ROOT}/bin/qmake

RUN qmake --version && cmake --version

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Multiple sequential RUN instructions are used for smoke testing. Each RUN instruction creates a new layer in the Docker image. Combining these read-only commands into a single RUN instruction reduces the number of layers and keeps the image cleaner.

RUN ls -l ${QT_ROOT}/lib && \
    find ${QT_DIR} -name "libQt6*.so*" && \
    echo "LD_LIBRARY_PATH: ${LD_LIBRARY_PATH}" && \
    ldd ${QT_ROOT}/bin/qmake && \
    qmake --version && \
    cmake --version

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.

1 participant