Repository navigation
ci: speed up windows tests and snapshot tests - #3688
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR consolidates CI tool installation via mise and optimizes build infrastructure. It adds ChangesCI Tool Consolidation & Windows Python Wheel Caching
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
6c664dc to
fe2f9af
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c664dcc85
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| anyhow = { workspace = true } | ||
| flate2 = { workspace = true } | ||
| reqwest = { workspace = true, features = [ "blocking", "rustls" ] } | ||
| reqwest = { workspace = true, features = [ "blocking", "rustls-no-provider" ] } |
There was a problem hiding this comment.
When this crate is used by the standalone baml wrapper (which now also enables only rustls-no-provider in crates/baml/Cargo.toml:19 and has no sys_native/baml_lsp_server dependency), no code calls rustls::crypto::*::install_default() before download_bytes() builds its blocking HTTPS client. reqwest's no-provider mode requires a process-default rustls provider, so install/update paths that fetch GitHub releases fail at client construction instead of downloading. Please either keep a provider feature on these downloader crates or install one before building the client.
Useful? React with 👍 / 👎.
Binary size checks passed✅ 7 passed
Generated by |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/cargo-tests.reusable.yaml:
- Around line 59-66: Add an explicit CI validation step that actually
compiles/tests with the RING_FEATURES environment to catch any missing/typo
features or compilation regressions: in the reusable workflow where
RING_FEATURES is defined, add a job/step that exports RING_FEATURES and runs
`cargo check --no-default-features --features "$RING_FEATURES"` (or `cargo
build`/`cargo test` as appropriate) and make it robust to network flakiness by
enabling cargo retry/cache or increasing fetch timeout so the CI run validates
the crypto-provider migration and heap_debug/ring-crypto feature combinations
referenced by the RING_FEATURES variable.
In `@baml_language/crates/baml_release/Cargo.toml`:
- Line 22: baml_release enables reqwest with rustls-no-provider but never
installs a rustls crypto provider; update the crate so the provider is installed
before any blocking HTTPS requests by calling the provider installer (e.g.
rustls_crypto_provider::install_default() or
sys_native::ensure_rustls_crypto_provider()) from inside baml_release: either
invoke it at crate initialization or call it in Fetcher::new (and/or at the
start of download_bytes()) so every Fetcher/request guarantees the provider is
present before using reqwest::blocking::Client.
In `@baml_language/crates/baml/Cargo.toml`:
- Line 19: The project enables reqwest with rustls-no-provider in baml's
Cargo.toml which requires a concrete rustls crypto provider at runtime; add
explicit wiring so every shipped binary that depends on baml declares a
sys_native dependency (with ring-crypto or aws-crypto features) or ensure baml
itself calls sys_native::ensure_rustls_crypto_provider() /
CryptoProvider::install_default() during guaranteed runtime initialization;
locate the reqwest = { workspace = true, features =
["blocking","rustls-no-provider"] } entry and either add a corresponding
sys_native dependency in each consuming Cargo.toml or move the
ensure_rustls_crypto_provider()/CryptoProvider::install_default() call into
baml's init path so consumers no longer must declare the provider.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ef72151c-ea1a-4c06-9c24-6349215459ff
📒 Files selected for processing (6)
.github/workflows/cargo-tests.reusable.yamlbaml_language/crates/baml/Cargo.tomlbaml_language/crates/baml_release/Cargo.tomlbaml_language/crates/baml_tests/Cargo.tomlbaml_language/crates/bex_engine/Cargo.tomlmise.toml
fe2f9af to
f8806e4
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
.github/workflows/cargo-tests.reusable.yaml (3)
239-245: 💤 Low valuemacOS archive job doesn't collect cargo timings or sccache stats.
The cross-build step at line 242 uses
cargo nextest archivewithout--timings, and the job has no sccache stats collection steps. The merged artifact jobs (lines 945-957) now depend on this job but won't receive any timing/stats artifacts from it.If tracking cross-build performance is valuable, consider adding
--timingsto the archive command and the sccache stats steps (similar to lines 138-163 in the Linux job). Otherwise, this is fine as-is since cross-build metrics may be less actionable.Also applies to: 945-957
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/cargo-tests.reusable.yaml around lines 239 - 245, The macOS cross-build archive invocation using cargo nextest archive (the command creating "$RUNNER_TEMP/macos-tests.tar.zst") is missing timing and sccache stats collection; update that command to include the --timings flag and add the same sccache stats steps used in the Linux job (the block that collects sccache --show-stats output and uploads it as artifact) so the merged-artifact jobs that depend on this run (the jobs referenced around the merged artifact block) will receive timing and sccache artifacts as well.
166-254: 💤 Low valueMissing
Swatinem/rust-cachestep in the macOS archive job.The Linux job (line 98-104) uses
Swatinem/rust-cacheto cache the Cargo registry and git checkouts, but this cross-build job doesn't include it. While sccache handles compilation artifacts, the registry/git cache can still save significant fetch time for the large git forks mentioned in the header comments.Consider adding a rust-cache step with a darwin-specific
shared-key(e.g.,"linux-darwin-cross") if cross-target builds can share registry state with native Linux builds, or confirm this omission is intentional because the job already benefits from thecargo fetchand sccache hitting warm artifacts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/cargo-tests.reusable.yaml around lines 166 - 254, Add the missing Swatinem/rust-cache step into the cargo-test-macos-archive job to cache Cargo registry/git checkouts: insert a step using Swatinem/rust-cache (e.g., uses: Swatinem/rust-cache@v1) before the "Fetch cargo dependencies" step, configure it to cache cargo registry and git checkouts and set a darwin-aware shared-key like "linux-darwin-cross" (or an appropriate shared-key per your cache policy) so cross-builds reuse the registry/git cache; ensure the step runs in the same job "cargo-test-macos-archive" and does not conflict with existing sccache usage.
255-279: 💤 Low valueConsider adding failure artifact upload for the macOS run job.
The Linux job (lines 129-136) uploads test failures (
*.snap.new,target/nextest/) when tests fail, but this macOS run job doesn't have an equivalent step. While the binaries are prebuilt, uploading nextest output on failure would help debug test failures without re-running locally.📦 Optional: Add failure artifact upload
- name: "Upload test results on failure" if: failure() uses: actions/upload-artifact@v7 with: name: test-failures-macos path: | baml_language/**/target/nextest/ if-no-files-found: ignore🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/cargo-tests.reusable.yaml around lines 255 - 279, The macOS job cargo-test-macos-run lacks a failure artifact upload step; add a step after the "Run prebuilt tests (no build)" step that runs only on failure (if: failure()), uses actions/upload-artifact@v7, and uploads nextest outputs and snapshot diffs (e.g., baml_language/**/target/nextest/ and any *.snap.new files) with a descriptive artifact name like test-failures-macos and if-no-files-found: ignore so debugging artifacts are available when tests fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/cargo-tests.reusable.yaml:
- Around line 239-245: The macOS cross-build archive invocation using cargo
nextest archive (the command creating "$RUNNER_TEMP/macos-tests.tar.zst") is
missing timing and sccache stats collection; update that command to include the
--timings flag and add the same sccache stats steps used in the Linux job (the
block that collects sccache --show-stats output and uploads it as artifact) so
the merged-artifact jobs that depend on this run (the jobs referenced around the
merged artifact block) will receive timing and sccache artifacts as well.
- Around line 166-254: Add the missing Swatinem/rust-cache step into the
cargo-test-macos-archive job to cache Cargo registry/git checkouts: insert a
step using Swatinem/rust-cache (e.g., uses: Swatinem/rust-cache@v1) before the
"Fetch cargo dependencies" step, configure it to cache cargo registry and git
checkouts and set a darwin-aware shared-key like "linux-darwin-cross" (or an
appropriate shared-key per your cache policy) so cross-builds reuse the
registry/git cache; ensure the step runs in the same job
"cargo-test-macos-archive" and does not conflict with existing sccache usage.
- Around line 255-279: The macOS job cargo-test-macos-run lacks a failure
artifact upload step; add a step after the "Run prebuilt tests (no build)" step
that runs only on failure (if: failure()), uses actions/upload-artifact@v7, and
uploads nextest outputs and snapshot diffs (e.g.,
baml_language/**/target/nextest/ and any *.snap.new files) with a descriptive
artifact name like test-failures-macos and if-no-files-found: ignore so
debugging artifacts are available when tests fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: de62007b-8950-4b99-a572-06a5c36668f8
📒 Files selected for processing (6)
.github/workflows/cargo-tests.reusable.yamlbaml_language/crates/baml/Cargo.tomlbaml_language/crates/baml_release/Cargo.tomlbaml_language/crates/baml_tests/Cargo.tomlbaml_language/crates/bex_engine/Cargo.tomlmise.toml
✅ Files skipped from review due to trivial changes (2)
- baml_language/crates/baml_tests/Cargo.toml
- baml_language/crates/baml/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- mise.toml
- baml_language/crates/baml_release/Cargo.toml
- baml_language/crates/bex_engine/Cargo.toml
f8806e4 to
3f6ba20
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/cargo-tests.reusable.yaml:
- Line 181: Replace tag-pinned GitHub Action references with immutable commit
SHAs: find the `uses:` entries referencing actions like
`useblacksmith/checkout@v1` (and the other tag-pinned uses at the mentioned
locations) and update each to the corresponding full commit SHA (e.g.,
`useblacksmith/checkout@<full-commit-sha>`). Ensure you look for the exact
`uses:` lines in the workflow and substitute the tag suffix (`@v1`, `@v6`,
`@v7`, etc.) with the full commit SHA for that action repository so the workflow
is pinned immutably.
- Around line 171-176: The cargo-test-macos-archive job currently always runs
and will hard-fail on fork PRs lacking R2 secrets; modify the workflow to gate
the archive path on the presence of the required secrets (or implement the
native-Mac fallback) by adding a conditional that checks the R2 secret variables
before running the cargo-test-macos-archive job (and the related archive steps
referenced around cargo-test-macos-run); ensure you target the job name
cargo-test-macos-archive and any archive/fetch steps so they only execute when
the R2 secrets exist (apply the same conditional to the other affected job block
around lines 207-219).
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fd06c961-c748-48b2-a558-39886beb93a1
📒 Files selected for processing (6)
.github/workflows/cargo-tests.reusable.yamlbaml_language/crates/baml/Cargo.tomlbaml_language/crates/baml_release/Cargo.tomlbaml_language/crates/baml_tests/Cargo.tomlbaml_language/crates/bex_engine/Cargo.tomlmise.toml
✅ Files skipped from review due to trivial changes (2)
- baml_language/crates/baml/Cargo.toml
- mise.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- baml_language/crates/baml_tests/Cargo.toml
- baml_language/crates/baml_release/Cargo.toml
- baml_language/crates/bex_engine/Cargo.toml
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/cargo-tests.reusable.yaml (1)
173-219:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate the macOS archive path when the SDK credentials are absent.
These secrets are optional at the workflow boundary, but Line 207 still unconditionally attempts the private SDK download. If either env var is empty,
aws s3 cpfails before the archive upload, socargo-test-macos-runnever gets an artifact to execute. Gate this job on credential presence or keep the native-mac fallback for secretless runs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/cargo-tests.reusable.yaml around lines 173 - 219, The job cargo-test-macos-archive unconditionally tries to download the private macOS SDK using BAML_SCCACHE_R2_ACCESS_KEY_ID and BAML_SCCACHE_R2_SECRET_ACCESS_KEY; guard that step (or the whole job) so it only runs when those secrets are present. Change the "Download macOS SDK from R2 + set SDKROOT" step to check the env vars (BAML_SCCACHE_R2_ACCESS_KEY_ID and BAML_SCCACHE_R2_SECRET_ACCESS_KEY) and skip the aws s3 cp / SDKROOT export if they are empty (or alternatively gate the entire cargo-test-macos-archive job with an if: that requires those env vars), ensuring cargo-test-macos-run still receives a valid artifact or the workflow falls back to the native-mac path when credentials are absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In @.github/workflows/cargo-tests.reusable.yaml:
- Around line 173-219: The job cargo-test-macos-archive unconditionally tries to
download the private macOS SDK using BAML_SCCACHE_R2_ACCESS_KEY_ID and
BAML_SCCACHE_R2_SECRET_ACCESS_KEY; guard that step (or the whole job) so it only
runs when those secrets are present. Change the "Download macOS SDK from R2 +
set SDKROOT" step to check the env vars (BAML_SCCACHE_R2_ACCESS_KEY_ID and
BAML_SCCACHE_R2_SECRET_ACCESS_KEY) and skip the aws s3 cp / SDKROOT export if
they are empty (or alternatively gate the entire cargo-test-macos-archive job
with an if: that requires those env vars), ensuring cargo-test-macos-run still
receives a valid artifact or the workflow falls back to the native-mac path when
credentials are absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 33413d5a-632c-495e-8bf8-2a3d016687f9
📒 Files selected for processing (6)
.github/workflows/cargo-tests.reusable.yamlbaml_language/crates/baml/Cargo.tomlbaml_language/crates/baml_release/Cargo.tomlbaml_language/crates/baml_tests/Cargo.tomlbaml_language/crates/bex_engine/Cargo.tomlmise.toml
✅ Files skipped from review due to trivial changes (2)
- baml_language/crates/baml_release/Cargo.toml
- baml_language/crates/baml_tests/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- baml_language/crates/bex_engine/Cargo.toml
- baml_language/crates/baml/Cargo.toml
3f6ba20 to
bb23aa8
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/cargo-tests.reusable.yaml (1)
966-967: 💤 Low valuemacOS jobs don't produce sccache stats or cargo timings artifacts.
The merged aggregation jobs now depend on
cargo-test-macos-archiveandcargo-test-macos-run, but neither job uploads sccache stats or cargo timings artifacts. The archive job runs the cross-build but lacks the "Dump sccache stats" and "Upload cargo timings" steps present in other jobs. The run job uses a prebuilt archive (no compilation/sccache).This is likely intentional (run job has no build, archive job could add stats), but worth confirming the merge patterns
cargo-timings-*andsccache-stats-*won't fail or produce misleading results when these jobs contribute no matching artifacts.Also applies to: 986-987
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/cargo-tests.reusable.yaml around lines 966 - 967, The macOS CI jobs cargo-test-macos-archive and cargo-test-macos-run are not producing sccache stats or cargo timings artifacts, which can cause the aggregation jobs that look for cargo-timings-* and sccache-stats-* to miss data or fail; update cargo-test-macos-archive to include the same "Dump sccache stats" and "Upload cargo timings" steps used by other build jobs (so it emits sccache-stats-* and cargo-timings-* artifacts) and/or make the aggregation jobs tolerant of missing artifacts from cargo-test-macos-run (which legitimately has no build) by skipping or conditionally aggregating when matching artifacts are absent. Ensure changes target the job definitions named cargo-test-macos-archive and cargo-test-macos-run and the aggregation patterns cargo-timings-* and sccache-stats-*.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/cargo-tests.reusable.yaml:
- Around line 966-967: The macOS CI jobs cargo-test-macos-archive and
cargo-test-macos-run are not producing sccache stats or cargo timings artifacts,
which can cause the aggregation jobs that look for cargo-timings-* and
sccache-stats-* to miss data or fail; update cargo-test-macos-archive to include
the same "Dump sccache stats" and "Upload cargo timings" steps used by other
build jobs (so it emits sccache-stats-* and cargo-timings-* artifacts) and/or
make the aggregation jobs tolerant of missing artifacts from
cargo-test-macos-run (which legitimately has no build) by skipping or
conditionally aggregating when matching artifacts are absent. Ensure changes
target the job definitions named cargo-test-macos-archive and
cargo-test-macos-run and the aggregation patterns cargo-timings-* and
sccache-stats-*.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7bc21f6e-ff79-49ce-b66d-c65101b49f17
📒 Files selected for processing (6)
.github/workflows/cargo-tests.reusable.yamlbaml_language/crates/baml/Cargo.tomlbaml_language/crates/baml_release/Cargo.tomlbaml_language/crates/baml_tests/Cargo.tomlbaml_language/crates/bex_engine/Cargo.tomlmise.toml
🚧 Files skipped from review as they are similar to previous changes (5)
- baml_language/crates/bex_engine/Cargo.toml
- baml_language/crates/baml/Cargo.toml
- baml_language/crates/baml_release/Cargo.toml
- mise.toml
- baml_language/crates/baml_tests/Cargo.toml
bb23aa8 to
a802e05
Compare
ad54f8a to
0c3c9c6
Compare
0c3c9c6 to
a039189
Compare
a039189 to
a9e8b10
Compare
a9e8b10 to
772ea49
Compare
sxlijin
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@baml_language/sdk_tests/crates/python_pydantic2/setup.ps1`:
- Around line 70-80: The current loop uses $skip to drop lines until a blank
line, which will also drop a following TOML section header if there's no blank
line; update the inner skip handling so that when $skip is true and a new
section header (match '^\[.*\]$') is seen you clear $skip and allow that header
to be processed (i.e., do not continue). Specifically modify the block that
tests if ($skip) to also detect '^\[.*\]$': if it matches, set $skip = $false
and let the code fall through to $out.Add($line); otherwise keep the existing
blank-line behavior and continue skipping.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4efc01d6-8da6-4ed3-93dd-088db652ea26
📒 Files selected for processing (6)
.github/actionlint.yaml.github/actions/setup-mise/action.yml.github/workflows/cargo-tests.reusable.yamlbaml_language/.config/nextest.tomlbaml_language/sdk_tests/crates/python_pydantic2/setup.ps1mise.toml
✅ Files skipped from review due to trivial changes (3)
- baml_language/.config/nextest.toml
- .github/actions/setup-mise/action.yml
- .github/actionlint.yaml
…ing, native per-platform, windows sdk uv dedupe
Summary by CodeRabbit