ci: discover opt Python path dynamically + sync upstream#8
Conversation
## Summary Pin floating external GitHub Actions workflow refs to immutable SHAs. ## Why See the rationale doc: https://docs.google.com/document/d/1qOURCNx2zszQ0uWx7Fj5ERu4jpiYjxLVWBWgKa2wTsA/edit?tab=t.0 ## Validation - `rg -n --pcre2 "uses:\s*(?!\./)(?!docker://)[^#\n]+@(?![0-9a-f]{40}(?:\s+#.*)?$)\S+" .github/workflows` - `git diff --check` - `git diff --stat -- .github/workflows`
/opt/python-3.12/bin/python3 no longer exists on the ubuntu-24.04-riscv runner image. Use find to locate whatever Python version is installed under /opt so the step is resilient to runner image updates. Signed-off-by: Bruno Verachten <gounthar@gmail.com>
|
Note Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 10 minutes and 21 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe pull request updates two GitHub Actions workflows. The riscv64 build workflow is modified to dynamically discover the Python interpreter path at runtime and conditionally install Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull request overview
Updates CI workflows to be more robust and secure by (1) making the RISC-V64 build resilient to runner image Python layout changes and (2) syncing an upstream change that pins GitHub Actions references to specific SHAs.
Changes:
- Pin
actions/*andpypa/cibuildwheelworkflow references to commit SHAs in the wheels build workflow. - Replace the hardcoded
/opt/python-3.12/bin/python3path in the RISC-V64 workflow with dynamic discovery under/opt.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
.github/workflows/build_wheels.yml |
Pins action references to SHAs; updates artifact merge step reference. |
.github/workflows/build-riscv64.yml |
Dynamically discovers an /opt Python interpreter before installing cffi on the RISC-V64 runner. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - uses: actions/upload-artifact@v6 | ||
| - uses: actions/upload-artifact@b7c566a772e6b6bfb58ed0dc250532a479d7789f # v6 | ||
| with: | ||
| name: cibw-wheels-${{ matrix.os }}-${{ strategy.job-index }} |
There was a problem hiding this comment.
Confirmed. build_sdist has no matrix, so both ${{ matrix.os }} and ${{ strategy.job-index }} expand to empty strings — the artifact ends up named cibw-wheels--. This came in via upstream sync, not this branch. Filed a follow-up issue to fix it.
| steps: | ||
| - name: Merge artifacts | ||
| uses: actions/upload-artifact/merge@v4 | ||
| uses: actions/upload-artifact/merge@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 |
| # Discover the path dynamically — runner image updates may change the version. | ||
| OPT_PYTHON=$(find /opt -maxdepth 3 -name "python3" -type f 2>/dev/null | head -1) | ||
| if [ -n "$OPT_PYTHON" ]; then |
There was a problem hiding this comment.
The entire OPT_PYTHON block has been removed in the latest commit. Since maturin build runs under uvx --python 3.14, there's no interpreter auto-discovery — the opt-Python cffi install was never exercised. The nondeterminism concern is now moot.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/build-riscv64.yml (2)
21-21: Inconsistent pinning policy across the two workflows.This PR pins external actions to commit SHAs in
build_wheels.yml, butbuild-riscv64.ymlstill uses floating tags (actions/checkout@v4,astral-sh/setup-uv@v7,actions/upload-artifact@v4,actions/download-artifact@v4). For the supply-chain rationale stated in the PR description to apply uniformly, consider pinning these here as well in a follow-up — otherwise this workflow remains a soft target for action-tag hijacking.Also applies to: 33-33, 63-63, 80-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-riscv64.yml at line 21, The workflow uses floating tags for GitHub Actions (actions/checkout@v4, astral-sh/setup-uv@v7, actions/upload-artifact@v4, actions/download-artifact@v4) which is inconsistent with the commit-SHA pinning used in build_wheels.yml; update each use to the exact commit SHA (the same pattern used in build_wheels.yml) so the workflow pins those actions to immutable SHAs, replacing the tag references with their corresponding commit SHAs for actions/checkout, astral-sh/setup-uv, actions/upload-artifact and actions/download-artifact to enforce consistent supply-chain protection.
47-48: TheOPT_PYTHONcffi installation (lines 43–46) is dead code with the explicit--python 3.14flag.
maturin build --python 3.14builds only for the specified interpreter and does not auto-discover other Python interpreters. The comment above (lines 38–41) describes maturin's auto-discovery behavior, but that path is not exercised here. Thesudo $OPT_PYTHON -m pip install cffistep is redundant and can be removed, along with theapt-get install libffi-devstep, unless kept as a defensive measure for future workflow changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-riscv64.yml around lines 47 - 48, The workflow runs `maturin build --python 3.14`, so the earlier `OPT_PYTHON` cffi install is dead code; remove the `sudo $OPT_PYTHON -m pip install cffi` step and the related `apt-get install libffi-dev` installation (or, if you intend to keep defensive packages, explicitly document that they are optional), and update the preceding comment to reflect that `maturin` is being invoked for a single interpreter via `maturin build --python 3.14` rather than using autodiscovery; reference `OPT_PYTHON`, the `maturin build --python 3.14` invocation, and `apt-get install libffi-dev` to locate the lines to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build-riscv64.yml:
- Around line 42-46: The find invocation that sets OPT_PYTHON is too
restrictive: increase the search depth (e.g., change -maxdepth 3 to at least
-maxdepth 6) and broaden the name pattern so it matches versioned Python
executables (e.g., use -name "python3*" or a glob matching python3.*) and
consider matching executable files (e.g., -type f -executable) so OPT_PYTHON
actually points to the runner's Python binary before running sudo "$OPT_PYTHON"
-m pip install --quiet cffi.
---
Nitpick comments:
In @.github/workflows/build-riscv64.yml:
- Line 21: The workflow uses floating tags for GitHub Actions
(actions/checkout@v4, astral-sh/setup-uv@v7, actions/upload-artifact@v4,
actions/download-artifact@v4) which is inconsistent with the commit-SHA pinning
used in build_wheels.yml; update each use to the exact commit SHA (the same
pattern used in build_wheels.yml) so the workflow pins those actions to
immutable SHAs, replacing the tag references with their corresponding commit
SHAs for actions/checkout, astral-sh/setup-uv, actions/upload-artifact and
actions/download-artifact to enforce consistent supply-chain protection.
- Around line 47-48: The workflow runs `maturin build --python 3.14`, so the
earlier `OPT_PYTHON` cffi install is dead code; remove the `sudo $OPT_PYTHON -m
pip install cffi` step and the related `apt-get install libffi-dev` installation
(or, if you intend to keep defensive packages, explicitly document that they are
optional), and update the preceding comment to reflect that `maturin` is being
invoked for a single interpreter via `maturin build --python 3.14` rather than
using autodiscovery; reference `OPT_PYTHON`, the `maturin build --python 3.14`
invocation, and `apt-get install libffi-dev` to locate the lines to change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5122b8a1-62fe-4e7d-abe9-760e9838ab47
📒 Files selected for processing (2)
.github/workflows/build-riscv64.yml.github/workflows/build_wheels.yml
maturin build --python 3.14 does not auto-discover interpreters, so the cffi installation into the RISE runner's opt Python was never used. Remove it along with the dynamic path discovery; --with cffi in the uvx invocation already provides cffi in the Python 3.14 build env. Signed-off-by: Bruno Verachten <gounthar@gmail.com>
Summary
Two changes in one branch:
Fix: RISE runner Python path
The scheduled build was failing with:
The runner image no longer has Python 3.12 at
/opt/python-3.12/bin/python3. Replace the hardcoded path with afindcall that locates whatever Python version is present under/opt/. If no opt Python is found, the step is skipped gracefully — maturin still builds for the explicit--python 3.14interpreter viauvx.Sync: upstream main (openai/tiktoken@dcb3928)
Merges one upstream commit:
[codex] Pin GitHub Actions workflow references (#515), which pins action SHAs inbuild_wheels.ymlfor supply-chain security.Test plan
uvx maturin buildstep without erroring on the Python path.Fixes: https://github.com/gounthar/tiktoken/actions/runs/24948658542
Summary by CodeRabbit