ci: Add manylinux_2_28 and musllinux_1_2 dependency image and binary jobs to clp-core-build GH workflow. #1261
Conversation
WalkthroughAdds manylinux_2_28 and musllinux_1_2 x86_64 image change signals, path filters, per-arch deps-image build jobs, and per-arch binaries test jobs to clp-core-build workflow. Updates tooling docs to reference/pull the new images and revises workflow documentation and diagram to include the new jobs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant GH as GitHub Actions
participant F as filter-relevant-changes
participant MLDeps as manylinux_2_28-x86_64-deps-image
participant MLBin as manylinux_2_28-x86_64-binaries
participant MUDeps as musllinux_1_2-x86_64-deps-image
participant MUBin as musllinux_1_2-x86_64-binaries
participant Reg as Container Registry
Dev->>GH: Push / PR
GH->>F: Run path filters
Note over F: Outputs<br/>manylinux_2_28_x86_64_image_changed<br/>musllinux_1_2_x86_64_image_changed
alt manylinux_2_28 image changed == true
F-->>MLDeps: gate passes
MLDeps->>Reg: Build & push deps image (OS_NAME=manylinux_2_28)
else
F-->>MLDeps: gate fails (skip)
end
alt musllinux_1_2 image changed == true
F-->>MUDeps: gate passes
MUDeps->>Reg: Build & push deps image (OS_NAME=musllinux_1_2)
else
F-->>MUDeps: gate fails (skip)
end
par Binaries (matrix use_shared_libs=true/false)
MLDeps-->>MLBin: need satisfied (if MLDeps ran)
F-->>MLBin: also needs filter outputs
MLBin->>Reg: Pull deps image (published or local)
MLBin->>MLBin: Run tests on image
MUDeps-->>MUBin: need satisfied (if MUDeps ran)
F-->>MUBin: also needs filter outputs
MUBin->>Reg: Pull deps image (published or local)
MUBin->>MUBin: Run tests on image
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Bill-hbrhbr
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/src/dev-guide/components-core/index.md (1)
80-95: Optional: Add container run examples for the new OS images (parity with Ubuntu example).Since the page now lists Manylinux and Musllinux native setups, a brief Docker example for those environments would help users who prefer containerised builds.
You can use these commands to start a container in which you can build and run CLP: ```shell # Make sure to change /path/to/clp/components/core and /path/to/my/logs below docker run --rm -it \ --name 'clp-build-env' \ -u$(id -u):$(id -g) \ -v$(readlink -f /path/to/clp/components/core):/mnt/clp \ -v$(readlink -f /path/to/my/logs):/mnt/logs \ ghcr.io/y-scope/clp/clp-core-dependencies-x86-ubuntu-jammy:main \ /bin/bash -l + +# Example (Manylinux 2.28): +# docker run --rm -it \ +# --name 'clp-build-env' \ +# -u$(id -u):$(id -g) \ +# -v$(readlink -f /path/to/clp/components/core):/mnt/clp \ +# -v$(readlink -f /path/to/my/logs):/mnt/logs \ +# ghcr.io/y-scope/clp/clp-core-dependencies-x86-manylinux_2_28:main \ +# /bin/bash -l + +# Example (Musllinux 1.2): +# docker run --rm -it \ +# --name 'clp-build-env' \ +# -u$(id -u):$(id -g) \ +# -v$(readlink -f /path/to/clp/components/core):/mnt/clp \ +# -v$(readlink -f /path/to/my/logs):/mnt/logs \ +# ghcr.io/y-scope/clp/clp-core-dependencies-x86-musllinux_1_2:main \ +# /bin/sh -lIf you’d like, I can adjust shells (/bin/bash vs /bin/sh) based on what’s actually present in the new images. </blockquote></details> </blockquote></details>📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.github/workflows/clp-core-build.yaml(4 hunks)docs/src/dev-guide/components-core/index.md(2 hunks)docs/src/dev-guide/components-core/manylinux-2-28-deps-install.md(1 hunks)docs/src/dev-guide/components-core/musllinux-1-2-deps-install.md(1 hunks)🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Bill-hbrhbr PR: y-scope/clp#1122 File: components/core/src/clp/clp/CMakeLists.txt:175-195 Timestamp: 2025-07-23T09:54:45.185Z Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.📚 Learning: 2025-05-26T16:03:05.519Z
Learnt from: quinntaylormitchell PR: y-scope/clp#918 File: .github/workflows/clp-execution-image-build.yaml:77-97 Timestamp: 2025-05-26T16:03:05.519Z Learning: In .github/workflows/clp-execution-image-build.yaml, the ubuntu-jammy-execution-image and ubuntu-noble-execution-image jobs are intentionally kept separate (rather than using a matrix strategy) to make it easier to remove individual platform versions when they reach end of life, such as when jammy eventually becomes obsolete.Applied to files:
.github/workflows/clp-core-build.yaml⏰ 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). (5)
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: musllinux-1-2-deps-image
- GitHub Check: manylinux-2-28-deps-image
- GitHub Check: ubuntu-jammy-deps-image
🔇 Additional comments (7)
docs/src/dev-guide/components-core/musllinux-1-2-deps-install.md (1)
15-18: Good call-out on manual uninstall for source-built packages.Clear expectation-setting; this avoids surprises for users who expect their system package manager to track these installs.
docs/src/dev-guide/components-core/manylinux-2-28-deps-install.md (1)
15-18: Good clarity on manual uninstall requirement.This note aligns user expectations with how the scripts install source-built libs.
docs/src/dev-guide/components-core/index.md (2)
72-76: New OS docs entries look consistent and discoverable.The “Native Environment” list correctly adds Manylinux 2.28 and Musllinux 1.2 and matches the toctree entries.
117-121: Toctree updates align with the new pages.The added entries correctly reference the new dependency-install guides.
.github/workflows/clp-core-build.yaml (3)
48-50: Outputs for new image-change signals are wired correctly.The new manylinux_2_28 and musllinux_1_2 outputs match the filter keys and are exposed for downstream jobs.
81-93: Filters cover the right paths for the new images.Patterns include the shared scripts, workflow, actions, and the specific Docker-image and lib_install trees for each OS target—this mirrors the existing CentOS/Ubuntu setup.
164-189: Same path verification for musllinux_1_2 (-x86_64 suffix).Mirrors the manylinux point; confirm the Dockerfile path exists and is intended to differ from centos/ubuntu.
See the script in my previous comment; it checks both targets. If these paths are correct, no action needed.
docs/src/dev-guide/components-core/manylinux-2-28-deps-install.md
Outdated
Show resolved
Hide resolved
docs/src/dev-guide/components-core/manylinux-2-28-deps-install.md
Outdated
Show resolved
Hide resolved
docs/src/dev-guide/components-core/musllinux-1-2-deps-install.md
Outdated
Show resolved
Hide resolved
docs/src/dev-guide/components-core/musllinux-1-2-deps-install.md
Outdated
Show resolved
Hide resolved
Bill-hbrhbr
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
docs/src/dev-guide/components-core/index.md(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
⏰ 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). (6)
- GitHub Check: ubuntu-jammy-deps-image
- GitHub Check: musllinux-1-2-deps-image
- GitHub Check: centos-stream-9-deps-image
- GitHub Check: manylinux-2-28-deps-image
- GitHub Check: antlr-code-committed (macos-15)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (1)
docs/src/dev-guide/components-core/index.md (1)
119-120: Verified: Dependency doc pages are correctly added and linkedBoth
manylinux-2-28-deps-install.mdandmusllinux-1-2-deps-install.mdexist underdocs/src/dev-guide/components-core/, each begins with a proper top-level H1 (# Manylinux 2.28and# Musllinux 1.2), and the hidden toctree inindex.mdreferences their slugs in the correct order. All checks pass—approving these changes.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
.github/workflows/clp-core-build.yaml (2)
312-318: Note on getconf fallback (acknowledging fail-fast preference)
getconfmay not be present in all environments; previously discussed and you prefer to let CI fail and fix as-needed. No action requested.
352-358: Same note on getconf for musllinuxSame consideration as above; acknowledging the fail-fast approach.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/clp-core-build.yaml(4 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1261
File: .github/workflows/clp-core-build.yaml:294-332
Timestamp: 2025-08-25T06:29:59.602Z
Learning: In the CLP project, Bill-hbrhbr prefers a "fail fast" approach for CI workflows - allowing potential command availability issues (like getconf in musllinux) to surface through CI failures rather than preemptively adding fallback logic, as they will fix issues when they occur.
📚 Learning: 2025-05-26T16:03:05.519Z
Learnt from: quinntaylormitchell
PR: y-scope/clp#918
File: .github/workflows/clp-execution-image-build.yaml:77-97
Timestamp: 2025-05-26T16:03:05.519Z
Learning: In .github/workflows/clp-execution-image-build.yaml, the ubuntu-jammy-execution-image and ubuntu-noble-execution-image jobs are intentionally kept separate (rather than using a matrix strategy) to make it easier to remove individual platform versions when they reach end of life, such as when jammy eventually becomes obsolete.
Applied to files:
.github/workflows/clp-core-build.yaml
📚 Learning: 2025-08-25T06:29:59.602Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1261
File: .github/workflows/clp-core-build.yaml:294-332
Timestamp: 2025-08-25T06:29:59.602Z
Learning: In the CLP project, Bill-hbrhbr prefers a "fail fast" approach for CI workflows - allowing potential command availability issues (like getconf in musllinux) to surface through CI failures rather than preemptively adding fallback logic, as they will fix issues when they occur.
Applied to files:
.github/workflows/clp-core-build.yaml
📚 Learning: 2025-07-23T09:54:45.185Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Applied to files:
.github/workflows/clp-core-build.yaml
📚 Learning: 2025-08-09T04:07:27.083Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Applied to files:
.github/workflows/clp-core-build.yaml
📚 Learning: 2025-08-04T17:26:17.098Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/CMakeLists.txt:167-170
Timestamp: 2025-08-04T17:26:17.098Z
Learning: In CLP's CMake configuration for BZip2, Bill-hbrhbr prefers using HINTS parameter for path-based resolution rather than version pinning in find_package(). The primary concern is ensuring the task-installed BZip2 is found instead of system copies, not enforcing specific versions.
Applied to files:
.github/workflows/clp-core-build.yaml
📚 Learning: 2025-08-19T14:41:28.901Z
Learnt from: junhaoliao
PR: y-scope/clp#1152
File: components/clp-package-utils/clp_package_utils/general.py:0-0
Timestamp: 2025-08-19T14:41:28.901Z
Learning: In the CLP codebase, prefer explicit failures over automatic directory creation in utility functions like dump_config. The user junhaoliao prefers to let file operations fail when parent directories don't exist, as this helps catch implementation errors during development rather than masking setup issues with automatic directory creation.
Applied to files:
.github/workflows/clp-core-build.yaml
📚 Learning: 2025-07-07T17:43:04.349Z
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-aarch64/build.sh:3-5
Timestamp: 2025-07-07T17:43:04.349Z
Learning: In CLP project build scripts (specifically build.sh files in docker-images directories), maintain consistency with the established pattern of using separate `set -eu` and `set -o pipefail` commands rather than combining them into `set -euo pipefail`, to ensure uniform script structure across all platform build scripts.
Applied to files:
.github/workflows/clp-core-build.yaml
📚 Learning: 2025-08-04T17:23:14.646Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/CMakeLists.txt:171-172
Timestamp: 2025-08-04T17:23:14.646Z
Learning: Bill-hbrhbr prefers concise, straightforward code over verbose defensive programming in CMake files. When find_package uses REQUIRED flag, additional existence checks are redundant and should be avoided.
Applied to files:
.github/workflows/clp-core-build.yaml
⏰ 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). (5)
- GitHub Check: ubuntu-jammy-deps-image
- GitHub Check: musllinux-1-2-deps-image
- GitHub Check: manylinux-2-28-deps-image
- GitHub Check: centos-stream-9-deps-image
- GitHub Check: manylinux-2-28-aarch64-deps-image
🔇 Additional comments (3)
.github/workflows/clp-core-build.yaml (3)
48-50: Outputs wiring for new images looks correctThe new outputs are named consistently and consumed by downstream jobs. No issues.
164-189: manylinux_2_28 x86_64 deps-image job looks goodOS_NAME vs Dockerfile path (with explicit
-x86_64) is consistent with your naming scheme; publication gating is correct.
190-215: musllinux_1_2 deps-image job looks goodConditionals, image path, and publish gating match the established pattern.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
.github/workflows/clp-core-build.yaml (3)
321-361: LGTM; previous notes on getconf/runner label already discussed.No change requested: the fail-fast stance on getconf and the ubuntu-24.04-arm runner label were covered earlier in the thread.
403-443: LGTM; prior getconf discussion applies here too.No action needed given the accepted fail-fast approach.
152-177: Nit: avoid “-x86-” prefix in AArch64 deps image names.The global DEPS_IMAGE_NAME_PREFIX includes “x86-”; overriding image_name here prevents misleading tags.
Apply:
- uses: "./.github/actions/clp-core-build-containers" env: OS_NAME: "manylinux_2_28-aarch64" with: - image_name: "${{env.DEPS_IMAGE_NAME_PREFIX}}${{env.OS_NAME}}" + image_name: "clp-core-dependencies-aarch64-${{env.OS_NAME}}" docker_context: "components/core" docker_file: "components/core/tools/docker-images/clp-env-base-${{env.OS_NAME}}/Dockerfile"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/clp-core-build.yaml(4 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
📚 Learning: 2025-05-26T16:03:05.519Z
Learnt from: quinntaylormitchell
PR: y-scope/clp#918
File: .github/workflows/clp-execution-image-build.yaml:77-97
Timestamp: 2025-05-26T16:03:05.519Z
Learning: In .github/workflows/clp-execution-image-build.yaml, the ubuntu-jammy-execution-image and ubuntu-noble-execution-image jobs are intentionally kept separate (rather than using a matrix strategy) to make it easier to remove individual platform versions when they reach end of life, such as when jammy eventually becomes obsolete.
Applied to files:
.github/workflows/clp-core-build.yaml
📚 Learning: 2025-08-25T06:29:59.602Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1261
File: .github/workflows/clp-core-build.yaml:294-332
Timestamp: 2025-08-25T06:29:59.602Z
Learning: In the CLP project, Bill-hbrhbr prefers a "fail fast" approach for CI workflows - allowing potential command availability issues (like getconf in musllinux) to surface through CI failures rather than preemptively adding fallback logic, as they will fix issues when they occur.
Applied to files:
.github/workflows/clp-core-build.yaml
📚 Learning: 2025-07-23T09:54:45.185Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Applied to files:
.github/workflows/clp-core-build.yaml
📚 Learning: 2025-08-09T04:07:27.083Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Applied to files:
.github/workflows/clp-core-build.yaml
📚 Learning: 2025-08-04T17:26:17.098Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/CMakeLists.txt:167-170
Timestamp: 2025-08-04T17:26:17.098Z
Learning: In CLP's CMake configuration for BZip2, Bill-hbrhbr prefers using HINTS parameter for path-based resolution rather than version pinning in find_package(). The primary concern is ensuring the task-installed BZip2 is found instead of system copies, not enforcing specific versions.
Applied to files:
.github/workflows/clp-core-build.yaml
📚 Learning: 2025-08-19T14:41:28.901Z
Learnt from: junhaoliao
PR: y-scope/clp#1152
File: components/clp-package-utils/clp_package_utils/general.py:0-0
Timestamp: 2025-08-19T14:41:28.901Z
Learning: In the CLP codebase, prefer explicit failures over automatic directory creation in utility functions like dump_config. The user junhaoliao prefers to let file operations fail when parent directories don't exist, as this helps catch implementation errors during development rather than masking setup issues with automatic directory creation.
Applied to files:
.github/workflows/clp-core-build.yaml
📚 Learning: 2025-08-04T17:23:14.646Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/CMakeLists.txt:171-172
Timestamp: 2025-08-04T17:23:14.646Z
Learning: Bill-hbrhbr prefers concise, straightforward code over verbose defensive programming in CMake files. When find_package uses REQUIRED flag, additional existence checks are redundant and should be avoided.
Applied to files:
.github/workflows/clp-core-build.yaml
📚 Learning: 2025-07-07T17:43:04.349Z
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-aarch64/build.sh:3-5
Timestamp: 2025-07-07T17:43:04.349Z
Learning: In CLP project build scripts (specifically build.sh files in docker-images directories), maintain consistency with the established pattern of using separate `set -eu` and `set -o pipefail` commands rather than combining them into `set -euo pipefail`, to ensure uniform script structure across all platform build scripts.
Applied to files:
.github/workflows/clp-core-build.yaml
⏰ 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). (7)
- GitHub Check: ubuntu-jammy-deps-image
- GitHub Check: musllinux_1_2-x86_64-deps-image
- GitHub Check: centos-stream-9-deps-image
- GitHub Check: manylinux_2_28-aarch64-deps-image
- GitHub Check: musllinux_1_2-aarch64-deps-image
- GitHub Check: manylinux_2_28-x86_64-deps-image
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (6)
.github/workflows/clp-core-build.yaml (6)
48-51: LGTM: new image change outputs are correctly plumbed through.Naming matches the filters you add below and integrates cleanly with existing jobs.
83-89: LGTM: manylinux_2_28_aarch64 filter looks complete.Covers actions/workflow, shared lib_install scripts, the aarch64 Dockerfile dir, and the per-OS install dir.
178-203: LGTM: manylinux_2_28-x86_64 deps-image job mirrors existing patterns.Conditioning, runner, and inputs are consistent with CentOS/Ubuntu jobs.
230-255: LGTM: musllinux_1_2-x86_64 deps-image job is consistent.Triggers, runner selection, and inputs align with the established convention.
362-402: LGTM: manylinux_2_28-x86_64 binaries job.Matrix, gating, and run-on-image usage are correct.
444-484: LGTM: musllinux_1_2-x86_64 binaries job mirrors the pattern faithfully.Conditionals, matrix, and command wiring look good.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/clp-core-build.yaml (1)
197-222: Fix: x86_64 musllinux deps image should use X86 prefix (current var is undefined)Use the X86-prefixed image name var to avoid undefined DEPS_IMAGE_NAME_PREFIX.
- uses: "./.github/actions/clp-core-build-containers" env: OS_NAME: "musllinux_1_2-x86_64" with: - image_name: "${{env.DEPS_IMAGE_NAME_PREFIX}}${{env.OS_NAME}}" + image_name: "${{env.DEPS_IMAGE_NAME_PREFIX_X86}}${{env.OS_NAME}}" docker_context: "components/core" docker_file: "components/core/tools/docker-images/clp-env-base-${{env.OS_NAME}}/Dockerfile" push_deps_image: >- ${{github.event_name != 'pull_request' && github.ref == 'refs/heads/main'}} token: "${{secrets.GITHUB_TOKEN}}"
♻️ Duplicate comments (1)
.github/workflows/clp-core-build.yaml (1)
171-196: Fix: AArch64 musllinux deps image still uses undefined DEPS_IMAGE_NAME_PREFIXThis AArch64 job should use the ARM prefix to avoid the old “-x86-” in the image name and to avoid an undefined variable reference.
- uses: "./.github/actions/clp-core-build-containers" env: OS_NAME: "musllinux_1_2-aarch64" with: - image_name: "${{env.DEPS_IMAGE_NAME_PREFIX}}${{env.OS_NAME}}" + image_name: "${{env.DEPS_IMAGE_NAME_PREFIX_ARM}}${{env.OS_NAME}}" docker_context: "components/core" docker_file: "components/core/tools/docker-images/clp-env-base-${{env.OS_NAME}}/Dockerfile" push_deps_image: >- ${{github.event_name != 'pull_request' && github.ref == 'refs/heads/main'}} token: "${{secrets.GITHUB_TOKEN}}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/clp-core-build.yaml(6 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
📚 Learning: 2025-05-26T16:03:05.519Z
Learnt from: quinntaylormitchell
PR: y-scope/clp#918
File: .github/workflows/clp-execution-image-build.yaml:77-97
Timestamp: 2025-05-26T16:03:05.519Z
Learning: In .github/workflows/clp-execution-image-build.yaml, the ubuntu-jammy-execution-image and ubuntu-noble-execution-image jobs are intentionally kept separate (rather than using a matrix strategy) to make it easier to remove individual platform versions when they reach end of life, such as when jammy eventually becomes obsolete.
Applied to files:
.github/workflows/clp-core-build.yaml
📚 Learning: 2025-07-23T09:54:45.185Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Applied to files:
.github/workflows/clp-core-build.yaml
📚 Learning: 2025-08-25T06:29:59.602Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1261
File: .github/workflows/clp-core-build.yaml:294-332
Timestamp: 2025-08-25T06:29:59.602Z
Learning: In the CLP project, Bill-hbrhbr prefers a "fail fast" approach for CI workflows - allowing potential command availability issues (like getconf in musllinux) to surface through CI failures rather than preemptively adding fallback logic, as they will fix issues when they occur.
Applied to files:
.github/workflows/clp-core-build.yaml
📚 Learning: 2025-08-09T04:07:27.083Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Applied to files:
.github/workflows/clp-core-build.yaml
📚 Learning: 2025-08-04T17:26:17.098Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/CMakeLists.txt:167-170
Timestamp: 2025-08-04T17:26:17.098Z
Learning: In CLP's CMake configuration for BZip2, Bill-hbrhbr prefers using HINTS parameter for path-based resolution rather than version pinning in find_package(). The primary concern is ensuring the task-installed BZip2 is found instead of system copies, not enforcing specific versions.
Applied to files:
.github/workflows/clp-core-build.yaml
📚 Learning: 2025-08-19T14:41:28.901Z
Learnt from: junhaoliao
PR: y-scope/clp#1152
File: components/clp-package-utils/clp_package_utils/general.py:0-0
Timestamp: 2025-08-19T14:41:28.901Z
Learning: In the CLP codebase, prefer explicit failures over automatic directory creation in utility functions like dump_config. The user junhaoliao prefers to let file operations fail when parent directories don't exist, as this helps catch implementation errors during development rather than masking setup issues with automatic directory creation.
Applied to files:
.github/workflows/clp-core-build.yaml
📚 Learning: 2025-07-07T17:43:04.349Z
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-aarch64/build.sh:3-5
Timestamp: 2025-07-07T17:43:04.349Z
Learning: In CLP project build scripts (specifically build.sh files in docker-images directories), maintain consistency with the established pattern of using separate `set -eu` and `set -o pipefail` commands rather than combining them into `set -euo pipefail`, to ensure uniform script structure across all platform build scripts.
Applied to files:
.github/workflows/clp-core-build.yaml
📚 Learning: 2025-08-04T17:23:14.646Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/CMakeLists.txt:171-172
Timestamp: 2025-08-04T17:23:14.646Z
Learning: Bill-hbrhbr prefers concise, straightforward code over verbose defensive programming in CMake files. When find_package uses REQUIRED flag, additional existence checks are redundant and should be avoided.
Applied to files:
.github/workflows/clp-core-build.yaml
🪛 actionlint (1.7.7)
.github/workflows/clp-core-build.yaml
139-139: context "matrix" is not allowed here. available contexts are "github", "inputs", "needs", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
139-139: context "matrix" is not allowed here. available contexts are "github", "inputs", "needs", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
139-139: context "matrix" is not allowed here. available contexts are "github", "inputs", "needs", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
149-149: got unexpected character '4' while lexing character following number 0, expecting ''', '}', '(', ')', '[', ']', '.', '!', '<', '>', '=', '&', '|', '*', ',', ' '
(expression)
288-288: job "manylinux_2_28-aarch64-binaries" needs job "manylinux_2_28-aarch64-deps-image" which does not exist in this workflow
(job-needs)
329-329: job "manylinux_2_28-x86_64-binaries" needs job "manylinux_2_28-x86_64-deps-image" which does not exist in this workflow
(job-needs)
⏰ 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). (1)
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (4)
.github/workflows/clp-core-build.yaml (4)
49-52: Outputs for new image-change flags look goodThe four new outputs are correctly wired to the filter step outputs. No issues spotted.
84-107: Filters correctly watch Dockerfile dirs and per-OS install scriptsGood coverage for both arches across manylinux_2_28 and musllinux_1_2, including install script subtrees. This should reliably flip the “image_changed” flags.
329-369: Blocking: same two issues for x86_64 manylinux binaries (needs + image-name var)Align with the deps-image matrix and use the X86 prefix.
- needs: - - "manylinux_2_28-x86_64-deps-image" - - "filter-relevant-changes" + needs: + - "deps-image" + - "filter-relevant-changes" @@ - uses: "./.github/actions/run-on-image" env: OS_NAME: "manylinux_2_28-x86_64" with: - image_name: "${{env.DEPS_IMAGE_NAME_PREFIX}}${{env.OS_NAME}}" + image_name: "${{env.DEPS_IMAGE_NAME_PREFIX_X86}}${{env.OS_NAME}}" use_published_image: >- ${{needs.filter-relevant-changes.outputs.manylinux_2_28_x86_64_image_changed == 'false' || (github.event_name != 'pull_request' && github.ref == 'refs/heads/main')}}⛔ Skipped due to learnings
Learnt from: quinntaylormitchell PR: y-scope/clp#918 File: .github/workflows/clp-execution-image-build.yaml:77-97 Timestamp: 2025-05-26T16:03:05.519Z Learning: In .github/workflows/clp-execution-image-build.yaml, the ubuntu-jammy-execution-image and ubuntu-noble-execution-image jobs are intentionally kept separate (rather than using a matrix strategy) to make it easier to remove individual platform versions when they reach end of life, such as when jammy eventually becomes obsolete.
411-451: Fix: x86_64 musllinux binaries should use X86 image prefixSame undefined var issue here; use the X86-prefixed variable.
- uses: "./.github/actions/run-on-image" env: OS_NAME: "musllinux_1_2-x86_64" with: - image_name: "${{env.DEPS_IMAGE_NAME_PREFIX}}${{env.OS_NAME}}" + image_name: "${{env.DEPS_IMAGE_NAME_PREFIX_X86}}${{env.OS_NAME}}" use_published_image: >- ${{needs.filter-relevant-changes.outputs.musllinux_1_2_x86_64_image_changed == 'false' || (github.event_name != 'pull_request' && github.ref == 'refs/heads/main')}}⛔ Skipped due to learnings
Learnt from: quinntaylormitchell PR: y-scope/clp#918 File: .github/workflows/clp-execution-image-build.yaml:77-97 Timestamp: 2025-05-26T16:03:05.519Z Learning: In .github/workflows/clp-execution-image-build.yaml, the ubuntu-jammy-execution-image and ubuntu-noble-execution-image jobs are intentionally kept separate (rather than using a matrix strategy) to make it easier to remove individual platform versions when they reach end of life, such as when jammy eventually becomes obsolete.
Bill-hbrhbr
changed the title
Bill-hbrhbr
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/src/dev-guide/tooling-containers.md (1)
30-43: Clarify x86 vs x86_64 naming to avoid confusionThis section mixes “x86” (image name) and “x86_64” (directory name). Consider adding a short note that repository tags use the “x86” prefix while the on-disk Dockerfile path uses “-x86_64”, or align the headings to “x86_64” for consistency.
Proposed tweak to the heading to make the arch explicit:
-### clp-core-dependencies-x86-manylinux_2_28 +### clp-core-dependencies-x86_64-manylinux_2_28 (image repo uses x86 prefix)
♻️ Duplicate comments (3)
docs/src/dev-guide/tooling-gh-workflows.md (1)
52-63: Use canonical identifiers “manylinux_2_28” / “musllinux_1_2” in proseFor consistency with PEP nomenclature and your job names, consider using the canonical identifiers in prose instead of “Manylinux 2.28” / “Musllinux 1.2”, and call them “target build environments (not OSes)”.
Suggested edit (example):
-* `manylinux_2_28-x86_64-deps-image`: Builds a container image ... in a Manylinux 2.28 x86 environment. +* `manylinux_2_28-x86_64-deps-image`: Builds a container image ... in a manylinux_2_28 (glibc) x86_64 target environment..github/workflows/clp-core-build.yaml (2)
255-295: Binaries job (manylinux): one operational note on CPU detectionCommand uses getconf for CPU count inside the container. If the base image ever lacks getconf, this will fail. You’ve indicated a “fail-fast” preference; leaving as-is is fine.
Optional robust form (only if you decide to harden later):
- --num-jobs $(getconf _NPROCESSORS_ONLN) + --num-jobs $(command -v getconf >/dev/null 2>&1 && getconf _NPROCESSORS_ONLN || nproc)
296-336: Binaries job (musllinux): mirrors manylinux flow; same CPU detection caveatPattern mirrors manylinux job with correct image gating. Same optional hardening applies for getconf if you ever change stance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.github/workflows/clp-core-build.yaml(4 hunks)docs/src/dev-guide/components-core/index.md(1 hunks)docs/src/dev-guide/tooling-containers.md(3 hunks)docs/src/dev-guide/tooling-gh-workflows.md(2 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
📚 Learning: 2025-08-25T07:27:19.449Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1262
File: docs/src/dev-guide/components-core/index.md:50-50
Timestamp: 2025-08-25T07:27:19.449Z
Learning: In the CLP project, nlohmann_json version 3.11.3 is installed through their dependency management process, but they don't enforce version constraints in CMake find_package calls - they control versions through installation rather than CMake version pinning.
Applied to files:
docs/src/dev-guide/components-core/index.md
📚 Learning: 2025-05-26T16:03:05.519Z
Learnt from: quinntaylormitchell
PR: y-scope/clp#918
File: .github/workflows/clp-execution-image-build.yaml:77-97
Timestamp: 2025-05-26T16:03:05.519Z
Learning: In .github/workflows/clp-execution-image-build.yaml, the ubuntu-jammy-execution-image and ubuntu-noble-execution-image jobs are intentionally kept separate (rather than using a matrix strategy) to make it easier to remove individual platform versions when they reach end of life, such as when jammy eventually becomes obsolete.
Applied to files:
docs/src/dev-guide/tooling-gh-workflows.md.github/workflows/clp-core-build.yaml
📚 Learning: 2025-08-25T06:29:59.610Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1261
File: .github/workflows/clp-core-build.yaml:294-332
Timestamp: 2025-08-25T06:29:59.610Z
Learning: In the CLP project, Bill-hbrhbr prefers a "fail fast" approach for CI workflows - allowing potential command availability issues (like getconf in musllinux) to surface through CI failures rather than preemptively adding fallback logic, as they will fix issues when they occur.
Applied to files:
.github/workflows/clp-core-build.yaml
📚 Learning: 2025-07-23T09:54:45.185Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Applied to files:
.github/workflows/clp-core-build.yaml
📚 Learning: 2025-08-09T04:07:27.083Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Applied to files:
.github/workflows/clp-core-build.yaml
📚 Learning: 2025-08-04T17:26:17.098Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/CMakeLists.txt:167-170
Timestamp: 2025-08-04T17:26:17.098Z
Learning: In CLP's CMake configuration for BZip2, Bill-hbrhbr prefers using HINTS parameter for path-based resolution rather than version pinning in find_package(). The primary concern is ensuring the task-installed BZip2 is found instead of system copies, not enforcing specific versions.
Applied to files:
.github/workflows/clp-core-build.yaml
📚 Learning: 2025-08-19T14:41:28.901Z
Learnt from: junhaoliao
PR: y-scope/clp#1152
File: components/clp-package-utils/clp_package_utils/general.py:0-0
Timestamp: 2025-08-19T14:41:28.901Z
Learning: In the CLP codebase, prefer explicit failures over automatic directory creation in utility functions like dump_config. The user junhaoliao prefers to let file operations fail when parent directories don't exist, as this helps catch implementation errors during development rather than masking setup issues with automatic directory creation.
Applied to files:
.github/workflows/clp-core-build.yaml
📚 Learning: 2025-08-04T17:23:14.646Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/CMakeLists.txt:171-172
Timestamp: 2025-08-04T17:23:14.646Z
Learning: Bill-hbrhbr prefers concise, straightforward code over verbose defensive programming in CMake files. When find_package uses REQUIRED flag, additional existence checks are redundant and should be avoided.
Applied to files:
.github/workflows/clp-core-build.yaml
📚 Learning: 2025-07-07T17:43:04.349Z
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-aarch64/build.sh:3-5
Timestamp: 2025-07-07T17:43:04.349Z
Learning: In CLP project build scripts (specifically build.sh files in docker-images directories), maintain consistency with the established pattern of using separate `set -eu` and `set -o pipefail` commands rather than combining them into `set -euo pipefail`, to ensure uniform script structure across all platform build scripts.
Applied to files:
.github/workflows/clp-core-build.yaml
🪛 LanguageTool
docs/src/dev-guide/tooling-gh-workflows.md
[uncategorized] ~52-~52: Loose punctuation mark.
Context: ...nt. * manylinux_2_28-x86_64-deps-image: Builds a container image containing the...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~54-~54: Loose punctuation mark.
Context: ...ent. * musllinux_1_2-x86_64-deps-image: Builds a container image containing the...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~56-~56: Loose punctuation mark.
Context: ...environment. * ubuntu-jammy-deps-image: Builds a container image containing the...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~58-~58: Loose punctuation mark.
Context: ...nvironment. * centos-stream-9-binaries: Builds the CLP-core binaries in the bui...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~60-~60: Loose punctuation mark.
Context: ...ests. * manylinux_2_28-x86_64-binaries: Builds the CLP-core binaries in the bui...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~62-~62: Loose punctuation mark.
Context: ...tests. * musllinux_1_2-x86_64-binaries: Builds the CLP-core binaries in the bui...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ 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). (4)
- GitHub Check: ubuntu-jammy-deps-image
- GitHub Check: manylinux_2_28-x86_64-deps-image
- GitHub Check: centos-stream-9-deps-image
- GitHub Check: musllinux_1_2-x86_64-deps-image
🔇 Additional comments (9)
docs/src/dev-guide/tooling-containers.md (3)
32-37: LGTM: Added GHCR link and pull command match workflow image_nameThe link target and pull command align with the workflow’s image_name prefix “clp-core-dependencies-x86-” and OS_NAME “manylinux_2_28”. Good consistency across docs and CI.
65-70: LGTM: Musllinux x86_64 entry consistent with workflow configurationThe GHCR page reference and pull command for musllinux_1_2 match the new CI jobs and naming pattern.
151-152: Validate new package links render correctlyGitHub Packages links for manylinux_2_28 and musllinux_1_2 use percent-encoding (clp%2F...). Please verify they resolve as expected after publish.
If desired, I can provide a small script to curl the package metadata and confirm availability once the images are pushed on main.
docs/src/dev-guide/components-core/index.md (1)
41-45: Naming consistency looks good; minor note on “json” labelUsing “json” as the label while linking to nlohmann/json.git is acceptable and clear in context. No action needed.
docs/src/dev-guide/tooling-gh-workflows.md (1)
32-42: Diagram edges match job IDs in the workflowThe added Mermaid edges for manylinux_2_28-x86_64 and musllinux_1_2-x86_64 correspond to the new jobs in clp-core-build.yaml. Good alignment.
.github/workflows/clp-core-build.yaml (4)
48-50: Outputs are well named and scopedNew outputs (manylinux_2_28_x86_64_image_changed, musllinux_1_2_x86_64_image_changed) align with filter keys and are consumed correctly by downstream jobs.
81-92: Filters correctly scope image rebuild triggersFilter sets include both Dockerfile directories and per-distro install scripts. This should prevent unnecessary rebuilds while catching relevant changes.
138-163: manylinux_2_28 deps-image: naming and paths consistent
- image_name uses the global x86- prefixed scheme and OS_NAME “manylinux_2_28”.
- docker_file path includes “-x86_64”, matching your repo layout.
Looks good.
164-189: musllinux_1_2 deps-image: mirrors manylinux pattern appropriatelyNaming, filter gating, and docker_file path are consistent with the manylinux job and the docs.
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/src/dev-docs/tooling-containers.md (1)
22-31: Clarify publishing status for aarch64 variants (avoid confusion).
Sections state “aarch64 or x86,” but only x86_64 GH Packages links/pull commands are provided. Consider adding a brief note that only x86_64 images are published today (if that’s the intent), or add the aarch64 links if they exist.Also applies to: 55-63
.github/workflows/clp-core-build.yaml (1)
130-133: Nit: avoid backslash line continuations in docker_file strings.
They’re brittle in YAML; single-line strings are clearer and less error-prone.Apply for each docker_file occurrence:
- docker_file: "components/core/tools/docker-images/clp-env-base-${{env.OS_NAME}}\ - /Dockerfile" + docker_file: "components/core/tools/docker-images/clp-env-base-${{env.OS_NAME}}/Dockerfile"and for the x86_64-specific ones:
- docker_file: "components/core/tools/docker-images/clp-env-base-${{env.OS_NAME}}-x86_64\ - /Dockerfile" + docker_file: "components/core/tools/docker-images/clp-env-base-${{env.OS_NAME}}-x86_64/Dockerfile"- images: >- - ghcr.io/${{steps.sanitize_repo_name.outputs.repository}}/clp-core-x86-${{env.OS_NAME}} + images: "ghcr.io/${{steps.sanitize_repo_name.outputs.repository}}/clp-core-x86-${{env.OS_NAME}}"Also applies to: 158-159, 184-185, 208-211, 443-446
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.github/workflows/clp-core-build.yaml(4 hunks)docs/src/dev-docs/tooling-containers.md(3 hunks)docs/src/dev-docs/tooling-gh-workflows.md(2 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Learnt from: quinntaylormitchell
PR: y-scope/clp#918
File: .github/workflows/clp-execution-image-build.yaml:77-97
Timestamp: 2025-05-26T16:03:05.519Z
Learning: In .github/workflows/clp-execution-image-build.yaml, the ubuntu-jammy-execution-image and ubuntu-noble-execution-image jobs are intentionally kept separate (rather than using a matrix strategy) to make it easier to remove individual platform versions when they reach end of life, such as when jammy eventually becomes obsolete.
📚 Learning: 2025-05-26T16:03:05.519Z
Learnt from: quinntaylormitchell
PR: y-scope/clp#918
File: .github/workflows/clp-execution-image-build.yaml:77-97
Timestamp: 2025-05-26T16:03:05.519Z
Learning: In .github/workflows/clp-execution-image-build.yaml, the ubuntu-jammy-execution-image and ubuntu-noble-execution-image jobs are intentionally kept separate (rather than using a matrix strategy) to make it easier to remove individual platform versions when they reach end of life, such as when jammy eventually becomes obsolete.
Applied to files:
docs/src/dev-docs/tooling-gh-workflows.md.github/workflows/clp-core-build.yaml
📚 Learning: 2025-08-25T06:29:59.610Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1261
File: .github/workflows/clp-core-build.yaml:294-332
Timestamp: 2025-08-25T06:29:59.610Z
Learning: In the CLP project, Bill-hbrhbr prefers a "fail fast" approach for CI workflows - allowing potential command availability issues (like getconf in musllinux) to surface through CI failures rather than preemptively adding fallback logic, as they will fix issues when they occur.
Applied to files:
.github/workflows/clp-core-build.yaml
📚 Learning: 2025-07-23T09:54:45.185Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Applied to files:
.github/workflows/clp-core-build.yaml
📚 Learning: 2025-08-09T04:07:27.083Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Applied to files:
.github/workflows/clp-core-build.yaml
📚 Learning: 2025-08-04T17:26:17.098Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/CMakeLists.txt:167-170
Timestamp: 2025-08-04T17:26:17.098Z
Learning: In CLP's CMake configuration for BZip2, Bill-hbrhbr prefers using HINTS parameter for path-based resolution rather than version pinning in find_package(). The primary concern is ensuring the task-installed BZip2 is found instead of system copies, not enforcing specific versions.
Applied to files:
.github/workflows/clp-core-build.yaml
📚 Learning: 2025-08-19T14:41:28.901Z
Learnt from: junhaoliao
PR: y-scope/clp#1152
File: components/clp-package-utils/clp_package_utils/general.py:0-0
Timestamp: 2025-08-19T14:41:28.901Z
Learning: In the CLP codebase, prefer explicit failures over automatic directory creation in utility functions like dump_config. The user junhaoliao prefers to let file operations fail when parent directories don't exist, as this helps catch implementation errors during development rather than masking setup issues with automatic directory creation.
Applied to files:
.github/workflows/clp-core-build.yaml
📚 Learning: 2025-07-07T17:43:04.349Z
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-aarch64/build.sh:3-5
Timestamp: 2025-07-07T17:43:04.349Z
Learning: In CLP project build scripts (specifically build.sh files in docker-images directories), maintain consistency with the established pattern of using separate `set -eu` and `set -o pipefail` commands rather than combining them into `set -euo pipefail`, to ensure uniform script structure across all platform build scripts.
Applied to files:
.github/workflows/clp-core-build.yaml
📚 Learning: 2025-08-04T17:23:14.646Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/CMakeLists.txt:171-172
Timestamp: 2025-08-04T17:23:14.646Z
Learning: Bill-hbrhbr prefers concise, straightforward code over verbose defensive programming in CMake files. When find_package uses REQUIRED flag, additional existence checks are redundant and should be avoided.
Applied to files:
.github/workflows/clp-core-build.yaml
🪛 LanguageTool
docs/src/dev-docs/tooling-gh-workflows.md
[uncategorized] ~52-~52: Loose punctuation mark.
Context: ...nt. * manylinux_2_28-x86_64-deps-image: Builds a container image containing the...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~54-~54: Loose punctuation mark.
Context: ...ent. * musllinux_1_2-x86_64-deps-image: Builds a container image containing the...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~56-~56: Loose punctuation mark.
Context: ...environment. * ubuntu-jammy-deps-image: Builds a container image containing the...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~58-~58: Loose punctuation mark.
Context: ...nvironment. * centos-stream-9-binaries: Builds the CLP-core binaries in the bui...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~60-~60: Loose punctuation mark.
Context: ...ests. * manylinux_2_28-x86_64-binaries: Builds the CLP-core binaries in the bui...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~62-~62: Loose punctuation mark.
Context: ...tests. * musllinux_1_2-x86_64-binaries: Builds the CLP-core binaries in the bui...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ 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). (6)
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: musllinux_1_2-x86_64-deps-image
- GitHub Check: ubuntu-jammy-deps-image
- GitHub Check: manylinux_2_28-x86_64-deps-image
- GitHub Check: antlr-code-committed (macos-15)
🔇 Additional comments (10)
docs/src/dev-docs/tooling-containers.md (3)
32-37: LGTM: adds x86_64 manylinux pull instructions and package link aligned with workflow image name.
Names and tag (:main) match the workflow’s image_name convention.
65-70: LGTM: adds x86_64 musllinux pull instructions and package link aligned with workflow image name.
Consistent with new jobs in clp-core-build.yaml.
151-152: LGTM: link refs resolve to expected GHCR package slugs.
The reference names match the in-text links.docs/src/dev-docs/tooling-gh-workflows.md (1)
30-44: Diagram update matches the new jobs and keeps the DAG readable.
LR orientation and edges reflect the added manylinux/musllinux paths..github/workflows/clp-core-build.yaml (6)
48-50: LGTM: new outputs expose image-change signals for manylinux and musllinux.
Downstream conditions can now gate jobs correctly.
81-92: Filters look correct and minimal.
They include action, workflow, common lib_install scripts, per-OS docker dirs, and per-OS install dirs.
138-163: Deps-image (manylinux x86_64): configuration and paths align with repo structure.
OS_NAME, image_name prefix, and Dockerfile path (-x86_64) are consistent with container docs.
164-189: Deps-image (musllinux x86_64): mirrors manylinux pattern correctly.
Naming and push conditions match existing conventions.
255-294: Binaries (manylinux x86_64): gating and run-on-image usage are consistent.
Conditional use of published image vs rebuilt image is correct; matrix over shared/static mirrors other jobs.
295-334: Binaries (musllinux x86_64): mirrors manylinux job; fine to fail-fast on getconf per team preference.
Pattern matches other platforms; no blockers.
|
Add an open issue about aarch64 manylinux/musllinux github workflows hanging for <os_name>-aarch64-deps-image and <os_name>-aarch64-binaries see https://github.com/y-scope/clp/actions/runs/17205948952/job/48874833249 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll create an issue about the aarch64 manylinux/musllinux GitHub workflows hanging problem. @Bill-hbrhbr I've created an issue to track the aarch64 manylinux/musllinux GitHub workflows hanging problem. The issue includes details about the affected jobs, references the specific GitHub Actions run you provided, and includes backlinks to this PR and your comment. 🏁 Script executed: Length of output: 1017 @Bill-hbrhbr Perfect! I've successfully created issue #1268 to track the aarch64 manylinux/musllinux GitHub workflows hanging problem. The issue includes:
You can view the issue at: #1268 🧠 Learnings used |
Bill-hbrhbr
changed the title
3 participants
Description
This PR updates the GitHub CI workflow to build and test CLP dependencies and core binaries on both stable
manylinux and musllinux platforms. This ensures portability across mainstream Linux distros,
supports Python extension packaging, improves Kubernetes/Alpine compatibility, and enforces stricter
compilation guarantees for more reliable builds.
Python extension support
Python wheels on PyPI must comply with manylinux/musllinux tags (PEP 513/656). Ensuring CLP builds
cleanly on these platforms makes it easier to release and distribute our Python extension libraries,
such as clp-ffi-py.
Cross-distro compatibility
manylinuxtargets glibc-based distros (Ubuntu, Debian, CentOS, Fedora, etc.).musllinuxtargets musl-based distros (e.g. Alpine Linux).Together, they make CLP binaries portable across nearly all mainstream Linux environments.
Kubernetes & Alpine compatibility
Many Kubernetes base images are built on Alpine Linux, making musllinux buildable is critical for
runtime compatibility in containerized deployments.
Stricter compilation requirements
musllinux enforces stricter compilation rules than glibc (e.g., requiring explicit headers).
A successful build on musllinux improves confidence that CLP will build reliably across other
supported environments.
Checklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Chores
Documentation
Tests