Restructure CI with matrix approach and multi-feature support#7376
Restructure CI with matrix approach and multi-feature support#7376youknowone merged 4 commits intoRustPython:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis 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
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
coderabbitai
bot
changed the title
.github/workflows/ci.yaml
Outdated
| os: [macos-latest, ubuntu-latest, windows-2025] | ||
| include: | ||
| - os: macos-latest | ||
| cargo_args: "" # TODO: Fix jit on macOS |
There was a problem hiding this comment.
jit seems to be fixed on macos now
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
scripts/whats_left.py (1)
452-455: Logic issue:joined_featurescomputed before conditional check.The
joined_featuresvariable is computed unconditionally on line 453, but the checkif args.features:on line 454 implies features could be empty/falsy. Ifargs.featuresis an empty list,joined_featureswould be an empty string"", and theif args.features:check would beFalse, so this works correctly. However, with the currentdefault=["ssl"],args.featuresis 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
📒 Files selected for processing (2)
.github/workflows/ci.yamlscripts/whats_left.py
scripts/whats_left.py
Outdated
| action="append", | ||
| help="which features to enable when building RustPython (default: [ssl])", | ||
| default=["ssl"], |
There was a problem hiding this comment.
Bug: action="append" with non-None default causes incorrect feature handling.
Using action="append" with default=["ssl"] has two issues:
- The default list is not cleared when
--featuresis provided—values append to it, causing["ssl", ...]always to includessl. - 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 argsAlternatively, 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.
…thon#7376) * `--features` flag can take multiple values * Simplify CI job * Remove bad default * Enable jit on macOS
2 participants
Summary by CodeRabbit