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 the mariadb-connector-cpp CMake generation and updates the clp-package Dockerfile to include 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 LibDir 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 #DDDDDD: LD_LIBRARY_PATH includes /opt/clp/lib
Loader->>LibDir: Lookup libmariadbcpp.so
LibDir-->>Loader: Return shared library
Loader-->>App: Library loaded
App-->>User: Continues execution
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Points to review:
Possibly related issues
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-10-13T03:32:19.293ZApplied to files:
📚 Learning: 2025-10-20T21:05:30.417ZApplied to files:
🪛 Checkov (3.2.334)tools/docker-images/clp-package/Dockerfile[low] 1-32: 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). (12)
🔇 Additional comments (3)
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).
| 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.
# Conflicts: # tools/docker-images/clp-package/Dockerfile
|
This draft PR depends on #1413 and therefore should be merged after #1413 is merged (@sitaowang1998 do you want to take a look at #1413 as well?) |
| FROM scratch | ||
| COPY --link --from=base / / | ||
|
|
||
| ARG UID=1000 |
There was a problem hiding this comment.
| ARG UID=1000 | |
| ARG USER=clp-user | |
| ARG UID=1000 | |
| ARG GID=1000 | |
| RUN groupadd -g "${GID}" "${UID}" \ | |
| && useradd --create-home --base-dir /home/ --shell /bin/bash --gid "${GID}" --uid "${UID}" "{USER}" | |
| USER "${USER}" | |
| WORKDIR "/home/${USER}" |
if you want to mimic a real user home working environment. I think it would be beneficial to pass in the user id and group id of the github CI runner that builds and runs this package.
There was a problem hiding this comment.
But if your system works as is, I don't really see a problem with it.
There was a problem hiding this comment.
good point. the changes on the user creation were actually done by another PR #1413 . since this PR aims to only make the spider binaries work, let's do the group creation in another PR.
@coderabbitai add an issue to track the group creation in the docker image
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue to track group creation in the docker image
gh issue create \
--title "Improve user and group setup in clp-package Docker image" \
--body "## Context
This issue tracks improvements to the user and group creation process in the \`clp-package\` Docker image, as discussed in PR #1414.
## Background
During review of PR #1414, it was suggested that the Docker image could benefit from proper group creation and a more realistic user home environment setup. The current implementation creates a user but does not create a dedicated group or set up a traditional home directory structure.
## Suggested Improvements
- Add group creation with configurable GID
- Create user with proper group assignment
- Set up a traditional home directory under \`/home/\`
- Make UID and GID configurable as build arguments
## References
- PR: https://github.com/y-scope/clp/pull/1414
- Comment: https://github.com/y-scope/clp/pull/1414#discussion_r2455360711
- Requested by: @junhaoliao
## Notes
These changes were deferred from PR #1414 to keep that PR focused on making the spider binaries work. PR #1413 already made some initial user creation changes." \
--assignee junhaoliao
Length of output: 42
@junhaoliao I've created issue #1508 to track the group creation improvements for the Docker image: #1508
The issue has been assigned to you and includes references back to this PR and the discussion. Good call on keeping this PR focused on the spider binaries functionality!
# Conflicts: # tools/docker-images/clp-package/Dockerfile
3 participants
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