Skip to content

fix(docker): Include libmariadbcpp.so (required by spider_scheduler and spider_worker) in the clp-package image; Set mariadb-connector-cpp build type to Release (fixes #1410). #1414

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Oct 27, 2025
Merged

fix(docker): Include libmariadbcpp.so (required by spider_scheduler and spider_worker) in the clp-package image; Set mariadb-connector-cpp build type to Release (fixes #1410). #1414

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ef046e5
fix(docker): Include `libmariadbcpp.so` (required by `spider_schedule…
junhaoliao Oct 13, 2025
cfa71d8
Make COPY arch-agnostic and include SONAME symlinks
junhaoliao Oct 13, 2025
f83a4b6
Merge branch 'main' into bundle-mariadb
junhaoliao Oct 14, 2025
24db721
Merge branch 'main' into bundle-mariadb
junhaoliao Oct 18, 2025
b19b1ec
Merge remote-tracking branch 'origin/main' into bundle-mariadb
junhaoliao Oct 20, 2025
42fba6c
merge with the latest from #1413
junhaoliao Oct 20, 2025
99d353a
Merge branch 'main' into bundle-mariadb
sitaowang1998 Oct 20, 2025
8580b7a
Merge branch 'main' into bundle-mariadb
junhaoliao Oct 21, 2025
b345b02
merge ENVs
junhaoliao Oct 23, 2025
7c028f1
Merge branch 'main' into bundle-mariadb
junhaoliao Oct 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions taskfiles/deps/main.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,6 +405,7 @@ tasks:
- task: "utils:install-remote-cmake-lib"
vars:
CMAKE_GEN_ARGS:
- "-DCMAKE_BUILD_TYPE=Release"
- "-DUSE_SYSTEM_INSTALLED_LIB=ON"
- "-DINSTALL_LAYOUT=DEB"
LIB_NAME: "mariadb-connector-cpp"
Expand Down
16 changes: 9 additions & 7 deletions tools/docker-images/clp-package/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,17 @@ RUN ./setup-scripts/install-prebuilt-packages.sh \
RUN apt-get clean \
&& rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/*

ENV CLP_HOME="/opt/clp"
ENV PATH="${CLP_HOME}/bin:${PATH}"
ENV PATH="${CLP_HOME}/sbin:${PATH}"
ENV PYTHONPATH="${CLP_HOME}/lib/python3/site-packages"

USER 1000:1000

COPY ./build/clp-package /opt/clp
COPY --link ./build/deps/cpp/mariadb-connector-cpp-install/lib/*/libmariadbcpp.so* /opt/clp/lib/

# Flatten the image
FROM scratch
COPY --from=base / /

ENV CLP_HOME="/opt/clp"
ENV LD_LIBRARY_PATH="${CLP_HOME}/lib" \
PATH="${CLP_HOME}/bin:${PATH}" \
PYTHONPATH="${CLP_HOME}/lib/python3/site-packages"
ENV PATH="${CLP_HOME}/sbin:${PATH}"
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Consolidate ENV and avoid duplicate PATH edits

Combine into one ENV to reduce layers and keep order deterministic. Optionally preserve any pre-set LD_LIBRARY_PATH.

-ENV CLP_HOME="/opt/clp"
-ENV LD_LIBRARY_PATH="${CLP_HOME}/lib" \
-    PATH="${CLP_HOME}/bin:${PATH}" \
-    PYTHONPATH="${CLP_HOME}/lib/python3/site-packages"
-ENV PATH="${CLP_HOME}/sbin:${PATH}"
+ENV CLP_HOME=/opt/clp \
+    PATH="${CLP_HOME}/sbin:${CLP_HOME}/bin:${PATH}" \
+    PYTHONPATH="${CLP_HOME}/lib/python3/site-packages" \
+    LD_LIBRARY_PATH="${CLP_HOME}/lib:${LD_LIBRARY_PATH}"
🤖 Prompt for AI Agents 
In tools/docker-images/clp-package/Dockerfile around lines 21 to 25, there are
two ENV directives that duplicate PATH modification and create extra image
layers; consolidate them into a single ENV that sets CLP_HOME, LD_LIBRARY_PATH
(preserving any existing value by appending :${LD_LIBRARY_PATH} if present),
PATH (prepend ${CLP_HOME}/sbin and ${CLP_HOME}/bin before existing ${PATH} to
keep deterministic order), and PYTHONPATH in one line so PATH is only modified
once and layer count is reduced.

Copy link
Member Author

@junhaoliao junhaoliao Oct 13, 2025

Choose a reason for hiding this comment

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

It is intended to split the ENV into separate lines - see #1413 (comment) for the rationale

Copy link
Contributor

@coderabbitai coderabbitai bot Oct 13, 2025

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch the discussion/comment from PR #1413 to understand the rationale
gh pr view 1413 --repo y-scope/clp --json comments --jq '.comments[] | select(.id == "r2425132080" or .url | contains("r2425132080"))'

# Also try fetching review comments
gh api repos/y-scope/clp/pulls/1413/comments --jq '.[] | select(.id == 2425132080 or (.html_url | contains("r2425132080")))'

Length of output: 3377

@junhaoliao, thank you for the clarification! I understand now that the team has an established preference for splitting ENV directives (especially PATH modifications) into separate lines for readability, even at the cost of additional layers. This aligns with the previous discussion in #1166 and makes sense given the dependency between the ENV statements (later ones reference CLP_HOME defined first).

I'll remember this convention for future reviews of Dockerfiles in this repository.

ฅ(=^・ω・^=)ฅ

✏️ Learnings added 
Learnt from: junhaoliao
PR: y-scope/clp#1414
File: tools/docker-images/clp-package/Dockerfile:20-24
Timestamp: 2025-10-13T03:32:19.263Z
Learning: In the clp repository's Dockerfiles (e.g., tools/docker-images/clp-package/Dockerfile), ENV directives should be split into separate lines for readability rather than consolidated to reduce layer count. This is especially true for PATH modifications, as agreed upon in PR #1166. Later ENV settings may depend on earlier ones (e.g., referencing CLP_HOME).

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


USER 1000:1000
Loading