[docker] Add option to set qtAV version and fix Rocky issues#3145
[docker] Add option to set qtAV version and fix Rocky issues#3145fabiencastan wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to specify the QtAliceVision version (QTAV_VERSION) during the build process, updating the CMake configuration, Dockerfiles, and build scripts accordingly. The review feedback suggests several Dockerfile optimizations, such as combining multiple ENV instructions to reduce layers, combining package installation commands with dnf clean all to minimize image size, and removing commented-out code. Additionally, it recommends quoting variables to prevent shell expansion issues, providing a default value for QTAV_VERSION, and redirecting error messages to stderr in the build script.
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.
| ARG CUDA_VERSION | ||
| ARG ROCKY_VERSION | ||
| FROM alicevision/meshroom-deps:${MESHROOM_VERSION}-av${AV_VERSION}-rocky${ROCKY_VERSION}-cuda${CUDA_VERSION} | ||
| ARG QTAV_VERSION |
| ENV MESHROOM_DEV=/opt/Meshroom | ||
| ENV MESHROOM_BUILD=/tmp/Meshroom_build | ||
| ENV MESHROOM_BUNDLE=/opt/Meshroom_bundle | ||
| ENV AV_INSTALL=/opt/AliceVision_install | ||
| ENV QT_DIR=/opt/Qt/6.8.3/gcc_64 | ||
| ENV PATH="${PATH}:${MESHROOM_BUNDLE}" |
There was a problem hiding this comment.
Splitting the environment variables into multiple ENV instructions increases the number of layers in the Docker image. It is recommended to combine them into a single ENV instruction using backslashes to keep the image metadata clean and minimize layers.
ENV MESHROOM_DEV=/opt/Meshroom \
MESHROOM_BUILD=/tmp/Meshroom_build \
MESHROOM_BUNDLE=/opt/Meshroom_bundle \
AV_INSTALL=/opt/AliceVision_install \
QT_DIR=/opt/Qt/6.8.3/gcc_64 \
PATH="${PATH}:${MESHROOM_BUNDLE}"
|
|
||
| # Build Meshroom plugins | ||
| RUN cmake "${MESHROOM_DEV}" -DALICEVISION_ROOT="${AV_INSTALL}" -DCMAKE_INSTALL_PREFIX="${MESHROOM_BUNDLE}/qtPlugins" | ||
| RUN cmake "${MESHROOM_DEV}" -DALICEVISION_ROOT="${AV_INSTALL}" -DQTALICEVISION_VERSION=${QTAV_VERSION} -DCMAKE_INSTALL_PREFIX="${MESHROOM_BUNDLE}/qtPlugins" |
There was a problem hiding this comment.
It is safer to wrap ${QTAV_VERSION} in double quotes to prevent potential shell expansion or word-splitting issues if the version string contains spaces or special characters.
RUN cmake "${MESHROOM_DEV}" -DALICEVISION_ROOT="${AV_INSTALL}" -DQTALICEVISION_VERSION="${QTAV_VERSION}" -DCMAKE_INSTALL_PREFIX="${MESHROOM_BUNDLE}/qtPlugins"
| # cp ${MESHROOM_BUNDLE}/lib/PySide6/Qt/lib/libQt6Concurrent.so.6 ${MESHROOM_BUNDLE}/lib/PySide6/Qt/qml/QtQuick/Dialogs && | ||
| # cp ${MESHROOM_BUNDLE}/lib/PySide6/Qt/qml/Qt3D/Core/libQt63DCore.so.6 ${MESHROOM_BUNDLE}/lib/PySide6/Qt/qml/QtQuick/Dialogs && \ | ||
| # cp ${MESHROOM_BUNDLE}/lib/PySide6/Qt/qml/Qt3D/Input/libQt63DInput.so.6 ${MESHROOM_BUNDLE}/lib/PySide6/Qt/qml/QtQuick/Dialogs && \ | ||
| # cp ${MESHROOM_BUNDLE}/lib/PySide6/Qt/qml/Qt3D/Logic/libQt63DLogic.so.6 ${MESHROOM_BUNDLE}/lib/PySide6/Qt/qml/QtQuick/Dialogs && \ | ||
| # cp ${MESHROOM_BUNDLE}/lib/PySide6/Qt/qml/Qt3D/Render/libQt63DRender.so.6 ${MESHROOM_BUNDLE}/lib/PySide6/Qt/qml/QtQuick/Dialogs |
| ENV MESHROOM_DEV=/opt/Meshroom | ||
| ENV MESHROOM_BUILD=/tmp/Meshroom_build | ||
| ENV QT_DIR=/opt/Qt/6.8.3/gcc_64 | ||
| ENV [email protected] | ||
| ENV QT_CI_P=azerty1. |
There was a problem hiding this comment.
Combining these environment variables into a single ENV instruction reduces the number of layers in the Docker image.
ENV MESHROOM_DEV=/opt/Meshroom \
MESHROOM_BUILD=/tmp/Meshroom_build \
QT_DIR=/opt/Qt/6.8.3/gcc_64 \
[email protected] \
QT_CI_P=azerty1.
| RUN dnf update -y --nobest | ||
| RUN dnf install -y flex fontconfig freetype glib2-devel libICE | ||
| RUN dnf install -y libX11 libXext libXi libXrender libSM | ||
| RUN dnf install -y libXt-devel mesa-libGLU-devel mesa-libOSMesa-devel mesa-libGL-devel mesa-libEGL-devel | ||
| RUN dnf install -y libXt-devel mesa-libGLU-devel mesa-libGL-devel mesa-libEGL-devel |
There was a problem hiding this comment.
Combining the package update and installation commands into a single RUN instruction reduces the layer count. Additionally, it is highly recommended to append && dnf clean all at the end of the package installation block to remove cached metadata and package files, which significantly reduces the final image size.
RUN dnf update -y --nobest && \
dnf install -y \
flex \
fontconfig \
freetype \
glib2-devel \
libICE \
libX11 \
libXext \
libXi \
libXrender \
libSM \
libXt-devel \
mesa-libGLU-devel \
mesa-libGL-devel \
mesa-libEGL-devel
| set -ex | ||
|
|
||
| test -z "$MESHROOM_VERSION" && MESHROOM_VERSION="$(git rev-parse --abbrev-ref HEAD)-$(git rev-parse --short HEAD)" | ||
| test -z "$QTAV_VERSION" && echo "qtAliceVision version not specified, set QTAV_VERSION in the environment" && exit 1 |
There was a problem hiding this comment.
Error messages should be redirected to standard error (stderr) instead of standard output (stdout) to follow standard CLI practices.
| test -z "$QTAV_VERSION" && echo "qtAliceVision version not specified, set QTAV_VERSION in the environment" && exit 1 | |
| test -z "$QTAV_VERSION" && echo "qtAliceVision version not specified, set QTAV_VERSION in the environment" >&2 && exit 1 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3145 +/- ##
===========================================
- Coverage 85.35% 85.34% -0.01%
===========================================
Files 73 73
Lines 11453 11460 +7
===========================================
+ Hits 9776 9781 +5
- Misses 1677 1679 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
No description provided.