Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 6 additions & 7 deletions .github/actions/clp-build-runtime-image/action.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,14 @@ inputs:
default: ""
description: "Container image registry password"
required: false
platform_id:
description: "Platform ID of the container (e.g. ubuntu)"
required: false
platform_version_id:
description: "Platform VERSION_ID / VERSION_CODENAME of the container
(e.g. jammy, focal, etc.)"
required: false
arch:
description: "Target architecture (amd64 or arm64)"
default: "amd64"
required: false
Comment on lines 19 to 20

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
default: "amd64"
required: false
required: true

Maybe open an issue to make this required?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@coderabbitai open an issue to track making the arch input required for .github/actions/clp-build-runtime-image. The issue should cover updating all current and future callers to pass arch explicitly, removing the default value, and verifying behavior for both amd64 and arm64. This is a follow-up cleanup and is out of scope for this PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
gh issue create \
  --repo "y-scope/clp" \
  --title "refactor(clp-build-runtime-image): make \`arch\` input required" \
  --assignee "junhaoliao" \
  --body "## Summary

The \`arch\` input for the \`.github/actions/clp-build-runtime-image\` composite action currently has a default value of \`\"amd64\"\`. This default masks potential misconfiguration and makes call sites less explicit about their intent.

## Tasks

- [ ] Remove the \`default: \"amd64\"\` from the \`arch\` input and set \`required: true\`.
- [ ] Audit all current callers of \`.github/actions/clp-build-runtime-image\` and update them to pass \`arch\` explicitly.
- [ ] Ensure any future callers are documented/required to pass \`arch\` explicitly.
- [ ] Verify the action behaves correctly for both \`amd64\` and \`arm64\`.

## Context

Raised during review of PR `#2273` (comment: https://github.com/y-scope/clp/pull/2273#discussion_r3268557767). This is a follow-up cleanup and was considered out of scope for that PR."

Length of output: 1091



@junhaoliao, I've opened #2301 to track this. It covers:

  • Removing the default: "amd64" and setting required: true on the arch input.
  • Auditing and updating all current callers to pass arch explicitly.
  • Documenting the requirement for future callers.
  • Verifying behaviour for both amd64 and arm64.

platform_version_codename:
description: >-
Platform version codename (e.g. jammy, noble) used as the runtime image base version.
required: true

runs:
using: "composite"
Expand Down Expand Up @@ -94,3 +91,5 @@ runs:
push: true
tags: "${{steps.extract-gh-meta.outputs.tags}}"
labels: "${{steps.extract-gh-meta.outputs.labels}}"
build-args: |
UBUNTU_VERSION_CODENAME=${{inputs.platform_version_codename}}
1 change: 1 addition & 0 deletions .github/workflows/clp-artifact-build.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -739,6 +739,7 @@ jobs:
image_registry_username: "${{github.actor}}"
image_registry_password: "${{secrets.GITHUB_TOKEN}}"
arch: "${{matrix.arch}}"
platform_version_codename: "jammy"

package-image-multiarch-manifest:
name: "package-image-multiarch-manifest"
Expand Down
9 changes: 4 additions & 5 deletions docs/src/dev-docs/building-package.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,10 @@ prebuilt version instead, check out the [releases](https://github.com/y-scope/cl

## Requirements

* An x86_64 Ubuntu 22.04 (Jammy) machine or container
* At runtime, the CLP package uses an Ubuntu Jammy container, so we need to build in a matching
environment.
* It should be possible to build a package for a different environment, it just requires a some
extra configuration.
* An amd64 or arm64 Ubuntu machine or container
* The package build creates a runtime image using the host Ubuntu version codename by default, so
host-built artifacts and the runtime image use matching Ubuntu environments.
* To reproduce official release package tarballs, use an amd64 Ubuntu 22.04 (Jammy) environment.
* [Docker]
* `containerd.io` >= 1.7.18
* `docker-buildx-plugin` >= 0.15.1
Expand Down
8 changes: 6 additions & 2 deletions tools/docker-images/clp-package/Dockerfile
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# syntax=docker/dockerfile:1

FROM ubuntu:jammy AS base
ARG UBUNTU_VERSION_CODENAME=jammy
Comment thread
Bill-hbrhbr marked this conversation as resolved.
FROM ubuntu:${UBUNTU_VERSION_CODENAME} AS base

WORKDIR /root

Expand All @@ -23,7 +24,10 @@ ENV LD_LIBRARY_PATH="${CLP_HOME}/lib" \
PYTHONPATH="${CLP_HOME}/lib/python3/site-packages" \
USER="clp-user"

RUN useradd --uid ${UID} --shell /bin/bash --home-dir ${CLP_HOME} ${USER}
RUN existing_user="$(getent passwd "${UID}" | cut --delimiter=: --fields=1)" || true; \
[ -z "${existing_user}" ] || userdel --remove "${existing_user}" \
|| { echo "ERROR: Cannot delete user '${existing_user}' that occupies UID ${UID}"; exit 1; }; \
useradd --uid ${UID} --shell /bin/bash --home-dir ${CLP_HOME} ${USER}
USER ${USER}
WORKDIR ${CLP_HOME}

Expand Down
9 changes: 9 additions & 0 deletions tools/docker-images/clp-package/build.sh
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,15 @@ build_cmd=(
--file "${script_dir}/Dockerfile"
)

ubuntu_version_codename="jammy"
if [[ -f /etc/os-release ]]; then
host_codename="$(. /etc/os-release && echo "$VERSION_CODENAME")"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Right now, task package builds a clp-package image using the same Ubuntu release as the host invoking the task command. However, I believe a 22.04 host should still be able to build a 24.04 clp-package image.

Should we expose the Ubuntu version codename as a Task argument or environment variable so the package target is not tied to the host release?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

dynamic ubuntu version detection on host should make more sense. the main reason why we want to avoid hardcoding a version of the ubuntu image is because whatever (Python wheels, glibc dependent binaries, etc.) a non-Jammy host builds do not run inside the Jammy-based image container.

the long-term plan is to migrate the base image from Ubuntu to Muslinux / Manylinux as @jackluo923 proposed. we (or whoever has bandwidth) can work on it in the next month. also, i think we shall explore setting up https://containers.dev/ too.

for now, this PR can unblock our developers to develop on different versions of Ubuntu hosts, without use of Ubuntu Jammy build containers

let me know what you think

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

just realized that tools/docker-images/clp-package/Dockerfile copies built artifacts instead of building the package itself. Yes I agree we should keep host and host-built container image consistent.

if [[ -n "$host_codename" ]]; then
ubuntu_version_codename="$host_codename"
fi
fi
build_cmd+=(--build-arg "UBUNTU_VERSION_CODENAME=${ubuntu_version_codename}")

if command -v git >/dev/null && git -C "$script_dir" rev-parse --is-inside-work-tree >/dev/null;
then
build_cmd+=(
Expand Down
Loading