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
Conversation
…r` and `spider_worker`) in the `clp-package` image; Set `mariadb-connector-cpp` build type to `Release` (fixes y-scope#1410).
WalkthroughAdds a Release build flag to mariadb-connector-cpp in Taskfile deps and updates the clp-package Dockerfile to include libmariadbcpp.so in the image and set relevant environment variables (including LD_LIBRARY_PATH), with some reordering of ENV and USER declarations in the final stage. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Container as clp-package Container
participant App as CLP App
participant Loader as Dynamic Linker
participant FS as /opt/clp/lib
User->>Container: Run CLP app
Container->>App: Start process (PATH resolves binary)
App->>Loader: Request libmariadbcpp.so
Note right of Loader: Uses LD_LIBRARY_PATH=/opt/clp/lib:...
Loader->>FS: Lookup libmariadbcpp.so
FS-->>Loader: Return shared library
Loader-->>App: Library loaded
App-->>User: Continues execution
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/docker-images/clp-package/Dockerfile (1)
1-27: Optional: add a simple HEALTHCHECKA lightweight HEALTHCHECK (e.g., invoking a “--version” on a core binary) improves observability. Checkov flagged its absence.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
taskfiles/deps/main.yaml(1 hunks)tools/docker-images/clp-package/Dockerfile(1 hunks)
🧰 Additional context used
🪛 Checkov (3.2.334)
tools/docker-images/clp-package/Dockerfile
[low] 1-27: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: package-image
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: rust-checks (ubuntu-22.04)
- GitHub Check: rust-checks (ubuntu-24.04)
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (1)
tools/docker-images/clp-package/Dockerfile (1)
27-27: Non‑root runtime: verify permissionsRunning as 1000:1000 is good, but please verify the user can read/execute binaries and any needed libs/configs under /opt/clp, and write to required dirs (logs, temp).
| - "-DCMAKE_BUILD_TYPE=Release" | ||
| - "-DUSE_SYSTEM_INSTALLED_LIB=ON" | ||
| - "-DINSTALL_LAYOUT=DEB" | ||
| LIB_NAME: "mariadb-connector-cpp" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Release build switch looks good; consider minor polish and verify debug needs
Setting mariadb-connector-cpp to Release aligns with the image size goal. If you don’t need install-time verbosity, consider adding LAZY install messaging for consistency with other deps. Also confirm no workflows rely on debug symbols from this lib.
CMAKE_GEN_ARGS:
- - "-DCMAKE_BUILD_TYPE=Release"
+ - "-DCMAKE_BUILD_TYPE=Release"
+ - "-DCMAKE_INSTALL_MESSAGE=LAZY"
- "-DUSE_SYSTEM_INSTALLED_LIB=ON"
- "-DINSTALL_LAYOUT=DEB"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - "-DCMAKE_BUILD_TYPE=Release" | |
| - "-DUSE_SYSTEM_INSTALLED_LIB=ON" | |
| - "-DINSTALL_LAYOUT=DEB" | |
| LIB_NAME: "mariadb-connector-cpp" | |
| CMAKE_GEN_ARGS: | |
| - "-DCMAKE_BUILD_TYPE=Release" | |
| - "-DCMAKE_INSTALL_MESSAGE=LAZY" | |
| - "-DUSE_SYSTEM_INSTALLED_LIB=ON" | |
| - "-DINSTALL_LAYOUT=DEB" | |
| LIB_NAME: "mariadb-connector-cpp" |
🤖 Prompt for AI Agents
In taskfiles/deps/main.yaml around lines 408 to 411, the CMake config sets
mariadb-connector-cpp to Release but lacks the LAZY install messaging used
elsewhere and no verification about debug-symbol dependencies; add the same LAZY
install message key/value used by other deps (to reduce install-time verbosity)
alongside the existing LIB_NAME and confirm (or document) that no CI/workflows
depend on debug symbols for this library, or if they do, switch to a debug or
split-symbols build accordingly.
| COPY --link ./build/deps/cpp/mariadb-connector-cpp-install/lib/x86_64-linux-gnu/libmariadbcpp.so \ | ||
| /opt/clp/lib/libmariadbcpp.so |
There was a problem hiding this comment.
🧩 Analysis chain
Make copy arch‑agnostic and include SONAME symlinks
Hardcoding x86_64-linux-gnu breaks arm64 builds, and copying only libmariadbcpp.so may omit versioned SONAME symlinks (e.g., .so.1), causing runtime loader failures. Copy all variants and be arch-agnostic.
-COPY --link ./build/deps/cpp/mariadb-connector-cpp-install/lib/x86_64-linux-gnu/libmariadbcpp.so \
- /opt/clp/lib/libmariadbcpp.so
+# Copy all SONAME variants across Debian multiarch subdirs (e.g., x86_64-linux-gnu, aarch64-linux-gnu)
+# and place them flat under /opt/clp/lib
+COPY --link ./build/deps/cpp/mariadb-connector-cpp-install/lib/*/libmariadbcpp.so* /opt/clp/lib/Optional: if you prefer strict control, add TARGETARCH mapping and select the correct triplet.
Also, confirm the builder uses BuildKit to support --link.
🏁 Script executed:
#!/bin/bash
echo "=== Dockerfile header ==="
head -n 20 tools/docker-images/clp-package/Dockerfile || echo "Dockerfile not found"
echo
echo "=== build/deps contents ==="
if [ -d build/deps/cpp/mariadb-connector-cpp-install/lib ]; then
find build/deps/cpp/mariadb-connector-cpp-install/lib -maxdepth 2 -type f | sed 's|^||'
else
echo "Path build/deps/cpp/mariadb-connector-cpp-install/lib not found"
fiLength of output: 631
Make COPY arch-agnostic and include SONAME symlinks
- Replace the hard-coded path with a glob to pick up all Debian multi-arch subdirs and SONAME variants:
- COPY --link ./build/deps/cpp/mariadb-connector-cpp-install/lib/x86_64-linux-gnu/libmariadbcpp.so \
- /opt/clp/lib/libmariadbcpp.so
+ COPY --link ./build/deps/cpp/mariadb-connector-cpp-install/lib/*/libmariadbcpp.so* /opt/clp/lib/- Confirm the build uses BuildKit (required for
--link), or remove--linkif not.
🤖 Prompt for AI Agents
In tools/docker-images/clp-package/Dockerfile around lines 14-15, the COPY uses
a hard-coded arch-specific path and single SONAME file; change it to copy all
architecture subdirs and SONAME variants (e.g., use a glob like
./build/deps/cpp/mariadb-connector-cpp-install/lib/*/libmariadbcpp.so* to copy
both the SONAME and symlinks into /opt/clp/lib/), and ensure the destination
folder exists or is created; also confirm the build uses BuildKit (required for
COPY --link) or remove the --link flag if BuildKit is not guaranteed.
| 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}" |
There was a problem hiding this comment.
🧹 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.
There was a problem hiding this comment.
It is intended to split the ENV into separate lines - see #1413 (comment) for the rationale
There was a problem hiding this comment.
🧩 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.
1 participant
Description
(Pending bug fixing / code refactoring changes in #1413 )
This PR includes the
libmariadbcpp.soshared library into theclp-packageDocker image. TheLD_LIBRARY_PATHin the Docker image is also updated so binaries executions will look up the clp ld library path for dynamic libraries.Also, to avoid bloating the
clp-packageimage, the build type ofmariadb-connector-cppis changed toReleasefrom the defaultRelWithDebInfo, which reduces thelibmariadbcpp.so's size from23135472 bytes (23M)to2329080 bytes (2.3M)which 10x size reduction.Checklist
breaking change.
Validation performed
Linkage fix
task docker-images:package.Size reduction
Summary by CodeRabbit
New Features
Chores