GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Restructure CI with matrix approach and multi-feature support by ShaharNaveh · Pull Request #7376 · RustPython/RustPython

Conversation

@ShaharNaveh

@ShaharNaveh ShaharNaveh commented Mar 7, 2026 •
edited by coderabbitai Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • Chores
    • Simplified and unified CI workflows to use a platform matrix, reducing duplicated steps and consolidating build and check steps across macOS, Linux, and Windows.
    • Improved local build script argument handling to allow multiple feature flags (as a list), passing them cleanly to build/run commands for more flexible feature combinations.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
ShaharNaveh added 2 commits March 7, 2026 09:43
@ShaharNaveh
--features flag can take multiple values
87aa7ad
@ShaharNaveh
Simplify CI job
89d4b84
@coderabbitai

coderabbitai Bot commented Mar 7, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 90f95a49-33ee-4145-9886-e9fd8f001f47

📥 Commits

Reviewing files that changed from the base of the PR and between 89d4b84 and a2a80ab.

📒 Files selected for processing (2)
  • .github/workflows/ci.yaml
  • scripts/whats_left.py

📝 Walkthrough

Walkthrough

This PR refactors the CI workflow to a matrix-driven, OS-explicit configuration with unified cargo build steps (including jit/threading) and adds a dedicated Mac setup step. It also changes scripts/whats_left.py to accept multiple --features entries (append) and pass them as a comma-separated --features value to cargo commands.

Changes

Cohort / File(s) Summary
CI workflow (matrix + unified builds)
.github/workflows/ci.yaml
Replaced flat OS list with an include-based matrix enumerating macos-latest, ubuntu-latest, and windows-2025; added a “Set up the Mac environment” step; consolidated Rust build invocations into single OS-agnostic cargo commands that always enable jit (and threading where applicable); removed OS-specific build branches and unified the "whats_left" check to use matrix-provided CARGO_ARGS. Also applied the same consolidation to snippets_cpython job.
Feature flag handling in script
scripts/whats_left.py
Changed --features CLI flag from action="store", default="ssl" to action="append", default=[]; join provided feature values into a comma-separated joined_features and only add --features joined_features to cargo commands when features are present. No change to --no-default-features behavior.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • Optimize CI cache usage #6707: Related changes to how whats_left is invoked in CI and feature/jit handling across platforms.
  • relocate scripts under ./scripts #6753: Touches the same script and CI step (scripts/whats_left.py and its CI invocation).

Suggested reviewers

  • youknowone

Poem

🐰 I hopped through matrices, tidy and bright,
I gathered features by moonlit night,
One call now sings them, comma in tune,
CI hums softly beneath the moon,
Builds leap together — a rabbit’s delight!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed Title check skipped as CodeRabbit has written the PR title.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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
  • X
  • Mastodon
  • Reddit
  • LinkedIn

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@coderabbitai coderabbitai Bot changed the title @coderabbitai Restructure CI with matrix approach and multi-feature support Mar 7, 2026
youknowone
youknowone reviewed Mar 7, 2026
View reviewed changes
Comment thread .github/workflows/ci.yaml Outdated
os: [macos-latest, ubuntu-latest, windows-2025]
include:
- os: macos-latest
cargo_args: "" # TODO: Fix jit on macOS

@youknowone youknowone Mar 7, 2026

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

jit seems to be fixed on macos now

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ShaharNaveh reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@ShaharNaveh ShaharNaveh Mar 7, 2026

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I'll try

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
coderabbitai[bot]
coderabbitai Bot reviewed Mar 7, 2026
View reviewed changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (1)
scripts/whats_left.py (1)

452-455: Logic issue: joined_features computed before conditional check.

The joined_features variable is computed unconditionally on line 453, but the check if args.features: on line 454 implies features could be empty/falsy. If args.features is an empty list, joined_features would be an empty string "", and the if args.features: check would be False, so this works correctly. However, with the current default=["ssl"], args.features is never empty, making the condition always true.

Once the default handling is fixed (per the comment above), ensure consistent behavior when no features are requested.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/whats_left.py` around lines 452 - 455, The code computes
joined_features before checking args.features; move the join into the
conditional so you only compute and append features when args.features is truthy
(i.e., when features were actually requested). Specifically, inside the block
that tests args.features, call ",".join(args.features) and then extend
cargo_build_command with ["--features", joined_features] (referencing
joined_features, args.features, and cargo_build_command) to ensure no empty
--features is added when no features are provided and to align with the
corrected default handling.
🤖 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/ci.yaml:
- Around line 451-453: The argparse setup in whats_left.py is incorrectly
pre-populating args.features with default=["ssl"], so when multiple --features
are passed (from CARGO_ARGS and matrix.cargo_args) the script erroneously
includes "ssl"; update the argument definition that uses action="append" for
features to use default=None instead of default=["ssl"] (so args.features will
be None unless flags provided), and ensure any downstream logic that joins
args.features handles None appropriately (e.g., treat None as empty list) to
avoid injecting the "ssl" feature.

In `@scripts/whats_left.py`:
- Around line 70-72: The current add_argument call for the --features flag uses
action="append" with default=["ssl"], which causes the default to persist and
prevents splitting comma-separated inputs; change the add_argument for
--features to use default=None (keep action="append" or consider action="store"
depending on desired behavior), and after parsing args in the main routine
(where args is available) set args.features = ["ssl"] if args.features is None;
additionally, if you want to support comma-separated lists like "ssl,sqlite",
normalize by splitting each entry on commas and flattening/stripping them (apply
this normalization to the values collected for args.features) so --features
"ssl,sqlite" yields ["ssl","sqlite"] and providing --features overrides the
default instead of appending to it.

---

Nitpick comments:
In `@scripts/whats_left.py`:
- Around line 452-455: The code computes joined_features before checking
args.features; move the join into the conditional so you only compute and append
features when args.features is truthy (i.e., when features were actually
requested). Specifically, inside the block that tests args.features, call
",".join(args.features) and then extend cargo_build_command with ["--features",
joined_features] (referencing joined_features, args.features, and
cargo_build_command) to ensure no empty --features is added when no features are
provided and to align with the corrected default handling.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 2a545921-11ab-482f-960e-90124e0a4b05

📥 Commits

Reviewing files that changed from the base of the PR and between ed5bffe and 89d4b84.

📒 Files selected for processing (2)
  • .github/workflows/ci.yaml
  • scripts/whats_left.py

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread .github/workflows/ci.yaml Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread scripts/whats_left.py Outdated
Comment on lines +70 to +72
action="append",
help="which features to enable when building RustPython (default: [ssl])",
default=["ssl"],

@coderabbitai coderabbitai Bot Mar 7, 2026

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Bug: action="append" with non-None default causes incorrect feature handling.

Using action="append" with default=["ssl"] has two issues:

  1. The default list is not cleared when --features is provided—values append to it, causing ["ssl", ...] always to include ssl.
  2. Comma-separated values like --features "ssl,sqlite" are treated as a single string element, not split into separate features.

Per the context snippet from .github/workflows/cron-ci.yaml:95-100, the workflow calls --features "ssl,sqlite", which would result in ["ssl", "ssl,sqlite"] and joined_features = "ssl,ssl,sqlite".

🐛 Proposed fix: Use `default=None` and set default after parsing
     parser.add_argument(
         "--features",
         action="append",
-        help="which features to enable when building RustPython (default: [ssl])",
-        default=["ssl"],
+        help="which features to enable when building RustPython (default: ssl)",
+        default=None,
     )

     args = parser.parse_args()
+    if args.features is None:
+        args.features = ["ssl"]
     return args

Alternatively, if the intent is to accept comma-separated features in a single --features argument (matching cargo's behavior), the values should be split:

🔧 Alternative: Split comma-separated values
+    # Flatten comma-separated features into individual items
+    if args.features:
+        args.features = [f for feat in args.features for f in feat.split(",")]
+    else:
+        args.features = ["ssl"]
     return args
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/whats_left.py` around lines 70 - 72, The current add_argument call
for the --features flag uses action="append" with default=["ssl"], which causes
the default to persist and prevents splitting comma-separated inputs; change the
add_argument for --features to use default=None (keep action="append" or
consider action="store" depending on desired behavior), and after parsing args
in the main routine (where args is available) set args.features = ["ssl"] if
args.features is None; additionally, if you want to support comma-separated
lists like "ssl,sqlite", normalize by splitting each entry on commas and
flattening/stripping them (apply this normalization to the values collected for
args.features) so --features "ssl,sqlite" yields ["ssl","sqlite"] and providing
--features overrides the default instead of appending to it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
ShaharNaveh added 2 commits March 7, 2026 09:57
@ShaharNaveh
Remove bad default
9a84b59
@ShaharNaveh
Enable jit on macOS
a2a80ab
youknowone
youknowone approved these changes Mar 7, 2026
View reviewed changes
@youknowone
youknowone merged commit ab32c78 into RustPython:main Mar 7, 2026
11 of 13 checks passed

Uh oh!

There was an error while loading. Please reload this page.

@coderabbitai coderabbitai Bot mentioned this pull request Mar 7, 2026
Unified action for installing macos deps #7379
Merged
youknowone pushed a commit to youknowone/RustPython that referenced this pull request Mar 8, 2026
@ShaharNaveh @youknowone
Restructure CI with matrix approach and multi-feature support (RustPy…
3821826
…thon#7376)

* `--features` flag can take multiple values

* Simplify CI job

* Remove bad default

* Enable jit on macOS
This was referenced Mar 9, 2026
Use matrix for cargo check #7388
Merged
Enable jit on macos (CI) #7401
Merged
Cleanup ci.yaml matrix usage #7260
Merged
@coderabbitai coderabbitai Bot mentioned this pull request Mar 19, 2026
Run CI tests with max processes #7254
Merged
youknowone pushed a commit to youknowone/RustPython that referenced this pull request Mar 22, 2026
@ShaharNaveh
Restructure CI with matrix approach and multi-feature support (RustPy…
aeeff99
…thon#7376)

* `--features` flag can take multiple values

* Simplify CI job

* Remove bad default

* Enable jit on macOS
This was referenced Mar 27, 2026
fix cron-ci #7483
Merged
Split cargo check matrix to individual targets. Avoid cache poisoning #7540
Merged
youknowone pushed a commit that referenced this pull request Sep 16, 2026
@ShaharNaveh
Restructure CI with matrix approach and multi-feature support (#7376)
973f2f0
* `--features` flag can take multiple values

* Simplify CI job

* Remove bad default

* Enable jit on macOS
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments
@youknowone youknowone youknowone approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@ShaharNaveh @youknowone
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.