feat(core): Add support for building and running on manylinux_2_28; Add manylinux_2_28 dependency container images. #1041
Conversation
WalkthroughThis update introduces new Docker build scripts and Dockerfiles for manylinux_2_28 (x86_64 and aarch64) environments, adds modular installation scripts for prebuilt and source packages, and revises documentation to detail the new container images. It also adjusts static linking detection and dependency requirements in build scripts. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BuildScript as build.sh
participant Docker
participant Dockerfile
participant InstallAll as install-all.sh
participant InstallPrebuilt as install-prebuilt-packages.sh
participant InstallSource as install-packages-from-source.sh
User->>BuildScript: Run build.sh
BuildScript->>Docker: docker buildx build (with labels)
Docker->>Dockerfile: Execute Dockerfile steps
Dockerfile->>InstallAll: Run install-all.sh
InstallAll->>InstallPrebuilt: Run install-prebuilt-packages.sh
InstallAll->>InstallSource: Run install-packages-from-source.sh
InstallAll->>InstallAll: Run check-cmake-version.sh
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 25
🔭 Outside diff range comments (1)
docs/src/dev-guide/components-core/index.md (1)
7-8: Broken self-reference:#native-environmentanchor no longer existsThe text links to
(#native-environment), but the heading was renamed to “Native Build Environment” (line 36). This will generate a dead link in the rendered docs and is already flagged by the docs pipeline.- We have built and tested CLP on the OSes listed [below](#native-environment). + We have built and tested CLP on the OSes listed [below](#native-build-environment).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
components/core/CMakeLists.txt(2 hunks)components/core/tools/docker-images/clp-env-base-manylinux_2_28_aarch64/Dockerfile(1 hunks)components/core/tools/docker-images/clp-env-base-manylinux_2_28_aarch64/build.sh(1 hunks)components/core/tools/docker-images/clp-env-base-manylinux_2_28_x86_64/Dockerfile(1 hunks)components/core/tools/docker-images/clp-env-base-manylinux_2_28_x86_64/build.sh(1 hunks)components/core/tools/scripts/lib_install/libarchive.sh(1 hunks)components/core/tools/scripts/lib_install/manylinux_2_28/install-all.sh(1 hunks)components/core/tools/scripts/lib_install/manylinux_2_28/install-packages-from-source.sh(1 hunks)components/core/tools/scripts/lib_install/manylinux_2_28/install-prebuilt-packages.sh(1 hunks)components/core/tools/scripts/utils/README.md(1 hunks)components/core/tools/scripts/utils/build-and-run-unit-tests.py(3 hunks)components/core/tools/scripts/utils/build-with-docker.py(1 hunks)components/core/tools/scripts/utils/build.py(1 hunks)docs/src/dev-guide/components-core/index.md(3 hunks)docs/src/dev-guide/components-core/manylinux_2_28-deps-install.md(1 hunks)docs/src/dev-guide/tooling-containers.md(2 hunks)
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: quinntaylormitchell
PR: y-scope/clp#968
File: docs/src/user-guide/quick-start/overview.md:53-54
Timestamp: 2025-06-18T20:48:48.990Z
Learning: CLP is designed to run on Linux systems where Python is typically pre-installed, so Python installation links are generally not needed in CLP documentation.
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.
Learnt from: gibber9809
PR: y-scope/clp#955
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-26
Timestamp: 2025-06-02T18:22:24.060Z
Learning: In the clp project, ANTLR code generation at build time is being removed by another PR. When reviewing CMake files, be aware that some temporary suboptimal configurations may exist to reduce merge conflicts between concurrent PRs, especially around ANTLR_TARGET calls.
Learnt from: LinZhihao-723
PR: y-scope/clp#486
File: components/core/tests/test-error_handling.cpp:37-38
Timestamp: 2024-11-26T19:12:43.982Z
Learning: In the CLP project, C++20 is used, allowing the utilization of C++20 features such as class template argument deduction (CTAD) with `std::array`.
Learnt from: jackluo923
PR: y-scope/clp#718
File: components/core/tools/scripts/utils/create-debian-package.py:41-41
Timestamp: 2025-02-12T22:24:17.723Z
Learning: For the clp-core Debian package creation script, strict version format validation is considered unnecessary complexity and should be avoided.
Learnt from: quinntaylormitchell
PR: y-scope/clp#968
File: docs/src/user-guide/quick-start/overview.md:73-109
Timestamp: 2025-06-18T20:39:05.899Z
Learning: The CLP project team prefers to use video content to demonstrate detailed procedural steps (like tarball extraction) rather than including every step in the written documentation, keeping the docs focused on conceptual guidance.
components/core/tools/scripts/lib_install/libarchive.sh (1)
Learnt from: AVMatthews
PR: y-scope/clp#595
File: components/core/tests/test-end_to_end.cpp:59-65
Timestamp: 2024-11-19T17:30:04.970Z
Learning: In 'components/core/tests/test-end_to_end.cpp', during the 'clp-s_compression_and_extraction_no_floats' test, files and directories are intentionally removed at the beginning of the test to ensure that any existing content doesn't influence the test results.
docs/src/dev-guide/tooling-containers.md (6)
Learnt from: quinntaylormitchell
PR: y-scope/clp#968
File: docs/src/user-guide/quick-start/overview.md:73-109
Timestamp: 2025-06-18T20:39:05.899Z
Learning: The CLP project team prefers to use video content to demonstrate detailed procedural steps (like tarball extraction) rather than including every step in the written documentation, keeping the docs focused on conceptual guidance.
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.
Learnt from: quinntaylormitchell
PR: y-scope/clp#968
File: docs/src/user-guide/quick-start/overview.md:53-54
Timestamp: 2025-06-18T20:48:48.990Z
Learning: CLP is designed to run on Linux systems where Python is typically pre-installed, so Python installation links are generally not needed in CLP documentation.
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.
Learnt from: LinZhihao-723
PR: y-scope/clp#486
File: components/core/tests/test-error_handling.cpp:37-38
Timestamp: 2024-11-26T19:12:43.982Z
Learning: In the CLP project, C++20 is used, allowing the utilization of C++20 features such as class template argument deduction (CTAD) with `std::array`.
Learnt from: jackluo923
PR: y-scope/clp#718
File: components/core/tools/scripts/utils/create-debian-package.py:41-41
Timestamp: 2025-02-12T22:24:17.723Z
Learning: For the clp-core Debian package creation script, strict version format validation is considered unnecessary complexity and should be avoided.
components/core/tools/scripts/lib_install/manylinux_2_28/install-all.sh (3)
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts in `components/clp-package-utils/clp_package_utils/scripts/`, note that it's preferred to keep error handling simple without adding extra complexity.
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:11-12
Timestamp: 2025-05-06T10:07:04.654Z
Learning: In CLP installation scripts, temporary directories with downloaded files should not be automatically cleaned up on failure (e.g., with EXIT traps) to preserve artifacts for debugging purposes.
components/core/tools/docker-images/clp-env-base-manylinux_2_28_aarch64/Dockerfile (1)
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.
components/core/CMakeLists.txt (2)
Learnt from: gibber9809
PR: y-scope/clp#955
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-26
Timestamp: 2025-06-02T18:22:24.060Z
Learning: In the clp project, ANTLR code generation at build time is being removed by another PR. When reviewing CMake files, be aware that some temporary suboptimal configurations may exist to reduce merge conflicts between concurrent PRs, especially around ANTLR_TARGET calls.
Learnt from: quinntaylormitchell
PR: y-scope/clp#968
File: docs/src/user-guide/quick-start/overview.md:53-54
Timestamp: 2025-06-18T20:48:48.990Z
Learning: CLP is designed to run on Linux systems where Python is typically pre-installed, so Python installation links are generally not needed in CLP documentation.
components/core/tools/docker-images/clp-env-base-manylinux_2_28_x86_64/Dockerfile (1)
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.
components/core/tools/docker-images/clp-env-base-manylinux_2_28_aarch64/build.sh (1)
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.
components/core/tools/scripts/lib_install/manylinux_2_28/install-packages-from-source.sh (5)
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts in `components/clp-package-utils/clp_package_utils/scripts/`, note that it's preferred to keep error handling simple without adding extra complexity.
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:11-12
Timestamp: 2025-05-06T10:07:04.654Z
Learning: In CLP installation scripts, temporary directories with downloaded files should not be automatically cleaned up on failure (e.g., with EXIT traps) to preserve artifacts for debugging purposes.
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/package-template/src/sbin/del-archives.sh:6-9
Timestamp: 2024-11-15T16:28:08.644Z
Learning: For wrapper scripts in the `components/package-template/src/sbin/` directory, keep them simple and avoid adding additional validation code.
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:15-23
Timestamp: 2025-05-06T09:46:42.639Z
Learning: For temporary solutions in installation scripts like those in `components/core/tools/scripts/lib_install/`, checksumming downloaded files is considered optional, particularly when working with trusted sources like GitHub raw content.
docs/src/dev-guide/components-core/index.md (6)
Learnt from: quinntaylormitchell
PR: y-scope/clp#968
File: docs/src/user-guide/quick-start/overview.md:73-109
Timestamp: 2025-06-18T20:39:05.899Z
Learning: The CLP project team prefers to use video content to demonstrate detailed procedural steps (like tarball extraction) rather than including every step in the written documentation, keeping the docs focused on conceptual guidance.
Learnt from: gibber9809
PR: y-scope/clp#955
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-26
Timestamp: 2025-06-02T18:22:24.060Z
Learning: In the clp project, ANTLR code generation at build time is being removed by another PR. When reviewing CMake files, be aware that some temporary suboptimal configurations may exist to reduce merge conflicts between concurrent PRs, especially around ANTLR_TARGET calls.
Learnt from: quinntaylormitchell
PR: y-scope/clp#968
File: docs/src/user-guide/quick-start/overview.md:53-54
Timestamp: 2025-06-18T20:48:48.990Z
Learning: CLP is designed to run on Linux systems where Python is typically pre-installed, so Python installation links are generally not needed in CLP documentation.
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.
Learnt from: LinZhihao-723
PR: y-scope/clp#486
File: components/core/tests/test-error_handling.cpp:37-38
Timestamp: 2024-11-26T19:12:43.982Z
Learning: In the CLP project, C++20 is used, allowing the utilization of C++20 features such as class template argument deduction (CTAD) with `std::array`.
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:11-12
Timestamp: 2025-05-06T10:07:04.654Z
Learning: In CLP installation scripts, temporary directories with downloaded files should not be automatically cleaned up on failure (e.g., with EXIT traps) to preserve artifacts for debugging purposes.
components/core/tools/scripts/lib_install/manylinux_2_28/install-prebuilt-packages.sh (3)
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts in `components/clp-package-utils/clp_package_utils/scripts/`, note that it's preferred to keep error handling simple without adding extra complexity.
Learnt from: anlowee
PR: y-scope/clp#925
File: taskfiles/deps/main.yaml:97-106
Timestamp: 2025-05-28T18:33:30.155Z
Learning: In the taskfiles dependency system (taskfiles/deps/main.yaml), echo commands are used to generate .cmake settings files that are consumed by the main CMake build process. These files set variables like ANTLR_RUNTIME_HEADER to point to dependency locations for use during compilation.
docs/src/dev-guide/components-core/manylinux_2_28-deps-install.md (3)
Learnt from: quinntaylormitchell
PR: y-scope/clp#968
File: docs/src/user-guide/quick-start/overview.md:73-109
Timestamp: 2025-06-18T20:39:05.899Z
Learning: The CLP project team prefers to use video content to demonstrate detailed procedural steps (like tarball extraction) rather than including every step in the written documentation, keeping the docs focused on conceptual guidance.
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.
Learnt from: quinntaylormitchell
PR: y-scope/clp#968
File: docs/src/user-guide/quick-start/overview.md:53-54
Timestamp: 2025-06-18T20:48:48.990Z
Learning: CLP is designed to run on Linux systems where Python is typically pre-installed, so Python installation links are generally not needed in CLP documentation.
components/core/tools/docker-images/clp-env-base-manylinux_2_28_x86_64/build.sh (3)
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.
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts in `components/clp-package-utils/clp_package_utils/scripts/`, note that it's preferred to keep error handling simple without adding extra complexity.
components/core/tools/scripts/utils/build.py (1)
Learnt from: gibber9809
PR: y-scope/clp#955
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-26
Timestamp: 2025-06-02T18:22:24.060Z
Learning: In the clp project, ANTLR code generation at build time is being removed by another PR. When reviewing CMake files, be aware that some temporary suboptimal configurations may exist to reduce merge conflicts between concurrent PRs, especially around ANTLR_TARGET calls.
🪛 markdownlint-cli2 (0.17.2)
docs/src/dev-guide/tooling-containers.md
6-6: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
43-43: Link and image reference definitions should be needed
Unused link or image reference definition: "core-deps-manylinux_2_28_x86_64"
(MD053, link-image-reference-definitions)
44-44: Link and image reference definitions should be needed
Unused link or image reference definition: "core-deps-manylinux_2_28_aarch64"
(MD053, link-image-reference-definitions)
components/core/tools/scripts/utils/README.md
4-4: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
6-6: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
6-6: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
9-9: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
9-9: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
11-11: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
docs/src/dev-guide/components-core/index.md
26-26: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
31-31: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
35-35: Multiple consecutive blank lines
Expected: 1; Actual: 2
(MD012, no-multiple-blanks)
52-52: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
105-105: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
🪛 GitHub Actions: clp-docs
docs/src/dev-guide/tooling-containers.md
[warning] 6-6: 'myst' cross-reference target not found: 'core-deps-manylinux_2_28_x86_64' [myst.xref_missing]
[warning] 6-6: 'myst' cross-reference target not found: 'core-deps-manylinux_2_28_aarch64' [myst.xref_missing]
docs/src/dev-guide/components-core/index.md
[warning] 7-7: 'myst' cross-reference target not found: 'native-environment' [myst.xref_missing]
[warning] 40-40: 'myst' cross-reference target not found: 'manylinux_2_28' [myst.xref_missing]
docs/src/dev-guide/components-core/manylinux_2_28-deps-install.md
[warning] 1-1: Document isn't included in any toctree.
🪛 Checkov (3.2.334)
components/core/tools/docker-images/clp-env-base-manylinux_2_28_aarch64/Dockerfile
[LOW] 13-13: Ensure that COPY is used instead of ADD in Dockerfiles
(CKV_DOCKER_4)
[LOW] 1-1: Ensure the base image uses a non latest version tag
(CKV_DOCKER_7)
[LOW] 1-23: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
[LOW] 1-23: Ensure that a user for the container has been created
(CKV_DOCKER_3)
components/core/tools/docker-images/clp-env-base-manylinux_2_28_x86_64/Dockerfile
[LOW] 13-13: Ensure that COPY is used instead of ADD in Dockerfiles
(CKV_DOCKER_4)
[LOW] 1-1: Ensure the base image uses a non latest version tag
(CKV_DOCKER_7)
[LOW] 1-23: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
[LOW] 1-23: Ensure that a user for the container has been created
(CKV_DOCKER_3)
🪛 Hadolint (2.12.0)
components/core/tools/docker-images/clp-env-base-manylinux_2_28_aarch64/Dockerfile
[warning] 1-1: Always tag the version of an image explicitly
(DL3006)
[error] 13-13: Use COPY instead of ADD for files and folders
(DL3020)
components/core/tools/docker-images/clp-env-base-manylinux_2_28_x86_64/Dockerfile
[warning] 1-1: Always tag the version of an image explicitly
(DL3006)
[error] 13-13: Use COPY instead of ADD for files and folders
(DL3020)
🪛 LanguageTool
components/core/tools/scripts/utils/README.md
[style] ~3-~3: To form a complete sentence, be sure to include a subject.
Context: ...egorized utility scripts. * build.py can be used to trigger a local platform bui...
(MISSING_IT_THERE)
[uncategorized] ~6-~6: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ... * manylinux_2_28 based images are used and the output binaries are expected...
(COMMA_COMPOUND_SENTENCE_2)
[style] ~11-~11: To form a complete sentence, be sure to include a subject.
Context: ...-p yes*build-and-run-unit-tests.py` can be used to perform a local platform ...
(MISSING_IT_THERE)
docs/src/dev-guide/components-core/index.md
[uncategorized] ~32-~32: Loose punctuation mark.
Context: ... ``` * --output <output_directory>: Directory to place the built CLP binary...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 Shellcheck (0.10.0)
components/core/tools/scripts/lib_install/manylinux_2_28/install-prebuilt-packages.sh
[warning] 58-58: Declare and assign separately to avoid masking return values.
(SC2155)
[info] 62-62: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 63-63: Double quote to prevent globbing and word splitting.
(SC2086)
🪛 Pylint (3.3.7)
components/core/tools/scripts/utils/build-and-run-unit-tests.py
[convention] 48-48: Line too long (103/100)
(C0301)
components/core/tools/scripts/utils/build.py
[convention] 1-1: Missing module docstring
(C0114)
[warning] 29-29: Use lazy % formatting in logging functions
(W1203)
[warning] 33-33: Use lazy % formatting in logging functions
(W1203)
[convention] 71-71: Missing function or method docstring
(C0116)
[warning] 90-90: Use lazy % formatting in logging functions
(W1203)
[warning] 101-101: Use lazy % formatting in logging functions
(W1203)
[warning] 106-106: Use lazy % formatting in logging functions
(W1203)
[warning] 109-109: Use lazy % formatting in logging functions
(W1203)
components/core/tools/scripts/utils/build-with-docker.py
[convention] 1-1: Missing module docstring
(C0114)
[convention] 1-1: Module name "build-with-docker" doesn't conform to snake_case naming style
(C0103)
[warning] 50-50: Use lazy % formatting in logging functions
(W1203)
[warning] 54-54: Use lazy % formatting in logging functions
(W1203)
[warning] 99-99: Use lazy % formatting in logging functions
(W1203)
[convention] 103-103: Missing function or method docstring
(C0116)
[warning] 127-127: Use lazy % formatting in logging functions
(W1203)
[warning] 128-128: Use lazy % formatting in logging functions
(W1203)
[warning] 129-129: Use lazy % formatting in logging functions
(W1203)
[warning] 4-4: Unused import pathlib
(W0611)
[warning] 8-8: Unused Optional imported from typing
(W0611)
🪛 Flake8 (7.2.0)
components/core/tools/scripts/utils/build.py
[error] 28-28: expected 2 blank lines, found 1
(E302)
components/core/tools/scripts/utils/build-with-docker.py
[error] 4-4: 'pathlib' imported but unused
(F401)
[error] 8-8: 'typing.Optional' imported but unused
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: ubuntu-jammy-deps-image
- GitHub Check: centos-stream-9-deps-image
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (13)
components/core/tools/scripts/lib_install/libarchive.sh (1)
66-67: Good call disabling OpenSSL in static buildAdding
-DENABLE_OPENSSL=OFFavoids unexpected dynamic linkage on some distros. Implementation is correct.components/core/CMakeLists.txt (1)
218-223: ```shell
#!/bin/bashLocate CurlDownloadHandler.cpp to confirm its path
rg -l 'CurlDownloadHandler.cpp'
Search the entire codebase for any usage of CURLOPT_HTTP_VERSION
rg -n 'CURLOPT_HTTP_VERSION' .
</details> <details> <summary>components/core/tools/docker-images/clp-env-base-manylinux_2_28_x86_64/build.sh (1)</summary> `3-17`: **Add `set -o pipefail` for robust error propagation** The script enables `-e` & `-u` but omits `pipefail`; if any piped command in the future fails silently, the build continues. Add it to keep the strict mode consistent with other CLP scripts. ```diff -set -euo pipefail +# shell options: exit on error, undefined var, and failed pipelines +set -euo pipefail⛔ Skipped due to learnings
Learnt from: haiqi96 PR: y-scope/clp#594 File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65 Timestamp: 2024-11-18T16:49:20.248Z Learning: When reviewing wrapper scripts in `components/clp-package-utils/clp_package_utils/scripts/`, note that it's preferred to keep error handling simple without adding extra complexity.Learnt from: kirkrodrigues PR: y-scope/clp#881 File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41 Timestamp: 2025-05-06T09:48:55.408Z Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.Learnt from: kirkrodrigues PR: y-scope/clp#881 File: components/core/tools/scripts/lib_install/macos/install-all.sh:11-12 Timestamp: 2025-05-06T10:07:04.654Z Learning: In CLP installation scripts, temporary directories with downloaded files should not be automatically cleaned up on failure (e.g., with EXIT traps) to preserve artifacts for debugging purposes.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.components/core/tools/scripts/lib_install/manylinux_2_28/install-all.sh (1)
14-15: Open TODO could block production imagesThe TODO references a gating CMake version check. If issue #795 is unresolved the image might ship with an incompatible CMake later on. Confirm the desired minimum/maximum CMake version is enforced before merging.
docs/src/dev-guide/components-core/manylinux_2_28-deps-install.md (1)
1-25: ```shell
#!/bin/bashDisplay the toctree entries around line 120 in components-core index
sed -n '110,140p' docs/src/dev-guide/components-core/index.md
</details> <details> <summary>components/core/tools/docker-images/clp-env-base-manylinux_2_28_aarch64/build.sh (1)</summary> `1-32`: **LGTM! Well-implemented Docker build script.** The script demonstrates excellent practices: - Proper error handling with `set -euo pipefail` - Safe command construction using arrays - Appropriate platform targeting for cross-compilation - Conditional Git metadata inclusion - Clear progress logging The cross-platform build support with automatic QEMU emulation is particularly valuable for CI/CD environments. </details> <details> <summary>components/core/tools/scripts/utils/build-and-run-unit-tests.py (2)</summary> `3-3`: **Good addition for CPU detection.** The `os` import is properly used for `os.cpu_count()` to set the default number of build jobs. --- `56-69`: **Excellent refactoring to centralize build logic.** The delegation to `build.py` promotes code reuse and maintainability. The logging statements provide good visibility into the build process, and the error handling through `subprocess.run(check=True)` is appropriate. </details> <details> <summary>components/core/tools/scripts/utils/build-with-docker.py (2)</summary> `29-38`: **Well-designed platform configuration architecture.** The platform configuration dictionary provides a clean and extensible way to manage different build environments. This makes it easy to add support for additional platforms in the future. --- `82-100`: **Solid Docker container orchestration.** The Docker command construction properly handles user permissions, volume mounting, and platform specification. The error handling and logging provide good visibility into the build process. </details> <details> <summary>components/core/tools/scripts/utils/build.py (2)</summary> `92-96`: **Excellent integration with project task system.** The use of `task clean` and `task deps:core` properly integrates with the project's existing build workflow and dependency management system. This ensures consistency across different build methods. --- `64-68`: **Robust project root detection.** The `_find_project_root()` function provides a reliable way to locate the project root regardless of where the script is invoked from. The use of a constant for depth makes it maintainable. </details> <details> <summary>docs/src/dev-guide/components-core/index.md (1)</summary> `40-44`: **Missing MyST targets for OS-specific pages** `manylinux_2_28`, `centos-stream-9-deps-install`, etc. are referenced as relative docs but the build warns that at least `manylinux_2_28` is absent. Verify that the file exists or adjust the link/slug to the correct path. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
components/core/tools/scripts/lib_install/manylinux_2_28/install-packages-from-source.sh
Outdated
Show resolved
Hide resolved
components/core/tools/scripts/lib_install/manylinux_2_28/install-prebuilt-packages.sh
Outdated
Show resolved
Hide resolved
components/core/tools/scripts/lib_install/manylinux_2_28/install-prebuilt-packages.sh
Outdated
Show resolved
Hide resolved
components/core/tools/scripts/lib_install/manylinux_2_28/install-prebuilt-packages.sh
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
docs/src/dev-guide/components-core/index.md (4)
51-61: Same fenced-block spacing issue in Docker exampleApply the same blank-line rule before and after the ```shell block (lines 52 and 60) to silence MD031.
-* Start a build container - ```shell +* Start a build container + + ```shell @@ - /bin/bash -l - ``` + /bin/bash -l + ```
104-110: Configure & Build sections also violate MD031Add blank lines around the code fences in both “Configure the cmake project” and “Build” lists.
-* Configure the cmake project: - ```shell +* Configure the cmake project: + + ```shell @@ -* Build: - ```shell +* Build: + + ```shell
18-25: Bullet count mismatch – update wording or structureThe intro says “There are two options” but three first-level bullets follow (native, docker, quick-build script). Either:
a) Change the sentence to “There are three options”; or
b) Nest the quick-build script as a sub-bullet under the Docker option (it’s container-based).This avoids reader confusion and reflects the actual choices.
25-35: Surround the fenced script block with blank lines (MD031, MD012)The ```shell block is flush against the bullet text and followed by two consecutive blank lines, triggering MD031/MD012. Insert one blank line before the fence and ensure exactly one blank line after the closing fence.
-* If you just want to quickly build a CLP binary compatible with most platforms, you can run the following script: - ```shell +* If you just want to quickly build a CLP binary compatible with most platforms, you can run the following script: + + ```shell # Build CLP binary using manylinux_2_28 for x86_64 or aarch64 platforms @@ - [--platform linux/amd64|linux/arm64] - ``` - + [--platform linux/amd64|linux/arm64] + ```
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
docs/src/dev-guide/components-core/index.md(4 hunks)docs/src/dev-guide/tooling-containers.md(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: quinntaylormitchell
PR: y-scope/clp#968
File: docs/src/user-guide/quick-start/overview.md:53-54
Timestamp: 2025-06-18T20:48:48.990Z
Learning: CLP is designed to run on Linux systems where Python is typically pre-installed, so Python installation links are generally not needed in CLP documentation.
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.
Learnt from: gibber9809
PR: y-scope/clp#955
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-26
Timestamp: 2025-06-02T18:22:24.060Z
Learning: In the clp project, ANTLR code generation at build time is being removed by another PR. When reviewing CMake files, be aware that some temporary suboptimal configurations may exist to reduce merge conflicts between concurrent PRs, especially around ANTLR_TARGET calls.
Learnt from: LinZhihao-723
PR: y-scope/clp#486
File: components/core/tests/test-error_handling.cpp:37-38
Timestamp: 2024-11-26T19:12:43.982Z
Learning: In the CLP project, C++20 is used, allowing the utilization of C++20 features such as class template argument deduction (CTAD) with `std::array`.
Learnt from: jackluo923
PR: y-scope/clp#718
File: components/core/tools/scripts/utils/create-debian-package.py:41-41
Timestamp: 2025-02-12T22:24:17.723Z
Learning: For the clp-core Debian package creation script, strict version format validation is considered unnecessary complexity and should be avoided.
Learnt from: quinntaylormitchell
PR: y-scope/clp#968
File: docs/src/user-guide/quick-start/overview.md:73-109
Timestamp: 2025-06-18T20:39:05.899Z
Learning: The CLP project team prefers to use video content to demonstrate detailed procedural steps (like tarball extraction) rather than including every step in the written documentation, keeping the docs focused on conceptual guidance.
docs/src/dev-guide/tooling-containers.md (6)
Learnt from: quinntaylormitchell
PR: y-scope/clp#968
File: docs/src/user-guide/quick-start/overview.md:73-109
Timestamp: 2025-06-18T20:39:05.899Z
Learning: The CLP project team prefers to use video content to demonstrate detailed procedural steps (like tarball extraction) rather than including every step in the written documentation, keeping the docs focused on conceptual guidance.
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.
Learnt from: quinntaylormitchell
PR: y-scope/clp#968
File: docs/src/user-guide/quick-start/overview.md:53-54
Timestamp: 2025-06-18T20:48:48.990Z
Learning: CLP is designed to run on Linux systems where Python is typically pre-installed, so Python installation links are generally not needed in CLP documentation.
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.
Learnt from: LinZhihao-723
PR: y-scope/clp#486
File: components/core/tests/test-error_handling.cpp:37-38
Timestamp: 2024-11-26T19:12:43.982Z
Learning: In the CLP project, C++20 is used, allowing the utilization of C++20 features such as class template argument deduction (CTAD) with `std::array`.
Learnt from: jackluo923
PR: y-scope/clp#718
File: components/core/tools/scripts/utils/create-debian-package.py:41-41
Timestamp: 2025-02-12T22:24:17.723Z
Learning: For the clp-core Debian package creation script, strict version format validation is considered unnecessary complexity and should be avoided.
docs/src/dev-guide/components-core/index.md (11)
Learnt from: quinntaylormitchell
PR: y-scope/clp#968
File: docs/src/user-guide/quick-start/overview.md:73-109
Timestamp: 2025-06-18T20:39:05.899Z
Learning: The CLP project team prefers to use video content to demonstrate detailed procedural steps (like tarball extraction) rather than including every step in the written documentation, keeping the docs focused on conceptual guidance.
Learnt from: gibber9809
PR: y-scope/clp#955
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-26
Timestamp: 2025-06-02T18:22:24.060Z
Learning: In the clp project, ANTLR code generation at build time is being removed by another PR. When reviewing CMake files, be aware that some temporary suboptimal configurations may exist to reduce merge conflicts between concurrent PRs, especially around ANTLR_TARGET calls.
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts in `components/clp-package-utils/clp_package_utils/scripts/`, note that it's preferred to keep error handling simple without adding extra complexity.
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.
Learnt from: jackluo923
PR: y-scope/clp#718
File: components/core/tools/scripts/utils/create-debian-package.py:41-41
Timestamp: 2025-02-12T22:24:17.723Z
Learning: For the clp-core Debian package creation script, strict version format validation is considered unnecessary complexity and should be avoided.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#0
File: :0-0
Timestamp: 2025-03-18T07:27:54.738Z
Learning: Double parentheses in the codebase are intentional and required for clang-tidy to pass. These should not be flagged as style issues in code reviews.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#0
File: :0-0
Timestamp: 2025-03-18T07:27:54.738Z
Learning: Double parentheses in REQUIRE statements (e.g., `REQUIRE((expression))`) and in conditional expressions are intentional and required for clang-tidy to pass. These should not be flagged as style issues in code reviews.
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:11-12
Timestamp: 2025-05-06T10:07:04.654Z
Learning: In CLP installation scripts, temporary directories with downloaded files should not be automatically cleaned up on failure (e.g., with EXIT traps) to preserve artifacts for debugging purposes.
Learnt from: LinZhihao-723
PR: y-scope/clp#486
File: components/core/tests/test-error_handling.cpp:37-38
Timestamp: 2024-11-26T19:12:43.982Z
Learning: In the CLP project, C++20 is used, allowing the utilization of C++20 features such as class template argument deduction (CTAD) with `std::array`.
Learnt from: quinntaylormitchell
PR: y-scope/clp#968
File: docs/src/user-guide/quick-start/overview.md:53-54
Timestamp: 2025-06-18T20:48:48.990Z
Learning: CLP is designed to run on Linux systems where Python is typically pre-installed, so Python installation links are generally not needed in CLP documentation.
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.
🪛 LanguageTool
docs/src/dev-guide/components-core/index.md
[uncategorized] ~32-~32: Loose punctuation mark.
Context: ... ``` * --output <output_directory>: Directory to place the built CLP binary...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
docs/src/dev-guide/components-core/index.md
26-26: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
31-31: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
35-35: Multiple consecutive blank lines
Expected: 1; Actual: 2
(MD012, no-multiple-blanks)
52-52: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
105-105: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: centos-stream-9-deps-image
- GitHub Check: ubuntu-jammy-deps-image
- GitHub Check: lint-check (macos-latest)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build (macos-latest)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
components/core/tools/scripts/utils/build.py (2)
35-35: Use lazy formatting in logging statements.The W1203 warnings persist from pylint. For consistency with logging best practices and better performance, use lazy formatting instead of f-strings.
- logger.info(f"Running: {' '.join(cmd)} in {cwd if cwd else Path.cwd()}") + logger.info("Running: %s in %s", ' '.join(cmd), cwd if cwd else Path.cwd()) - logger.error(f"Command failed: {' '.join(cmd)} (in {cwd})\nReturn code: {e.returncode}") + logger.error("Command failed: %s (in %s)\nReturn code: %s", ' '.join(cmd), cwd, e.returncode) - logger.info(f"Inferred project root: {project_root}") + logger.info("Inferred project root: %s", project_root) - logger.info(f"Creating build directory: {build_path}") + logger.info("Creating build directory: %s", build_path) - logger.info(f"Configuring CMake project in {core_path} -> {build_path}") + logger.info("Configuring CMake project in %s -> %s", core_path, build_path) - logger.info(f"Building project in {build_path} with {parsed_args.num_jobs} jobs") + logger.info("Building project in %s with %s jobs", build_path, parsed_args.num_jobs)Also applies to: 39-39, 98-98, 109-109, 114-114, 117-117
79-79: Add function docstring.The main function should have a docstring describing its purpose.
def main(argv: List[str]) -> int: + """ + Main entry point for building CLP-core binaries locally. + + Args: + argv: Command line arguments + + Returns: + Exit code (0 for success) + """docs/src/dev-guide/components-core/index.md (1)
26-31: Address remaining markdown formatting issues.The static analysis flags MD031 (fenced blocks need blank lines) and MD012 (multiple consecutive blank lines) warnings.
-* If you just want to quickly build a CLP binary compatible with most platforms, you can run the following script: +* If you just want to quickly build a CLP binary compatible with most platforms, you can run the following script: + ```shell # Build CLP binary using manylinux_2_28 for x86_64 or aarch64 platforms /path/to/clp/components/core/tools/scripts/utils/build-with-docker.py \ --output <output_directory> \ [--platform linux/amd64|linux/arm64]
--output <output_directory>: Directory to place the built CLP binary.--platform linux/amd64|linux/arm64(optional): Target platform. Defaults to linux/amd64 if not specified.
Also applies to: 35-35 </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: ASSERTIVE** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 53f5974df00df7a5db282993dee69736863d8450 and 73563d551a575254015d9c62eed601dcaf8d17c1. </details> <details> <summary>📒 Files selected for processing (5)</summary> * `components/core/tools/scripts/lib_install/manylinux_2_28/install-packages-from-source.sh` (1 hunks) * `components/core/tools/scripts/lib_install/manylinux_2_28/install-prebuilt-packages.sh` (1 hunks) * `components/core/tools/scripts/utils/build.py` (1 hunks) * `docs/src/dev-guide/components-core/index.md` (3 hunks) * `docs/src/dev-guide/tooling-containers.md` (2 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧠 Learnings (6)</summary> <details> <summary>📓 Common learnings</summary>Learnt from: quinntaylormitchell
PR: #968
File: docs/src/user-guide/quick-start/overview.md:53-54
Timestamp: 2025-06-18T20:48:48.990Z
Learning: CLP is designed to run on Linux systems where Python is typically pre-installed, so Python installation links are generally not needed in CLP documentation.Learnt from: kirkrodrigues
PR: #881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (likeapt-get install -f) when installing packages to give users more control over their system.Learnt from: gibber9809
PR: #955
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-26
Timestamp: 2025-06-02T18:22:24.060Z
Learning: In the clp project, ANTLR code generation at build time is being removed by another PR. When reviewing CMake files, be aware that some temporary suboptimal configurations may exist to reduce merge conflicts between concurrent PRs, especially around ANTLR_TARGET calls.Learnt from: LinZhihao-723
PR: #486
File: components/core/tests/test-error_handling.cpp:37-38
Timestamp: 2024-11-26T19:12:43.982Z
Learning: In the CLP project, C++20 is used, allowing the utilization of C++20 features such as class template argument deduction (CTAD) withstd::array.Learnt from: jackluo923
PR: #718
File: components/core/tools/scripts/utils/create-debian-package.py:41-41
Timestamp: 2025-02-12T22:24:17.723Z
Learning: For the clp-core Debian package creation script, strict version format validation is considered unnecessary complexity and should be avoided.Learnt from: quinntaylormitchell
PR: #968
File: docs/src/user-guide/quick-start/overview.md:73-109
Timestamp: 2025-06-18T20:39:05.899Z
Learning: The CLP project team prefers to use video content to demonstrate detailed procedural steps (like tarball extraction) rather than including every step in the written documentation, keeping the docs focused on conceptual guidance.</details> <details> <summary>docs/src/dev-guide/tooling-containers.md (8)</summary>Learnt from: quinntaylormitchell
PR: #968
File: docs/src/user-guide/quick-start/overview.md:73-109
Timestamp: 2025-06-18T20:39:05.899Z
Learning: The CLP project team prefers to use video content to demonstrate detailed procedural steps (like tarball extraction) rather than including every step in the written documentation, keeping the docs focused on conceptual guidance.Learnt from: haiqi96
PR: #594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts incomponents/clp-package-utils/clp_package_utils/scripts/, note that it's preferred to keep error handling simple without adding extra complexity.Learnt from: gibber9809
PR: #504
File: components/core/src/clp_s/search/kql/CMakeLists.txt:29-29
Timestamp: 2024-10-22T15:36:04.655Z
Learning: When reviewing pull requests, focus on the changes within the PR and avoid commenting on issues outside the scope of the PR.Learnt from: quinntaylormitchell
PR: #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.Learnt from: quinntaylormitchell
PR: #968
File: docs/src/user-guide/quick-start/overview.md:53-54
Timestamp: 2025-06-18T20:48:48.990Z
Learning: CLP is designed to run on Linux systems where Python is typically pre-installed, so Python installation links are generally not needed in CLP documentation.Learnt from: kirkrodrigues
PR: #881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (likeapt-get install -f) when installing packages to give users more control over their system.Learnt from: LinZhihao-723
PR: #486
File: components/core/tests/test-error_handling.cpp:37-38
Timestamp: 2024-11-26T19:12:43.982Z
Learning: In the CLP project, C++20 is used, allowing the utilization of C++20 features such as class template argument deduction (CTAD) withstd::array.Learnt from: jackluo923
PR: #718
File: components/core/tools/scripts/utils/create-debian-package.py:41-41
Timestamp: 2025-02-12T22:24:17.723Z
Learning: For the clp-core Debian package creation script, strict version format validation is considered unnecessary complexity and should be avoided.</details> <details> <summary>components/core/tools/scripts/lib_install/manylinux_2_28/install-packages-from-source.sh (5)</summary>Learnt from: kirkrodrigues
PR: #881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (likeapt-get install -f) when installing packages to give users more control over their system.Learnt from: haiqi96
PR: #594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts incomponents/clp-package-utils/clp_package_utils/scripts/, note that it's preferred to keep error handling simple without adding extra complexity.Learnt from: kirkrodrigues
PR: #881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:11-12
Timestamp: 2025-05-06T10:07:04.654Z
Learning: In CLP installation scripts, temporary directories with downloaded files should not be automatically cleaned up on failure (e.g., with EXIT traps) to preserve artifacts for debugging purposes.Learnt from: haiqi96
PR: #594
File: components/package-template/src/sbin/del-archives.sh:6-9
Timestamp: 2024-11-15T16:28:08.644Z
Learning: For wrapper scripts in thecomponents/package-template/src/sbin/directory, keep them simple and avoid adding additional validation code.Learnt from: kirkrodrigues
PR: #881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:15-23
Timestamp: 2025-05-06T09:46:42.639Z
Learning: For temporary solutions in installation scripts like those incomponents/core/tools/scripts/lib_install/, checksumming downloaded files is considered optional, particularly when working with trusted sources like GitHub raw content.</details> <details> <summary>components/core/tools/scripts/lib_install/manylinux_2_28/install-prebuilt-packages.sh (7)</summary>Learnt from: kirkrodrigues
PR: #881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (likeapt-get install -f) when installing packages to give users more control over their system.Learnt from: haiqi96
PR: #594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts incomponents/clp-package-utils/clp_package_utils/scripts/, note that it's preferred to keep error handling simple without adding extra complexity.Learnt from: kirkrodrigues
PR: #881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:11-12
Timestamp: 2025-05-06T10:07:04.654Z
Learning: In CLP installation scripts, temporary directories with downloaded files should not be automatically cleaned up on failure (e.g., with EXIT traps) to preserve artifacts for debugging purposes.Learnt from: kirkrodrigues
PR: #881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:15-23
Timestamp: 2025-05-06T09:46:42.639Z
Learning: For temporary solutions in installation scripts like those incomponents/core/tools/scripts/lib_install/, checksumming downloaded files is considered optional, particularly when working with trusted sources like GitHub raw content.Learnt from: jackluo923
PR: #718
File: components/core/tools/scripts/utils/create-debian-package.py:41-41
Timestamp: 2025-02-12T22:24:17.723Z
Learning: For the clp-core Debian package creation script, strict version format validation is considered unnecessary complexity and should be avoided.Learnt from: haiqi96
PR: #594
File: components/clp-package-utils/clp_package_utils/scripts/native/del_archives.py:104-110
Timestamp: 2024-11-15T16:21:52.122Z
Learning: Inclp_package_utils/scripts/native/del_archives.py, when deleting archives, thearchivevariable retrieved from the database is controlled and is always a single string without path components. Therefore, it's acceptable to skip additional validation checks for directory traversal in this context.Learnt from: anlowee
PR: #925
File: taskfiles/deps/main.yaml:97-106
Timestamp: 2025-05-28T18:33:30.155Z
Learning: In the taskfiles dependency system (taskfiles/deps/main.yaml), echo commands are used to generate .cmake settings files that are consumed by the main CMake build process. These files set variables like ANTLR_RUNTIME_HEADER to point to dependency locations for use during compilation.</details> <details> <summary>docs/src/dev-guide/components-core/index.md (14)</summary>Learnt from: quinntaylormitchell
PR: #968
File: docs/src/user-guide/quick-start/overview.md:73-109
Timestamp: 2025-06-18T20:39:05.899Z
Learning: The CLP project team prefers to use video content to demonstrate detailed procedural steps (like tarball extraction) rather than including every step in the written documentation, keeping the docs focused on conceptual guidance.Learnt from: gibber9809
PR: #955
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-26
Timestamp: 2025-06-02T18:22:24.060Z
Learning: In the clp project, ANTLR code generation at build time is being removed by another PR. When reviewing CMake files, be aware that some temporary suboptimal configurations may exist to reduce merge conflicts between concurrent PRs, especially around ANTLR_TARGET calls.Learnt from: quinntaylormitchell
PR: #961
File: docs/src/dev-guide/design-clp-structured/single-file-archive-format.md:216-219
Timestamp: 2025-06-18T14:35:20.485Z
Learning: In clp-s documentation, technical abbreviations like "MPT" (Merged Parse Tree) should be defined at first use to improve reader clarity and comprehension.Learnt from: haiqi96
PR: #594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts incomponents/clp-package-utils/clp_package_utils/scripts/, note that it's preferred to keep error handling simple without adding extra complexity.Learnt from: Bill-hbrhbr
PR: y-scope/clp#0
File: :0-0
Timestamp: 2025-03-18T07:27:54.738Z
Learning: Double parentheses in the codebase are intentional and required for clang-tidy to pass. These should not be flagged as style issues in code reviews.Learnt from: kirkrodrigues
PR: #881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (likeapt-get install -f) when installing packages to give users more control over their system.Learnt from: jackluo923
PR: #718
File: components/core/tools/scripts/utils/create-debian-package.py:41-41
Timestamp: 2025-02-12T22:24:17.723Z
Learning: For the clp-core Debian package creation script, strict version format validation is considered unnecessary complexity and should be avoided.Learnt from: Bill-hbrhbr
PR: y-scope/clp#0
File: :0-0
Timestamp: 2025-03-18T07:27:54.738Z
Learning: Double parentheses in REQUIRE statements (e.g.,REQUIRE((expression))) and in conditional expressions are intentional and required for clang-tidy to pass. These should not be flagged as style issues in code reviews.Learnt from: haiqi96
PR: #523
File: components/core/src/clp/clp/FileCompressor.hpp:58-78
Timestamp: 2024-10-24T14:25:17.978Z
Learning: When reviewing legacy code refactors, avoid suggesting changes that would extend the scope of the PR.Learnt from: kirkrodrigues
PR: #881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:11-12
Timestamp: 2025-05-06T10:07:04.654Z
Learning: In CLP installation scripts, temporary directories with downloaded files should not be automatically cleaned up on failure (e.g., with EXIT traps) to preserve artifacts for debugging purposes.Learnt from: LinZhihao-723
PR: #558
File: components/core/tests/test-ffi_KeyValuePairLogEvent.cpp:331-335
Timestamp: 2024-10-14T03:45:21.323Z
Learning: When reviewing code, ensure that suggestions about unnecessary use ofstd::movewithstd::shared_ptrare only made ifstd::moveis actually used in the code.Learnt from: gibber9809
PR: #504
File: components/core/src/clp_s/search/kql/CMakeLists.txt:29-29
Timestamp: 2024-10-22T15:36:04.655Z
Learning: When reviewing pull requests, focus on the changes within the PR and avoid commenting on issues outside the scope of the PR.Learnt from: quinntaylormitchell
PR: #968
File: docs/src/user-guide/quick-start/overview.md:53-54
Timestamp: 2025-06-18T20:48:48.990Z
Learning: CLP is designed to run on Linux systems where Python is typically pre-installed, so Python installation links are generally not needed in CLP documentation.Learnt from: LinZhihao-723
PR: #486
File: components/core/tests/test-error_handling.cpp:37-38
Timestamp: 2024-11-26T19:12:43.982Z
Learning: In the CLP project, C++20 is used, allowing the utilization of C++20 features such as class template argument deduction (CTAD) withstd::array.</details> <details> <summary>components/core/tools/scripts/utils/build.py (1)</summary>Learnt from: gibber9809
PR: #955
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-26
Timestamp: 2025-06-02T18:22:24.060Z
Learning: In the clp project, ANTLR code generation at build time is being removed by another PR. When reviewing CMake files, be aware that some temporary suboptimal configurations may exist to reduce merge conflicts between concurrent PRs, especially around ANTLR_TARGET calls.</details> </details><details> <summary>🪛 Shellcheck (0.10.0)</summary> <details> <summary>components/core/tools/scripts/lib_install/manylinux_2_28/install-prebuilt-packages.sh</summary> [warning] 62-62: Declare and assign separately to avoid masking return values. (SC2155) </details> </details> <details> <summary>🪛 LanguageTool</summary> <details> <summary>docs/src/dev-guide/components-core/index.md</summary> [uncategorized] ~32-~32: Loose punctuation mark. Context: ... ``` * `--output <output_directory>`: Directory to place the built CLP binary... (UNLIKELY_OPENING_PUNCTUATION) </details> </details> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> <details> <summary>docs/src/dev-guide/components-core/index.md</summary> 26-26: Fenced code blocks should be surrounded by blank lines null (MD031, blanks-around-fences) --- 31-31: Fenced code blocks should be surrounded by blank lines null (MD031, blanks-around-fences) --- 35-35: Multiple consecutive blank lines Expected: 1; Actual: 2 (MD012, no-multiple-blanks) </details> </details> <details> <summary>🪛 Pylint (3.3.7)</summary> <details> <summary>components/core/tools/scripts/utils/build.py</summary> [warning] 35-35: Use lazy % formatting in logging functions (W1203) --- [warning] 39-39: Use lazy % formatting in logging functions (W1203) --- [convention] 79-79: Missing function or method docstring (C0116) --- [warning] 98-98: Use lazy % formatting in logging functions (W1203) --- [warning] 109-109: Use lazy % formatting in logging functions (W1203) --- [warning] 114-114: Use lazy % formatting in logging functions (W1203) --- [warning] 117-117: Use lazy % formatting in logging functions (W1203) </details> </details> <details> <summary>🪛 GitHub Actions: clp-lint</summary> <details> <summary>components/core/tools/scripts/utils/build.py</summary> [error] 1-20: Black formatting check failed. The file would be reformatted. Run 'black --write' to fix code style issues. </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms (7)</summary> * GitHub Check: centos-stream-9-deps-image * GitHub Check: ubuntu-jammy-deps-image * GitHub Check: build-macos (macos-14, false) * GitHub Check: build-macos (macos-14, true) * GitHub Check: build-macos (macos-15, true) * GitHub Check: build-macos (macos-13, true) * GitHub Check: build-macos (macos-13, false) </details> <details> <summary>🔇 Additional comments (4)</summary><blockquote> <details> <summary>components/core/tools/scripts/lib_install/manylinux_2_28/install-packages-from-source.sh (1)</summary> `1-23`: **Script implementation looks solid.** The installation script follows CLP project conventions with proper error handling, explicit sequencing (boost first), and consistent patterns. The past review comments have addressed the main concerns. </details> <details> <summary>components/core/tools/scripts/utils/build.py (1)</summary> `1-126`: **Address Black formatting issues.** The pipeline failure indicates Black formatting check failed. Run `black --write` on this file to fix code style issues. </details> <details> <summary>docs/src/dev-guide/tooling-containers.md (1)</summary> `6-14`: **Documentation update looks good.** The addition of manylinux_2_28 Docker images with proper reference-style links and consistent formatting aligns well with the project's containerised build approach. Also applies to: 43-44 </details> <details> <summary>docs/src/dev-guide/components-core/index.md (1)</summary> `18-34`: **Excellent restructuring of build environment documentation.** The new "Build Environment" section provides clear options and guidance. The reorganisation significantly improves user experience by consolidating environment setup information upfront. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
components/core/tools/scripts/lib_install/manylinux_2_28/install-prebuilt-packages.sh
Outdated
Show resolved
Hide resolved
jackluo923
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
components/core/tools/scripts/lib_install/manylinux_2_28/install-all.sh (1)
3-10: Minor style tweak: collapse shell options into one lineFor consistency with other CLP install scripts you can use a single, terse directive:
-set -e -set -o pipefail -set -u +set -euo pipefailPurely cosmetic; feel free to ignore.
components/core/tools/docker-images/clp-env-base-manylinux_2_28_aarch64/Dockerfile (1)
20-23: Same hardening comment as x86_64 imageCreating a non-root user and adding a trivial
HEALTHCHECKavoids the warnings raised by Checkov/Hadolint and aligns both architecture images.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
components/core/CMakeLists.txt(2 hunks)components/core/tools/docker-images/clp-env-base-manylinux_2_28_aarch64/Dockerfile(1 hunks)components/core/tools/docker-images/clp-env-base-manylinux_2_28_x86_64/Dockerfile(1 hunks)components/core/tools/scripts/lib_install/manylinux_2_28/install-all.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
components/core/tools/docker-images/clp-env-base-manylinux_2_28_x86_64/Dockerfile (5)
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.
Learnt from: anlowee
PR: y-scope/clp#925
File: .github/workflows/clp-s-antlr-generation.yaml:24-27
Timestamp: 2025-05-27T20:04:51.498Z
Learning: The clp codebase uses commit SHAs instead of version tags for GitHub Actions (like actions/checkout) as an established pattern across workflow files.
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts in `components/clp-package-utils/clp_package_utils/scripts/`, note that it's preferred to keep error handling simple without adding extra complexity.
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:11-12
Timestamp: 2025-05-06T10:07:04.654Z
Learning: In CLP installation scripts, temporary directories with downloaded files should not be automatically cleaned up on failure (e.g., with EXIT traps) to preserve artifacts for debugging purposes.
components/core/tools/scripts/lib_install/manylinux_2_28/install-all.sh (4)
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts in `components/clp-package-utils/clp_package_utils/scripts/`, note that it's preferred to keep error handling simple without adding extra complexity.
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:11-12
Timestamp: 2025-05-06T10:07:04.654Z
Learning: In CLP installation scripts, temporary directories with downloaded files should not be automatically cleaned up on failure (e.g., with EXIT traps) to preserve artifacts for debugging purposes.
Learnt from: sitaowang1998
PR: y-scope/clp#1044
File: .github/workflows/clp-core-build-macos.yaml:0-0
Timestamp: 2025-06-27T01:49:15.724Z
Learning: In the clp project, manually setting LDFLAGS for library paths can cause runtime errors because it interferes with CMake's built-in library discovery and linking mechanisms. CMake can identify and correctly link to libraries on its own without requiring manual LDFLAGS configuration.
components/core/CMakeLists.txt (4)
Learnt from: sitaowang1998
PR: y-scope/clp#1044
File: .github/workflows/clp-core-build-macos.yaml:0-0
Timestamp: 2025-06-27T01:49:15.724Z
Learning: In the clp project, manually setting LDFLAGS for library paths can cause runtime errors because it interferes with CMake's built-in library discovery and linking mechanisms. CMake can identify and correctly link to libraries on its own without requiring manual LDFLAGS configuration.
Learnt from: gibber9809
PR: y-scope/clp#955
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-26
Timestamp: 2025-06-02T18:22:24.060Z
Learning: In the clp project, ANTLR code generation at build time is being removed by another PR. When reviewing CMake files, be aware that some temporary suboptimal configurations may exist to reduce merge conflicts between concurrent PRs, especially around ANTLR_TARGET calls.
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts in `components/clp-package-utils/clp_package_utils/scripts/`, note that it's preferred to keep error handling simple without adding extra complexity.
Learnt from: quinntaylormitchell
PR: y-scope/clp#968
File: docs/src/user-guide/quick-start/overview.md:53-54
Timestamp: 2025-06-18T20:48:48.990Z
Learning: CLP is designed to run on Linux systems where Python is typically pre-installed, so Python installation links are generally not needed in CLP documentation.
components/core/tools/docker-images/clp-env-base-manylinux_2_28_aarch64/Dockerfile (6)
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.
Learnt from: anlowee
PR: y-scope/clp#925
File: .github/workflows/clp-s-antlr-generation.yaml:24-27
Timestamp: 2025-05-27T20:04:51.498Z
Learning: The clp codebase uses commit SHAs instead of version tags for GitHub Actions (like actions/checkout) as an established pattern across workflow files.
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts in `components/clp-package-utils/clp_package_utils/scripts/`, note that it's preferred to keep error handling simple without adding extra complexity.
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:11-12
Timestamp: 2025-05-06T10:07:04.654Z
Learning: In CLP installation scripts, temporary directories with downloaded files should not be automatically cleaned up on failure (e.g., with EXIT traps) to preserve artifacts for debugging purposes.
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/package-template/src/sbin/del-archives.sh:6-9
Timestamp: 2024-11-15T16:28:08.644Z
Learning: For wrapper scripts in the `components/package-template/src/sbin/` directory, keep them simple and avoid adding additional validation code.
🪛 Checkov (3.2.334)
components/core/tools/docker-images/clp-env-base-manylinux_2_28_x86_64/Dockerfile
[LOW] 1-23: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
[LOW] 1-23: Ensure that a user for the container has been created
(CKV_DOCKER_3)
components/core/tools/docker-images/clp-env-base-manylinux_2_28_aarch64/Dockerfile
[LOW] 1-23: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
[LOW] 1-23: Ensure that a user for the container has been created
(CKV_DOCKER_3)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: ubuntu-jammy-deps-image
- GitHub Check: centos-stream-9-deps-image
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: lint-check (macos-latest)
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: build-macos (macos-13, false)
🔇 Additional comments (1)
components/core/CMakeLists.txt (1)
218-223: Lowering CURL requirement may break newer API usage – please verifyThe minimum version drops from 7.68.0 to 7.61.1. If the code relies on features such as
curl_url*(added in 7.62.0) this will fail at link time on older images. Grep the sources forcurl_url/CURLU/CURLUPartto be safe.
components/core/tools/docker-images/clp-env-base-manylinux_2_28_x86_64/Dockerfile
Outdated
Show resolved
Hide resolved
| # The bundled CMake 4.x version is too new; downgrading to CMake 3.27 is | ||
| # necessary to resolve build issues with third-party libraries. Even after | ||
| # we added the "-DCMAKE_POLICY_VERSION_MINIMUM=3.5" flag, compilation issues | ||
| # in libraries such as googletest-src and yaml-cpp still persist. | ||
| # See sample error below: | ||
| # CMake Error at _deps/googletest-src/CMakeLists.txt:4 (cmake_minimum_required): | ||
| # Support for CMake versions below 3.5 has been dropped. | ||
| # | ||
| # Adjust the VERSION argument’s minimum value, or use the <min>...<max> syntax | ||
| # to indicate the required CMake version range. | ||
| # | ||
| # Alternatively, you can try adding -DCMAKE_POLICY_VERSION_MINIMUM=3.5 | ||
| export PLATFORM=$(uname -m) | ||
| export CMAKE_VERSION=3.27.9 | ||
| export CMAKE_RELEASE_URL=https://github.com/Kitware/CMake/releases/download/v${CMAKE_VERSION}/cmake-${CMAKE_VERSION}-linux-${PLATFORM}.tar.gz | ||
| echo "Downgrading CMAKE to 3.27.9" | ||
| curl -L "${CMAKE_RELEASE_URL}" | tar -xzf - | ||
| mv "cmake-${CMAKE_VERSION}-linux-${PLATFORM}" "/opt/cmake-${CMAKE_VERSION}" | ||
| ln -sf "/opt/cmake-${CMAKE_VERSION}/bin/cmake" /usr/local/bin/cmake |
There was a problem hiding this comment.
Can we use pip3 to install cmake with a specified version? It worked on our private containers.
…ts to explain choice of source packages.
…ssary comments; Use pipx to downgrade CMake to v3.
kirkrodrigues
changed the title
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (3)
components/core/tools/docker-images/clp-env-base-manylinux-2.28-x86_64/Dockerfile (2)
1-1: Base image not pinned to digest
10-11: Combine install & cleanup into one layer / run as non-rootcomponents/core/tools/docker-images/clp-env-base-manylinux-2.28-aarch64/Dockerfile (1)
1-1: Base image should be pinned by digest for reproducibility
Using only the tag makes builds non-deterministic and vulnerable to upstream image changes. Pinquay.io/pypa/manylinux_2_28_aarch64to its SHA-256 digest just like the x86_64 image.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
components/core/tools/docker-images/clp-env-base-manylinux-2.28-aarch64/Dockerfile(1 hunks)components/core/tools/docker-images/clp-env-base-manylinux-2.28-x86_64/Dockerfile(1 hunks)components/core/tools/scripts/lib_install/manylinux-2.28/install-all.sh(1 hunks)components/core/tools/scripts/lib_install/manylinux-2.28/install-packages-from-source.sh(1 hunks)components/core/tools/scripts/lib_install/manylinux-2.28/install-prebuilt-packages.sh(1 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)
🧰 Additional context used
🧠 Learnings (7)
docs/src/dev-guide/components-core/index.md (12)
Learnt from: quinntaylormitchell
PR: y-scope/clp#968
File: docs/src/user-guide/quick-start/overview.md:53-54
Timestamp: 2025-06-18T20:48:48.990Z
Learning: CLP is designed to run on Linux systems where Python is typically pre-installed, so Python installation links are generally not needed in CLP documentation.
Learnt from: quinntaylormitchell
PR: y-scope/clp#968
File: docs/src/user-guide/quick-start/overview.md:73-109
Timestamp: 2025-06-18T20:39:05.899Z
Learning: The CLP project team prefers to use video content to demonstrate detailed procedural steps (like tarball extraction) rather than including every step in the written documentation, keeping the docs focused on conceptual guidance.
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts in `components/clp-package-utils/clp_package_utils/scripts/`, note that it's preferred to keep error handling simple without adding extra complexity.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#0
File: :0-0
Timestamp: 2025-03-18T07:27:54.738Z
Learning: Double parentheses in the codebase are intentional and required for clang-tidy to pass. These should not be flagged as style issues in code reviews.
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.
Learnt from: jackluo923
PR: y-scope/clp#718
File: components/core/tools/scripts/utils/create-debian-package.py:41-41
Timestamp: 2025-02-12T22:24:17.723Z
Learning: For the clp-core Debian package creation script, strict version format validation is considered unnecessary complexity and should be avoided.
Learnt from: gibber9809
PR: y-scope/clp#955
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-26
Timestamp: 2025-06-02T18:22:24.060Z
Learning: In the clp project, ANTLR code generation at build time is being removed by another PR. When reviewing CMake files, be aware that some temporary suboptimal configurations may exist to reduce merge conflicts between concurrent PRs, especially around ANTLR_TARGET calls.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#0
File: :0-0
Timestamp: 2025-03-18T07:27:54.738Z
Learning: Double parentheses in REQUIRE statements (e.g., `REQUIRE((expression))`) and in conditional expressions are intentional and required for clang-tidy to pass. These should not be flagged as style issues in code reviews.
Learnt from: haiqi96
PR: y-scope/clp#523
File: components/core/src/clp/clp/FileCompressor.hpp:58-78
Timestamp: 2024-10-24T14:25:17.978Z
Learning: When reviewing legacy code refactors, avoid suggesting changes that would extend the scope of the PR.
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:11-12
Timestamp: 2025-05-06T10:07:04.654Z
Learning: In CLP installation scripts, temporary directories with downloaded files should not be automatically cleaned up on failure (e.g., with EXIT traps) to preserve artifacts for debugging purposes.
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/tests/test-ffi_KeyValuePairLogEvent.cpp:331-335
Timestamp: 2024-10-14T03:45:21.323Z
Learning: When reviewing code, ensure that suggestions about unnecessary use of `std::move` with `std::shared_ptr` are only made if `std::move` is actually used in the code.
Learnt from: gibber9809
PR: y-scope/clp#504
File: components/core/src/clp_s/search/kql/CMakeLists.txt:29-29
Timestamp: 2024-10-22T15:36:04.655Z
Learning: When reviewing pull requests, focus on the changes within the PR and avoid commenting on issues outside the scope of the PR.
docs/src/dev-guide/components-core/manylinux-2.28-deps-install.md (6)
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.
Learnt from: quinntaylormitchell
PR: y-scope/clp#968
File: docs/src/user-guide/quick-start/overview.md:73-109
Timestamp: 2025-06-18T20:39:05.899Z
Learning: The CLP project team prefers to use video content to demonstrate detailed procedural steps (like tarball extraction) rather than including every step in the written documentation, keeping the docs focused on conceptual guidance.
Learnt from: quinntaylormitchell
PR: y-scope/clp#968
File: docs/src/user-guide/quick-start/overview.md:53-54
Timestamp: 2025-06-18T20:48:48.990Z
Learning: CLP is designed to run on Linux systems where Python is typically pre-installed, so Python installation links are generally not needed in CLP documentation.
Learnt from: jackluo923
PR: y-scope/clp#718
File: components/core/tools/scripts/utils/create-debian-package.py:41-41
Timestamp: 2025-02-12T22:24:17.723Z
Learning: For the clp-core Debian package creation script, strict version format validation is considered unnecessary complexity and should be avoided.
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts in `components/clp-package-utils/clp_package_utils/scripts/`, note that it's preferred to keep error handling simple without adding extra complexity.
Learnt from: sitaowang1998
PR: y-scope/clp#1044
File: .github/workflows/clp-core-build-macos.yaml:0-0
Timestamp: 2025-06-27T01:49:15.724Z
Learning: In the clp project, manually setting LDFLAGS for library paths can cause runtime errors because it interferes with CMake's built-in library discovery and linking mechanisms. CMake can identify and correctly link to libraries on its own without requiring manual LDFLAGS configuration.
components/core/tools/docker-images/clp-env-base-manylinux-2.28-x86_64/Dockerfile (4)
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.
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts in `components/clp-package-utils/clp_package_utils/scripts/`, note that it's preferred to keep error handling simple without adding extra complexity.
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:11-12
Timestamp: 2025-05-06T10:07:04.654Z
Learning: In CLP installation scripts, temporary directories with downloaded files should not be automatically cleaned up on failure (e.g., with EXIT traps) to preserve artifacts for debugging purposes.
components/core/tools/scripts/lib_install/manylinux-2.28/install-all.sh (5)
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts in `components/clp-package-utils/clp_package_utils/scripts/`, note that it's preferred to keep error handling simple without adding extra complexity.
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:11-12
Timestamp: 2025-05-06T10:07:04.654Z
Learning: In CLP installation scripts, temporary directories with downloaded files should not be automatically cleaned up on failure (e.g., with EXIT traps) to preserve artifacts for debugging purposes.
Learnt from: sitaowang1998
PR: y-scope/clp#1044
File: .github/workflows/clp-core-build-macos.yaml:0-0
Timestamp: 2025-06-27T01:49:15.724Z
Learning: In the clp project, manually setting LDFLAGS for library paths can cause runtime errors because it interferes with CMake's built-in library discovery and linking mechanisms. CMake can identify and correctly link to libraries on its own without requiring manual LDFLAGS configuration.
Learnt from: anlowee
PR: y-scope/clp#925
File: taskfiles/deps/main.yaml:97-106
Timestamp: 2025-05-28T18:33:30.155Z
Learning: In the taskfiles dependency system (taskfiles/deps/main.yaml), echo commands are used to generate .cmake settings files that are consumed by the main CMake build process. These files set variables like ANTLR_RUNTIME_HEADER to point to dependency locations for use during compilation.
components/core/tools/docker-images/clp-env-base-manylinux-2.28-aarch64/Dockerfile (2)
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.
Learnt from: anlowee
PR: y-scope/clp#925
File: .github/workflows/clp-s-antlr-generation.yaml:24-27
Timestamp: 2025-05-27T20:04:51.498Z
Learning: The clp codebase uses commit SHAs instead of version tags for GitHub Actions (like actions/checkout) as an established pattern across workflow files.
components/core/tools/scripts/lib_install/manylinux-2.28/install-packages-from-source.sh (1)
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.
components/core/tools/scripts/lib_install/manylinux-2.28/install-prebuilt-packages.sh (2)
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.
Learnt from: anlowee
PR: y-scope/clp#925
File: taskfiles/deps/main.yaml:97-106
Timestamp: 2025-05-28T18:33:30.155Z
Learning: In the taskfiles dependency system (taskfiles/deps/main.yaml), echo commands are used to generate .cmake settings files that are consumed by the main CMake build process. These files set variables like ANTLR_RUNTIME_HEADER to point to dependency locations for use during compilation.
🪛 Checkov (3.2.334)
components/core/tools/docker-images/clp-env-base-manylinux-2.28-x86_64/Dockerfile
[LOW] 1-1: Ensure the base image uses a non latest version tag
(CKV_DOCKER_7)
[LOW] 1-14: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
[LOW] 1-14: Ensure that a user for the container has been created
(CKV_DOCKER_3)
components/core/tools/docker-images/clp-env-base-manylinux-2.28-aarch64/Dockerfile
[LOW] 1-1: Ensure the base image uses a non latest version tag
(CKV_DOCKER_7)
[LOW] 1-14: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
[LOW] 1-14: Ensure that a user for the container has been created
(CKV_DOCKER_3)
🪛 Hadolint (2.12.0)
components/core/tools/docker-images/clp-env-base-manylinux-2.28-x86_64/Dockerfile
[warning] 1-1: Always tag the version of an image explicitly
(DL3006)
components/core/tools/docker-images/clp-env-base-manylinux-2.28-aarch64/Dockerfile
[warning] 1-1: Always tag the version of an image explicitly
(DL3006)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: centos-stream-9-deps-image
- GitHub Check: ubuntu-jammy-deps-image
- GitHub Check: lint-check (macos-latest)
- GitHub Check: lint-check (ubuntu-latest)
🔇 Additional comments (2)
docs/src/dev-guide/components-core/index.md (1)
66-69: Minor style nit – OS list now out of alphabetical orderWith the addition of manylinux 2.28, the list order becomes: CentOS, macOS, manylinux, Ubuntu. Consider re-ordering (CentOS → macOS → manylinux → Ubuntu) or adding a brief note about the deliberate ordering for clarity.
⛔ Skipped due to learnings
Learnt from: quinntaylormitchell PR: y-scope/clp#968 File: docs/src/user-guide/quick-start/overview.md:53-54 Timestamp: 2025-06-18T20:48:48.990Z Learning: CLP is designed to run on Linux systems where Python is typically pre-installed, so Python installation links are generally not needed in CLP documentation.components/core/tools/scripts/lib_install/manylinux-2.28/install-packages-from-source.sh (1)
6-8:script_dircalculation is correct – nice
The standardBASH_SOURCEpattern avoids issues when the script is sourced or symlinked.
| pipx uninstall cmake | ||
| pipx install cmake~=3.0 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
pipx may not exist – script will abort under set -e
pipx uninstall cmake fails with exit-status 1 when cmake (or even pipx) is absent, causing the whole script to exit because of set -e. Guard against that and install pipx only when required.
-# Downgrade to CMake v3 to work around https://github.com/y-scope/clp/issues/795
-pipx uninstall cmake
-pipx install cmake~=3.0
+# Downgrade to CMake v3 to work around https://github.com/y-scope/clp/issues/795
+# Ensure pipx is present
+if ! command -v pipx >/dev/null; then
+ python3 -m pip install --upgrade --no-cache-dir pipx
+ export PATH="$PATH:$(python3 -m pipx ensurepath --force)"
+fi
+# Ignore uninstall error if cmake is not yet managed by pipx
+pipx uninstall --yes cmake || true
+pipx install --force cmake~=3.0This keeps the script idempotent and prevents unexpected exits.
📝 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.
| pipx uninstall cmake | |
| pipx install cmake~=3.0 | |
| # Downgrade to CMake v3 to work around https://github.com/y-scope/clp/issues/795 | |
| # Ensure pipx is present | |
| if ! command -v pipx >/dev/null; then | |
| python3 -m pip install --upgrade --no-cache-dir pipx | |
| export PATH="$PATH:$(python3 -m pipx ensurepath --force)" | |
| fi | |
| # Ignore uninstall error if cmake is not yet managed by pipx | |
| pipx uninstall --yes cmake || true | |
| pipx install --force cmake~=3.0 |
🤖 Prompt for AI Agents
In
components/core/tools/scripts/lib_install/manylinux-2.28/install-prebuilt-packages.sh
around lines 44 to 45, the commands using pipx assume pipx and cmake are
installed, but if either is missing, the script will exit due to set -e. Modify
the script to first check if pipx is installed and install it if not, then check
if cmake is installed via pipx before attempting to uninstall it. This ensures
the script does not fail unexpectedly and remains idempotent.
| ```shell | ||
| components/core/tools/scripts/lib_install/manylinux-2.28/install-all.sh | ||
| ``` |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Explicit sudo improves copy-paste usability
Most users aren’t root when reading docs. Prefix the install command with sudo (or add a note) so the snippet works verbatim.
-```shell
-components/core/tools/scripts/lib_install/manylinux-2.28/install-all.sh
-```
+```shell
+sudo components/core/tools/scripts/lib_install/manylinux-2.28/install-all.sh
+```🤖 Prompt for AI Agents
In docs/src/dev-guide/components-core/manylinux-2.28-deps-install.md around
lines 15 to 17, the install command lacks the sudo prefix, which can cause
permission errors for non-root users. Update the code block to prefix the
command with sudo so it reads "sudo
components/core/tools/scripts/lib_install/manylinux-2.28/install-all.sh" to
improve usability and ensure the snippet works as-is.
| RUN mkdir -p ./tools/scripts/lib_install | ||
| COPY ./tools/scripts/lib_install ./tools/scripts/lib_install | ||
|
|
||
| RUN ./tools/scripts/lib_install/manylinux-2.28/install-all.sh |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Redundant mkdir layer – COPY already creates the path
RUN mkdir -p ./tools/scripts/lib_install adds an extra layer and is unnecessary because the subsequent COPY will create the directory hierarchy if it doesn’t exist.
-RUN mkdir -p ./tools/scripts/lib_install
-COPY ./tools/scripts/lib_install ./tools/scripts/lib_install
+COPY ./tools/scripts/lib_install ./tools/scripts/lib_installThis shaves one layer and simplifies the file.
🤖 Prompt for AI Agents
In
components/core/tools/docker-images/clp-env-base-manylinux-2.28-x86_64/Dockerfile
lines 5 to 8, remove the RUN mkdir -p ./tools/scripts/lib_install command
because the following COPY command already creates the directory path if it does
not exist. This eliminates an unnecessary Docker layer and simplifies the
Dockerfile.
| set -eu | ||
| set -o pipefail | ||
|
|
||
| script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Warn early if the script isn’t run as root
The subscripts invoke dnf and will fail without sufficient privileges. A quick UID check with a clear message prevents confusing permission errors later.
if [[ $EUID -ne 0 ]]; then
echo "Error: please run this script with root privileges (e.g., via sudo)" >&2
exit 1
fi🤖 Prompt for AI Agents
In components/core/tools/scripts/lib_install/manylinux-2.28/install-all.sh at
line 6, add a check at the start of the script to verify if it is run as root by
checking if the effective user ID (EUID) is 0. If not, print an error message to
stderr indicating the need for root privileges and exit with status 1. This
prevents permission errors later when subscripts invoke dnf.
| FROM quay.io/pypa/manylinux_2_28_aarch64 | ||
|
|
||
| WORKDIR /root | ||
|
|
||
| RUN mkdir -p ./tools/scripts/lib_install | ||
| COPY ./tools/scripts/lib_install ./tools/scripts/lib_install | ||
|
|
||
| RUN ./tools/scripts/lib_install/manylinux-2.28/install-all.sh | ||
|
|
||
| # Remove cached files | ||
| RUN dnf clean all && rm -rf /var/cache/dnf /tmp/* /var/tmp/* | ||
|
|
||
| # NOTE: Don't flatten the image or else we'll lose any environment modifications from the base | ||
| # image. |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding HEALTHCHECK and running as non-root
The image currently misses:
• HEALTHCHECK to allow orchestration platforms to detect liveliness.
• Creation of a non-root user (useradd … && USER …) and switching to it after package installs, improving container security.
Both are low-effort and align with container best-practices.
🧰 Tools
🪛 Checkov (3.2.334)
[LOW] 1-1: Ensure the base image uses a non latest version tag
(CKV_DOCKER_7)
[LOW] 1-14: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
[LOW] 1-14: Ensure that a user for the container has been created
(CKV_DOCKER_3)
🪛 Hadolint (2.12.0)
[warning] 1-1: Always tag the version of an image explicitly
(DL3006)
🤖 Prompt for AI Agents
In
components/core/tools/docker-images/clp-env-base-manylinux-2.28-aarch64/Dockerfile
lines 1 to 14, add a HEALTHCHECK instruction to verify container liveliness,
such as a simple command that confirms the environment is responsive. Also,
create a non-root user with useradd after installing packages, then switch to
this user using the USER directive before finishing the Dockerfile to improve
container security.
| # NOTE: The remaining installation scripts depend on boost, so we install it beforehand. | ||
| "${lib_install_scripts_dir}/install-boost.sh" 1.87.0 | ||
|
|
||
| # NOTE: | ||
| # 1. libarchive may statically link with LZMA, LZ4, and Zstandard, so we install them beforehand. | ||
| # 2. The versions of libarchive, LZMA, LZ4, and Zstandard available in the manylinux 2.28 package | ||
| # repositories are either dated or don't include static libraries, so we install more recent | ||
| # versions from source. | ||
| "${lib_install_scripts_dir}/liblzma.sh" 5.8.1 | ||
| "${lib_install_scripts_dir}/lz4.sh" 1.10.0 | ||
| "${lib_install_scripts_dir}/zstandard.sh" 1.5.7 | ||
| "${lib_install_scripts_dir}/libarchive.sh" 3.8.0 | ||
|
|
||
| "${lib_install_scripts_dir}/msgpack.sh" 7.0.0 |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Extract version numbers into variables for easier maintenance
Hard-coding the package versions in every invocation spreads single-source-of-truth information across multiple files. Defining them once at the top (or sourcing a shared versions.sh) simplifies upgrades.
-# NOTE: The remaining installation scripts depend on boost, so we install it beforehand.
-"${lib_install_scripts_dir}/install-boost.sh" 1.87.0
+# Versions
+BOOST_VER=1.87.0
+LIBLZMA_VER=5.8.1
+LZ4_VER=1.10.0
+ZSTD_VER=1.5.7
+LIBARCHIVE_VER=3.8.0
+MSGPACK_VER=7.0.0
+
+# NOTE: The remaining installation scripts depend on Boost, so we install it beforehand.
+"${lib_install_scripts_dir}/install-boost.sh" "$BOOST_VER"
...
-"${lib_install_scripts_dir}/libarchive.sh" 3.8.0
+"${lib_install_scripts_dir}/libarchive.sh" "$LIBARCHIVE_VER"
...
-"${lib_install_scripts_dir}/msgpack.sh" 7.0.0
+"${lib_install_scripts_dir}/msgpack.sh" "$MSGPACK_VER"This change reduces drift risk when updating dependencies.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
components/core/tools/scripts/lib_install/manylinux-2.28/install-packages-from-source.sh
around lines 9 to 22, the package version numbers are hard-coded directly in
each script invocation. To improve maintainability and reduce the risk of
version drift, define variables for each package version at the top of the
script or source a shared versions.sh file, then use these variables in the
script calls instead of hard-coded literals.
components/core/tools/scripts/lib_install/manylinux-2.28/install-prebuilt-packages.sh
Outdated
Show resolved
Hide resolved
|
|
||
| # Downgrade to CMake v3 to work around https://github.com/y-scope/clp/issues/795 | ||
| pipx uninstall cmake | ||
| pipx install cmake~=3.0 |
There was a problem hiding this comment.
Can we require a higher minor version like ~=3.30?
There was a problem hiding this comment.
I think overall this file is a little weird for the following reasons:
- "manylinux" refers to a set of platform compatibility standards, it's not a clear target/OS users directly use.
- The installation script is not generally available for all platforms supported by
manylinux. For example, this script won't work on Ubuntu by default since it's usingdnfas the package manager.
We should probably constraint the use case of this installation script with the explanation: this environment isn't like other ones: it should only be used for building binaries that are compatible for the platforms supported by manylinux, and we expect the build to be inside a container.
There was a problem hiding this comment.
Fair enough. How about I just remove it?
There was a problem hiding this comment.
Sure, let's remove it for now. I think the explanation in tooling-containers.md should be sufficient.
|
|
||
| ## clp-core-dependencies-<arch>-manylinux-2.28 | ||
|
|
||
| Images containing the dependencies necessary to build CLP core in a manylinux 2.28 environment |
There was a problem hiding this comment.
Might be worth to add a reference to manylinux? https://github.com/pypa/manylinux?tab=readme-ov-file#manylinux_2_28-almalinux-8-based
Co-authored-by: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com>
This reverts commit f96a13a.
components/core/CMakeLists.txt
Outdated
| @@ -72,7 +72,15 @@ if (CLP_USE_STATIC_LIBS) | |||
| if (APPLE) | |||
| set(CLP_STATIC_LIBS_UNSUPPORTED_PLATFORM "macOS") | |||
| elseif (EXISTS "/etc/centos-release") | |||
| set(CLP_STATIC_LIBS_UNSUPPORTED_PLATFORM "CentOS") | |||
| # NOTE: | |||
| # 1. We don't support static linking on any CentOS-based distro except manylinux 2.28 (which | |||
There was a problem hiding this comment.
we should change manylinux 2.28 to manylinux_2_28 because manylinux 2.28 is not a distribution + version. 2_28 refers to the glibc version iirc, not the version of many linux.
components/core/CMakeLists.txt
Outdated
| # NOTE: | ||
| # 1. We don't support static linking on any CentOS-based distro except manylinux 2.28 (which | ||
| # shows up as "AlmaLinux"). | ||
| # 2. A released called "AlmaLinux" doesn't guarantee we're running on a manylinux distro, |
There was a problem hiding this comment.
Is A released called a typoe? i.e. A release called
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (5)
components/core/tools/scripts/lib_install/manylinux_2_28/install-prebuilt-packages.sh (2)
6-14: Trim image size by suppressing weak deps
dnfinstalls weak dependencies by default, pulling in ~100 MB of packages that the build tool-chain never touches. Add--setopt=install_weak_deps=False(and optionally--nodocs) to keep the final container lean without functional impact.
43-45: Guard against missingpipxbefore invoking itThe manylinux base images do not guarantee that
pipxis present. Callingpipxunconditionally will abort the build. Add a small check that installspipxwhen absent and makepipx uninstall cmakenon-fatal if CMake is not yet managed bypipx.-# Downgrade to CMake v3 to work around https://github.com/y-scope/clp/issues/795 -pipx uninstall cmake -pipx install cmake~=3.31 +# Downgrade to CMake v3 to work around https://github.com/y-scope/clp/issues/795 +if ! command -v pipx >/dev/null ; then + python3 -m pip install --upgrade pipx + python3 -m pipx ensurepath +fi +pipx uninstall cmake || true +pipx install cmake~=3.31components/core/CMakeLists.txt (1)
75-83: CentOS derivative detection still too narrowEarlier feedback to include “Rocky” and “Oracle” alongside “AlmaLinux” has not been applied. These RHEL-8 clones ship the same ABI as AlmaLinux and should also bypass the static-linking block.
-if(NOT "${CENTOS_RELEASE_CONTENT}" MATCHES "AlmaLinux") +if(NOT "${CENTOS_RELEASE_CONTENT}" MATCHES "(AlmaLinux|Rocky|Oracle)") set(CLP_STATIC_LIBS_UNSUPPORTED_PLATFORM "CentOS") endif()This prevents false negatives when the build runs on other manylinux_2_28–compatible images.
components/core/tools/scripts/lib_install/manylinux_2_28/install-packages-from-source.sh (1)
6-8: Samereadonlynitpick as previously notedEarlier feedback already suggested declaring
script_dirand
lib_install_scripts_dirasreadonly. They are still mutable here.docs/src/dev-guide/tooling-containers.md (1)
22-37: Add Packages links & pull commands for manylinux 2.28 imagesOther images list a GitHub Packages page and a
docker pullsnippet, but the
new manylinux 2.28 (aarch64 & x86_64) sections only show the local path,
making them inconsistent and less discoverable. Please mirror the format used
by the CentOS and Ubuntu entries.
Example:* [GitHub Packages page][core-deps-manylinux_2_28_x86_64] * Pull command: ```bash docker pull ghcr.io/y-scope/clp/clp-core-dependencies-x86_64-manylinux_2_28:mainDon’t forget to add the link reference definitions at the bottom. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: ASSERTIVE** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 31e987497f7acc13e099e7b1a6cf09e473552a45 and 57cda645f9a3f53b1b6bd1785b3973bd9aa8a02b. </details> <details> <summary>📒 Files selected for processing (9)</summary> * `components/core/CMakeLists.txt` (2 hunks) * `components/core/tools/docker-images/clp-env-base-manylinux_2_28-aarch64/Dockerfile` (1 hunks) * `components/core/tools/docker-images/clp-env-base-manylinux_2_28-aarch64/build.sh` (1 hunks) * `components/core/tools/docker-images/clp-env-base-manylinux_2_28-x86_64/Dockerfile` (1 hunks) * `components/core/tools/docker-images/clp-env-base-manylinux_2_28-x86_64/build.sh` (1 hunks) * `components/core/tools/scripts/lib_install/manylinux_2_28/install-all.sh` (1 hunks) * `components/core/tools/scripts/lib_install/manylinux_2_28/install-packages-from-source.sh` (1 hunks) * `components/core/tools/scripts/lib_install/manylinux_2_28/install-prebuilt-packages.sh` (1 hunks) * `docs/src/dev-guide/tooling-containers.md` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧠 Learnings (9)</summary> <details> <summary>components/core/tools/docker-images/clp-env-base-manylinux_2_28-aarch64/Dockerfile (1)</summary>Learnt from: quinntaylormitchell
PR: #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.</details> <details> <summary>components/core/tools/scripts/lib_install/manylinux_2_28/install-all.sh (5)</summary>Learnt from: haiqi96
PR: #594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts incomponents/clp-package-utils/clp_package_utils/scripts/, note that it's preferred to keep error handling simple without adding extra complexity.Learnt from: kirkrodrigues
PR: #881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (likeapt-get install -f) when installing packages to give users more control over their system.Learnt from: kirkrodrigues
PR: #881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:11-12
Timestamp: 2025-05-06T10:07:04.654Z
Learning: In CLP installation scripts, temporary directories with downloaded files should not be automatically cleaned up on failure (e.g., with EXIT traps) to preserve artifacts for debugging purposes.Learnt from: sitaowang1998
PR: #1044
File: .github/workflows/clp-core-build-macos.yaml:0-0
Timestamp: 2025-06-27T01:49:15.724Z
Learning: In the clp project, manually setting LDFLAGS for library paths can cause runtime errors because it interferes with CMake's built-in library discovery and linking mechanisms. CMake can identify and correctly link to libraries on its own without requiring manual LDFLAGS configuration.Learnt from: anlowee
PR: #925
File: taskfiles/deps/main.yaml:97-106
Timestamp: 2025-05-28T18:33:30.155Z
Learning: In the taskfiles dependency system (taskfiles/deps/main.yaml), echo commands are used to generate .cmake settings files that are consumed by the main CMake build process. These files set variables like ANTLR_RUNTIME_HEADER to point to dependency locations for use during compilation.</details> <details> <summary>components/core/tools/scripts/lib_install/manylinux_2_28/install-packages-from-source.sh (6)</summary>Learnt from: kirkrodrigues
PR: #881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (likeapt-get install -f) when installing packages to give users more control over their system.Learnt from: haiqi96
PR: #594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts incomponents/clp-package-utils/clp_package_utils/scripts/, note that it's preferred to keep error handling simple without adding extra complexity.Learnt from: haiqi96
PR: #594
File: components/package-template/src/sbin/del-archives.sh:6-9
Timestamp: 2024-11-15T16:28:08.644Z
Learning: For wrapper scripts in thecomponents/package-template/src/sbin/directory, keep them simple and avoid adding additional validation code.Learnt from: kirkrodrigues
PR: #881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:15-23
Timestamp: 2025-05-06T09:46:42.639Z
Learning: For temporary solutions in installation scripts like those incomponents/core/tools/scripts/lib_install/, checksumming downloaded files is considered optional, particularly when working with trusted sources like GitHub raw content.Learnt from: kirkrodrigues
PR: #881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:11-12
Timestamp: 2025-05-06T10:07:04.654Z
Learning: In CLP installation scripts, temporary directories with downloaded files should not be automatically cleaned up on failure (e.g., with EXIT traps) to preserve artifacts for debugging purposes.Learnt from: gibber9809
PR: #504
File: components/core/src/clp_s/search/kql/CMakeLists.txt:29-29
Timestamp: 2024-10-22T15:36:04.655Z
Learning: When reviewing pull requests, focus on the changes within the PR and avoid commenting on issues outside the scope of the PR.</details> <details> <summary>components/core/tools/scripts/lib_install/manylinux_2_28/install-prebuilt-packages.sh (7)</summary>Learnt from: kirkrodrigues
PR: #881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (likeapt-get install -f) when installing packages to give users more control over their system.Learnt from: haiqi96
PR: #594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts incomponents/clp-package-utils/clp_package_utils/scripts/, note that it's preferred to keep error handling simple without adding extra complexity.Learnt from: kirkrodrigues
PR: #881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:11-12
Timestamp: 2025-05-06T10:07:04.654Z
Learning: In CLP installation scripts, temporary directories with downloaded files should not be automatically cleaned up on failure (e.g., with EXIT traps) to preserve artifacts for debugging purposes.Learnt from: kirkrodrigues
PR: #881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:15-23
Timestamp: 2025-05-06T09:46:42.639Z
Learning: For temporary solutions in installation scripts like those incomponents/core/tools/scripts/lib_install/, checksumming downloaded files is considered optional, particularly when working with trusted sources like GitHub raw content.Learnt from: jackluo923
PR: #718
File: components/core/tools/scripts/utils/create-debian-package.py:41-41
Timestamp: 2025-02-12T22:24:17.723Z
Learning: For the clp-core Debian package creation script, strict version format validation is considered unnecessary complexity and should be avoided.Learnt from: haiqi96
PR: #594
File: components/clp-package-utils/clp_package_utils/scripts/native/del_archives.py:104-110
Timestamp: 2024-11-15T16:21:52.122Z
Learning: Inclp_package_utils/scripts/native/del_archives.py, when deleting archives, thearchivevariable retrieved from the database is controlled and is always a single string without path components. Therefore, it's acceptable to skip additional validation checks for directory traversal in this context.Learnt from: anlowee
PR: #925
File: taskfiles/deps/main.yaml:97-106
Timestamp: 2025-05-28T18:33:30.155Z
Learning: In the taskfiles dependency system (taskfiles/deps/main.yaml), echo commands are used to generate .cmake settings files that are consumed by the main CMake build process. These files set variables like ANTLR_RUNTIME_HEADER to point to dependency locations for use during compilation.</details> <details> <summary>components/core/tools/docker-images/clp-env-base-manylinux_2_28-x86_64/Dockerfile (1)</summary>Learnt from: quinntaylormitchell
PR: #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.</details> <details> <summary>components/core/tools/docker-images/clp-env-base-manylinux_2_28-aarch64/build.sh (3)</summary>Learnt from: quinntaylormitchell
PR: #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.Learnt from: haiqi96
PR: #594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts incomponents/clp-package-utils/clp_package_utils/scripts/, note that it's preferred to keep error handling simple without adding extra complexity.Learnt from: kirkrodrigues
PR: #881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (likeapt-get install -f) when installing packages to give users more control over their system.</details> <details> <summary>docs/src/dev-guide/tooling-containers.md (7)</summary>Learnt from: quinntaylormitchell
PR: #968
File: docs/src/user-guide/quick-start/overview.md:73-109
Timestamp: 2025-06-18T20:39:05.899Z
Learning: The CLP project team prefers to use video content to demonstrate detailed procedural steps (like tarball extraction) rather than including every step in the written documentation, keeping the docs focused on conceptual guidance.Learnt from: haiqi96
PR: #594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts incomponents/clp-package-utils/clp_package_utils/scripts/, note that it's preferred to keep error handling simple without adding extra complexity.Learnt from: gibber9809
PR: #504
File: components/core/src/clp_s/search/kql/CMakeLists.txt:29-29
Timestamp: 2024-10-22T15:36:04.655Z
Learning: When reviewing pull requests, focus on the changes within the PR and avoid commenting on issues outside the scope of the PR.Learnt from: haiqi96
PR: #523
File: components/core/src/clp/clp/FileCompressor.hpp:58-78
Timestamp: 2024-10-24T14:25:17.978Z
Learning: When reviewing legacy code refactors, avoid suggesting changes that would extend the scope of the PR.Learnt from: quinntaylormitchell
PR: #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.Learnt from: anlowee
PR: #925
File: .github/workflows/clp-s-antlr-generation.yaml:24-27
Timestamp: 2025-05-27T20:04:51.498Z
Learning: The clp codebase uses commit SHAs instead of version tags for GitHub Actions (like actions/checkout) as an established pattern across workflow files.Learnt from: quinntaylormitchell
PR: #968
File: docs/src/user-guide/quick-start/overview.md:53-54
Timestamp: 2025-06-18T20:48:48.990Z
Learning: CLP is designed to run on Linux systems where Python is typically pre-installed, so Python installation links are generally not needed in CLP documentation.</details> <details> <summary>components/core/tools/docker-images/clp-env-base-manylinux_2_28-x86_64/build.sh (3)</summary>Learnt from: quinntaylormitchell
PR: #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.Learnt from: haiqi96
PR: #594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts incomponents/clp-package-utils/clp_package_utils/scripts/, note that it's preferred to keep error handling simple without adding extra complexity.Learnt from: kirkrodrigues
PR: #881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (likeapt-get install -f) when installing packages to give users more control over their system.</details> <details> <summary>components/core/CMakeLists.txt (7)</summary>Learnt from: sitaowang1998
PR: #1044
File: .github/workflows/clp-core-build-macos.yaml:0-0
Timestamp: 2025-06-27T01:49:15.724Z
Learning: In the clp project, manually setting LDFLAGS for library paths can cause runtime errors because it interferes with CMake's built-in library discovery and linking mechanisms. CMake can identify and correctly link to libraries on its own without requiring manual LDFLAGS configuration.Learnt from: gibber9809
PR: #955
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-26
Timestamp: 2025-06-02T18:22:24.060Z
Learning: In the clp project, ANTLR code generation at build time is being removed by another PR. When reviewing CMake files, be aware that some temporary suboptimal configurations may exist to reduce merge conflicts between concurrent PRs, especially around ANTLR_TARGET calls.Learnt from: haiqi96
PR: #594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts incomponents/clp-package-utils/clp_package_utils/scripts/, note that it's preferred to keep error handling simple without adding extra complexity.Learnt from: jackluo923
PR: #718
File: components/core/tools/scripts/utils/create-debian-package.py:41-41
Timestamp: 2025-02-12T22:24:17.723Z
Learning: For the clp-core Debian package creation script, strict version format validation is considered unnecessary complexity and should be avoided.Learnt from: LinZhihao-723
PR: #873
File: components/core/src/clp/ffi/ir_stream/search/test/utils.hpp:15-18
Timestamp: 2025-05-02T22:11:55.711Z
Learning: The team plans to systematically improve include paths by adding${PROJECT_SOURCE_DIR}/srcto CMake targets and using direct include paths instead of deep relative ones in a future PR.Learnt from: junhaoliao
PR: #939
File: components/package-template/src/etc/clp-config.yml:64-64
Timestamp: 2025-06-24T20:13:46.758Z
Learning: When users ask CodeRabbit to create an issue after providing suggestions, they want a GitHub issue created with the high-level requirements and context, not specific code implementations.Learnt from: quinntaylormitchell
PR: #968
File: docs/src/user-guide/quick-start/overview.md:53-54
Timestamp: 2025-06-18T20:48:48.990Z
Learning: CLP is designed to run on Linux systems where Python is typically pre-installed, so Python installation links are generally not needed in CLP documentation.</details> </details><details> <summary>🪛 Checkov (3.2.334)</summary> <details> <summary>components/core/tools/docker-images/clp-env-base-manylinux_2_28-aarch64/Dockerfile</summary> [LOW] 1-1: Ensure the base image uses a non latest version tag (CKV_DOCKER_7) --- [LOW] 1-14: Ensure that HEALTHCHECK instructions have been added to container images (CKV_DOCKER_2) --- [LOW] 1-14: Ensure that a user for the container has been created (CKV_DOCKER_3) </details> <details> <summary>components/core/tools/docker-images/clp-env-base-manylinux_2_28-x86_64/Dockerfile</summary> [LOW] 1-1: Ensure the base image uses a non latest version tag (CKV_DOCKER_7) --- [LOW] 1-14: Ensure that HEALTHCHECK instructions have been added to container images (CKV_DOCKER_2) --- [LOW] 1-14: Ensure that a user for the container has been created (CKV_DOCKER_3) </details> </details> <details> <summary>🪛 Hadolint (2.12.0)</summary> <details> <summary>components/core/tools/docker-images/clp-env-base-manylinux_2_28-aarch64/Dockerfile</summary> [warning] 1-1: Always tag the version of an image explicitly (DL3006) </details> <details> <summary>components/core/tools/docker-images/clp-env-base-manylinux_2_28-x86_64/Dockerfile</summary> [warning] 1-1: Always tag the version of an image explicitly (DL3006) </details> </details> <details> <summary>🪛 LanguageTool</summary> <details> <summary>docs/src/dev-guide/tooling-containers.md</summary> [uncategorized] ~11-~11: A punctuation mark might be missing here. Context: ...re in a [manylinux_2_28][manylinux_2_28] environment (aarch64 or x86). Binaries ... (AI_EN_LECTOR_MISSING_PUNCTUATION) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms (6)</summary> * GitHub Check: centos-stream-9-deps-image * GitHub Check: ubuntu-jammy-deps-image * GitHub Check: build-macos (macos-15, false) * GitHub Check: build-macos (macos-13, false) * GitHub Check: build-macos (macos-14, false) * GitHub Check: build-macos (macos-14, true) </details> <details> <summary>🔇 Additional comments (1)</summary><blockquote> <details> <summary>components/core/CMakeLists.txt (1)</summary> `223-223`: **👍 Relaxed libcurl floor looks correct** Targeting 7.61.1 aligns with the version bundled in the manylinux 2.28 images; no concerns here. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| @@ -0,0 +1,14 @@ | |||
| FROM quay.io/pypa/manylinux_2_28_x86_64 | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Pin base image with an immutable digest for repeatable builds
quay.io/pypa/manylinux_2_28_x86_64 is a moving tag. Switching to a digest (@sha256:…) or a dated tag guarantees that future rebuilds use the exact same environment, avoiding “works-yesterday-breaks-today” scenarios.
🧰 Tools
🪛 Checkov (3.2.334)
[LOW] 1-1: Ensure the base image uses a non latest version tag
(CKV_DOCKER_7)
[LOW] 1-14: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
[LOW] 1-14: Ensure that a user for the container has been created
(CKV_DOCKER_3)
🪛 Hadolint (2.12.0)
[warning] 1-1: Always tag the version of an image explicitly
(DL3006)
🤖 Prompt for AI Agents
In
components/core/tools/docker-images/clp-env-base-manylinux_2_28-x86_64/Dockerfile
at line 1, the base image is currently specified using a mutable tag which can
change over time. To ensure repeatable builds, replace the tag with an immutable
digest by finding the corresponding sha256 digest for the desired image version
and updating the FROM line to use the digest format (e.g., FROM
quay.io/pypa/manylinux_2_28_x86_64@sha256:...). This pins the base image to a
specific version and prevents unexpected changes in future builds.
| # Remove cached files | ||
| RUN dnf clean all && rm -rf /var/cache/dnf /tmp/* /var/tmp/* | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Clean up yum metadata before final layer
dnf clean all is good, but rm -rf /var/cache/dnf is redundant after clean all. Removing both saves a few kilobytes and one layer.
🤖 Prompt for AI Agents
In
components/core/tools/docker-images/clp-env-base-manylinux_2_28-x86_64/Dockerfile
around lines 10 to 12, the command uses both 'dnf clean all' and 'rm -rf
/var/cache/dnf' which is redundant since 'dnf clean all' already removes the
cache. Remove the 'rm -rf /var/cache/dnf' part to simplify the command and
reduce image size and layers.
| @@ -0,0 +1,14 @@ | |||
| FROM quay.io/pypa/manylinux_2_28_aarch64 | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Base image tag should be immutable
Same comment as the x86_64 Dockerfile: pin the manylinux base image by digest to ensure deterministic builds.
🧰 Tools
🪛 Checkov (3.2.334)
[LOW] 1-1: Ensure the base image uses a non latest version tag
(CKV_DOCKER_7)
[LOW] 1-14: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
[LOW] 1-14: Ensure that a user for the container has been created
(CKV_DOCKER_3)
🪛 Hadolint (2.12.0)
[warning] 1-1: Always tag the version of an image explicitly
(DL3006)
🤖 Prompt for AI Agents
In
components/core/tools/docker-images/clp-env-base-manylinux_2_28-aarch64/Dockerfile
at line 1, the base image is currently referenced by tag only, which can lead to
non-deterministic builds. Update the FROM instruction to pin the manylinux base
image by its digest instead of just the tag. Retrieve the correct digest for
quay.io/pypa/manylinux_2_28_aarch64 and replace the tag with the digest to
ensure the image is immutable and builds are reproducible.
| # Remove cached files | ||
| RUN dnf clean all && rm -rf /var/cache/dnf /tmp/* /var/tmp/* | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Redundant cache purge
dnf clean all already wipes /var/cache/dnf. Removing the explicit rm -rf /var/cache/dnf simplifies the layer.
🤖 Prompt for AI Agents
In
components/core/tools/docker-images/clp-env-base-manylinux_2_28-aarch64/Dockerfile
at lines 10 to 12, the command redundantly removes /var/cache/dnf after running
dnf clean all, which already clears that cache. Simplify the RUN command by
removing the explicit rm -rf /var/cache/dnf to avoid unnecessary duplication
while still cleaning the cache and temporary files.
| build_cmd=( | ||
| docker buildx build | ||
| --platform linux/arm64 | ||
| --tag clp-core-dependencies-aarch64-manylinux_2_28:dev | ||
| "$component_root" | ||
| --file "${script_dir}/Dockerfile" | ||
| --load | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Allow caller-supplied tag/version
Hard-coding clp-core-dependencies-aarch64-manylinux_2_28:dev limits reuse. Consider exposing the tag via an environment variable or --tag CLI option so CI can publish versioned images easily.
🤖 Prompt for AI Agents
In
components/core/tools/docker-images/clp-env-base-manylinux_2_28-aarch64/build.sh
around lines 9 to 16, the Docker image tag is hard-coded, limiting flexibility.
Modify the script to accept a tag/version from an environment variable or a
command-line argument, defaulting to the current tag if none is provided. Update
the build_cmd array to use this dynamic tag value instead of the fixed string.
| if command -v git >/dev/null && git -C "$script_dir" rev-parse --is-inside-work-tree >/dev/null ; | ||
| then | ||
| build_cmd+=( | ||
| --label "org.opencontainers.image.revision=$(git -C "$script_dir" rev-parse HEAD)" | ||
| --label "org.opencontainers.image.source=$(git -C "$script_dir" remote get-url origin)" | ||
| ) | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Git labels: handle detached HEAD & shallow clones
git rev-parse HEAD fails in a shallow clone without the commit object. Guard with git rev-parse --verify or fallback to CI_COMMIT_SHA to avoid build crashes in minimal CI checkouts.
🤖 Prompt for AI Agents
In
components/core/tools/docker-images/clp-env-base-manylinux_2_28-aarch64/build.sh
around lines 18 to 24, the git commands for setting image labels can fail in
shallow clones or detached HEAD states. Modify the script to first verify the
commit hash with 'git rev-parse --verify HEAD' and if that fails, fallback to
using the environment variable CI_COMMIT_SHA for the revision label. This
prevents build crashes when the commit object is missing in minimal CI
checkouts.
| set -eu | ||
| set -o pipefail | ||
|
|
||
| script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )" | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Declare script_dir as a readonly constant
The value of script_dir is meant to stay immutable for the lifetime of the script.
Marking it readonly makes that intent explicit and guards against accidental reassignment.
-script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )"
+readonly script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )"🤖 Prompt for AI Agents
In components/core/tools/scripts/lib_install/manylinux_2_28/install-all.sh
around lines 3 to 7, the variable script_dir is assigned but not marked
readonly. To prevent accidental reassignment and make the intent clear, declare
script_dir as a readonly variable immediately after its assignment by adding the
readonly keyword before or after the assignment.
| script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )" | ||
| component_root="${script_dir}/../../../" | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Make path variables readonly for clarity & safety
Both script_dir and component_root are constants after initialisation.
Lock them down to prevent accidental mutation.
-script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )"
-component_root="${script_dir}/../../../"
+readonly script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )"
+readonly component_root="${script_dir}/../../../"📝 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.
| script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )" | |
| component_root="${script_dir}/../../../" | |
| readonly script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )" | |
| readonly component_root="${script_dir}/../../../" |
🤖 Prompt for AI Agents
In
components/core/tools/docker-images/clp-env-base-manylinux_2_28-x86_64/build.sh
around lines 6 to 8, the variables script_dir and component_root are assigned
once and should not change. Declare both variables as readonly immediately after
their assignment to prevent accidental modification and improve code clarity.
| if command -v git >/dev/null && git -C "$script_dir" rev-parse --is-inside-work-tree >/dev/null ; | ||
| then | ||
| build_cmd+=( | ||
| --label "org.opencontainers.image.revision=$(git -C "$script_dir" rev-parse HEAD)" | ||
| --label "org.opencontainers.image.source=$(git -C "$script_dir" remote get-url origin)" | ||
| ) | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use repository root for Git metadata labels
When the script lives in a sub-directory, git -C "$script_dir" returns the
SHA of the script’s sub-tree, not the root commit that produced the image.
Switching to $component_root ensures the labels always reference the true
repo revision and its remote.
- --label "org.opencontainers.image.revision=$(git -C "$script_dir" rev-parse HEAD)"
- --label "org.opencontainers.image.source=$(git -C "$script_dir" remote get-url origin)"
+ --label "org.opencontainers.image.revision=$(git -C "$component_root" rev-parse HEAD)"
+ --label "org.opencontainers.image.source=$(git -C "$component_root" remote get-url origin)"📝 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.
| if command -v git >/dev/null && git -C "$script_dir" rev-parse --is-inside-work-tree >/dev/null ; | |
| then | |
| build_cmd+=( | |
| --label "org.opencontainers.image.revision=$(git -C "$script_dir" rev-parse HEAD)" | |
| --label "org.opencontainers.image.source=$(git -C "$script_dir" remote get-url origin)" | |
| ) | |
| fi | |
| if command -v git >/dev/null && git -C "$script_dir" rev-parse --is-inside-work-tree >/dev/null ; | |
| then | |
| build_cmd+=( | |
| --label "org.opencontainers.image.revision=$(git -C "$component_root" rev-parse HEAD)" | |
| --label "org.opencontainers.image.source=$(git -C "$component_root" remote get-url origin)" | |
| ) | |
| fi |
🤖 Prompt for AI Agents
In
components/core/tools/docker-images/clp-env-base-manylinux_2_28-x86_64/build.sh
around lines 18 to 24, the git commands use "$script_dir" which points to a
sub-directory, causing the labels to reflect the sub-tree commit instead of the
repository root commit. Update the git commands to use "$component_root" instead
of "$script_dir" to ensure the labels reference the true root commit and remote
URL of the repository.
|
Everything LGTM and approve the changes. |
kirkrodrigues
changed the title
…dd manylinux_2_28 dependency container images. (y-scope#1041) Co-authored-by: Kirk Rodrigues <2454684+kirkrodrigues@users.noreply.github.com> Co-authored-by: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com>
Description
This PR adds support for building with
manylinux_2_28. The manylinux project provides standardized and portable Linux build environments for Python binaries (wheels), ensuring wide compatibility across a variety of Linux distributions and platforms. By leveragingmanylinux_2_28, we can produce CLP binaries for both x86_64 and aarch64 architectures that are easily distributable and require minimal external dependencies. These binaries are expected to be compatible with Debian 10+, Ubuntu 18.10+, Fedora 29+, CentOS/RHEL 8+, and many other Linux distributions.Note1: The minimum required version of
libcurlin CLP has been reduced from 7.68.0 to 7.61.1 to support building with manylinux_2_28.libcurl’s ABI is highly stable, so using an older version should not pose compatibility issues, and @LinZhihao-723 has cross-referenced API usage with docs and @jackluo923 has tested the functionality manually. If there is a need to support even lower versions in the future, we will revisit this decision.Note2:
LibArchiveis now built with the-DENABLE_OPENSSL=OFFCMake option to prevent it from searching for static OpenSSL libraries during static linking, since they are no longer provided by most Linux distributions for security reasons.This PR also restructures the containers docs since not all images are published now, and there is more information we want to add per container.
Checklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Chores