Repository navigation
fix(pack): update libsui and test musl - #4283
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):
|
📝 WalkthroughWalkthroughThe PR updates Linux ELF packing to use ChangesMusl packaging
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant cargo_nextest
participant baml_cli
participant packed_binary
CI->>cargo_nextest: run standard musl tests excluding pack_e2e
CI->>baml_cli: run pack_e2e
baml_cli->>packed_binary: produce and execute packed program
packed_binary-->>CI: return exit code 42
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/tests/test_release_pipeline_contract.py (1)
671-673: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the
pack_e2enextest exclusion.The workflow intentionally excludes
pack_e2efrom nextest and runs it separately, but this contract only checks the generic nextest command. Assert the exact filter so the test catches duplicate or incorrectly combined execution.Proposed contract assertion
self.assertIn("cargo nextest run --workspace", musl) + self.assertIn("-E 'not binary(=pack_e2e)'", musl) self.assertIn("cargo test -p baml_cli --test pack_e2e", musl)🤖 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 `@scripts/tests/test_release_pipeline_contract.py` around lines 671 - 673, Update the contract assertions around the musl workflow string in the test method containing the existing nextest checks to verify that the generic nextest command excludes the pack_e2e test via its exact filter. Preserve the separate `cargo test -p baml_cli --test pack_e2e` assertion and ensure the assertion rejects duplicate or combined execution..github/workflows/cargo-tests.reusable.yaml (1)
169-190: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftCover both musl targets required by the PR.
The new workflow and contract test exercise only
x86_64-unknown-linux-musl, while the stated objective includesaarch64-unknown-linux-musl. Without a matching CI path, aarch64-specific packaging regressions can remain undetected.
.github/workflows/cargo-tests.reusable.yaml#L169-L190: add an aarch64 musl job or provide equivalent existing coverage.scripts/tests/test_release_pipeline_contract.py#L659-L665: include the aarch64 target and assert its workflow job.🤖 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 169 - 190, The musl coverage currently exercises only x86_64, so add equivalent aarch64 musl CI coverage in the cargo-test workflow, including the required target/toolchain setup and test execution. Update scripts/tests/test_release_pipeline_contract.py at lines 659-665 to include aarch64-unknown-linux-musl and assert its workflow job; both listed sites require changes.
🤖 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 169-190: The musl coverage currently exercises only x86_64, so add
equivalent aarch64 musl CI coverage in the cargo-test workflow, including the
required target/toolchain setup and test execution. Update
scripts/tests/test_release_pipeline_contract.py at lines 659-665 to include
aarch64-unknown-linux-musl and assert its workflow job; both listed sites
require changes.
In `@scripts/tests/test_release_pipeline_contract.py`:
- Around line 671-673: Update the contract assertions around the musl workflow
string in the test method containing the existing nextest checks to verify that
the generic nextest command excludes the pack_e2e test via its exact filter.
Preserve the separate `cargo test -p baml_cli --test pack_e2e` assertion and
ensure the assertion rejects duplicate or combined execution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e24f590-04da-4930-b6e2-dcdd719efdf4
📒 Files selected for processing (2)
.github/workflows/cargo-tests.reusable.yamlscripts/tests/test_release_pipeline_contract.py
Binary size checks passed✅ 7 passed
Generated by |
Issue Reference
baml pack --target *-unknown-linux-muslalways fails: "Unsupported sh_offset value 0x238 for section '.hash'" #4279Changes
libsui::Elf::append.x86_64-unknown-linux-musl, including the target-compatible workspace suite andpack_e2e.baml.sys.exit(42)programs propagate exit code 42.Testing
Commands run:
cargo test -p baml_cli --test pack_e2e --all-featurespython3 -m unittest scripts.tests.test_release_pipeline_contractcargo fmt --all -- --checkThe new musl execution runs in CI.
Screenshots
Not applicable.
PR Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests