Ci gen opcode metadata #6677
Ci gen opcode metadata #6677
Conversation
📝 WalkthroughWalkthroughTwo GitHub Actions workflows are updated to include automatic opcode metadata generation: the CI linting phase now validates metadata is current, while the auto-format PR workflow executes the metadata generation script. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/pr-auto-commit.yaml (1)
66-71: Update commit message to reflect all automated changes.The commit message mentions only
cargo fmt --all, but the workflow now also runs ruff formatting and opcode metadata generation. The message should accurately describe all changes being committed.📝 Update commit message
- name: Commit and push formatting changes if: steps.check-changes.outputs.has_changes == 'true' run: | git add -u - git commit -m "Auto-format: cargo fmt --all" + git commit -m "Auto-format: cargo fmt, ruff, and opcode metadata" git push origin HEAD:${{ github.event.pull_request.head.ref }}
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yaml:
- Around line 334-339: The CI step "Ensure Lib/_opcode_metadata is updated" has
inverted logic: it currently fails when the working tree is clean because it
uses if [ -z "$(git status --porcelain)" ] then exit 1; fix it by inverting the
test to if [ -n "$(git status --porcelain)" ] then echo a helpful message (e.g.
"Lib/_opcode_metadata out of date") and exit 1; keep running python
scripts/generate_opcode_metadata.py and use the git status --porcelain output to
decide failure so the job only fails when changes are present.
🧹 Nitpick comments (2)
.github/workflows/pr-auto-commit.yaml (1)
50-50: Add explicit Python setup step before running the script.The workflow runs a Python script but doesn't explicitly set up Python. While
ubuntu-latestincludes Python by default, declaring it as a dependency makes the workflow more maintainable and self-documenting.🐍 Add setup-python step
Add this step before line 50:
- name: Setup Python uses: actions/setup-python@v6.1.0 with: python-version: "3.13.1".github/workflows/ci.yaml (1)
334-336: Add explicit Python setup step for the lint job.The lint job runs a Python script but doesn't set up Python. While
ubuntu-latestincludes Python, explicitly declaring the dependency improves workflow clarity and ensures the expected Python version is available.🐍 Add setup-python step to lint job
Add this step before line 334:
- name: Setup Python uses: actions/setup-python@v6.1.0 with: python-version: ${{ env.PYTHON_VERSION }}This reuses the
PYTHON_VERSIONenvironment variable already defined at line 109.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Lib/_opcode_metadata.pyis excluded by!Lib/**
📒 Files selected for processing (2)
.github/workflows/ci.yaml.github/workflows/pr-auto-commit.yaml
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-12-27T14:03:49.034Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: Applies to **/*.py : Use ruff for linting Python code
Applied to files:
.github/workflows/pr-auto-commit.yaml.github/workflows/ci.yaml
📚 Learning: 2025-12-27T14:03:49.034Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: Applies to **/*.rs : Always run clippy to lint code (`cargo clippy`) before completing tasks and fix any warnings or lints introduced by changes
Applied to files:
.github/workflows/ci.yaml
📚 Learning: 2025-12-27T14:03:49.034Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: When modifying bytecode instructions, perform a full clean build by running `rm -r target/debug/build/rustpython-* && find . | grep -E '\.pyc$' | xargs rm -r`
Applied to files:
.github/workflows/ci.yaml
📚 Learning: 2025-12-27T14:03:49.034Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: Applies to **/*.rs : Follow the default rustfmt code style by running `cargo fmt` to format Rust code
Applied to files:
.github/workflows/ci.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (windows-2025)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-2025)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run tests under miri
| - name: Ensure Lib/_opcode_metadata is updated | ||
| run: | | ||
| python scripts/generate_opcode_metadata.py | ||
| if [ -z "$(git status --porcelain)" ]; then | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Fix inverted validation logic - critical correctness issue.
The condition checks if the working tree is clean (no changes) and exits with failure in that case. This is backwards: the check should fail when changes are detected (metadata is stale) and pass when no changes are detected (metadata is current).
Currently:
- Metadata up-to-date (no changes) → Fails ❌
- Metadata stale (changes detected) → Passes ❌
Expected:
- Metadata up-to-date (no changes) → Passes ✅
- Metadata stale (changes detected) → Fails ✅
🐛 Fix to invert the logic
- name: Ensure Lib/_opcode_metadata is updated
run: |
python scripts/generate_opcode_metadata.py
- if [ -z "$(git status --porcelain)" ]; then
+ if [ -n "$(git status --porcelain)" ]; then
+ echo "Error: Lib/_opcode_metadata is out of date. Run 'python scripts/generate_opcode_metadata.py' locally and commit the changes."
exit 1
fiThe fix changes -z (zero length/empty) to -n (non-zero length/not empty) and adds a helpful error message.
🤖 Prompt for AI Agents
In @.github/workflows/ci.yaml around lines 334 - 339, The CI step "Ensure
Lib/_opcode_metadata is updated" has inverted logic: it currently fails when the
working tree is clean because it uses if [ -z "$(git status --porcelain)" ] then
exit 1; fix it by inverting the test to if [ -n "$(git status --porcelain)" ]
then echo a helpful message (e.g. "Lib/_opcode_metadata out of date") and exit
1; keep running python scripts/generate_opcode_metadata.py and use the git
status --porcelain output to decide failure so the job only fails when changes
are present.
|
It seems like I can't test it unless it's merged to main:/ |
|
Thank you so much! lets try! |
* Autogen opcode metadata * Manually modify opcode metadata
2 participants
Closes: #6676
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.