Skip to content

Conversation

@jiridanek
Copy link
Member

@jiridanek jiridanek commented Sep 23, 2025

Description

How Has This Been Tested?

Self checklist (all need to be checked):

  • Ensure that you have run make test (gmake on macOS) before asking for review
  • Changes to everything except Dockerfile.konflux files should be done in odh/notebooks and automatically synced to rhds/notebooks. For Konflux-specific changes, modify Dockerfile.konflux files directly in rhds/notebooks as these require special attention in the downstream repository and flow to the upcoming RHOAI release.

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

Summary by CodeRabbit

  • New Features

    • Added cross-compilation support enabling builds for additional target architectures
    • Integrated Zig toolchain for enhanced compilation capabilities
    • Expanded image build matrix for Python 3.12 releases, including additional variants
  • Chores

    • Enhanced build system with improved toolchain provisioning and architecture handling

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 23, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 23, 2025

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

This PR introduces Zig cross-compilation toolchain support to the build system. It adds architecture normalization and Zig installation targets to the Makefile, creates a new zigcc compiler wrapper in Go, updates the sandbox script to configure cross-compilation targeting s390x-linux-gnu.2.34, adjusts Docker runtime configurations, and expands the Python 3.12 image build matrix.

Changes

Cohort / File(s) Summary
Zig Toolchain Installation
Makefile
Introduces architecture normalization (amd64 → x86_64, arm64 → aarch64), defines ZIG_VERSION and ZIG_BINARY variables, adds bin/zig-$(ZIG_VERSION) target to download and extract Zig for system architecture with s390x-linux-gnu.2.34 shim wrappers, adds phony install-zig and clean-zig targets, expands all-images build matrix for Python 3.12 releases
Zig Compiler Wrapper
scripts/zigcc/Makefile, scripts/zigcc/go.mod, scripts/zigcc/zigcc.go, scripts/zigcc/zigcc_test.go
Introduces new Go-based compiler wrapper module for zigcc with Makefile targets (build, test, fmt, vet, clean), establishes go.mod for zigcc module at Go 1.24, implements wrapper that maps executable name to Zig subcommand, transforms and expands -Wp,... arguments, and executes Zig with fixed s390x-linux-gnu.2.34 target, includes table-driven tests for -Wp argument processing
Cross-Compilation Setup
scripts/sandbox.py, wrapper.py
Extends sandbox.py with target-specific s390x-linux-gnu.2.34 configuration, replaces simple command construction with substitution mechanism for "{};" placeholder injection of Docker volumes/environment flags, ensures bin/zig-0.15.1 and zigcc build prerequisites exist before sandbox setup, adds diagnostic log blocks; introduces new wrapper.py script that normalizes -Wp,-D... flags, detects zig-cc/zig-c++ invocations, and executes through /mnt/zig with appropriate target triple
Docker Runtime Configuration
runtimes/minimal/ubi9-python-3.12/Dockerfile.cpu
Adds no-op RUN command for /mnt listing, adjusts architecture-specific OS package set for s390x and ppc64le to replace gcc g++ make with gcc g++ ninja-build

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

  • scripts/sandbox.py: Review the substitution mechanism for "{};" placeholder injection, build prerequisite logic for Zig toolchain, and environment variable handling for cross-compilation
  • scripts/zigcc/zigcc.go: Verify argument transformation logic (-Wp flag expansion), Zig invocation correctness, and target triple handling
  • Makefile: Validate architecture normalization, Zig version/binary definitions, and build target dependencies
  • Integration points: Ensure sandbox.py correctly builds and stages zigcc artifacts, and that wrapper.py complements the Go wrapper's functionality

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Description check ⚠️ Warning The PR description is largely incomplete. While it contains the required template structure, the Description section only references an external GitHub link without explaining the actual changes, and the 'How Has This Been Tested?' section is empty with all checklist items unchecked. Provide a detailed explanation of the changes in the Description section, document how you tested the Zig setup (especially cross-compilation), and verify all self-checklist items are completed before requesting review.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: introducing Zig cross-compilation setup for GitHub Actions and local development, which aligns with the comprehensive changes across Makefile, Dockerfile, scripts, and tooling additions.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 23, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign jesuino for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@github-actions github-actions bot added the review-requested GitHub Bot creates notification on #pr-review-ai-ide-team slack channel label Sep 23, 2025
@jiridanek jiridanek marked this pull request as ready for review November 12, 2025 18:47
@openshift-ci openshift-ci bot added size/l and removed size/l labels Nov 12, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 56c9cde and c6c9441.

📒 Files selected for processing (8)
  • Makefile (1 hunks)
  • runtimes/minimal/ubi9-python-3.12/Dockerfile.cpu (2 hunks)
  • scripts/sandbox.py (2 hunks)
  • scripts/zigcc/Makefile (1 hunks)
  • scripts/zigcc/go.mod (1 hunks)
  • scripts/zigcc/zigcc.go (1 hunks)
  • scripts/zigcc/zigcc_test.go (1 hunks)
  • wrapper.py (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
wrapper.py (1)
scripts/sandbox.py (1)
  • main (27-80)
🪛 checkmake (0.2.2)
scripts/zigcc/Makefile

[warning] 1-1: Missing required phony target "all"

(minphony)

🪛 Ruff (0.14.4)
scripts/sandbox.py

51-51: Local variable target is assigned to but never used

Remove assignment to unused variable target

(F841)


62-62: f-string without any placeholders

Remove extraneous f prefix

(F541)


64-64: f-string without any placeholders

Remove extraneous f prefix

(F541)


65-65: f-string without any placeholders

Remove extraneous f prefix

(F541)


84-84: String contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?

(RUF001)


146-146: subprocess call: check for execution of untrusted input

(S603)


148-148: subprocess call: check for execution of untrusted input

(S603)


150-150: subprocess call: check for execution of untrusted input

(S603)

wrapper.py

1-1: Shebang is present but file is not executable

(EXE001)


16-16: Consider iterable unpacking instead of concatenation

Replace with iterable unpacking

(RUF005)


18-18: Consider iterable unpacking instead of concatenation

Replace with iterable unpacking

(RUF005)


20-20: Avoid specifying long messages outside the exception class

(TRY003)


22-22: Starting a process without a shell

(S606)

Comment on lines 35 to 37
if [ "$ARCH" = "s390x" ] || [ "$ARCH" = "ppc64le" ]; then \
PACKAGES="$PACKAGES gcc g++ make openssl-devel autoconf automake libtool cmake"; \
PACKAGES="$PACKAGES gcc g++ ninja-build openssl-devel autoconf automake libtool cmake"; \
fi && \
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 | 🟠 Major

Restore make for non-ninja builds on s390x/ppc64le

Dropping make here leaves those arches without the classic build tool that many Python/C extensions still expect, so pip install of projects that call make will now fail. Please keep make while adding ninja-build.

-        PACKAGES="$PACKAGES gcc g++ ninja-build openssl-devel autoconf automake libtool cmake"; \
+        PACKAGES="$PACKAGES gcc g++ make ninja-build openssl-devel autoconf automake libtool cmake"; \
🤖 Prompt for AI Agents
In runtimes/minimal/ubi9-python-3.12/Dockerfile.cpu around lines 35 to 37, the
s390x/ppc64le PACKAGES list removed make which breaks builds that invoke make;
update the PACKAGES assignment to include make alongside ninja-build (e.g., add
"make" to the quoted package list) so both classic make and ninja are available
for those architectures.

Comment on lines +1 to +3
module zigcc

go 1.24
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 | 🔴 Critical

Target a released Go toolchain

go 1.24 is not available yet, so current builders (Go ≤1.22/1.23) will abort with “requires go >=1.24”. Drop to the latest released version you actually need (e.g. 1.22).

-go 1.24
+go 1.22
📝 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
module zigcc
go 1.24
module zigcc
go 1.22
🤖 Prompt for AI Agents
In scripts/zigcc/go.mod around lines 1 to 3, the module specifies "go 1.24"
which is not yet released and will abort builds on current Go toolchains; change
the go directive to an actual released version your project needs (for example
"go 1.22" or the minimum released version that supports the features you use),
update any CI/docker builders to match that version if necessary, and run go mod
tidy/build locally to verify compatibility.

@jiridanek
Copy link
Member Author

@coderabbitai provide commentary and improvement suggestions for the zig cc approach

@jiridanek
Copy link
Member Author

jiridanek commented Nov 20, 2025

dnf install -y bcc-tools
/usr/share/bcc/tools/execsnoop
sudo apt-get install bpfcc-tools
        printf '%s\n' '#!/bin/sh' 'exec /mnt/zig cc -target s390x-linux-gnu.2.34 "$@"' > $$TMPDIR/zig-cc
	printf '%s\n' '#!/bin/sh' 'exec /mnt/zig c++ -target s390x-linux-gnu.2.34 "$@"' > $$TMPDIR/zig-c++
	chmod +x $$TMPDIR/zig-cc
	chmod +x $$TMPDIR/zig-c++

@openshift-ci openshift-ci bot added size/l and removed size/l labels Nov 20, 2025
@jiridanek
Copy link
Member Author

            # Speed things up
            "--env=MAKEFLAGS=-j8",
            "--env=CMAKE_BUILD_PARALLEL_LEVEL=8",
            "--volume=/notebooks-zig:/opt/app-root/.cache/zig",

@jiridanek
Copy link
Member Author

            "Install micropipenv and uv to deploy packages from requirements.txt": textwrap.dedent(r'''
                RUN --mount=type=cache,target=/opt/app-root/src/.cache/pip \
                    pip install --cache-dir=/opt/app-root/src/.cache/pip --extra-index-url https://pypi.org/simple -U "micropipenv[toml]==1.9.0" "uv==0.8.12"'''),

…ance cross-compilation tool wrapping, and document usage in README.
@jiridanek
Copy link
Member Author

            # Parallelism
            "--env=UV_CONCURRENT_BUILDS=8",
            "--unsetenv=UV_CONCURRENT_BUILDS",
            "--env=UV_CONCURRENT_DOWNLOADS=10",
            "--unsetenv=UV_CONCURRENT_DOWNLOADS",

@jiridanek
Copy link
Member Author


            "--env=bustcachez=xx",
            "--unsetenv=bustcachez",

@jiridanek
Copy link
Member Author

Prepared 109 packages in 7m 12s
error: Failed to install: ipython-9.5.0-py3-none-any.whl (ipython==9.5.0)
  Caused by: failed to create directory `/opt/app-root/share`: Permission denied (os error 13)
Error: building at STEP "RUN --mount=type=cache,target=/opt/app-root/src/.cache/uv /bin/bash <<'EOF'": while running runtime: exit status 2

this seems a flaky fail?!?

@openshift-ci openshift-ci bot added size/l and removed size/l labels Nov 20, 2025
…rget` logic, add glibc version handling, and integrate debugging tools. Document usage and background in README. Add tests and Dockerfile for validation.
@openshift-ci openshift-ci bot added size/l and removed size/l labels Nov 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/work-in-progress review-requested GitHub Bot creates notification on #pr-review-ai-ide-team slack channel size/l

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants