Skip to content
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
64b7d8d
feat: Add support for manylinux_2_28 and use it to build x86_64 or aa…
jackluo923 Jun 26, 2025
917cecd
fix: resolved python lint issues
jackluo923 Jun 26, 2025
53f5974
fix: resolved docs issues
jackluo923 Jun 26, 2025
73563d5
fix: resolved issues raised by code-rabbit
jackluo923 Jun 26, 2025
0b81039
fix: resolved more python linting issue
jackluo923 Jun 26, 2025
5ab5946
fix: resolved issues with github workflow
jackluo923 Jun 26, 2025
117ff00
fix: reduced the scope of the PR
jackluo923 Jun 27, 2025
9112825
Revert "fix: resolved issues with github workflow"
jackluo923 Jun 27, 2025
b18c2f3
fix: resolved issues raised by coderabbit
jackluo923 Jun 27, 2025
c6aea71
Merge branch 'main' into manylinux_2_28
jackluo923 Jun 27, 2025
258ba7b
Merge branch 'main' into manylinux_2_28
jackluo923 Jun 29, 2025
ab09ee3
fix: added back a line that wasn't supposed to be removed
jackluo923 Jun 29, 2025
af88773
Restructure docs before adding new, unpublished container images.
kirkrodrigues Jun 29, 2025
8378fdb
refactor: Remove unnecessary comments from build scripts; Make style …
kirkrodrigues Jun 29, 2025
9c5c624
Use latest conventions in install-all.sh
kirkrodrigues Jun 29, 2025
c31a6c0
Use latest conventions in install-packages-from-source.sh; Add commen…
kirkrodrigues Jun 29, 2025
dfa89e9
Use latest conventions in install-prebuilt-packages.sh; Remove unnece…
kirkrodrigues Jun 29, 2025
c744237
Simplify comment.
kirkrodrigues Jun 29, 2025
8cb738f
Rename images for consistency.
kirkrodrigues Jun 29, 2025
a35809d
Add new images to docs.
kirkrodrigues Jun 29, 2025
b1716df
Edit comment.
kirkrodrigues Jun 29, 2025
e916ecb
Don't lock base image version so that we can tell if something breaks…
kirkrodrigues Jun 29, 2025
e1809e8
Improve comment.
kirkrodrigues Jun 29, 2025
502a31c
Rename manylinux scripts directory.
kirkrodrigues Jun 29, 2025
f96a13a
Update more docs.
kirkrodrigues Jun 29, 2025
10dc855
Set minimum CMake version to 3.31.
kirkrodrigues Jun 30, 2025
02d257d
Add link to manylinux 2.28.
kirkrodrigues Jun 30, 2025
31e9874
Update components/core/CMakeLists.txt
kirkrodrigues Jun 30, 2025
8fd2fa0
Revert "Update more docs."
kirkrodrigues Jun 30, 2025
57cda64
Replace manylinux 2.28 with manylinux_2_28.
kirkrodrigues Jun 30, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions components/core/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

# shows up as "AlmaLinux").
# 2. A released called "AlmaLinux" doesn't guarantee we're running on a manylinux distro,
Copy link
Member Author

Choose a reason for hiding this comment

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

Is A released called a typoe? i.e. A release called

# but we can improve this check when someone reports an issue.
file(READ "/etc/centos-release" CENTOS_RELEASE_CONTENT)
if(NOT "${CENTOS_RELEASE_CONTENT}" MATCHES "AlmaLinux")
set(CLP_STATIC_LIBS_UNSUPPORTED_PLATFORM "CentOS")
endif()
endif()

if (DEFINED CLP_STATIC_LIBS_UNSUPPORTED_PLATFORM)
Expand Down Expand Up @@ -212,7 +220,7 @@ endif()
# Find and setup libcurl
# By default, CURL does not provide static libraries
if(CLP_NEED_CURL)
find_package(CURL 7.68.0 REQUIRED)
find_package(CURL 7.61.1 REQUIRED)
if(CURL_FOUND)
message(STATUS "Found CURL ${CURL_VERSION_STRING}")
else()
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
FROM quay.io/pypa/manylinux_2_28_aarch64
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Pin the base image digest for reproducibility

Same rationale as for the x86_64 image—lock the manylinux base by digest.

🧰 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 referenced by tag only, which can lead to
non-reproducible builds. Update the FROM instruction to pin the image by its
specific digest (sha256) to ensure the exact image version is used consistently.
Retrieve the digest for quay.io/pypa/manylinux_2_28_aarch64 and replace the tag
with the digest reference.


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.
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 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.

Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
#!/usr/bin/env bash

set -eu
set -o pipefail

script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )"
component_root="${script_dir}/../../../"

build_cmd=(
docker buildx build
--platform linux/arm64
--tag clp-core-dependencies-aarch64-manylinux-2.28:dev
"$component_root"
--file "${script_dir}/Dockerfile"
--load
)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

docker buildx build flag order breaks – place context last

docker buildx build stops parsing flags at the first non-flag argument (the build context).
Placing $component_root before --file causes --file (and all subsequent flags) to be interpreted as part of the context string, resulting in “unknown flag” errors.

-build_cmd=(
-    docker buildx build
-    --platform linux/arm64
-    --tag clp-core-dependencies-aarch64-manylinux-2.28:dev
-    "$component_root"
-    --file "${script_dir}/Dockerfile"
-    --load
-)
+build_cmd=(
+    docker buildx build
+    --platform linux/arm64
+    --tag clp-core-dependencies-aarch64-manylinux-2.28:dev
+    --file "${script_dir}/Dockerfile"
+    --load
+    "$component_root"
+)

After this change the command succeeds irrespective of Docker/Buildx version.

📝 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.

Suggested change
build_cmd=(
docker buildx build
--platform linux/arm64
--tag clp-core-dependencies-aarch64-manylinux-2.28:dev
"$component_root"
--file "${script_dir}/Dockerfile"
--load
)
build_cmd=(
docker buildx build
--platform linux/arm64
--tag clp-core-dependencies-aarch64-manylinux-2.28:dev
--file "${script_dir}/Dockerfile"
--load
"$component_root"
)
🤖 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 buildx build command places the build context
($component_root) before the --file flag, causing flags after the context to be
misinterpreted as part of the context and resulting in errors. To fix this,
reorder the command so that all flags, including --file, come before the build
context argument, placing $component_root last in the command array.


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
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Minor: use repo root for Git metadata to avoid submodule edge-cases

If this script is moved inside a git-submodule, git -C "$script_dir" will return the
submodule commit, not the main repo’s.
Consider git -C "$component_root" rev-parse --show-toplevel to always capture the
top-level commit/tag used for the build.

🤖 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 use "$script_dir" which can point to a
submodule, causing incorrect commit information. Change the git commands to use
"$component_root" instead, and verify that "$component_root" is set to the
top-level repository root. This ensures the labels reflect the main repo's
commit and source URL, avoiding submodule edge cases.


echo "Running: ${build_cmd[*]}"
"${build_cmd[@]}"
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
FROM quay.io/pypa/manylinux_2_28_x86_64
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Pin the base image to an immutable digest

Using only the tag (manylinux_2_28_x86_64) leaves you open to silent upstream changes. Pinning by digest guarantees reproducible builds:

-FROM quay.io/pypa/manylinux_2_28_x86_64
+FROM quay.io/pypa/manylinux_2_28_x86_64@sha256:<digest-here>

Update whenever you intentionally pull in a new base.

📝 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.

Suggested change
FROM quay.io/pypa/manylinux_2_28_x86_64
FROM quay.io/pypa/manylinux_2_28_x86_64@sha256:<digest-here>
🧰 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 referenced by tag only, which can lead to
non-reproducible builds due to upstream changes. Replace the tag reference with
the image's immutable digest by finding the digest for
quay.io/pypa/manylinux_2_28_x86_64 and updating the FROM line to use that
digest. This ensures builds are reproducible and only update when you
intentionally change the digest.


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
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 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_install

This 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.


# 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.
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
#!/usr/bin/env bash

set -eu
set -o pipefail

script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )"
component_root="${script_dir}/../../../"

build_cmd=(
docker buildx build
--platform linux/amd64
--tag clp-core-dependencies-x86-manylinux-2.28:dev
"$component_root"
--file "${script_dir}/Dockerfile"
--load
)
Copy link
Contributor

@coderabbitai coderabbitai bot Jun 29, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Pin latest base image at build time

Adding --pull guarantees we always build on the freshest manylinux_2_28 base instead of whatever happens to be cached locally.

 docker buildx build
     --platform linux/amd64
+    --pull
     --tag clp-core-dependencies-x86-manylinux-2.28:dev
📝 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.

Suggested change
build_cmd=(
docker buildx build
--platform linux/amd64
--tag clp-core-dependencies-x86-manylinux-2.28:dev
"$component_root"
--file "${script_dir}/Dockerfile"
--load
)
build_cmd=(
docker buildx build
--platform linux/amd64
--pull
--tag clp-core-dependencies-x86-manylinux-2.28:dev
"$component_root"
--file "${script_dir}/Dockerfile"
--load
)
🤖 Prompt for AI Agents
In
components/core/tools/docker-images/clp-env-base-manylinux-2.28-x86_64/build.sh
around lines 9 to 16, the docker build command lacks the --pull flag, which
ensures the base image is always updated to the latest version instead of using
a potentially stale cached image. Add the --pull option to the docker buildx
build command array to force pulling the freshest manylinux_2_28 base image at
build time.

Copy link
Member

Choose a reason for hiding this comment

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

@coderabbitai, can you open an issue to address this for all images in another PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

🧩 Analysis chain

@kirkrodrigues I'll create an issue to address this across all Docker images. Let me first check what other Docker build scripts exist in the repository to ensure we capture the full scope.


🏁 Script executed:

#!/bin/bash
# Find all build.sh scripts in Docker image directories
fd -t f "build.sh" components/core/tools/docker-images/

# Also check for any Dockerfiles to get a complete picture
fd -t f "Dockerfile" components/core/tools/docker-images/

# Look for any other Docker build commands in the repository
rg "docker.*build" --type sh -A 2 -B 2

Length of output: 3813


Now I'll create a GitHub issue to address adding the --pull flag to all Docker build scripts.


🏁 Script executed:

#!/bin/bash
# Create a GitHub issue to address adding --pull flag to all Docker build scripts

gh issue create \
  --title "Add --pull flag to all Docker build scripts to ensure fresh base images" \
  --body "## Description

Currently, Docker build scripts across the repository do not use the \`--pull\` flag, which means they may use stale cached base images instead of pulling the latest versions. This can lead to builds using outdated base images with potential security vulnerabilities or missing updates.

## Affected Files

The following build scripts need to be updated to include the \`--pull\` flag:

### Components Core Docker Images
- \`components/core/tools/docker-images/clp-core-ubuntu-jammy/build.sh\`
- \`components/core/tools/docker-images/clp-env-base-centos-stream-9/build.sh\`  
- \`components/core/tools/docker-images/clp-env-base-manylinux-2.28-aarch64/build.sh\`
- \`components/core/tools/docker-images/clp-env-base-manylinux-2.28-x86_64/build.sh\`
- \`components/core/tools/docker-images/clp-env-base-ubuntu-jammy/build.sh\`

### Tools Docker Images  
- \`tools/docker-images/clp-execution-base-ubuntu-jammy/build.sh\`

## Proposed Solution

Add the \`--pull\` flag to all \`docker build\` and \`docker buildx build\` commands in the build scripts. This ensures that:

1. Base images are always pulled fresh from the registry
2. Builds use the latest security updates and patches
3. Consistent behavior across different environments
4. Reduced risk of builds succeeding locally but failing in CI due to image differences

## Example

For scripts using \`docker build\`:
\`\`\`bash
docker build --pull --tag ... 
\`\`\`

For scripts using \`docker buildx build\`:
\`\`\`bash  
docker buildx build --pull --platform ... --tag ...
\`\`\`

## References

- Original discussion: https://github.com/y-scope/clp/pull/1041#discussion_r2173903209
- Related PR: https://github.com/y-scope/clp/pull/1041

## Acceptance Criteria

- [ ] All 6 build scripts listed above include the \`--pull\` flag
- [ ] Build scripts continue to work correctly with the added flag
- [ ] Documentation is updated if necessary" \
  --assignee kirkrodrigues

Length of output: 2049


@kirkrodrigues I've successfully created issue #1051 to address adding the --pull flag to all Docker build scripts across the repository.

The issue covers all 6 build scripts I found:

  • 5 scripts in components/core/tools/docker-images/
  • 1 script in tools/docker-images/

It includes the rationale for the change (ensuring fresh base images with latest security updates), examples of how to implement it, and references back to this PR discussion. The issue has been assigned to you as requested.


🧠 Learnings used
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: 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.


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
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Use repo root for Git metadata

When the script lives in a sub-directory, git -C "$script_dir" can resolve to a nested repo or fail if build context is exported elsewhere. Pointing at component_root is safer.

-        --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.

Suggested change
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 may point to a
sub-directory and cause issues if the build context is exported elsewhere.
Change the git commands to use "$component_root" instead of "$script_dir" to
ensure they reference the repository root for Git metadata, making the script
more reliable.


echo "Running: ${build_cmd[*]}"
"${build_cmd[@]}"
4 changes: 2 additions & 2 deletions components/core/tools/scripts/lib_install/libarchive.sh
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,8 @@ fi
cd ${extracted_dir}
mkdir -p cmake-build-release
cd cmake-build-release
# NOTE: Disable expat so the static libarchive doesn't look for it at link time
cmake -DENABLE_EXPAT=OFF ../
# NOTE: Disable Expat and OpenSSL so the static libarchive doesn't look for them at link time.
cmake -DENABLE_EXPAT=OFF -DENABLE_OPENSSL=OFF ../
make -j${num_cpus}

# Check if checkinstall is installed
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
#!/usr/bin/env bash

set -eu
set -o pipefail

script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )"
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 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.


"${script_dir}/install-prebuilt-packages.sh"
"${script_dir}/install-packages-from-source.sh"

# TODO: https://github.com/y-scope/clp/issues/795
"${script_dir}/../check-cmake-version.sh"
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
#!/usr/bin/env bash

set -eu
set -o pipefail

script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )"
lib_install_scripts_dir="${script_dir}/.."

# 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
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 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.

Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
#!/usr/bin/env bash

set -eu
set -o pipefail

dnf install -y \
gcc-c++ \
java-11-openjdk \
jq \
libcurl-devel \
mariadb-connector-c-devel \
openssl-devel \
zlib-devel \
zlib-static

# Determine architecture for `task` release to install
rpm_arch=$(rpm --eval "%{_arch}")
case "$rpm_arch" in
"x86_64")
task_pkg_arch="amd64"
;;
"aarch64")
task_pkg_arch="arm64"
;;
*)
echo "Error: Unsupported architecture - $rpm_arch"
exit 1
;;
esac

# Install `task`
# NOTE: We lock `task` to a version < 3.43 to avoid https://github.com/y-scope/clp/issues/872
task_pkg_path=$(mktemp -t --suffix ".rpm" task-pkg.XXXXXXXXXX) || exit 1
curl \
--fail \
--location \
--output "$task_pkg_path" \
--show-error \
"https://github.com/go-task/task/releases/download/v3.42.1/task_linux_${task_pkg_arch}.rpm"
dnf install --assumeyes "$task_pkg_path"
rm "$task_pkg_path"

# Downgrade to CMake v3 to work around https://github.com/y-scope/clp/issues/795
pipx uninstall cmake
pipx install cmake~=3.0
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ 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.0

This 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.

Suggested change
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.

Copy link
Member

Choose a reason for hiding this comment

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

Can we require a higher minor version like ~=3.30?

2 changes: 2 additions & 0 deletions docs/src/dev-guide/components-core/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ See the relevant README for your OS:

* [CentOS Stream 9](centos-stream-9-deps-install)
* [macOS](macos-deps-install)
* [manylinux 2.28](manylinux-2.28-deps-install)
* [Ubuntu 22.04](ubuntu-jammy-deps-install)

Want to build natively on an OS not listed here? You can file a [feature request][feature-req].
Expand Down Expand Up @@ -108,6 +109,7 @@ the relevant paths on your machine.

centos-stream-9-deps-install
macos-deps-install
manylinux-2.28-deps-install
ubuntu-jammy-deps-install
regex-utils
:::
Expand Down
17 changes: 17 additions & 0 deletions docs/src/dev-guide/components-core/manylinux-2.28-deps-install.md
Copy link
Member

Choose a reason for hiding this comment

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

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 using dnf as 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.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough. How about I just remove it?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, let's remove it for now. I think the explanation in tooling-containers.md should be sufficient.

Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# manylinux 2.28 setup

To install the dependencies required to build clp-core, follow the steps below. These same steps are
used by our Docker containers.

## Installing dependencies

:::{caution}
Before you run any commands below, you should review the scripts to ensure they will not install
any dependencies or apply any configurations that you don't expect.
:::

To install all dependencies, run the following with elevated privileges:

```shell
components/core/tools/scripts/lib_install/manylinux-2.28/install-all.sh
```
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 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.

104 changes: 90 additions & 14 deletions docs/src/dev-guide/tooling-containers.md
Original file line number Diff line number Diff line change
@@ -1,34 +1,110 @@
# Containers

We publish (to [GitHub packages][gh-packages]) several Docker container images useful for building
and running CLP:
We maintain several Docker container images that are useful for building and running CLP. All images
can be built and used locally, but some are available to download from
[GitHub Packages][gh-packages].

* An [image][core-deps-centos-stream-9] containing the dependencies necessary to build CLP core in a
Centos Stream 9 x86 environment.
To build an image locally, run the `build.sh` script in the image's directory.

## clp-core-dependencies-&lt;arch&gt;-manylinux-2.28

Images containing the dependencies necessary to build CLP core in a manylinux 2.28 environment
Copy link
Member

Choose a reason for hiding this comment

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

(aarch64 or x86).

Binaries built on manylinux 2.28 (based on AlmaLinux 8) are expected to be compatible with other
distros using glibc 2.28+, including:

* CentOS/RHEL 8+
* Debian 10+
* Fedora 29+
* Ubuntu 18.10+

### clp-core-dependencies-aarch64-manylinux-2.28

* Path:

```text
ghcr.io/y-scope/clp/clp-core-dependencies-x86-centos-stream-9:main
components/core/tools/docker-images/clp-env-base-manylinux-2.28-aarch64
```

* An [image][core-deps-ubuntu-jammy] containing the dependencies necessary to build CLP core in an
Ubuntu Jammy x86 environment.
### clp-core-dependencies-x86-manylinux-2.28

* Path:

```text
ghcr.io/y-scope/clp/clp-core-dependencies-x86-ubuntu-jammy:main
components/core/tools/docker-images/clp-env-base-manylinux-2.28-x86_64
```

Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Add GitHub-Packages links for the new manylinux 2.28 images

All other images list a Packages page and pull command, but the aarch64/x86 manylinux 2.28
entries do not—readers have no quick copy-paste source.

Recommend adding, for each image, something like:

* [GitHub Packages page][core-deps-manylinux_2_28_aarch64]
* Pull command:
  ```bash
  docker pull ghcr.io/y-scope/clp/clp-core-dependencies-aarch64-manylinux_2_28:main

and corresponding reference definitions at the bottom of the file.

This keeps the table of images consistent and discoverable.

<details>
<summary>🤖 Prompt for AI Agents</summary>

In docs/src/dev-guide/tooling-containers.md around lines 22 to 37, the manylinux
2.28 aarch64 and x86 image sections lack GitHub Packages page links and pull
commands. Add these links and commands below each image description, matching
the format used for other images. Also, add the corresponding reference
definitions for these links at the bottom of the file to maintain consistency
and ease of use.


</details>

<!-- This is an auto-generated comment by CodeRabbit -->

<!-- fingerprinting:phantom:triton:cougar -->

## clp-core-dependencies-x86-centos-stream-9

An image containing the dependencies necessary to build CLP core in a CentOS Stream 9 x86
environment.

* [GitHub Packages page][core-deps-centos-stream-9]
* Pull command:

```bash
docker pull ghcr.io/y-scope/clp/clp-core-dependencies-x86-centos-stream-9:main
```

* An [image][core-ubuntu-jammy] containing the CLP core binaries (`clg`, `clp`, `clp-s`, `glt`,
etc.) built in an Ubuntu Jammy x86 environment.
* Path:

```text
ghcr.io/y-scope/clp/clp-core-x86-ubuntu-jammy:main
components/core/tools/docker-images/clp-env-base-centos-stream-9
```

## clp-core-dependencies-x86-ubuntu-jammy

An image containing the dependencies necessary to build CLP core in an Ubuntu Jammy x86
environment.

* [GitHub Packages page][core-deps-ubuntu-jammy]
* Pull command:

```bash
docker pull ghcr.io/y-scope/clp/clp-core-dependencies-x86-ubuntu-jammy:main
```

* Path:

```text
components/core/tools/docker-images/clp-env-base-ubuntu-jammy
```

## clp-core-x86-ubuntu-jammy

An image containing the CLP core binaries (`clg`, `clp`, `clp-s`, `glt`, etc.) built in an Ubuntu
Jammy x86 environment.

* [GitHub Packages page][core-ubuntu-jammy]
* Pull command:

```bash
docker pull ghcr.io/y-scope/clp/clp-core-x86-ubuntu-jammy:main
```

* Path:

```text
components/core/tools/docker-images/clp-core-ubuntu-jammy
```

## clp-execution-x86-ubuntu-jammy

An image containing the dependencies necessary to run the CLP package in an Ubuntu Jammy x86
environment.

* [GitHub Packages page][exe-ubuntu-jammy]
* Pull command:

```bash
docker pull ghcr.io/y-scope/clp/clp-execution-x86-ubuntu-jammy:main
```

* An [image][exe-ubuntu-jammy] containing the dependencies necessary to run the CLP package in an
Ubuntu Jammy x86 environment.
* Path:

```text
ghcr.io/y-scope/clp/clp-execution-x86-ubuntu-jammy:main
tools/docker-images/clp-execution-base-ubuntu-jammy
```

[core-deps-centos-stream-9]: https://github.com/y-scope/clp/pkgs/container/clp%2Fclp-core-dependencies-x86-centos-stream-9
Expand Down
Loading